Need to be able to set multiple values for the same header [backwards incompatible] #159
Comments
wonderfly
added
type: feature request
|
From yan...@google.com on October 22, 2012 12:31:51 I completed this for the Authorization header here: https://codereview.appspot.com/6736062/ Need to make the same change for all other headers. |
|
From yan...@google.com on October 22, 2012 12:32:09 |
|
From yan...@google.com on October 24, 2012 06:44:10 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. Another alternative is to try to do something "clever" like returning Iterable 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? |
|
From yan...@google.com on October 24, 2012 06:46:18 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. |
|
From yan...@google.com on October 24, 2012 12:52:07 Another question is whether we want to have a single "setAuthorization(Object)" or two methods "setAuthorization(String)" and "setAuthorization(Iterable)" (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. |
|
From yan...@google.com on November 14, 2012 11:24:29 Owner: yan...@google.com |
|
From yan...@google.com on November 15, 2012 13:27:35 Summary: Need to be able to set headers multiple times [backwards incompatible] |
|
From yan...@google.com on November 16, 2012 05:36:48 https://codereview.appspot.com/6846062/ Summary: Need to be able to set multiple values for the same header [backwards incompatible] |
|
From yan...@google.com on November 28, 2012 09:11:15 Status: Fixed |
|
From daniel...@google.com on December 19, 2012 18:36:50 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: |
|
From yan...@google.com on December 20, 2012 05:05:40 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 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. |
From yan...@google.com on October 20, 2012 06:29:53
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 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: http://code.google.com/p/google-http-java-client/issues/detail?id=159
The text was updated successfully, but these errors were encountered: