Added another newResponse method to address issue 32. #59
Conversation
…s a map of strings that allows the caller to specify custom headers.
| @@ -107,6 +107,27 @@ public static FullHttpResponse newResponse( | |||
| return response; | |||
| } | |||
|
|
|||
| /* | |||
There was a problem hiding this comment.
Should be javadoc formatted (/** at start).
| * "access-control-allow-origin", "*" | ||
| */ | ||
| public static FullHttpResponse newResponse( | ||
| HttpResponseStatus status, ByteBuf buffer, ContentType contentType, Map<String, String> customHeaders) { |
There was a problem hiding this comment.
There's a mix of the request content argument being named buffer and being named payload in this file. Would you mind renaming all of them to payload, for consistency & clarity?
| * | ||
| * Headers should be specified as a map of strings. For example, to allow CORS, add the | ||
| * following key and value: | ||
| * "access-control-allow-origin", "*" |
There was a problem hiding this comment.
We never want an allowed origin of *.
A better method to handle CORS is to have a configured set of allowed origins (possibly with wildcards), and echo back the request's Origin header as the allowed origin when it matches the configuration.
| HttpResponseStatus status, ByteBuf buffer, ContentType contentType, Map<String, String> customHeaders) { | ||
| FullHttpResponse response = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, status, buffer); | ||
|
|
||
| response.headers().set(CONTENT_TYPE, contentType.value); |
There was a problem hiding this comment.
These should probably go after the header iteration.
Either way, you should document the behavior (customHeaders overrides provided content type + length, or customHeaders content type + length are ignored).
… allowed origin of *.
…ayload, to be consistent across all methods
…th could be passed in, leading to confusing behavior. Added an explanatory comment.
|
Jesse, thanks for the comments! I think I've addressed most of them with the last few commits. Hmm, that's a good point about there being a better way to handle CORS- this change really is for custom headers, not CORS specifically. I will take a look at implementing that today. |
| HttpResponseStatus status, ByteBuf buffer, ContentType contentType) { | ||
| FullHttpResponse response = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, status, buffer); | ||
| HttpResponseStatus status, ByteBuf payload, ContentType contentType) { | ||
| FullHttpResponse response = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, status, payload); |
There was a problem hiding this comment.
Update this to use your new method?
// java.util.Maps below.
return newResponse(status, payload, contentType, Maps.emptyMap());… newResponse method with an empty map. Also added check in case the map parameter is null.
# Conflicts: # src/main/java/com/nordstrom/xrpc/server/http/Recipes.java
|
I fixed the conflict that this request had- what is the next step to merge? @jkinkead |
|
I will merge now. :) |
|
I think this change needs to be released before I can pull it in as a dependency in another project, right? If I follow the instructions in CONTRIBUTING.md, can I go ahead and do this? |
|
If you'd like, yes. Else, either @spenserpothier or myself can run a release. |
* 'master' of github.com:nordstrom/xrpc: Added another newResponse method to address issue 32. (#59)
New method takes a map of strings that allows the caller to specify custom headers.
Also, please feel free to let me know if I've misunderstood anything about this fix or the PR process. Thanks!