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

add support for nullable struct members when generating AWS SDKs #2916

Merged
merged 43 commits into from
Sep 21, 2023
Merged

add support for nullable struct members when generating AWS SDKs #2916

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
9c17050
add support for nullable struct members when generating AWS SDKs
Velfi Aug 10, 2023
b1c7ec5
add CHANGELOG.next.toml entries
Velfi Aug 10, 2023
0f27839
fix SDK adhoc tests config generation
Velfi Aug 10, 2023
81f428b
fix XML parseStructure
Velfi Aug 10, 2023
633c257
Merge branch 'main' into zhessler-nullability-now!
Velfi Aug 14, 2023
266f54f
fix server codegen issue
Velfi Aug 14, 2023
86dc322
remove TODO
Velfi Aug 14, 2023
a5565a3
update route53 resource ID trimmer
Velfi Aug 14, 2023
2d20462
add back the prefix loop that I accidentally removed
Velfi Aug 14, 2023
acff7bd
more fixing for route53
Velfi Aug 14, 2023
976f3a2
fix query serializer generator ref
Velfi Aug 14, 2023
397c4ab
fix the QuerySerializerGenerator correctly
Velfi Aug 14, 2023
80bb768
fix idempotency token generation
Velfi Aug 15, 2023
a44ae99
no really, fix idempotency token generation
Velfi Aug 15, 2023
916265a
fix serde bugs
Velfi Aug 16, 2023
761c1e4
fix more tests
Velfi Aug 16, 2023
36b7127
fix server client and eventstream stuff
Velfi Aug 16, 2023
3cf8288
Merge branch 'main' into zhessler-nullability-now!
Velfi Aug 16, 2023
7e464cc
remove insurmountable comma
Velfi Aug 16, 2023
d2b2c55
fix another server example
Velfi Aug 16, 2023
505c778
Merge branch 'main' into zhessler-nullability-now!
Velfi Aug 17, 2023
f0e4a1f
fix gradle build config typo
Velfi Aug 17, 2023
ae6a636
Update codegen-client/src/main/kotlin/software/amazon/smithy/rust/cod…
Velfi Aug 17, 2023
491ca43
respond to PR comments
Velfi Aug 17, 2023
66fbddb
Merge branch 'main' into zhessler-nullability-now!
Velfi Aug 17, 2023
c4b9958
add missing author to changelog
Velfi Aug 17, 2023
dac4679
update CHANGELOG
Velfi Aug 17, 2023
b0c63c9
Merge branch 'main' of github.com:awslabs/smithy-rs into zhessler-nul…
rcoh Sep 11, 2023
5e6f6b9
Fix endpoints decorator test
rcoh Sep 12, 2023
94c8dbb
Create ClientBuilderInstatiator & manuall create BuildError
rcoh Sep 12, 2023
cd5e33c
Update ChangeLog
rcoh Sep 12, 2023
1374e95
Fix server
rcoh Sep 12, 2023
e661075
Merge branch 'main' of github.com:awslabs/smithy-rs into zhessler-nul…
rcoh Sep 20, 2023
09c02f7
Fix unit tests
rcoh Sep 20, 2023
b417977
Add a default instantiator that works for tests
rcoh Sep 20, 2023
0bffcc0
add protocol test for error correction & nullability
rcoh Sep 20, 2023
eee07db
Merge branch 'main' of github.com:awslabs/smithy-rs into zhessler-nul…
rcoh Sep 20, 2023
58cf184
Add nullability test and fix some bugs
rcoh Sep 20, 2023
7a4fd41
A few more fixes
rcoh Sep 21, 2023
e75ad16
wip
rcoh Sep 21, 2023
99de898
Fix request id accessor when it is marked as required
rcoh Sep 21, 2023
969e32f
Refactor && remove println
rcoh Sep 21, 2023
7de1892
Fix unit test & add an ignore for useless question mark
rcoh Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...t/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/ClientRustSettings.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ data class ClientCodegenConfig(
ClientCodegenConfig(
formatTimeoutSeconds = coreCodegenConfig.formatTimeoutSeconds,
debugMode = coreCodegenConfig.debugMode,
generateOptionsForRequiredShapes = node.get().getBooleanMemberOrDefault("generateOptionsForRequiredShapes", defaultGenerateOptionsForRequiredShapes),
generateOptionsForRequiredShapes = coreCodegenConfig.generateOptionsForRequiredShapes,
)
}
}
Expand Down
24 changes: 14 additions & 10 deletions ...e/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ProtocolParserGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,15 +137,15 @@ class ProtocolParserGenerator(
withBlock("Err(match error_code {", "})") {
val errors = operationShape.operationErrors(model)
errors.forEach { error ->
val errorShape = model.expectShape(error.id, software.amazon.smithy.model.shapes.StructureShape::class.java)
val errorShape = model.expectShape(error.id, StructureShape::class.java)
val variantName = symbolProvider.toSymbol(model.expectShape(error.id)).name
val errorCode = httpBindingResolver.errorCode(errorShape).dq()
withBlock(
"$errorCode => #1T::$variantName({",
"}),",
errorSymbol,
) {
software.amazon.smithy.rust.codegen.core.rustlang.Attribute.AllowUnusedMut.render(this)
Attribute.AllowUnusedMut.render(this)
assignment("mut tmp") {
rustBlock("") {
renderShapeParser(
Expand All @@ -166,14 +166,18 @@ class ProtocolParserGenerator(
)
}
}
if (errorShape.errorMessageMember() != null) {
rust(
"""
if tmp.message.is_none() {
tmp.message = _error_message;
}
""",
)
val errorMessageMember = errorShape.errorMessageMember()
// If the message member is optional and wasn't set, we set a generic error message.
jdisanti marked this conversation as resolved.
Show resolved Hide resolved
if (errorMessageMember != null) {
if (errorMessageMember.isOptional) {
rust(
"""
if tmp.message.is_none() {
tmp.message = _error_message;
}
""",
)
}
Comment on lines +161 to +172
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this dropping it if it's not optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT, this code is meant to set an error message. When the message is optional and wasn't provided, we set a generic message. This code isn't relevant when the message is required, as we can count on one being included in the response.

}
rust("tmp")
}
Expand Down
16 changes: 13 additions & 3 deletions ...in/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,10 +509,20 @@ class JsonParserGenerator(
"Builder" to symbolProvider.symbolForBuilder(shape),
)
deserializeStructInner(shape.members())
if (hasFallibleBuilder(shape, symbolProvider)) {
rust("Ok(Some(builder.build()?))")

// TODO the need for this check should be eliminated or the server code should be updated.
if (codegenContext.target == CodegenTarget.SERVER) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's unfortunate that I put so much effort into making the core classes agnostic from whether we were generating a client or a server when I implemented constraint traits, only to have these introduced again. I think it's best we make the core classes discriminate based on features that each artifact type has rather than on the artifacts themselves. To that end, a suggestion could be to pass to JsonParserGenerator a function that, given a StructureShape, returns the code to finalize the builder binding. That logic could be contained in the two builder generators (BuilderGenerator, ServerBuilderGenerator), and there we could make the checks to hasFallibleBuilder in the client or returnSymbolToParse in the server.

if (returnSymbolToParse.isUnconstrained) {
rust("Ok(Some(builder))")
} else {
rust("Ok(Some(builder.build()))")
}
} else {
rust("Ok(Some(builder.build()))")
if (hasFallibleBuilder(shape, symbolProvider)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can be if (hasFaillibleBuilder || !returnSymbolToParse.isUnconstrained)?

Alternatively, can we push returnSymbolToParse as an argument to hasFallibleBuilder? That might help us make sure we caught all of these locations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how the logic can be simplified that way. I also don't see how passing returnSymbolToParse as an argument to hasFallibleBuilder simplifies things. returnSymbolToParse is about whether we should parse a value for a shape into its associated unconstrained type. In the client this is always false because it has no notion of constraint traits.

rust("Ok(Some(builder.build()?))")
} else {
rust("Ok(Some(builder.build()))")
}
}
}
}
Expand Down