Unexpected nested query params handling when specifying content-type

[kachayev]

The client handles nester query params properly only in the case when content-type is either not given or set to :x-www-form-urlencoded. Which is unexpected as content-type is intended to be used to determine the content of the body of the request, not the query string in the URL (here and here).

Right now:

user=> (-> (client/post "https://requestb.in/wii5hvwi" {:query-params {:name {:first "John"}}}) :body)
"ok"

As expected:

None
QUERYSTRING
name[first]: John

HEADERS
Content-Length: 0
Via: 1.1 vegur
Connect-Time: 1
Host: requestb.in
User-Agent: Apache-HttpClient/4.5.3 (Java/1.8.0_121)
Cf-Ipcountry: UA
Cf-Ray: 3e7ece1ccba8843c-KBP
Connection: close
Cf-Visitor: {"scheme":"https"}
Accept-Encoding: gzip
X-Request-Id: 173abcf6-d8ba-44ae-9660-1669a6846bb4
Total-Route-Time: 0

RAW BODY
None

But this:

user=> (-> (client/post "https://requestb.in/wii5hvwi" {:query-params {:scope {:field "user"}} :form-params {:firstName "John"} :content-type :json}) :body)
"ok"

leads to

FORM/POST PARAMETERS
None
QUERYSTRING
scope: {:field "user"}

HEADERS
Content-Length: 20
Via: 1.1 vegur
Connection: close
Content-Type: application/json
User-Agent: Apache-HttpClient/4.5.3 (Java/1.8.0_121)
Cf-Ray: 3e7ecfcd8a39844e-KBP
Cf-Visitor: {"scheme":"https"}
Accept-Encoding: gzip
X-Request-Id: 4dfd3865-ef04-4354-9be9-1099e61e39f4
Host: requestb.in
Total-Route-Time: 0
Cf-Ipcountry: UA
Connect-Time: 1

RAW BODY
{"firstName":"John"}

Note improper QUERYSTRING value.

Behavior was initially introduced here: 4044f85

Potential fixes/workarounds:

• do not check content-type when packing :query-params (only :form-params) - might break someones code
• introduce new setting :query-string-encoding to set encoding method for query string directly (independently from content-type)

@dakrone what do you think?

[dakrone]

Hmm... I'm thinking that introducing :query-string-encoding might be the best option. I think it'd be beneficial to be able to encode them differently, and it can default to whatever :content-type is set to. I'll add this

[dakrone]

Hmm actually maybe not, still looking into the best way to do this.

[dakrone]

@kachayev okay, I've pushed a small fix and test for this to master here: a6e7d66 can you take a look and let me know what you think? If you like it I'll backport to the 3.x branch.

[kachayev]

Hmmm. Not really what a user might expect still.

Let's say I have the request with the following set of params:

{:method :post
 :query-params {:name {:first "Jonh"}}
 :form-params {:name {:last "Dow"}}}

As we use :x-www-form-urlencoded as a default content type, this should be equivalent to

{:method :post
 :content-type :x-www-form-urlencoded
 :query-params {:name {:first "Jonh"}}
 :form-params {:name {:last "Dow"}}}

And I expect it to produce the request in form of

querystring: "name%5Bfirst%5D%3DJonh" (decoded: "name[first]=Jonh")
raw body: "name%5Blast%5D%3DDow" (decoded: "name[last]=Dow")

Assume we set :content-type explicitly now to :json:

{:method :post
 :content-type :json
 :query-params {:name {:first "Jonh"}}
 :form-params {:name {:last "Dow"}}}

What I expect is the following:

querystring: "name%5Bfirst%5D%3DJonh" (decoded: "name[first]=Jonh")
raw body: "{\"name\":{\"last\":\"Down\"}}" (packed using :content-type setting)

Note that :query-string is still packed as x-www-form-urlencoded with proper handling for nested params when :body is coerced to JSON as was specified by the user.

Right now the library produces different result cause of this branching. It forces appropriate middleware to leave :query-params and :form-params AS IS when content type is not :x-www-form-urlencoded, which is correct and expected behavior for :form-params, but not for :query-params (here we process them equally).

/cc @dakrone

[dakrone]

Okay, I reverted the commits that I had on master to take another look at this.

[dakrone]

@kachayev okay, I thought of a different way to handle this that hopefully works better, can you check out this branch: https://github.com/dakrone/clj-http/compare/nested-param-fix and let me know what you think? (I added documentation so hopefully it's clear enough to follow what was added/changed)

[kachayev]

@dakrone 👍 That works! Fine-grained control is always a good thing. I'm not sure if it's okay to let the user passing contradictory params. Let's say

{:ignore-nested-query-string true
 :flatten-nested-form-params false
 :flatten-nested-keys [:query-params :form-params]}

The result might lead to confusion. It's kinda clear enough from the documentation, but maybe it makes sense to throw IllegalArgumentException here, explaining to the user that request options given are somewhat ambiguous?

[dakrone]

@kachayev that's a good idea, I'll throw an exception if they are both specified.

[dakrone]

@kachayev I've merged #433 which fixes this to 3.x and 4.x, this should be available in the next release, thanks for bringing it up!

[kachayev]

@dakrone Great, thanks!