smithy-lang / smithy

Smithy is a protocol-agnostic interface definition language and set of tools for generating clients, servers, and documentation for any programming language.
https://smithy.io
Apache License 2.0
1.7k stars 201 forks source link

Allow invalid params values in @smithy.test#smokeTests #2282

Closed robin-aws closed 1 day ago

robin-aws commented 1 month ago

@smokeTests is a great testing trait that should apply to my use case nicely as well (https://github.com/awslabs/smithy-dafny), but I had planned to use it to check that validation is happening correctly, and it turns out I can't do that because the trait validation asserts that params satisfies the traits like @required and constraint traits like @range on the operation's input shape, and the raised validation events are at the ERROR severity level so I can't suppress them.

I'd propose that the validation be loosened to only assert that the params node types are compatible with the input shape of the operation.

Two more minor points I'd like to address if possible:

I'm willing to cut a PR if we agree on a solution. :)

JordonPhillips commented 2 weeks ago

Smoke tests aren't intended to test client serialization (or deserialization), they're intended to make connections to live services to be a sanity check that the service and client are interacting properly.

Additionally, clients shouldn't be doing validation of anything but types because client-side validation is forwards-incompatible. For an example of this, look to the difficulty around EC2's increased instance id size several years ago. A number of clients had been making assertions that instance ids were a particular size (which was modeled iirc), and they all broke when EC2 started returning ids that were longer. EC2 had to spend years warning people and migrating.

With regards to binary data, this is more of a limitation in Smithy itself, since there's no way in the IDL or the AST to indicate binary data. Unfortunately, changing the way nodes are parsed to expect binary data to be base64 or similarly encoded would be a breaking change.