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

Support for IDLv2 nullability semantics #1767

Closed
sugmanue opened this issue Sep 23, 2022 · 7 comments
Closed

Support for IDLv2 nullability semantics #1767

sugmanue opened this issue Sep 23, 2022 · 7 comments
Assignees
Milestone

Comments

@sugmanue
Copy link
Contributor

In order to be able to take full advantage of the new nullability semantics in IDLv2 the clients needs to have a mechanism to keep track whether or not a member with a default value has been updated, quoting from the Defaults and Model Evolution document

To allow servers to change default values if necessary, clients SHOULD NOT serialize default values unless the member is explicitly set to the default value. This implies that clients SHOULD implement a kind of "presence tracking" of defaulted members.

@Velfi Velfi added this to the SDK GA milestone Sep 26, 2022
@jnicholls
Copy link

jnicholls commented Sep 28, 2022

I believe this is being tracked in another issue, and it is certainly on the roadmap to match v2 semantics in their entirety before a 1.0 release.

@jdisanti
Copy link
Collaborator

jdisanti commented Oct 6, 2022

I believe this is being tracked in another issue, and it is certainly on the roadmap to match v2 semantics in their entirety before a 1.0 release.

Yeah, there is a SDK issue for it: awslabs/aws-sdk-rust#536

I'm going to leave this open to track implementation in smithy-rs.

@jdisanti
Copy link
Collaborator

jdisanti commented Oct 6, 2022

I think we need to make some code generator changes to StructureGenerator and BuilderGenerator to fully support the new nullability semantics. I generated a new SDK with the latest IDLv2 SDK models and manually inspected the codegen diff looking for changes in optionality, and there was none.

I wrote a quick test in StructureGeneratorTest which fails:

    @Test
    fun `IDLv2 client @required fields are not options`() {
        val model = """
            namespace com.test
            structure MyStruct {
               @required
               foo: String,
               @required
               baz: Integer,
            }
        """.asSmithyModel(smithyVersion = "2.0")
        val struct = model.lookup<StructureShape>("com.test#MyStruct")

        val provider = testSymbolProvider(model)
        val project = TestWorkspace.testProject(provider)

        project.useShapeWriter(inner) { writer ->
            writer.withModule("model") {
                StructureGenerator(model, provider, writer, struct).render()
            }

            writer.rustBlock("fn check_optionality(s: &MyStruct)") {
                rust(
                    """
                    let _: &String = &s.foo;
                    let _: &i32 = &s.baz;
                    """,
                )
            }
        }
        project.compileAndTest()
    }

The SymbolVisitor is coded up to use Smithy's NullabilityIndex to correctly determine if a shape should be optional or not, but it seems like something is getting lost in translation between that decision and the actual code generation for it.

Edit: Actually, everything is wired up correctly, and I managed to get it to code generate without Option by changing the CheckMode from CLIENT_ZERO_VALUE_V1 to CLIENT_CAREFUL. I'm not familiar with the reason we're using CLIENT_ZERO_VALUE_V1, or what is required to "upgrade" to CLIENT_CAREFUL, so I need to talk with the Smithy folks to get more context.

@Velfi
Copy link
Contributor

Velfi commented Oct 31, 2022

We're still waiting on some internal developments before we can begin work on this issue.

@jnicholls
Copy link

This appears to be assigned to Zelda now. Is this now scheduled for work on the smithy side? Really looking forward to this update, thank you!

@Velfi
Copy link
Contributor

Velfi commented Mar 27, 2023

I'm aiming to get to it in Q2, but who can know what the future holds?

@david-perez
Copy link
Contributor

Regarding the interaction of @input/@output with structure member optionality, the below is something we should address when landing this.

Structure member optionality says in the @required row, "Non-authoritative" column:

Present unless also @clientOptional or part of an @input structure

Not all structures used as operation inputs are marked as @input in Smithy:

operation MyOperation {
   input: SharedStruct
}

structure SharedStruct {
    @required
    member: String
}

So a smithy-rs user with the above model would hope that this would get synthesized as:

struct SharedStruct {
    member: String
}

However, we unconditionally apply the OperationNormalizer transform. This effectively adds a MyOperationInput structure shape to the model tagged with @input:

operation MyOperation {
   input: MyOperationInput
}

@input
structure MyOperationInput {
    @required
    member: String
}

Hence the cell in that table says we should render this as:

struct MyOperationInput {
    member: Option<String>
}

And that will upset smithy-rs users, because from their perspective they rightfully think that they should get a non Optional field there.

@goabv goabv assigned rcoh and unassigned Velfi Sep 12, 2023
github-merge-queue bot pushed a commit that referenced this issue Sep 20, 2023
## Motivation and Context
To implement #1767 we need to support error-correction to default values
when instantiating builders.

-
https://smithy.io/2.0/spec/aggregate-types.html?highlight=error%20correction#client-error-correction
## Description
Adds `pub(crate) correct_errors_<shape>` method that will be used in
deserialization to set default values for required fields when not set
in the serialized response. This only applies to client via
`ClientBuilderInstantiator`

## Testing
- added a new test that's fairly exhaustive

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] 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._
@rcoh rcoh closed this as completed Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants