http issue 65 (HttpResponseInterceptor) & issue 159 (multiple header values)

http issue 65 (HttpResponseInterceptor) & issue 159 (multiple header values)

http://code.google.com/p/google-http-java-client/issues/detail?id=65
Create a HttpResponseInterceptor

http://code.google.com/p/google-http-java-client/issues/detail?id=159
Need to be able to set headers multiple times

[ngmiceli, 2012-11-20 19:32:37]

Just some thoughts, nothing too pressing.

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/main/jav...
File
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java
(right):

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:94:
private Long contentLength;
It still feels just a little strange to me that we allow the user multiple
values for numerous headers that wouldn't make sense to have (such as contentMD5
- by definition a body can only have one unique hash) but not allow multiple
contentLengths. Just from a design point of view, might it be better just to be
consistent?

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:841:
private static String toStringValue(Object headerValue) {
Based on how this method is used below, I assume this only intended to be used
on single header values, not lists? Otherwise that condition isn't handled, and
the default toString of most List implementations probably won't give us what we
want.

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:1128:
// TODO(yanivi): override equals and hashCode
It might be nice to have a method that takes a given header and outputs it as a
String. 

Via this: http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
"It MUST be possible to combine the multiple header fields into one "field-name:
field-value" pair, without changing the semantics of the message, by appending
each subsequent field-value to the first, each separated by a comma."
While we don't have to support that as a feature of this class, it seems
appropriate.

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/test/jav...
File
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java
(right):

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/test/jav...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:809:
request.getHeaders().set("accept", Arrays.asList("text/plain"));
Arrays.asList feels a little awkward here. Just my opinion, but might it be
better for the .set(String, Object) method to check if the second parameter is a
non-list type and if so, convert it there?
If it already does this, I would suggest not putting this in the test as tests
give a good sample on how to use the methods being tested/mocked.

[yanivi, 2012-11-20 21:28:36]

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/main/jav...
File
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java
(right):

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:94:
private Long contentLength;
On 2012/11/20 19:32:37, ngmiceli wrote:
> It still feels just a little strange to me that we allow the user multiple
> values for numerous headers that wouldn't make sense to have (such as
contentMD5
> - by definition a body can only have one unique hash) but not allow multiple
> contentLengths. Just from a design point of view, might it be better just to
be
> consistent?

Can you please be more specific about exactly what change you'd propose?

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:841:
private static String toStringValue(Object headerValue) {
On 2012/11/20 19:32:37, ngmiceli wrote:
> Based on how this method is used below, I assume this only intended to be used
> on single header values, not lists? Otherwise that condition isn't handled,
and
> the default toString of most List implementations probably won't give us what
we
> want.

Yes, single header value, not lists.

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:1128:
// TODO(yanivi): override equals and hashCode
On 2012/11/20 19:32:37, ngmiceli wrote:
> It might be nice to have a method that takes a given header and outputs it as
a
> String. 
> 
> Via this: http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
> "It MUST be possible to combine the multiple header fields into one
"field-name:
> field-value" pair, without changing the semantics of the message, by appending
> each subsequent field-value to the first, each separated by a comma."
> While we don't have to support that as a feature of this class, it seems
> appropriate.

This is one of those parts of the HTTP that makes no sense at all.  So for
example, dates may contain commas, so there is no way to determine if the commas
indicates a new header value or is part of the header value.  So I choose to
ignore that statement in the specification and pretend the option does not exist
to use commas as a header value separator :)

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/test/jav...
File
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java
(right):

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/test/jav...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:809:
request.getHeaders().set("accept", Arrays.asList("text/plain"));
On 2012/11/20 19:32:37, ngmiceli wrote:
> Arrays.asList feels a little awkward here. Just my opinion, but might it be
> better for the .set(String, Object) method to check if the second parameter is
a
> non-list type and if so, convert it there?
> If it already does this, I would suggest not putting this in the test as tests
> give a good sample on how to use the methods being tested/mocked. 

We are specifically testing out unusual functionality in HttpHeaders.  We don't
really want people to do this for the "accept" header, and would much rather
people call setAccept.

