Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Test for supporting various conventions for array fields #157

Closed
wants to merge 2 commits into from

2 participants

Jen-Mei Wu Bartosz Blimke
Jen-Mei Wu

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
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
Jen-Mei Wu

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.

Bartosz Blimke
Owner

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?

Jen-Mei Wu

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?

Bartosz Blimke
Owner

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

Bartosz Blimke
Owner

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.

Jen-Mei Wu

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?

Bartosz Blimke
Owner

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

Bartosz Blimke bblimke closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 30, 2012
  1. Test for supporting various conventions for array fields, e.g. "?a[]=…

    Jen-Mei Wu & Kurt Ruppel authored
    …1&a[]=2" or "?a=1&a=2".
Commits on Jan 31, 2012
  1. URI params w/ duplicate keys were being lost on normalization. Change…

    Jen-Mei Wu & Kurt Ruppel authored
    …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").
This page is out of date. Refresh to see the latest.
25 lib/webmock/request_pattern.rb
View
@@ -106,11 +106,30 @@ def matches?(uri)
end
end
+ # Someone might want to look at URIRegexpPattern and see if this should
+ # be used there, too.
+ def convert_hash_to_array(query_params)
+ query_params.inject([]) do |memo, pair|
+ key, value = *pair
+ if value.kind_of?(Array)
+ value.each do |val|
+ memo << [key + '[]', val]
+ end
+ else
+ memo << [key + '[]', value]
+ end
+ memo
+ end
+ end
+
def add_query_params(query_params)
- if !query_params.is_a?(Hash)
- query_params = Addressable::URI.parse('?' + query_params).query_values
+ query_params = convert_hash_to_array(query_params) if query_params.is_a?(Hash)
+
+ if !query_params.is_a?(Array)
+ query_params = Addressable::URI.parse('?' + query_params).query_values(:notation => :flat_array)
end
- @pattern.query_values = (@pattern.query_values || {}).merge(query_params)
+ qv = (@pattern.query_values(:notation => :flat_array) || []) + query_params #(@pattern.query_values || {}).merge(query_params)
+ @pattern.query_values = qv.sort
end
def to_s
4 lib/webmock/util/uri.rb
View
@@ -17,7 +17,7 @@ class URI
NORMALIZED_URIS = Hash.new do |hash, uri|
normalized_uri = WebMock::Util::URI.heuristic_parse(uri)
- normalized_uri.query_values = sort_query_values(normalized_uri.query_values) if normalized_uri.query_values
+ normalized_uri.query_values = sort_query_values(normalized_uri.query_values(:notation => :flat_array)) if normalized_uri.query_values
normalized_uri = normalized_uri.normalize #normalize! is slower
normalized_uri.port = normalized_uri.inferred_port unless normalized_uri.port
hash[uri] = normalized_uri
@@ -74,7 +74,7 @@ def self.is_uri_localhost?(uri)
private
def self.sort_query_values(query_values)
- Hash[*query_values.sort.inject([]) { |values, pair| values + pair}]
+ query_values.sort
end
def self.uris_with_inferred_port_and_without(uris)
2  spec/acceptance/httpclient/httpclient_spec_helper.rb
View
@@ -9,7 +9,7 @@ def http_request(method, uri, options = {}, &block)
c.ssl_config.verify_mode = OpenSSL::SSL::VERIFY_NONE
c.set_basic_auth(nil, uri.user, uri.password) if uri.user
params = [method, "#{uri.omit(:userinfo, :query).normalize.to_s}",
- uri.query_values, options[:body], options[:headers] || {}]
+ uri.query_values(:notation => :flat_array), options[:body], options[:headers] || {}]
if HTTPClientSpecHelper.async_mode
connection = c.request_async(*params)
connection.join
23 spec/unit/util/uri_spec.rb
View
@@ -172,6 +172,29 @@
lambda { WebMock::Util::URI.normalize_uri(uri) }.should_not raise_error(ArgumentError)
end
+ context "with array parameters" do
+
+ it "should successfully handle array parameters" do
+ uri = 'http://www.example.com:80/path?a[]=b&a[]=c'
+ lambda { WebMock::Util::URI.normalize_uri(uri) }.should_not raise_error(ArgumentError)
+ end
+
+ context "in various formats" do
+ it "should be able to turn array parameters into a query string of the format ?a[]=b&a[]=c" do
+ uri_string = 'http://www.example.com:80/path?a[]=b&a[]=c'
+ uri = WebMock::Util::URI.normalize_uri(uri_string)
+ Addressable::URI.unencode(uri.query).should == "a[]=b&a[]=c"
+ end
+
+ it "should be able to turn array parameters into a query string of the format ?a=b&a=c" do
+ uri_string = 'http://www.example.com:80/path?a=b&a=c'
+ uri = WebMock::Util::URI.normalize_uri(uri_string)
+ Addressable::URI.unencode(uri.query).should == "a=b&a=c"
+ end
+ end
+
+ end
+
end
describe "stripping default port" do
Something went wrong with that request. Please try again.