-
Notifications
You must be signed in to change notification settings - Fork 193
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
Upgrade Smithy to 1.50.0 #3728
Upgrade Smithy to 1.50.0 #3728
Conversation
A new generated diff is ready to view.
A new doc preview is ready to view. |
This commit disables the said tests, assuming that the reason for failures is the same as those disabled for AwsJson1_0. Should verify with the server team.
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
@@ -141,12 +138,15 @@ class ServerProtocolTestGenerator( | |||
// These tests are broken because they are missing a target header. | |||
FailingTest.RequestTest(AWS_JSON_10, "AwsJson10ServerPopulatesNestedDefaultsWhenMissingInRequestBody"), | |||
FailingTest.RequestTest(AWS_JSON_10, "AwsJson10ServerPopulatesDefaultsWhenMissingInRequestBody"), | |||
FailingTest.RequestTest(REST_JSON, "RestJsonServerPopulatesDefaultsWhenMissingInRequestBody"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RestJsonServerPopulatesDefaultsWhenMissingInRequestBody
fails because defaultNullDocument
has a null
default, so the hasNonNullDefault
check here:
Line 537 in 9af72f5
if (member.hasNonNullDefault()) { |
is false
and so aws_smithy_types::Document::Null
is not used by the instantiator.
But even if we improved the check and special-cased document
member shapes, there's an additional problem: the same check is used by Smithy itself here when determining whether the structure member should be optional or not. Since the node for the @default
value of the member shape is null
, Smithy says the structure member should be optional. But the generated code for default value setting expects the member to be non-Option
al, since there's a default value.
In general, this begs the question of whether a required (non-Option
al) document
shape makes sense at all. The user can always set it to aws_smithy_types::Document::Null
and convey the same semantics as if shape were nullable and None
had been set for it. To belabor the point, consider this model:
structure OperationInput {
@required
document: Document
}
This gets generated in Rust as:
struct OperationInput {
document: aws_smithy_types::Document
}
The user can instantiate this as:
let op_input = OperationInput::builder()
.document(aws_smithy_types::Document::Null)
.build();
Which will get serialized on the wire as { document: null }
or { }
, depending on whether the protocol serializes nulls or not, respectively. Either way, we're allowing the user to not provide a value for a required member shape, violating the model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed explanation! Moved the test to the bottom of the list and added a link to this discussion for more context.
.../amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt
Show resolved
Hide resolved
A new generated diff is ready to view.
A new doc preview is ready to view. |
This commit adds a link to a discussion: #3728 (comment) on why the test fails and the test itself might be questionable
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
.../amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt
Show resolved
Hide resolved
This commit addresses #3728 (comment)
A new generated diff is ready to view.
A new doc preview is ready to view. |
Motivation and Context
This PR upgrades Smithy to 1.50.0. The majority of the changes follow
TODO
added in #3690. Other than that, a few adjustments needed to be made:RestJsonClientPopulatesDefaultValuesInInput
andRestJsonClientUsesExplicitlyProvidedMemberValuesOverDefaults
to known failing tests for the same reason hererest-xml-extras.smithy
sinceRestXmlMustSupportParametersInContentType
is now available upstream Smithy 1.50.0awsJson1_0
counterparts are already in the list, but we need the server team to verify this assumption & provide additionalTODO
comments if necessary)RestJsonServerPopulatesDefaultsWhenMissingInRequestBody
RestJsonServerPopulatesDefaultsInResponseWhenMissingInParams
,RestJsonServerPopulatesNestedDefaultValuesWhenMissingInInResponseParams
Testing
Existing tests in CI
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.