With respect to set(String, Object) and detecting if the 2nd parameter is a
non-list type: the problem is detecting that the assigned field is supposed to
be a list type.  We do want to support in general the ability to have a
singleton value for fields.  But I guess I don't really see the benefit either,
because after all if you're using the set method you're already a low-level
customer and it is important for you to understand that it is in fact a list.

[ngmiceli, 2012-11-20 23:41:09]

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/main/jav...
File
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java
(right):

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:94:
private Long contentLength;
On 2012/11/20 21:28:36, yanivi wrote:
> On 2012/11/20 19:32:37, ngmiceli wrote:
> > It still feels just a little strange to me that we allow the user multiple
> > values for numerous headers that wouldn't make sense to have (such as
> contentMD5
> > - by definition a body can only have one unique hash) but not allow multiple
> > contentLengths. Just from a design point of view, might it be better just to
> be
> > consistent?
> 
> Can you please be more specific about exactly what change you'd propose?

I'm not saying I think this is better per say, I'm just saying its worth
consideration.

The internal representation of almost every header is now a List and supports
multiple of that header. The exception appears to be contentLength, which is
still a Long and thus there can only be one. If we're going to support multiple
headers on other non-intuitive values we should do it on every header. Thus,
perhaps this should be List<Long> and support multiple contentLengths just like
the others.

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/test/jav...
File
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java
(right):

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/test/jav...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:809:
request.getHeaders().set("accept", Arrays.asList("text/plain"));
Regarding the following:
> With respect to set(String, Object) and detecting if the 2nd parameter is a
> non-list type: the problem is detecting that the assigned field is supposed to
> be a list type. 

Couldn't this be solved by simply requiring all fields to be a list type? This
follows my other comment about consistency.

[yanivi, 2012-11-21 14:23:32]

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/main/jav...
File
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java
(right):

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:94:
private Long contentLength;
On 2012/11/20 23:41:09, ngmiceli wrote:
> On 2012/11/20 21:28:36, yanivi wrote:
> > On 2012/11/20 19:32:37, ngmiceli wrote:
> > > It still feels just a little strange to me that we allow the user multiple
> > > values for numerous headers that wouldn't make sense to have (such as
> > contentMD5
> > > - by definition a body can only have one unique hash) but not allow
multiple
> > > contentLengths. Just from a design point of view, might it be better just
to
> > be
> > > consistent?
> > 
> > Can you please be more specific about exactly what change you'd propose?
> 
> I'm not saying I think this is better per say, I'm just saying its worth
> consideration.
> 
> The internal representation of almost every header is now a List and supports
> multiple of that header. The exception appears to be contentLength, which is
> still a Long and thus there can only be one. If we're going to support
multiple
> headers on other non-intuitive values we should do it on every header. Thus,
> perhaps this should be List<Long> and support multiple contentLengths just
like
> the others.

Done.

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/test/jav...
File
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java
(right):

https://codereview.appspot.com/6846062/diff/1/google-http-client/src/test/jav...
google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java:809:
request.getHeaders().set("accept", Arrays.asList("text/plain"));
On 2012/11/20 23:41:09, ngmiceli wrote:
> Regarding the following:
> > With respect to set(String, Object) and detecting if the 2nd parameter is a
> > non-list type: the problem is detecting that the assigned field is supposed
to
> > be a list type. 
> 
> Couldn't this be solved by simply requiring all fields to be a list type? This
> follows my other comment about consistency.

That would be a significant breaking functionality change, and I don't actually
agree with it.  Let's discuss this in person.

[ngmiceli, 2012-11-21 15:15:02]

Test failures. For example:

Tests run: 9, Failures: 0, Errors: 5, Skipped: 0, Time elapsed: 0.008 sec <<<
FAILURE!
testClone_changingEntrySet(com.google.api.client.util.GenericDataTest)  Time
elapsed: 0.003 sec  <<< ERROR!
java.lang.NoSuchMethodError:
com.google.api.client.util.GenericData.set(Ljava/lang/String;Ljava/lang/Object;)V
        at
com.google.api.client.util.GenericDataTest.testClone_changingEntrySet(GenericDataTest.java:44)

Almost all are NoSuchMethodExceptions involving GenericData.

[ngmiceli, 2012-11-21 15:16:05]

Disregard.

LGTM