Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

allow nested array query params #629

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

GordianNaught
Copy link

The only issue I foresee is the handling of empty dictionary query params.
I'm assuming, based on the lack of test coverage, that this isn't a real use case?

@dakrone
Copy link
Owner

dakrone commented Jan 26, 2023

@GordianNaught it looks like this causes a number of test failures (with lein test :all), with regard to changed multi-value query params:

lein test :only clj-http.test.client-test/multi-valued-query-params

FAIL in (multi-valued-query-params) (client_test.clj:1698)
multi-valued query params in indexed-style
a[1]=2&a[2]=3&b[0]=x&b[2]=z&a[0]=1&b[1]=y
expected: (.contains query-string "b[0]=x&b[1]=y&b[2]=z")
  actual: false

(It also changes things with empty offsets like a[]=1&a[]=2)

@GordianNaught
Copy link
Author

I just looked at those tests.
I have to make the wrap-nested-params middleware aware of the multi-param-style and the encoding.
I can do that, but this gets wonky pretty quickly as I can't recur on multi-param-entries.
As it is currently written, multi-param-entries will not look deeper into the nested structure.
This looks like it will require a larger refactor than I was anticipating to support these other parameter styles.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants