Request with query containing array is incorrectly altered

[weppos]

I have a test that is failing because the expectation can't be properly verified. The request has a query that contains a value with an indexed array:

"http://example.test:80/call.cgi?street%5B0%5D=1234+SW+1st+Street&street%5B1%5D=Apt+3&title=Dog+Walker"

Basically the parameter street is passed as it follows:

Faraday.get("http://example.test:80/call.cgi", "street[0]" => "foo", "street[1]" => "bar")

However, when the request is stored in the registry, it is altered and the values 0 and 1 in street[X] are stripped and replaced with simply street[].

This is the portion of code where the issue happens:

        if normalized_uri.query_values
          sorted_query_values = sort_query_values(WebMock::Util::QueryMapper.query_to_values(normalized_uri.query, notation: Config.instance.query_values_notation) || {})
          normalized_uri.query = WebMock::Util::QueryMapper.values_to_query(sorted_query_values, notation: WebMock::Config.instance.query_values_notation)
        end

specifically, the bug is caused by query_to_values and values_to_query:

(byebug) WebMock::Util::QueryMapper.query_to_values(normalized_uri.query)
{"street"=>["1234 SW 1st Street", "Apt 3"], "title"=>"Dog Walker"}
(byebug) WebMock::Util::QueryMapper.values_to_query(WebMock::Util::QueryMapper.query_to_values(normalized_uri.query))
"street%5B%5D=1234%20SW%201st%20Street&street%5B%5D=Apt%203&title=Dog%20Walker"

It should be

(byebug) WebMock::Util::QueryMapper.values_to_query(WebMock::Util::QueryMapper.query_to_values(normalized_uri.query))
"street%5B0%5D=1234%20SW%201st%20Street&street%5B1%5D=Apt%203&title=Dog%20Walker"

[bblimke]

@weppos thank you for reporting the issue.

Could you please let me know what is the issue you are experiencing as a side effect of that behaviour of the request registry? Is that lack of precision when matching executed request or that they don't match at all?

Request registry stores normalized query params. There are different ways that query params, representing an array can be serialized. There is no RFC standard that defines that. WebMock had to choose one.
This is required to allow matching request url with params against a request pattern with query params defined as a hash or when query params have different order than in the pattern.

Saying above, perhaps WebMock should offer some way persisting the original url and allowing matching requests with request pattern, only when url string is exactly the same.

[aeden]

@bblimke the issue manifests in an expectation failure:

expect(WebMock).to have_requested(:get, "http://example.test/")
           .with(query: hash_including(params))

The request GET http://example.test/ with query params hash_including({"city"=>"Miami", "command"=>"AddContact", "country"=>"US", "email"=>"anthony@example.com", "fax"=>"+1.4445556666", "first_name"=>"Anthony", "last_name"=>"Eden", "name"=>"Anthony Eden", "organization"=>"Dot Dog Walker", "phone"=>"+1.5554443333", "state"=>"FL", "street[0]"=>"1234 SW 1st Street", "street[1]"=>"Apt 3", "title"=>"Dog Walker", "zip"=>"12454"}) was expected to execute 1 time but it executed 0 times

The following requests were made:

GET http://example.test/?city=Miami&command=AddContact&country=US&email=anthony@example.com&fax=%2B1.4445556666&first_name=Anthony&last_name=Eden&name=Anthony%20Eden&organization=Dot%20Dog%20Walker&phone=%2B1.5554443333&s_login=test-username&s_pw=test-password&state=FL&street%5B%5D=1234%20SW%201st%20Street&street%5B%5D=Apt%203&title=Dog%20Walker&zip=12454 with headers {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'User-Agent'=>'Faraday v0.15.2'} was made 1 time

[bblimke]

@aeden does it help if you set WebMock::Config.instance.query_values_notation = :flat_array?

[aeden]

does it help if you set WebMock::Config.instance.query_values_notation = :flat_array?

I tried setting this directly before the expectation call, but it does not appear to have an impact. The expectation still fails the same way as before.

[bblimke]

@aeden thanks. Does it match if you change the query params to "street"=>["1234 SW 1st Street", "Apt 3"]?

[aeden]

Does it match if you change the query params to "street"=>["1234 SW 1st Street", "Apt 3"]?

If I remove the WebMock::Config.instance.query_values_notation = :flat_array option and I change the query params to match "street" to an array, then it passes.

[andrisbriedis]

Looks like I am also affected by this issue.
My tests are working against Shopify REST API. Particularty this call.
In my test I want to verify, that products in the collection were sorted in the right order, but because of this automatic parameter sortnig my tests are faiilng, even though the passed in product ids are in the right order. Any suggestions?

[kgeoir]

Hello,

