smithy-lang / smithy-typescript

Smithy code generators for TypeScript. (in development)
Apache License 2.0
225 stars 84 forks source link

Validation list has wrong property name #522

Open eduardomourar opened 2 years ago

eduardomourar commented 2 years ago

Whenever we have a validation error response, the model property names are being presented but they should be the actual values that we send over the wire. In our case, we modify the properties with the @jsonName trait and that is completely ignored when generating the validation message. For instance, if you define a required property called someModelName (as name as JSON name) and then don't send it in the input. the following message is presented:

Value null at '/someModelName' failed to satisfy constraint: Member must not be null

The API user doesn't know about the name in the model, they only have access to the OpenAPI definition, which only mentions the JSON name.

adamthom-amzn commented 2 years ago

We intend for the pointer in the ValidationException to be the logical path to the member in the Smithy model, so that model-aware clients can bind the error message to UI elements in a protocol-agnostic manner. @jsonName is a serialization detail that does not apply to all protocols. In other words, the validation is applied to the object, not the payload. The same is true for validation failures for members that are bound to query string parameters or headers.

Grundlefleck commented 2 years ago

@adamthom-amzn while keeping ValidationException bound to the model, could serialization-specific code generators catch validation failures and translate before re-throwing, in order to honour the serialisation format? I recognise that would break generated clients which were not aware of the serialization.

To me staying true to the model highlights that client generation will always be prioritised over exposing the protocol+serialization directly (like an HTTP + JSON API). Is that a fair assessment? It's no criticism, I ask in order to get more aligned to Smithy's intended usage.

adamthom-amzn commented 2 years ago

Yes, Smithy's goal is to allow customers to interact with web services at a higher level of abstraction. In practice, with Smithy's predecessor, services often support multiple protocols with wildly differing semantics that can be moved between without any change to the end user's code, if that user is using a code generated client.

As a tangent, this is what I very much dislike about the REST protocols. They pull HTTP concepts into the model, making them first class citizens. It becomes very confusing to users who run services that support the same model over both an RPC and REST protocol.

I recognize that the intended user interface for some services is HTTP itself, and is common for developers who are used to OpenAPI, or are wishing to retain wire-level compatibility with an existing, non-Smithy service.

As a practical matter, by the time validation is run in our service frameworks, we have completely discarded any information about where the data comes from; it would be a pretty extreme change to reliably reinsert those details into the error.