Keep query values data type when sorting #318

Merged
merged 13 commits into from Sep 27, 2014

5 participants

@tjsousa

Query values can be an array when the :flat_array notation option is used on the query mapper, so this conversion to hash is removing repeated query params (which can exist).

This is a fix for bblimke/webmock#227

@tjsousa tjsousa Keep query values data type when sorting
Query values can be an array when the :flat_array notation option is used on the query mapper, so this conversion to hash is removing repeated query params (which can exist).

This is a fix for bblimke/webmock#227
2bf5f5c
@bblimke
Owner

Thank you for this fix!

How can one set flat_array notation on query mapper if WebMock api doesn't offer this option?

In order to pull this request without any additional work on my side, I'd need unit test + acceptance test.
I can work on it on my own, when I have time, but not sure when that will be :)

@tjsousa

Only after reading sporkmonger/addressable#77 did I understand the query mapper in use was borrowed from a previous version of Addressable (which provided the option), for backward compatibility reasons.

I now added a commit with WebMock configuration variable that you can set globally in your code, or when using allow_net_connect! and disable_net_connect! as an option.

I had to dig this rabbit hole but I'm not really into the details of WebMock, so is this the proper way of doing it?

@bblimke
Owner

Adding this option to allow_net_connect and disable_net_connect doesn't make sense, as it's not related to them in any way. I'd add a separate method to the api.

@bblimke
Owner

@tjsousa do you plan any more work on this pull request or is it complete?

@tjsousa

It was meant to be for up review again with those last two commits passing on Travis CI.

At the moment, I don't have much time for additional work on this one. Do you think it is useful for now?

@lengarvey

This particular issue affects me with a 3rd party API I have to work with. The API accepts duplicate querystring parameters. I'd appreciate if this could be merged in and am happy to assist in any way if this is needed. Right now I'm pointing my Gemfile to the fixed version from @tjsousa

@jure

Spent an hour trying to figure out why the URL WebMock is reporting does not contain my query parameters.

This is the URL (note the multiple occurences of fq):

http://api.plos.org/search?fq=cross_published_journal_key%3APLoSCompBiol&q=everything%3Abiology&fq=doc_type:full&fq=!article_type_facet:%22Issue%20Image%22&fl=id,pmid,publication_date,received_date,accepted_date,title,cross_published_journal_name,author_display,editor_display,article_type,affiliate,subject,financial_disclosure&wt=json&facet=false&rows=25&hl=false

These are the parameters with CGI.parse (note the array for fq param):

CGI.parse(URI.parse(a).query)
=> {"fq"=>["cross_published_journal_key:PLoSCompBiol", "doc_type:full", "!article_type_facet:\"Issue Image\""],
 "q"=>["everything:biology"],
 "fl"=>["id,pmid,publication_date,received_date,accepted_date,title,cross_published_journal_name,author_display,editor_display,article_type,affiliate,subject,financial_disclosure"],
 "wt"=>["json"],
 "facet"=>["false"],
 "rows"=>["25"],
 "hl"=>["false"]}

And this is the URL that WebMock is reporting (note the single occurence of fq param):

WebMock::NetConnectNotAllowedError: Real HTTP connections are disabled. Unregistered request: GET http://api.plos.org/search?facet=false&fl=id,pmid,publication_date,received_date,accepted_date,title,cross_published_journal_name,author_display,editor_display,article_type,affiliate,subject,financial_disclosure&fq=!article_type_facet:%22Issue%20Image%22&hl=false&q=everything:biology&rows=25&wt=json with headers {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Host'=>'api.plos.org', 'User-Agent'=>'Ruby'}

What is the main reason this doesn't work out of the box and does this PR address it? Is it something that would possibly introduce a backwards incompatibility, perhaps for people who have stubbed their request based on what WebMock said, which is in fact an incomplete URL as their URL contains multiple parameters with the same name, and now these stubs would suddenly start failing?

Anyway, I'd be interested in doing any additional work that needs to be done to get this merged.

@jure jure added a commit to lagotto/alm-report that referenced this pull request Sep 20, 2014
@jure jure Add a spec for limiting results to specific journals with PLOS's API,…
… and then because of the way this works, i.e. using multiple instances of the fq parameter, going through and fixing all existing PLOS Solr specs/mocks, because WebMock does not support multiple parameters with the same name (bblimke/webmock#318 (comment)).
b9b4bd7
@bblimke bblimke merged commit 870464e into bblimke:master Sep 27, 2014

1 check passed

Details default The Travis CI build passed
@bblimke
Owner

@jure "What is the main reason this doesn't work out of the box" - the default notation used to serialize/deserialize urls is subscript and it doesn't support duplicate parameters names.

The one that supports it :flat_array, is incompatible with :subscript

The changes are now merged. You can do the following to support duplicate keys in parameters.

WebMock::Config.instance.query_values_notation = :flat_array
@jure

Fantastic! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment