rlalfo / google-http-java-client

Automatically exported from code.google.com/p/google-http-java-client
0 stars 0 forks source link

GenericUrl should construct Collection instance variables as array query param #253

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
Version of google-http-java-client (e.g. 1.17.0-rc)?

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

Describe the problem.
Classes that extend GenericUrl and set their instance variable as a Collection 
will have the value of the type set as toString() as opposed to being converted 
to an array query parameter.

How would you expect it to be fixed?

Given a class:

class A extends GenericUrl {
  @Key protected Map<String, Object> param1;
  @Key protected List<String> param2;
}

when we build() an instance of object A I would expect param1 to be constructed 
as:

param1[key1]=value1&param1[key2]=value2&...&param1[keyN]=valueN

Likewise, with a list:

param2[]=value1&param2[]=value2&...&param2[]=valueN

What GenericUrl will do currently is set param1 the toString(), and then url 
encoded, representation of the Map type, which would look something like:

param1=%7Bkey1%20%3Dvalue2%7D

Original issue reported on code.google.com by jamie.co...@gmail.com on 20 Dec 2013 at 2:47

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
I have uploaded a diff to codereview.appspot - I have yet to read any 
convention about cross posting issues reported between these two systems - I'm 
new to using these tools.

I do have a suggested fix.  The code for review is here [1].  I'm not familiar 
with these processes - I'll assume code reviews are picked up adhoc, and are 
not pre-assigned?

[1] https://codereview.appspot.com/34660044/

Original comment by jamie.co...@gmail.com on 20 Dec 2013 at 3:42

GoogleCodeExporter commented 9 years ago
Actually, the current behavior is correct.  Parsing URLs as strings will work 
under the original scheme, whereas introducing something like this would break 
parsing.

Another problem is that my @Key List<String> param2 case will not work as 
expected, under the current patch, due to addQueryParams() checking for a 
collection type - which is necessary when parsing input as 
"http://example.com/foo?a=one&b=two&a=three"

I would concede this is not a good idea to introduce.  However, I am somewhat 
constrained to change the query parameter construction, based on some model, 
rather than parsing string data.  In other words, I'm finding it difficult to 
extend GenericUrl and the behavior of creating query parameters under the 
current design, since it was intended to prevent people from changing this 
behavior.

Would it be reasonable to ask that GenericUrl be changed to become somewhat 
flexible to change; the behavior of query parameter construction potentially 
changed by callers that extend this class?

I realize the RFC GenericUrl / GenericData conforms to is probably the 'sane 
thing to use' (tm) but I am constrained by web services that do not accept data 
in this way.  The web API requires input of collections as I specified in the 
original description.

Original comment by jamie.co...@gmail.com on 27 Jan 2014 at 9:11