Test for supporting various conventions for array fields #157

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@jenmei
jenmei commented Jan 30, 2012

For our purposes, we've been attempting to use VCR/WebMock to playback requests to the Echo Nest API.

Some of their API methods take parameters w/ multiple values, e.g. see bucket, http://developer.echonest.com/docs/v4/artist.html, http://developer.echonest.com/api/v4/artist/profile?api_key=N6E4NIOVYMTHNDM8J&id=ARH6W4X1187B99274F&format=json&bucket=biographies&bucket=blogs&bucket=familiarity&bucket=hotttnesss&bucket=images&bucket=news&bucket=reviews&bucket=terms&bucket=urls&bucket=video&bucket=id:rdio-us-streaming.

In several places (WebMock::Util::URI.normalize_uri or #matches? methods from WebMock::URIPattern subclasses), WebMock uses a Hash to set an Addressable::URI instance's query_values. The fallout from this approach is that with a query string a la ?a=1&a=2, since a Ruby hash has unique keys, information is lost when duplicate keys exist, i.e. ?a=1&a=2 translates to query_values of something like { 'a' => '2' }.

Consequently, we're unable to use WebMock because several param key/value pairs are missing, resulting in unexpected response values.

Jen-Mei Wu &... added some commits Jan 30, 2012
Jen-Mei Wu & Kurt Ruppel Test for supporting various conventions for array fields, e.g. "?a[]=…
…1&a[]=2" or "?a=1&a=2".
5df99d1
Jen-Mei Wu & Kurt Ruppel URI params w/ duplicate keys were being lost on normalization. Change…
…d to use Addressable::URI's :flat_array notation, which preserves data in the case of duplicate keys (e.g. "?a=1&a=2" as opposed to "?a[]=1&a[]=2").
1fc074b
@jenmei
jenmei commented Jan 31, 2012

We've patched this to work for our case (so far) but you will probably want to take a look at it. For one thing, we only fixed it for URIStringPattern and didn't touch URIRegexpPattern.

@bblimke
Owner
bblimke commented Feb 4, 2012

This made me think and search on the internet for some specs and rfcs.
Looks like there is not spec on how to handle duplicated params.

Here is an answer:
http://stackoverflow.com/questions/1746507/authoritative-position-of-duplicate-http-get-query-keys

Webmock currently implements "last given" approach.
I dodn't see any problems to change it to your solution though.

convert_hash_to_array method handles only hashes one level deep.
Shouldn't it support hashes nested many levels down, with nested hashes and arrays?

@jenmei
jenmei commented Feb 6, 2012

I thought of that, too, but after digging into more standards stuff, we came to the conclusion that there isn't a specification for what's a legal query string. E.g., separating query fields by ampersands seems to be a convention, not part of any specification. See: http://www.w3.org/Addressing/URL/4_URI_Recommentations.html:

"The query string represents some operation applied to the object, but this specification gives no common syntax or semantics for it. In practice the syntax and sematics may depend on the scheme and may even on the base URI."

So ... it seems like what might be best is to proceed from known use cases. In this case, VCR works for many cases already, but there are some known cases (e.g. EchoNest) where field names are non-unique. In the future, you might find that there are some query strings that cannot be split on, say, ampersands, and that doing so may cause all kinds of problems. But ... maybe it'd be best until those surfaced until trying to account for them? Or maybe allow enough flexibility that people can choose various ways of determining query string uniqueness, etc. Thoughts?

@bblimke
Owner
bblimke commented Feb 6, 2012

That sounds good.
and what about convert_hash_to_array' method. Shouldn't it support nested structures?

@bblimke
Owner
bblimke commented Feb 19, 2012

Adding this feature to WebMock would require more changes.

  1. convert_hash_to_array has to recursive to handle nested query values.
  2. It needs to work with URIRegexpPattern, which handles query values differently.

Maybe the best way, would be to refactor RequestPattern and extract QueryPattern which
would be responsible for query values, while URIPattern would be just responsible for host.

@jenmei
jenmei commented Feb 21, 2012

Sorry for not following up earlier. I've been preoccupied with various projects but will look at this again soon. I think recursive would be nice, but it would be good to have a real world test case. I'll take a look at that. But for the greatest flexibility, since it's possible for the query string to contain data in uncommon formats, what do you think if there was an option to pass a block to the API for determining uniqueness?

@bblimke
Owner
bblimke commented Feb 21, 2012

Could you show an example of how API with block would work?

@bblimke bblimke closed this Jul 23, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment