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

Server enums should not have Unknown variants #1187

Closed
david-perez opened this issue Feb 14, 2022 · 4 comments · Fixed by #1398
Closed

Server enums should not have Unknown variants #1187

david-perez opened this issue Feb 14, 2022 · 4 comments · Fixed by #1398
Assignees
Labels
server Rust server SDK

Comments

@david-perez
Copy link
Contributor

When generating enums for the server, we're generating an Unknown variant.

https://github.com/awslabs/smithy-rs/blob/548f6ed4307faae5bb98e431db96ae90a59e489d/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/EnumGenerator.kt#L154-L155

Server enums should not contain this variant. This also means we can no longer impl From<&str> for MyEnum, since From is required to never fail. We should instead implement TryFrom. This also means deserialization code will be affected, since deserializing a &str into an enum can now fail.

@david-perez david-perez added the server Rust server SDK label Feb 14, 2022
@llgerard
Copy link

Strong enums are a big reason for me to use Rust when writing server code (no need to handle corner cases all the time).

Not having Unknown on the server side would require to do a big refactor similar to #1243 ?

@david-perez
Copy link
Contributor Author

Not having Unknown on the server side would require to do a big refactor similar to #1243 ?

@llgerard I don't think so. We should be able to bubble up from the code path that creates the UnknownVariant, treat it as an error in deserialization code and reject the request.

@guymguym
Copy link
Contributor

@david-perez Should a server be able to hook into this error case of unknown enums? A service might need to specify which error to return in each case. Functionally-wise, a specific error could make the difference between a client receiving a generic "Bad request" error, vs. getting a specific "InvalidXXX" error.

Just one quick example from the top of my mind is the InvalidStorageClass returned by few S3 operations when an invalid storage class enum is provided inside an input structure (both as header and xml).

        "com.amazonaws.s3#StorageClass": {
            "type": "string",
            "traits": {
                "smithy.api#enum": [
                    {
                        "value": "STANDARD",
                        "name": "STANDARD"
                    },
                    {
                        "value": "REDUCED_REDUNDANCY",
                        "name": "REDUCED_REDUNDANCY"
                    },
                    ...
                    ...
                ]
            }
        },

@david-perez
Copy link
Contributor Author

@guymguym I think yes. Ideally, users should be able to opt into hooking into errors resulting from constraint traint violations, and map them to modeled errors.

I was thinking of doing something like this: #1199 (comment)

If smithy-build.json has disableDefaultValidation set to true, code generation adds a required parameter to handlers for a callback that can turn ValidationFailure[] into an error (or undefined, if it wants to suppress the error)

We're still far away from implementing such hooks, since constraint traits (besides required and enum) are not yet implemented.

@82marbag 82marbag self-assigned this May 10, 2022
82marbag pushed a commit that referenced this issue May 18, 2022
The server must have the most up to date variants and the unknown enum
variant should not be used. Clients are generated with it because they
might not have the most recent model and the server might return
an unknown variant to them.

Closes #1187

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
82marbag pushed a commit that referenced this issue May 18, 2022
The server must have the most up to date variants and the unknown enum
variant should not be used. Clients are generated with it because they
might not have the most recent model and the server might return
an unknown variant to them.

Closes #1187

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
82marbag pushed a commit that referenced this issue May 20, 2022
The server must have the most up to date variants and the unknown enum
variant should not be used. Clients are generated with it because they
might not have the most recent model and the server might return
an unknown variant to them.

Closes #1187

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
crisidev added a commit that referenced this issue May 23, 2022
The server must have the most up to date variants and the unknown enum
variant should not be used. Clients are generated with it because they
might not have the most recent model and the server might return
an unknown variant to them.

Closes #1187

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>

Co-authored-by: Daniele Ahmed <ahmeddan@amazon.de>
Co-authored-by: david-perez <d@vidp.dev>
Co-authored-by: Matteo Bigoi <1781140+crisidev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Rust server SDK
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants