-
Notifications
You must be signed in to change notification settings - Fork 221
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
Deprecate sets in favor of uniqueItems #1278
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
c5cdc3f
to
b7a9abb
Compare
kstich
approved these changes
Jun 17, 2022
kstich
reviewed
Jun 17, 2022
...test/resources/software/amazon/smithy/model/errorfiles/validators/unique-items-floats.errors
Outdated
Show resolved
Hide resolved
33f67c1
to
84981ff
Compare
This commit is a reversal of a previous decision to deprecate uniqueItems. This reversal is due to an observation that sets aren't all that useful or desirable for client/server scenarios, which is the primary use case for Smithy. First, we added a requirement that sets maintain insertion order. This was to guarantee that validation errors returned by servers are actionable by clients. If a client and server don't agree on what element `0` means, then a client doesn't know which value in their unordered set is incorrect. Next, we limited what kinds of values can be stored in a set because some types like float makes equality checks in languages like Rust difficult. We also realized that clients with imperfect knowledge of a model (which is basically all clients) often drop unknown structure members. If a server sends two structures to a client and one of the structures uses a member the client doesn't know about, the client will drop the member, and then drop the second structure because it considers it a duplicate of the first. So at this point, we realized that sets are more confusing than useful. The ideal state is that clients ignore unique-ness constraints and leave that to servers to perform as _validation_ and not as part of the type system. It turns out, that's exactly the use case of the @Uniqueitems trait. So instead of removing the @Uniqueitems trait, we're doubling down on it. The trait can once again target any kind of type (except for float, double, and document), we're defining how to determine equality, order is implicit because the trait targets a list, and the fact that this is _validation_ and not type refinement is implicit because it is a "constraint trait".
84981ff
to
93577c9
Compare
kstich
approved these changes
Jun 21, 2022
Merged
gosar
added a commit
to gosar/smithy
that referenced
this pull request
Jun 30, 2022
malformed-sets tests were changed to malformed-uniqueitems as part of smithy-lang#1278. But since uniqueItems is constraint trait, as opposed to set being a type, these should be ValidationException responses not SerializationException. Looks like we don't already have uniqueItems protocol tests in smithy-aws-protocol-tests/model/restJson1/validation. They should be added. For now, removing these tests to unblock smithy 1.22.0 release.
gosar
added a commit
that referenced
this pull request
Jun 30, 2022
malformed-sets tests were changed to malformed-uniqueitems as part of #1278. But since uniqueItems is constraint trait, as opposed to set being a type, these should be ValidationException responses not SerializationException. Looks like we don't already have uniqueItems protocol tests in smithy-aws-protocol-tests/model/restJson1/validation. They should be added. For now, removing these tests to unblock smithy 1.22.0 release.
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit is a reversal of a previous decision to deprecate
uniqueItems. This reversal is due to an observation that sets
aren't all that useful or desirable for client/server scenarios,
which is the primary use case for Smithy.
First, we added a requirement that sets maintain insertion order.
This was to guarantee that validation errors returned by servers
are actionable by clients. If a client and server don't agree on
what element
0
means, then a client doesn't know which value intheir unordered set is incorrect.
Next, we limited what kinds of values can be stored in a set because
some types like float makes equality checks in languages like Rust
difficult. We also realized that clients with imperfect knowledge
of a model (which is basically all clients) often drop unknown
structure members. If a server sends two structures to a client
and one of the structures uses a member the client doesn't know
about, the client will drop the member, and then drop the second
structure because it considers it a duplicate of the first.
So at this point, we realized that sets are more confusing than
useful. The ideal state is that clients ignore unique-ness
constraints and leave that to servers to perform as validation
and not as part of the type system. It turns out, that's exactly
the use case of the @Uniqueitems trait.
So instead of removing the @Uniqueitems trait, we're doubling
down on it. The trait can once again target any kind of type
(except for float, double, and document), we're defining how to
determine equality, order is implicit because the trait targets a
list, and the fact that this is validation and not type
refinement is implicit because it is a "constraint trait".
Sets will be removed from IDL v2.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.