I got a similar issue with query and array:
`"args[:url]"
"http://localhost:3001/api/v1/?%7B%22msg_id%22%3A%222aa8916%22%2C%22payload%22%3A%7B%7D%2C%22history%22%3A%5B%7B%22src%22%3Anull%2C%22dest%22%3Anull%2C%22status_code%22%3Anull%2C%22success%22%3Anull%2C%22error%22%3Anull%2C%22ts_starts%22%3A1571392569%2C%22ts_spent%22%3Anull%7D%5D%7D"
"uri:"
#<URI::HTTP http://localhost:3001/api/v1/?%7B%22msg_id%22%3A%222aa8916%22%2C%22payload%22%3A%7B%7D%2C%22history%22%3A%5B%7B%22src%22%3Anull%2C%22dest%22%3Anull%2C%22status_code%22%3Anull%2C%22success%22%3Anull%2C%22error%22%3Anull%2C%22ts_starts%22%3A1571392569%2C%22ts_spent%22%3Anull%7D%5D%7D>
"==============> uri.query:"
{"msg_id"=>"2aa8916",
"payload"=>{},
"history"=>
[{"src"=>nil,
"dest"=>nil,
"status_code"=>nil,
"success"=>nil,
"error"=>nil,
"ts_starts"=>1571392569,
"ts_spent"=>nil}]}
">>>>>>>>>>>>>>>>>>>>>> IN REQUEST <<<<<<<<<<<<<<<<<<<<<<<<<"
"rq:"
#<WebMock::RequestSignature:0x00007f9a25535270
@Body=nil,
@headers=
{"Accept"=>"/",
"User-Agent"=>"rest-client/2.1.0 (darwin18.2.0 x86_64) ruby/2.6.1p33",
"Accept-Encoding"=>"gzip;q=1.0,deflate;q=0.6,identity;q=0.3",
"Host"=>"localhost:3001"},
@method=:get,
@uri=
#<Addressable::URI:0x3fcd12ad68fc URI:http://localhost:3001/api/v1/?%7B%22msg_id%22:%222aa8916%22,%22payload%22:%7B%7D,%22history%22:%5B%7B%22src%22:null,%22dest%22:null,%22status_code%22:null,%22success%22:null,%22error%22:null,%22ts_starts%22:1571392569,%22ts_spent%22:null%7D%7D%5D>>
"rq.uri: "
#<Addressable::URI:0x3fcd12ad68fc URI:http://localhost:3001/api/v1/?%7B%22msg_id%22:%222aa8916%22,%22payload%22:%7B%7D,%22history%22:%5B%7B%22src%22:null,%22dest%22:null,%22status_code%22:null,%22success%22:null,%22error%22:null,%22ts_starts%22:1571392569,%22ts_spent%22:null%7D%7D%5D>
F

Failures:

1. EvsClient Sending a simple service to service call should handle history
   Failure/Error: msg = EvsService::Message.deserialize(JSON.parse(URI.unescape(rq.uri.query)) || {})

   JSON::ParserError:
   416: unexpected token at '}]'`

When I create the query I am using:
uri.query = URI.encode_www_form(params) args[:url] = uri.to_s pp "args[:url]", args[:url] pp "uri:", uri pp "==============> uri.query:", JSON.parse(URI.unescape(uri.query))
So my params are correctly setup and the end of the url is %7D%5D%7D But once I arrive in my stubed request I got: %7D%7D%5D.... Therefor my JSON is not valide anymore.

WebMock::Config.instance.query_values_notation = :flat_array Seems to fix it, I guess the issue is somewhere in the deep array encoding/decoding

[surkova]

I will chime in and bump this issue. I'm query Gmail API, you can fetch message with format metadata and specify headers as an array. Only last element of this array appears in the normalized URI (I assume, that's because values_to_query calls .to_hash).

[bblimke]

@surkova could you please provide an example? Is there a problem with matching query or headers in your case?

[surkova]

@bblimke I have a screenshot illustrating the problem. I took it while in the debugger navigating inside query_mapper. This resulted in that I couldn't pass in query params in any form (I tried list of tuples) into stub_request, so I ended up doing this:

    # Note that only the last value of metadataHeaders is enough to make a match.
    query_string = "?fields=sizeEstimate,id,payload&format=metadata&metadataHeaders=Reply-To&quotaUser=#{@account.id}"
    url += query_string
    stub_request(:get, url).to_return(
      status: 200,
      body: fixture,
      headers: {
        'Content-Type' => 'application/json',
      }
    )

The request in my case is generated by https://github.com/googleapis/google-api-ruby-client and it sends metadataHeaders=From&metadataHeaders=Reply-To in the query string.

[bblimke]

@surkova thank you for clarifying. have you tried setting WebMock::Config.instance.query_values_notation = :flat_array?

[surkova]

@bblimke I just tried this, it worked:

    url = "https://www.googleapis.com/gmail/v1/users/#{google_user_id}/messages/#{message_id}"
    WebMock::Config.instance.query_values_notation = :flat_array
    query_string = "fields=sizeEstimate,id,payload&format=metadata&metadataHeaders=From&metadataHeaders=Reply-To&quotaUser=#{@account.id}"
    stub_request(:get, url).with(query: query_string).to_return(
      status: 200,
      body: fixture,
      headers: {
        'Content-Type' => 'application/json',
      }
    )

I haven't found any examples on how to specify query as an actual array of tuples, only this test case. I'd assume I'd need to loop and join array elements myself.

Thank you for pointing out this solution. I'd assume that keeping repeated query params is the default behavior.

[bblimke]

@surkova I agree it's confusing. It should be at least mentioned in README.

[surkova]

In this test case the config is changed for all tests in the context. Would it be possible to change config directly on stub_request?

[bblimke]

@surkova it's not possible. I think it will also be a pain to know when to use flat array and when not to, therefore it would be better to either allow WebMock to do that automatically or find a way to avid various notations completely.
What negative side effects do you see in your other tests, when flat_array is enabled globally?

[surkova]

@bblimke we noticed random test failures when I introduced :flat_array, the weird thing was that after a number of runs the tests would go through, then fail again (mismatched stub). We never actually found out the root cause, unfortunately (no time for this atm).

My expectation from the library would be the following: the library should always assume there can be duplicate keys in the query string if query is supplied as an array of tuples or a string and handles that on the fly without adding any particular settings in the config.

[bblimke]

@surkova

the library should always assume there can be duplicate keys in the query

I agree. The reason is the representation of query parameters as a hash, same limitation as e.g Rails has when it comes to parameters handling. It's unlikely this is going to get fixed soon, unless someone will be willing to spend time on rebuilding this part of WebMock.
The only thing I can suggest for now is to enable :flat_array only for the specs with duplicate params.