Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Add testcase to demonstrate broken exploded query parameters #113

Merged
merged 6 commits into from
Oct 31, 2023
Merged

Add testcase to demonstrate broken exploded query parameters #113

merged 6 commits into from
Oct 31, 2023

Conversation

papegaaij
Copy link
Contributor

@papegaaij papegaaij commented Oct 20, 2023 •
edited by baywet
Loading

fixes #114

@papegaaij papegaaij mentioned this pull request Oct 20, 2023
Closed
baywet
baywet requested changes Oct 20, 2023
View reviewed changes
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request. Blocking until the changes mentioned in the issue are resolved

baywet
baywet requested changes Oct 23, 2023
View reviewed changes
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the updates, a couple of additional remarks

request_information_test.go Outdated Show resolved Hide resolved
request_information.go Outdated Show resolved Hide resolved
@papegaaij
Copy link
Contributor Author

I'm currently on vacation. I'll have a look at it next week.

@papegaaij
Add testcase to demonstrate broken exploded query parameters
db50c67
@papegaaij
Change type of QueryParameters to map[string]any
999bfb1 
This fixes support for exploded query parameters. With the current
implementation of std-uritemplate, a conversion from []string to []any
is required to render the parameters correctly. Also, url encoding turns
out a bit different for the comma, but I think that's ok.
@papegaaij
Revert QueryParameters to map[string]string and add QueryParametersAny
ca104d1 
This keeps source, binary and runtime behavior in a backward compatible
way. It does however require to do some bookkeeping twice. The old
QueryParameters is still available and works in the same way. Most
values are still stored in the old QueryParameters. Only lists are also
stored in the new QueryParametersAny. Any value in QueryParametersAny
will override the value from QueryParameters.
@papegaaij
Add support for []any query parameters
ce2802a
baywet
baywet requested changes Oct 31, 2023
View reviewed changes
request_information.go Show resolved Hide resolved
baywet
baywet approved these changes Oct 31, 2023
View reviewed changes
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution and for making the changes!

@baywet baywet merged commit c6b1cb2 into microsoft:main Oct 31, 2023
7 checks passed
@papegaaij papegaaij deleted the exploded-query-params branch October 31, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baywet baywet baywet approved these changes

@andrueastman andrueastman Awaiting requested review from andrueastman

@zengin zengin Awaiting requested review from zengin

@MIchaelMainer MIchaelMainer Awaiting requested review from MIchaelMainer

@ddyett ddyett Awaiting requested review from ddyett

@rkodev rkodev Awaiting requested review from rkodev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Exploded query parameters are not working
2 participants
@papegaaij @baywet