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

No protocol tests for @uniqueItems #1541

Closed
david-perez opened this issue Dec 15, 2022 · 2 comments
Closed

No protocol tests for @uniqueItems #1541

david-perez opened this issue Dec 15, 2022 · 2 comments
Labels
protocol-test New protocol tests are needed

Comments

@david-perez
Copy link
Contributor

The repository used to contain protocol tests for set shapes e.g.:

  • RestJsonMalformedSetDuplicateItems
  • RestJsonMalformedSetNullItem
  • RestJsonMalformedSetDuplicateBlobs

In a7a16df, sets were deprecated in favor of lists with @uniqueItems, and the name of these tests changed.

In July, in 1b5737a, the tests were removed cc @gosar:

For now, removing these tests to unblock smithy 1.22.0 release.

Re-adding the tests would be nice to ensure smithy-rs is compliant (e.g. same validation error message).

As an aside, shouldn't we also add protocol tests for set shapes, not only list shapes with @uniqueItems? While deprecated in IDL v2, users can still use them. In addition, don't we still have to support IDL v1? In which case we need to ensure we return SerializationExceptions if the model was an IDL v1 model, and ValidationException in case it was an IDL v2 (?). Alternatively, since in smithy-rs we've never supported @uniqueItems or the semantics of set shapes:

  1. we could abort if we detect a set shape in an IDL v1 model when running the server SDK generator, and instruct the user to update their model.
  2. we could apply a model transformation to convert set shapes to list shapes with @uniqueItems, and print a warning saying we will be returning ValidationException.

Guidance is appreciated on this.

@mtdowling
Copy link
Member

Yeah, let's add uniqueItems tests.

I don't think we need set tests though. Sets were completely removed in IDL v2. The Rust code generator should treat sets from IDL v1 exactly like a list + uniqueItems, and that's easy to do since a uniqueItems trait is added to every IDL v1 set shape automatically (we document that in the 1.0 spec too).

@srchase srchase added the protocol-test New protocol tests are needed label Dec 30, 2022
@srchase
Copy link
Contributor

srchase commented Feb 27, 2023

Validation protocol tests for @uniqueItems were added in #1622.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol-test New protocol tests are needed
Projects
None yet
Development

No branches or pull requests

3 participants