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.

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

Fix RestJsonZeroAndFalseQueryValues protocol test #2167

Merged

Conversation

milesziemer
Copy link
Contributor

Background

Intially added in #2070, RestJsonZeroAndFalseQueryValues didn't include the correct params that would be deserialized by a server. Specifically it omitted the @httpQueryParams member queryParamsMapOfStringList. This member was added back in #2132, but the map keys corresponded to member names in the input struct, rather than the literal query parameter keys. This meant that clients running this protocol test would actually have a query string of

?Integer=0&Boolean=false&queryInteger=0&queryBoolean=false

instead of the intended

?Integer=0&Boolean=false

(forbidQueryParams wasn't set in the protocol test, so clients wouldn't fail here).
servers would fail this test because they'd be expecting to get

{
    queryInteger: 0,
    queryBoolean: false,
    queryParamsMapOfStringList: {
        queryInteger: ["0"],
        queryBoolean: ["false"]
    }
}

from a query string of

?Integer=0&Boolean=false

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Intially added in smithy-lang#2070,
`RestJsonZeroAndFalseQueryValues` didn't include the correct params
that would be deserialized by a server. Specifically it omitted the
`@httpQueryParams` member `queryParamsMapOfStringList`. This member
was added back in smithy-lang#2132,
but the map keys corresponded to member names in the input struct,
rather than the literal query parameter keys. This meant that
_clients_ running this protocol test would actually have a query
string of
```
?Integer=0&Boolean=false&queryInteger=0&queryBoolean=false
```
instead of the intended
```
?Integer=0&Boolean=false
```
(`forbidQueryParams` wasn't set in the protocol test, so clients
wouldn't fail here).
_servers_ would fail this test because they'd be expecting to get
```
{
    queryInteger: 0,
    queryBoolean: false,
    queryParamsMapOfStringList: {
        queryInteger: ["0"],
        queryBoolean: ["false"]
    }
}
```
from a query string of
```
?Integer=0&Boolean=false
```
@milesziemer milesziemer requested a review from a team as a code owner February 27, 2024 21:24
@milesziemer milesziemer requested a review from syall February 27, 2024 21:24
milesziemer added a commit to milesziemer/aws-sdk-js-v3 that referenced this pull request Feb 27, 2024
The smithy test is wrong and will be fixed by:
smithy-lang/smithy#2167
In the meantime, this commit ignores the invalid test.
milesziemer added a commit to milesziemer/aws-sdk-js-v3 that referenced this pull request Feb 27, 2024
The test is wrong and will be fixed by
smithy-lang/smithy#2167
In the meantime, this commit ignores the invalid test.
milesziemer added a commit to milesziemer/aws-sdk-js-v3 that referenced this pull request Feb 28, 2024
The test is wrong and will be fixed by
smithy-lang/smithy#2167
In the meantime, this commit ignores the invalid test.
@milesziemer milesziemer merged commit 73936a8 into smithy-lang:main Mar 5, 2024
10 checks passed
milesziemer added a commit to milesziemer/smithy-rs that referenced this pull request Mar 5, 2024
A few protocol tests that were added to smithy-rs have been added
to smithy upstream, so can be removed. Some smithy-rs protocol
tests were also not being generated because they weren't attached
to the model, which has been fixed. One of these test cases,
RestJsonZeroAndFalseQueryValuesFixed, was also fixed in the same
manner as smithy-lang/smithy#2167.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants