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 errors for unions with unit target membershape #3547

Merged
merged 5 commits into from
Apr 9, 2024

Conversation

drganjoo
Copy link
Contributor

@drganjoo drganjoo commented Apr 3, 2024

Motivation and Context

Unions that have a unit target member shape do not have an associated data in the generated Rust enum.

Closes Issue: 2546

Description

On the server side, when the union has constrained members, the code generated for the conversion from the Unconstrained type to the Constrained type incorrectly assumed that each Rust enum would have associated data.

rust-server-codegen/src/unconstrained.rs:31:129
  |
  |               crate::unconstrained::some_union_with_unit_unconstrained::SomeUnionWithUnitUnconstrained::Option1(unconstrained) => Self::Option1(
    |                                                                                                                                   -^^^^^^^^^^^^- help: consider using a semicolon here to finish the statement: `;`
    |  _________________________________________________________________________________________________________________________________|
  | |
  | |                 unconstrained
  | |             ),
  | |_____________- call expression requires function
    |
   ::: rust-server-codegen/src/model.rs:152:5
    |
    |       Option1,
    |       ------- `SomeUnionWithUnit::Option1` defined here

The marshaling code for event streams with unit target types incorrectly assumed that the variant would have associated data.

rust-server-codegen/src/event_stream_serde.rs

impl ::aws_smithy_eventstream::frame::MarshallMessage for TestEventMarshaller {
        fn marshal() {
            let payload = match input {
                Self::Input::KeepAlive(inner) => {

On the client side, the event_stream_serde code incorrectly assumes that a union member, which has the @streaming trait applied to it, takes a model::Unit type.

rust-client-codegen/src/event_stream_serde.rs:
    |
    |                     crate::types::TestEvent::KeepAlive(crate::types::Unit::builder().build()),
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------------------------------------
    |                     |
    |                     call expression requires function
    |
   ::: rust-client-codegen/src/types/_test_event.rs
    |
    |     KeepAlive,
    |     --------- `TestEvent::KeepAlive` defined here

Testing

A unit test has been added that tests the following model:

            $version: "2"
            namespace com.example
            use aws.protocols#restJson1
            use smithy.framework#ValidationException
            
            @restJson1 @title("Test Service") 
            service TestService { 
                version: "0.1", 
                operations: [ 
                    TestOperation
                    TestSimpleUnionWithUnit
                ] 
            }
            
            @http(uri: "/testunit", method: "POST")
            operation TestSimpleUnionWithUnit {
                input := {
                    @required
                    request: SomeUnionWithUnit
                }
                output := {
                    result : SomeUnionWithUnit
                }
                errors: [
                    ValidationException
                ]
            }
            
            @length(min: 13)
            string StringRestricted
            
            union SomeUnionWithUnit {
                Option1: Unit
                Option2: StringRestricted
            }

            @http(uri: "/test", method: "POST")
            operation TestOperation {
                input := { payload: String }
                output := {
                    @httpPayload
                    events: TestEvent
                },
                errors: [ValidationException]
            }
            
            @streaming
            union TestEvent {
                KeepAlive: Unit,
                Response: TestResponseEvent,
            }
            
            structure TestResponseEvent { 
                data: String 
            }            

Copy link

github-actions bot commented Apr 3, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

@drganjoo drganjoo force-pushed the fahadzub/union-unit branch 3 times, most recently from d6c6cb8 to 70ab968 Compare April 3, 2024 17:23
Copy link

github-actions bot commented Apr 3, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

@drganjoo drganjoo force-pushed the fahadzub/union-unit branch from 70ab968 to 24ef5a7 Compare April 3, 2024 22:36
Copy link

github-actions bot commented Apr 3, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

@drganjoo drganjoo marked this pull request as ready for review April 4, 2024 10:46
@drganjoo drganjoo requested review from a team as code owners April 4, 2024 10:46
}
""".asSmithyModel()

serverIntegrationTest(model) { _, rustCrate ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why we need the tests below if all we want to check is that the model compiles correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that we can extend the same test to RpcV2Cbor (and possibly other protocols). Since event streams are not covered by the protocol tests, and none of the other tests were failing, I assume we do not have a test that actually checks whether data is being correctly serialized/deserialized. However, having said that, I think it is better to add comprehensive tests that use a client to serialize and then use the server to deserialize it. Currently, these tests directly use the inner data types/methods rather than invoking the server. Will remove these tests for now and we can add one in v2Cbor.

@david-perez
Copy link
Contributor

Closes #2546.

@drganjoo drganjoo force-pushed the fahadzub/union-unit branch from 2be83a3 to 03059e4 Compare April 4, 2024 13:07
Copy link

github-actions bot commented Apr 4, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

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

Reviewed primarily those in codegen-core

Copy link

github-actions bot commented Apr 8, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

@drganjoo drganjoo enabled auto-merge April 8, 2024 15:45
Copy link

github-actions bot commented Apr 9, 2024

A new generated diff is ready to view.

A new doc preview is ready to view.

@drganjoo drganjoo added this pull request to the merge queue Apr 9, 2024
Merged via the queue into main with commit 64b9b91 Apr 9, 2024
44 checks passed
@drganjoo drganjoo deleted the fahadzub/union-unit branch April 9, 2024 16:27
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