Issue 6121048: Adding HttpRequest.serializeHeaders (Closed)
DescriptionChanges to support http://codereview.appspot.com/5539055/.
Patch Set 1 #Patch Set 2 : Minor fix #
Total comments: 22
Patch Set 3 : Addressing code review comments #Patch Set 4 : Minor fix #Patch Set 5 : Minor fix #Patch Set 6 : Removing MultipartMixedContent #
Total comments: 6
Patch Set 7 : Addressing code review comments #Patch Set 8 : Minor fix #Patch Set 9 : Minor fix #Patch Set 10 : Minor fix #
MessagesTotal messages: 7
some interesting design questions that this brings up... http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... File google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java (right): http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:949: LowLevelHttpRequest lowLevelHttpRequest, String name, Object value, OutputStream out) [optional] This may be cleaner if we didn't provide an OutputStream parameter. Instead we would make an implementation of LowLevelHttpRequest whose addHeader implementation calls MultipartContent.writeHeader. I like this because it simplifies the logic here. Note that it requires adding a throws IOException to addHeader, which I think is probably a good idea anyway. http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:991: */ @since 1.9 http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:992: public static void serializeHeaders(HttpHeaders headers, StringBuilder logbuf, Logger logger, [optional] move this to HttpHeaders? http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... File google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java (right): http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:27: * href="http://www.ietf.org/rfc/rfc2046.txt">RFC 2046: Multipurpose Internet Mail Extensions</a>. [optional] I prefer the HTML format: http://tools.ietf.org/html/rfc2046 http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:41: public final class MultipartMixedContent extends AbstractHttpContent { how about we rename this to MultipartContent and generalize it so it can be used to replace MultipartRelatedContent? I'd love to be able to deprecate MultipartRelatedContent. http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:43: private static final byte[] NEWLINE = StringUtils.LINE_SEPARATOR.getBytes(); My reading of the spec is that NEWLINE is CRLF regardless of platform: http://tools.ietf.org/html/rfc2046#section-4.1.1 that's what I have in MultipartRelatedContent http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:57: public MultipartMixedContent(List<HttpRequest> requests, String boundary) { if we generalize this, we would need to add something like a "String subType" parameter to specify the subtype of the ContentType to use, i.e. "mixed" or "related" http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:57: public MultipartMixedContent(List<HttpRequest> requests, String boundary) { conceptually the general-purpose multipart content should take something like List<Pair<HttpHeader,HttpContent>> instead of List<HttpRequest>. Then, we should add something like HttpHttpContent implements HttpContent that takes HttpRequest as a parameter to its constructor, has getType() application/http, and whose writeTo(OutputStream) method does the stuff below in the current writeTo(OutputStream) method inside the for loop. http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:58: Preconditions.checkNotNull(requests); [optional] precondition check that !requests.isEmpty() http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:77: out.write("Content-Type: application/http".getBytes()); maybe it would be simpler to just use an OutputStreamWriter since that one converts strings to bytes for you? http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:79: out.write("Content-Transfer-Encoding: binary".getBytes()); Shouldn't these just be headers on the HttpRequest?
Sign in to reply to this message.
http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... File google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java (right): http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:949: LowLevelHttpRequest lowLevelHttpRequest, String name, Object value, OutputStream out) On 2012/04/26 19:53:37, yanivi wrote: > [optional] This may be cleaner if we didn't provide an OutputStream parameter. > Instead we would make an implementation of LowLevelHttpRequest whose addHeader > implementation calls MultipartContent.writeHeader. I like this because it > simplifies the logic here. Note that it requires adding a throws IOException to > addHeader, which I think is probably a good idea anyway. Added a TODO. http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:991: */ On 2012/04/26 19:53:37, yanivi wrote: > @since 1.9 Done. http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:992: public static void serializeHeaders(HttpHeaders headers, StringBuilder logbuf, Logger logger, On 2012/04/26 19:53:37, yanivi wrote: > [optional] move this to HttpHeaders? Done. http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... File google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java (right): http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:27: * href="http://www.ietf.org/rfc/rfc2046.txt">RFC 2046: Multipurpose Internet Mail Extensions</a>. On 2012/04/26 19:53:37, yanivi wrote: > [optional] I prefer the HTML format: http://tools.ietf.org/html/rfc2046 Done. http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:41: public final class MultipartMixedContent extends AbstractHttpContent { On 2012/04/26 19:53:37, yanivi wrote: > how about we rename this to MultipartContent and generalize it so it can be used > to replace MultipartRelatedContent? > > I'd love to be able to deprecate MultipartRelatedContent. Yes this will be good to tackle after we get the batch feature in. I changed the issue I filed previously for this: http://code.google.com/p/google-http-java-client/issues/detail?id=63 Should I go ahead and rename this class to MultipartContent or should I do that when I tackle that issue? http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:43: private static final byte[] NEWLINE = StringUtils.LINE_SEPARATOR.getBytes(); On 2012/04/26 19:53:37, yanivi wrote: > My reading of the spec is that NEWLINE is CRLF regardless of platform: > http://tools.ietf.org/html/rfc2046#section-4.1.1 > > that's what I have in MultipartRelatedContent Done. http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:57: public MultipartMixedContent(List<HttpRequest> requests, String boundary) { On 2012/04/26 19:53:37, yanivi wrote: > if we generalize this, we would need to add something like a "String subType" > parameter to specify the subtype of the ContentType to use, i.e. "mixed" or > "related" Will be done in http://code.google.com/p/google-http-java-client/issues/detail?id=63 http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:57: public MultipartMixedContent(List<HttpRequest> requests, String boundary) { On 2012/04/26 19:53:37, yanivi wrote: > conceptually the general-purpose multipart content should take something like > List<Pair<HttpHeader,HttpContent>> instead of List<HttpRequest>. Then, we > should add something like HttpHttpContent implements HttpContent that takes > HttpRequest as a parameter to its constructor, has getType() application/http, > and whose writeTo(OutputStream) method does the stuff below in the current > writeTo(OutputStream) method inside the for loop. Will be done in http://code.google.com/p/google-http-java-client/issues/detail?id=63 http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:58: Preconditions.checkNotNull(requests); On 2012/04/26 19:53:37, yanivi wrote: > [optional] precondition check that !requests.isEmpty() Done. http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:77: out.write("Content-Type: application/http".getBytes()); On 2012/04/26 19:53:37, yanivi wrote: > maybe it would be simpler to just use an OutputStreamWriter since that one > converts strings to bytes for you? Done. http://codereview.appspot.com/6121048/diff/2001/google-http-client/src/main/j... google-http-client/src/main/java/com/google/api/client/http/MultipartMixedContent.java:79: out.write("Content-Transfer-Encoding: binary".getBytes()); I dont think so, looking at the design doc for batch, the request looks like this: --batch_foobarbaz Content-Type: application/http Content-Transfer-Encoding: binary Content-ID: <item2:12930812@some.server.com> PUT /zoo/animals/sheep Content-Type: application/json Content-Length: ### If-Match: “etag/sheep” Where everything after the method and url of the request are the headers of the request. Whereas everything before (Content-Type, Content-Transfer-Encoding, ContentID) are the multipart headers which is why I separated them here.
Sign in to reply to this message.
Removed MultipartMixedContent
Sign in to reply to this message.
http://codereview.appspot.com/6121048/diff/10008/google-http-client/src/main/... File google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java (right): http://codereview.appspot.com/6121048/diff/10008/google-http-client/src/main/... google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:602: // TODO(rmistry): Explore option of not providing a Writer, instead use an implementation [optional] I think this TODO doesn't make sense too much sense, because once we have a serializeHeaders method that takes Writer, we have to support this use case (or deprecate it). If this is something that we decide we want, we should just do it now. The way I envision it would work is that the caller does something like this: class MyLLHR extends LowLevelHttpRequest { private final Writer writer; MyLLHR(Writer writer) { this.writer = writer; } @Override public void addHeader(String name, String value) throws IOException { writer.write(name); writer.write(": "); writer.write(value); writer.write("\r\n"); } @Override public void setContent(HttpContent content) throws IOException { throw new UnsupportedOperationException(); } @Override public LowLevelHttpResponse execute() throws IOException { throw new UnsupportedOperationException(); } } The catch is that addHeader doesn't throw IOException. Simplest option is that we probably should allow IOException to be thrown -- sound familiar :) Alternatively, one can cache the header name/value pairs and provide an extra method to actually write them out at the end. That would resolve the backwards-compatibility issue. http://codereview.appspot.com/6121048/diff/10008/google-http-client/src/main/... google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:632: if (writer != null) { I find this awkward. It's not obvious that this is the right implementation for writing the headers. For example, how do we know what line separator to use? Perhaps the way to think about this instead is that the JavaDoc of serializeHeaders method explicitly document the exact header encoding we use, and specify that it is built precisely for the use case of multi-part requests. http://codereview.appspot.com/6121048/diff/10008/google-http-client/src/main/... google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:653: public static void serializeHeaders(HttpHeaders headers, StringBuilder logbuf, Logger logger, I don't like the idea of having a serializeHeaders method that takes both LowLevelHttpRequest and Writer. Even if you want to retain the functionality as is, I'd still prefer that the public surface be two serializeHeaders methods, one that takes LowLevelHttpRequest and one that takes Writer. For the one for Writer, you may specify the multi-part request use case in the name of the method itself to make it more obvious. But you can still keep this method that takes both parameters if you make it private.
Sign in to reply to this message.
http://codereview.appspot.com/6121048/diff/10008/google-http-client/src/main/... File google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java (right): http://codereview.appspot.com/6121048/diff/10008/google-http-client/src/main/... google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:602: // TODO(rmistry): Explore option of not providing a Writer, instead use an implementation On 2012/04/30 19:12:37, yanivi wrote: > [optional] > I think this TODO doesn't make sense too much sense, because once we have a > serializeHeaders method that takes Writer, we have to support this use case (or > deprecate it). > > If this is something that we decide we want, we should just do it now. The way > I envision it would work is that the caller does something like this: > > class MyLLHR extends LowLevelHttpRequest { > private final Writer writer; > > MyLLHR(Writer writer) { > this.writer = writer; > } > > @Override > public void addHeader(String name, String value) throws IOException { > writer.write(name); > writer.write(": "); > writer.write(value); > writer.write("\r\n"); > } > > @Override > public void setContent(HttpContent content) throws IOException { > throw new UnsupportedOperationException(); > } > > @Override > public LowLevelHttpResponse execute() throws IOException { > throw new UnsupportedOperationException(); > } > } > > The catch is that addHeader doesn't throw IOException. Simplest option is that > we probably should allow IOException to be thrown -- sound familiar :) > > Alternatively, one can cache the header name/value pairs and provide an extra > method to actually write them out at the end. That would resolve the > backwards-compatibility issue. I think it is going to be useful to have two methods for serializing Headers one for lowLevelHttpRequest and one for writer that is specifically for multipart requests (like you mention below). http://codereview.appspot.com/6121048/diff/10008/google-http-client/src/main/... google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:632: if (writer != null) { On 2012/04/30 19:12:37, yanivi wrote: > I find this awkward. It's not obvious that this is the right implementation for > writing the headers. For example, how do we know what line separator to use? > Perhaps the way to think about this instead is that the JavaDoc of > serializeHeaders method explicitly document the exact header encoding we use, > and specify that it is built precisely for the use case of multi-part requests. Done. http://codereview.appspot.com/6121048/diff/10008/google-http-client/src/main/... google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:653: public static void serializeHeaders(HttpHeaders headers, StringBuilder logbuf, Logger logger, On 2012/04/30 19:12:37, yanivi wrote: > I don't like the idea of having a serializeHeaders method that takes both > LowLevelHttpRequest and Writer. Even if you want to retain the functionality as > is, I'd still prefer that the public surface be two serializeHeaders methods, > one that takes LowLevelHttpRequest and one that takes Writer. > > For the one for Writer, you may specify the multi-part request use case in the > name of the method itself to make it more obvious. But you can still keep this > method that takes both parameters if you make it private. Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
