Adjust query string parameter tests for servers #857
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
On the server side, @httpQueryParams holds all of the query parameter values in the request, so if any of the query parameters has a list value, then @httpQueryParams must target a map with a list value, otherwise a 400 will be thrown during deserialization, instead of discarding excess query param values.
Comment on lines
+108
to
+110
| queryParamsMapOfStringList: { | ||
| "QueryParamsStringKeyA": ["Foo"], | ||
| "QueryParamsStringKeyB": ["Bar"], |
There was a problem hiding this comment.
I agree that we should have this test, but why do we need to get rid of the old one?
There was a problem hiding this comment.
If @httpQueryParams is on a member, it has to be on a map of lists if any other @httpQuery member can have multiple values. So this isn't really about removing or adding tests, it's about making this test compatible with servers.
JordonPhillips
approved these changes
Jul 14, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
On the server side,
@httpQueryParamsholds all of the query parameter valuesin the request, so if any of the query parameters has a list value, then
@httpQueryParamsmust target a map with a list value, otherwise a 400 will bethrown during deserialization, instead of discarding excess query param values.
An alternative solution could be to adjust SSDK behavior where
@httpQueryParamswould only receive query parameter values that were not otherwise explicitly bound to a structure member.Prior art in this area is for the service to truncate to the last specified query parameter value when binding all query parameters to a map of strings, and since that's not at all a desirable behavior, and this doesn't change what gets put on the wire, we have considerable leeway to choose the behavior we want.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.