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

Error struct member with default implementation cause rs clients to fail on build #3182

Closed
codypenta opened this issue Nov 11, 2023 · 3 comments

Comments

@codypenta
Copy link
Contributor

When an error member has a default value, it will generate errors with smithy-rs client unless you manually specify @required

For example:

// Valid Smithy, fails client creation
@error("client")
structure ReservedGroupUsedError {
    message: String = "Cannot use a reserved group name or any variation of"
}
@error("client")
structure ReservedGroupUsedError {
    @required // This makes the client render again
    message: String = "Cannot use a reserved group name or any variation of"
}

I think the code is question is here

I cloned and made this edit below and it worked for me

// If the message member is optional and wasn't set, we set a generic error message.
if (errorMessageMember != null) {
    if (errorMessageMember.isOptional && !errorMessageMember.hasNonNullDefault()) {
        rust(
            """
            if tmp.message.is_none() {
                tmp.message = _error_message;
            }
            """,
        )
    }
}

Happy to make a PR if this is the right approach

@jdisanti
Copy link
Collaborator

To better assess the proposed change, can you provide what the error was before the change?

@codypenta
Copy link
Contributor Author

codypenta commented Nov 13, 2023

            if tmp.message.is_none() {
                tmp.message = _error_message;
            }

In the rust code this would throw is_none() does not exist on type String. And I think there was some code try accessing it as a String but it was an Option<String>

@jdisanti
Copy link
Collaborator

I see. I think the proposed change will fix it in some cases, but potentially not all. To make it work in all cases, the NullableIndex must be used to determine if it's wrapped in an Option or not, or better yet, check if the member's symbol is marked as optional (which is determined by the NullableIndex earlier in the codegen). Something like:

if (errorMessageMember != null) {
    val symbol = symbolProvider.toSymbol(errorMessageMember)
    if (symbol.is_optional()) {
        ...

Feel free to open a PR.

github-merge-queue bot pushed a commit that referenced this issue Nov 14, 2023
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->

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
<!--- Describe your changes in detail -->

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

```kotlin
if (errorMessageMember != null) {
    val symbol = symbolProvider.toSymbol(errorMessageMember)
    if (symbol.isOptional()) {
        rust(
            """
            if tmp.message.is_none() {
                tmp.message = _error_message;
            }
            """,
        )
    }
}
..... 
```


```smithy
@error("client")
structure Error {
    @required
    requestId: String

    @required
    message: String

    code: String = "400"

    context: String
}
```

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->

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

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] 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._

---------

Co-authored-by: John DiSanti <john@vinylsquid.com>
@jdisanti jdisanti closed this as completed Apr 5, 2024
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

No branches or pull requests

2 participants