Need to be able to set multiple values for the same header [backwards incompatible]

[GoogleCodeExporter]

External references, such as a standards document, or specification?

http://javadoc.google-http-java-client.googlecode.com/hg/1.11.0-beta/com/google/
api/client/http/HttpHeaders.html#getAuthorization()

Java environments (e.g. Java 6, Android 2.3, App Engine, or All)?

All

Please describe the feature requested.

Try running this code:

  public static void main(String[] args) throws IOException {
    HttpHeaders headers = new HttpHeaders();
    headers.put("Authorization", ImmutableList.of("Foo", "Bar"));
    ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
    Writer writer = new OutputStreamWriter(outputStream);
    HttpHeaders.serializeHeadersForMultipartRequests(headers, null, null, writer);
    System.out.println(outputStream);
  }

You would get an exception:

java.lang.IllegalArgumentException
    at sun.reflect.UnsafeObjectFieldAccessorImpl.set(UnsafeObjectFieldAccessorImpl.java:63)
    at java.lang.reflect.Field.set(Field.java:657)
    at com.google.api.client.util.FieldInfo.setFieldValue(FieldInfo.java:247)
    at com.google.api.client.util.FieldInfo.setValue(FieldInfo.java:208)
    at com.google.api.client.util.GenericData.put(GenericData.java:103)

but if you change "Authorization" to "Other" it will work nicely and print:

Accept-Encoding: gzip
other: Foo
other: Bar

The reason is the way the authorization header is declared in HttpHeaders.  If 
you change it from:
  /** {@code "Authorization"} header. */
  @Key("Authorization")
  private String authorization;

to:

  /** {@code "Authorization"} header. */
  @Key("Authorization")
  private List<String> authorization;

It will fix the problem.

We can still have "String getAuthorization()" and "HttpHeaders 
setAuthorization(String)" to optimize the developer experience for the more 
common use case.  It would just treat it as a List with a single element.

More generally, we should do this for ALL headers, not just Authorization.  The 
HTTP specification allows this for all headers.

We should also add an "HttpHeaders addHeader(String name, String value)".  The 
logic would be to check if the header already has a value.  If it has no value, 
it simply sets the value.  If that value is a List, it should add to the list.  
If it is not a List, and it already has a value, it should throw an exception.

Original issue reported on code.google.com by yan...@google.com on 20 Oct 2012 at 1:29

[GoogleCodeExporter]

I completed this for the Authorization header here:
https://codereview.appspot.com/6736062/

Need to make the same change for all other headers.

Original comment by yan...@google.com on 22 Oct 2012 at 7:31

[GoogleCodeExporter]

Original comment by yan...@google.com on 22 Oct 2012 at 7:32

[GoogleCodeExporter]

We'll also need to do this for all subclasses of HttpHeaders in the sister 
projects, such as GoogleHeaders and SubscriptionHeaders

Also, we should change the setter methods to accept any Object -- not just  
String -- and return this (and be overridden in the subclass to change return 
type).

The getter method is the more problematic one.  We could keep the current 
signature and just cast to String and let a ClassCastException be thrown.  
Users may rightly complain that this is a bug because multiple headers are a 
legitimate use case.  This is the only backwards-compatible option, but 
thankfully we're still in Beta so it is not strictly required.

Another object is to not cast and change the return type to Object.  Although 
theoretically more correct, it is also more annoying because you have to cast 
the result and have them worry about it potentially having a value of any kind, 
whereas typically it will only be String or List<String>.

Another alternative is to try to do something "clever" like returning 
Iterable<String> which if instanceof String would return 
Collections.singleton() of the value.  We could even write a static utility 
method to simplify this, so the implementation would be for example "return 
asIterableString(authorization);".

Opinions?

Original comment by yan...@google.com on 24 Oct 2012 at 1:44

[GoogleCodeExporter]

The paragraph above should instead be:

Another option is to not cast and instead change the return type to Object.  
Although theoretically more correct, it is also more annoying because 
developers will have to cast the result and worry about return value 
potentially being of any type, whereas typically it will only be String or 
Iterable<String>.

Original comment by yan...@google.com on 24 Oct 2012 at 1:46

[GoogleCodeExporter]

Another question is whether we want to have a single "setAuthorization(Object)" 
or two methods "setAuthorization(String)" and 
"setAuthorization(Iterable<String>)" (either Iterable or Collection).  We could 
even add "setAuthorization(String, String...)" instead of 
"setAuthorization(String)".  I don't have a strong feeling about it at this 
point.

Original comment by yan...@google.com on 24 Oct 2012 at 7:52

[GoogleCodeExporter]

Original comment by yan...@google.com on 14 Nov 2012 at 7:24

[GoogleCodeExporter]

Original comment by yan...@google.com on 15 Nov 2012 at 9:27

• Changed title: Need to be able to set headers multiple times [backwards incompatible]

[GoogleCodeExporter]

https://codereview.appspot.com/6846062/

Original comment by yan...@google.com on 16 Nov 2012 at 1:36

• Changed title: Need to be able to set multiple values for the same header [backwards incompatible]
• Changed state: Started

[GoogleCodeExporter]

Original comment by yan...@google.com on 28 Nov 2012 at 5:11

• Changed state: Fixed

[GoogleCodeExporter]

Concerned that changing all header fields to be a list is going to break 
parsing of date.  The http header date specification doesn't say it allows 
brackets:
See section 14.18: 
http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.18

Original comment by daniel...@google.com on 20 Dec 2012 at 2:36

[GoogleCodeExporter]

Daniel, as we discussed the problem is that any code that calls get(String) and 
then toString() is now going to get a bracket because internally we represent 
the header value as a List<String> instead of String (to match the HTTP 
specification).

The fix is to call getFirstHeaderStringValue(String) instead.  That way you 
don't rely on the internal implementation details of HttpHeaders.

Thanks for the feedback though.  I'm sure a lot of developers are going to run 
into a similar problem.

Original comment by yan...@google.com on 20 Dec 2012 at 1:05