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

How should a server reject unknown union variants? #1222

Closed
david-perez opened this issue May 13, 2022 · 9 comments
Closed

How should a server reject unknown union variants? #1222

david-perez opened this issue May 13, 2022 · 9 comments
Labels
guidance Question that needs advice or information. protocol-test New protocol tests are needed

Comments

@david-perez
Copy link
Contributor

Since enum is a constraint trait, if a server encounters an unknown enum variant, it should reject the request just like it would fail to constrain any other value for a constrained shape: that is, the server should parse the request successfully and then constrain the resulting value, returning a collection of constraint violations, if any. In the case of an unknown enum variant, one of those errors should convey that there was an unknown enum variant. Here is one protocol test in the validation suite enforcing this behavior:

https://github.com/awslabs/smithy/blob/1863d672dde488ccd81d9c00c64f57a26728f75d/smithy-aws-protocol-tests/model/restJson1/validation/malformed-enum.smithy#L17-L49

(Aside: it would be nice if the spec on constraint traits documented this server behavior instead of using the protocol tests as a normative reference).


My question is what should happen if a server encounters unknown union variants. It is not obvious, since a union is a shape itself and not a constraint trait like enum. If a server is parsing a union, it will first read the tag and match it against the known modeled tags. If it encounters an unknown tag, should it then:

  1. Immediately reject the request with a deserialization failure; or
  2. Store the unknown tag, discard the union's value (since it can't know its structure), parse the rest of the request successfuly, and insert some sort of validation error in the response, just like we should do for constraint traits?

I think the answer should be (1); it'd be nice if a test could be added to malformed-union.smithy and the spec could call this out if so.

@adamthom-amzn
Copy link
Contributor

I agree the answer should be 1.

@mtdowling
Copy link
Member

(1) would still be a 400 response in HTTP based protocols, right?

@adamthom-amzn
Copy link
Contributor

yes, SerializationException is a 400

@david-perez
Copy link
Contributor Author

david-perez commented Sep 7, 2022

I was wondering if the behavior for unknown enum variants has changed with IDL v2. In IDL v2, we have enum shapes instead of enum constraint traits, so presumably logic would dictate that they should now be handled like in the case of unknown union variants? That is a big behavioral change.

@mtdowling
Copy link
Member

Hm, no, enums are still open types. There's effectively no behavioral change, just a modeling change.

@david-perez
Copy link
Contributor Author

I'm guessing set shapes in IDL v1 should be rejected in the case of duplicate items in the same way as list shapes with uniqueItems in IDL v2 then: the request is parsed entirely and only then the check is performed. So, another modeling change, but no behavioral change in this regard.

@mtdowling
Copy link
Member

Looks like this is answered

@david-perez
Copy link
Contributor Author

Do we have a protocol test establishing this behavior? I thought that was why the label was added.

@mtdowling
Copy link
Member

Oh missed that. Pushed #2022

mtdowling added a commit that referenced this issue Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guidance Question that needs advice or information. protocol-test New protocol tests are needed
Projects
None yet
Development

No branches or pull requests

5 participants