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

Deprecate sets in favor of uniqueItems #1278

Merged
merged 1 commit into from
Jun 21, 2022
Merged

Deprecate sets in favor of uniqueItems #1278

merged 1 commit into from
Jun 21, 2022

Conversation

mtdowling
Copy link
Member

@mtdowling mtdowling commented Jun 17, 2022

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".

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.

@mtdowling mtdowling marked this pull request as ready for review June 17, 2022 16:37
@mtdowling mtdowling requested a review from a team as a code owner June 17, 2022 16:37
@mtdowling mtdowling force-pushed the deprecate-sets branch 2 times, most recently from 33f67c1 to 84981ff Compare June 19, 2022 22:58
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".
@mtdowling mtdowling merged commit a7a16df into main Jun 21, 2022
@gosar gosar mentioned this pull request Jun 29, 2022
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.
@mtdowling mtdowling deleted the deprecate-sets branch July 29, 2022 23:12
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.

2 participants