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 error struct with default impl #3190

Merged
merged 5 commits into from
Nov 14, 2023
Merged

Fix error struct with default impl #3190

merged 5 commits into from
Nov 14, 2023

Conversation

codypenta
Copy link
Contributor

Motivation and Context

When an error member has a default value, it will generate errors (specifically is_none not found on type String) with smithy-rs client unless you manually specify @required

More Here: #3182

Description

Adds recomendation change to address error structures with a default implementation like so:

if (errorMessageMember != null) {
    val symbol = symbolProvider.toSymbol(errorMessageMember)
    if (symbol.isOptional()) {
        rust(
            """
            if tmp.message.is_none() {
                tmp.message = _error_message;
            }
            """,
        )
    }
}
..... 
@error("client")
structure Error {
    @required
    requestId: String

    @required
    message: String

    code: String = "400"

    context: String
}

Testing

Added the above and ran ./gradlew codegen-client-test:build with a build successful

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codypenta codypenta requested review from a team as code owners November 13, 2023 21:01
@codypenta
Copy link
Contributor Author

@jdisanti let me know if I need to make any changes, pretty new to the code base 😄

Copy link
Collaborator

@jdisanti jdisanti 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 taking the time to do this! ❤️

Just a couple minor things.

@@ -27,6 +27,10 @@ structure Error {

@required
message: String

code: String = "400"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for thinking to add a test!

It would be nice for this to get its own isolated test case though so that it doesn't get lost in the future.

Would you mind creating a ProtocolParserGeneratorTest.kt file in src/test/kotlin/... same path as ProtocolParserGenerator .../ that tests this in isolation? Here's an example of how to do a client integration test that runs the full client code generator against a test model and verifies it compiles:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure can, would you like me to also revert the changes made in the current file or should I leave that in?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, please revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and done 😄

CHANGELOG.next.toml Outdated Show resolved Hide resolved
CHANGELOG.next.toml Outdated Show resolved Hide resolved
@rcoh rcoh enabled auto-merge November 14, 2023 20:55
@rcoh rcoh added this pull request to the merge queue Nov 14, 2023
Merged via the queue into smithy-lang:main with commit 4128662 Nov 14, 2023
38 of 39 checks passed
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.

3 participants