Add server SDK constraint traits RFC

[david-perez]

Rendered.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

A new doc preview is ready to view.

[github-actions]

A new generated diff is ready to view.

• No codegen difference in the AWS SDK
• No codegen difference in the Server Test

[crisidev]

For the sake of security I think we should bail out as soon as we find the first validation error.

[adamthom-amzn]

I have the opposite opinion. Validation doesn't protect the parser (you've already parsed), and it's extremely useful to applications that are gathering user input, such as web forms, to indicate all invalid inputs so that they can be fixed by the user at once. This is why the ValidationException built into Smithy has the structure that it does.

[jonhoo]

I would recommend having a customizeable max-limit on the number of validation errors that can be encountered before an early return, with the option to set it to None or something for no limit.

[crisidev]

Thanks for the doc Adam, i understand the reasoning and I agree with you and Jon..

[crisidev]

Should we really care about this? At the end the server is the authority and we do not validate outputs..

[david-perez]

At the very least I'd consider it to likely be an implementation error from the user with regards to what they modeled, and as such performing validation and logging warnings would help them identify it. So we should care, but very little.

[llgerard]

As a service owner you probably do care that your outputs do not match your model: your server is breaking the contract you want to set with your clients.
So being able to have clear metrics about it seems important. Consistent logging would allow that.

[crisidev]

This is clever, I like it!

[crisidev]

I think the first option follows a better approach in terms of design, but I would be fine also doing 2. if we find problems with 1. Both options solve the problem in a good way :)

[Velfi]

Consider using RangeInclusive::contains here

[Velfi]

Good catch! Unicode is hard.

[jonhoo]

I don't know that chars().count() actually gives you what you want. Specifically, I'm not actually positive that char == code point. From memory, char is actually a "Unicode Scalar Value", which is subtly different from a Unicode code point. See also burntsushi's excellent write-up here: BurntSushi/aho-corasick#72 (comment). Although it could be that you're right, an .chars() actually iterates over code points. But worth double-checking.

[david-perez]

Unicode scalar values are Unicode Code Points minus the "surrogate pairs range" from D800 to DFFF which is only used in UTF-16. Since in Rust strings are UTF-8 encoded we can't count those. See also smithy-lang/smithy#1089

[Velfi]

It might be too much information, but I always prefer errors that explain not only what the invalid value was, but what counts as a valid value.

[Velfi]

Nevermind, I see in the next paragraph that you say you'll do this.

[Velfi]

once_cell is now preferred to lazy_static

[jonhoo]

Came here to say exactly this 👍

[jonhoo]

Also, the link in the RFC text is broken.

[Velfi]

Personally, I'm all for using the num crate in situations like this.

[david-perez]

What specifically would you use from the num crate?

[jonhoo]

My experience has been that the num crate introduces more problems than it solves. I don't think it gives us functionality that'd be super relevant here.

[jonhoo]

I don't know that chars().count() actually gives you what you want. Specifically, I'm not actually positive that char == code point. From memory, char is actually a "Unicode Scalar Value", which is subtly different from a Unicode code point. See also burntsushi's excellent write-up here: BurntSushi/aho-corasick#72 (comment). Although it could be that you're right, an .chars() actually iterates over code points. But worth double-checking.

[jonhoo]

These don't render right. The <> bits need to be in backtics, otherwise they're treated as custom HTML elements.

[jonhoo]

I think the name here may need to be more complex if it's possible to have multiple constraints of the same type. For example:

@length(min: 1)
@length(max: 69)

This would (presumably) make two variants, one for each constraint, and both of them are length constraints. This may also be helpful given, as you say later, that we may want different templates for different configurations of a given constraint.

[david-perez]

I didn't see it documented in the spec so I tested it; Smithy does not allow you to specify the same constraint more than once on the same shape.

ERROR: com.amazonaws.simple#HeaderBoundData (Model)
     @ /local/home/davidpz/workplace/smithy-ws/src/SmithyRsSource/codegen-server-test/model/simple.smithy
     |
  29 | @length(min: 1)
     |         ^
     = Conflicting `smithy.api#length` trait found on shape `com.amazonaws.simple#HeaderBoundData`. The previous trait was defined at `/local/home/davidpz/workplace/smithy-ws/src/SmithyRsSource/codegen-server-test/model/simple.smithy [28, 9]`, and a conflicting trait was defined at `/local/home/davidpz/workplace/smithy-ws/src/SmithyRsSource/codegen-server-test/model/simple.smithy [29, 9]`.

[jonhoo]

I'm not sure I follow the motivation for doing the deserialization in two steps rather than one? It can easily lead to excessive allocations/heap pressure if the conversions don't take care to re-use previous allocations.

[david-perez]

How can we do it in one step? Ultimately the wrapper tuple struct holds e.g. a String, so don't we first have to deserialize the bytes slice into a String?

[jonhoo]

I thought we specifically couldn't use len for String?

[david-perez]

😅

[jonhoo]

I like this idea, but it does mean that if the constraints change, the representation of the type also changes, and the server code may need to be adjusted accordingly to work with the new types. If we're okay with a constraint change leading to required changes to user (server) code, then 👍

[david-perez]

Very good point. I'm of the opinion that if you add / remove a constraint, you're backwards-incompatibly affecting your service's behavior, so a backwards-incompatible change in server user code seems appropriate.

[adamthom-amzn]

I think that's a mostly okay line of thinking but it does imply that a model is written by a service team for a service they own, and in practice that is not the only way models are written. A common pattern has been that one team writes a common service interface that is then vended to many unrelated teams, who write service implementations that conform to this specification. For these scenarios, model updates resulting in code generation changes are much more similar to what you would model as client code generation.

[david-perez]

Isn't it in the best interest of the teams that implement the service to be alerted if the model to which they must conform is going to change backwards-incompatibly? Breaking their build is the best kind of notification :)

Although I'll concede that if they have their integer types explicitly typed in many signatures that they eventually use to construct the constrained tuple struct, a change from e.g. i32 to u32 might incur unnecessary churn.

[adamthom-amzn]

Yeah, I think breaking the build is fine if a constraint has been tightened, but if it's been loosened, that's really unfair. In my opinion, the types in the target language should be derived solely from the smithy types, and they shouldn't be impacted by the constraint traits outside of @required/@default.

[oddg]

Would it be possible to regard those "custom Rust features" as Smithy traits that live outside of the official Smithy implementation?

For example one could decide to create the following trait

@trait(selector: "integer")
structure rustInteger {
      @required
      int: String
}

@rustInteger("u8")
Integer MyInteger

and them modify smithy-rs to support that rustInteger trait?

That would allow a team using Rust to apply custom traits to a model shared with other team via the external trait application.

[david-perez]

@oddg Certainly, you can do all sorts of custom traits in Smithy. I think I like more the idea of having an @unsigned trait for this.

---

One thing to note is that if we were to generate unsigned types we would be expanding the range of accepted values by Smithy. For example, an integer shape in Smithy is a 32-bit integer with range from -2^31 to (2^31)-1. That's exactly the range of i32 in Rust. If we tack on @unsigned or @range(min: 0) to the shape, it's unclear that it's too helpful to generate an u32, because the upper bound of that type is (2^32)-1. The wrapper tuple MyInteger(u32) struct would only have to check the upper bound.

I'm now inclined to drop this feature and have range on integers not affect whether we generate signed or unsigned integer types.

[rcoh]

if you wanted to, you could impl an .unsigned() method when the model allowed to enable converting from i32 to u32 fearlessly

[jdisanti]

Worth mentioning that the clients should also support this if the server does, or there will be issues when a server responds with something outside of the signed int range.

[jonhoo]

I would recommend having a customizeable max-limit on the number of validation errors that can be encountered before an early return, with the option to set it to None or something for no limit.

[jonhoo]

I think the answer to this is definitely yes. Even just for testing, having the ability to create invalid values is invaluable, as long as it's behind a controlled interface. I agree though that we probably don't want to encourage its use for transient values.

[david-perez]

What (testing) scenarios do you think would make this feature valuable?

[jonhoo]

How?

[david-perez]

The wrapper types that we're generating are not particularly complex so the memory layout could be predictable, so I was thinking that a sufficiently motivated user that wanted to bypass validation could use std::mem::transmute.

[jonhoo]

I prefer the primary design, though with the addendum that I think we should provide conversions back to the primitive type. Something like fn into_unconstrained.

[david-perez]

Good point, need to add this to the doc. I was thinking of into_inner but into_unconstrained is more explicit.

[oddg]

I also prefer the primary design as it fosters correctness.

[adamthom-amzn]

Preceding frameworks have allowed validation to be disabled or customized - from message all the way up to a completely different error. If a service implementer wanted to take control of the display of validation failures without opting out of the Rust SSDK validation framework entirely, how would they do so?

[david-perez]

A completely different error might be very challenging, since the error struct provided / configured by the user would have to compile against everything else. A plugin system of some sorts would have to be built.

As for customizing the format of error messages, that's something we could add support for. I'd imagine the user would have to provide custom templates in the configuration of our plugin in smithy-build.json (?)

[adamthom-amzn]

This is the sort of thing that is much easier to do if you don't bake validation into the type system - in TS, handlers can be configured to accept a callback that is passed an array of ValidationFailure objects, and the user can turn those into an error of their choice that is then rendered by the framework. They still get the benefit of validation without giving up the presentation of modeled exceptions coming from their service.

Which reminds me of another point - we have largely done away with synthetic exceptions in Smithy. Validation is no difference - are you requiring that a service's operations be associated with a particular error structure?

[david-perez]

in TS, handlers can be configured to accept a callback that is passed an array of ValidationFailure objects, and the user can turn those into an error of their choice that is then rendered by the framework.

Ah, I hadn't thought about that idea; I had imagined they'd want the errors to be configured ahead-of-time, not at runtime. That's neat. I don't see why we would not be able to build it into the Rust framework. We could have users return structs (or functions that return structs) that implement std::error::Error (or some trait of our own) for each of the ValidationFailures in that callback, and we would use those representations when rendering the response.

Which reminds me of another point - we have largely done away with synthetic exceptions in Smithy.

What are synthetic exceptions?

are you requiring that a service's operations be associated with a particular error structure?

If the operation in the Smithy model is fallible (has an errors field), we require that the operation handlers return a Result<T, E>, where the error has to be the enum that we generate holding all the variants of their Smithy errors field.

[adamthom-amzn]

Synthetic exceptions are an idea from a legacy framework - exceptions that every service throws that don't have to be declared in the model. In that framework, ValidationException is synthetic, and can always be thrown.

In SSDKs, we'd like to restrict that idea to only things that are unavoidably thrown, either by virtue of "this is how programming works" (SerializationException, InternalFailure) or the underlying protocol (incorrect values for Accept, etc).

In TS, you have two options:

• If every operation in the service has smithy.framework#ValidationException attached, and no customization is done in smithy-build.json, then the default validation behavior is engaged and the handlers cannot customize error rendering.
• If smithy-build.json has disableDefaultValidation set to true, code generation adds a required parameter to handlers for a callback that can turn ValidationFailure[] into an error (or undefined, if it wants to suppress the error)

If you don't attach smithy.framework#ValidationException to every operation in the service and you don't disable default validation in smithy-build, then code generation fails.

All this to say that you have to guarantee your validation failures result in modeled exceptions being rendered, and not unmodeled 400s.

[llgerard]

+1 for having the option to provide the function doing ValidationFailure[] -> CustomErrorAlwaysMappedTo400

[oddg]

you have to guarantee your validation failures result in modeled exceptions being rendered, and not unmodeled 400s.

I agree. Otherwise the server implementation breaks the Smithy contract. (I am actually curious about the current behavior if the input does not parse at all.)

As you mentioned, one approach to let the user choose which error the operation will return is to generate for each operation input a type for validation error and ask the user to provide a function mapping this type to the operation error type.

Taking the weather forecast example it would look like.

    enum GetCityInputError {
        CityId(CityIdValidationError)
    }

    enum CityIdValidationError {
        Pattern(String)
    }

A user would have to implement a mapping fn(GetCityInputError) -> GetCityError.

[david-perez]

@adamthom-amzn I didn't know about ValidationException, I don't see it documented in the docsite. Is the intention for this shape to be special, to be the default way for service frameworks to report validation errors?

adds a required parameter to handlers for a callback that can turn ValidationFailure[] into an error

I'm assuming this error is modeled and associated to the operation. If the operation in the model does not define at least one error, code generation fails, right?

---

@oddg Would that mapping be static per operation (i.e. two identical invalid requests resulting in the same GetCityError)? Would it be a simple mapping (i.e. no access to state)?

If I were to implement this, I'm thinking that if default validation is disabled (e.g. in smithy-build.json) we could generate the operation registry to take in both the operation handler and the error mapping function per operation registration. Any other ideas as to where the user could provide the error mapping function?

[adamthom-amzn]

Is the intention for this shape to be special, to be the default way for service frameworks to report validation errors?

Special? No, we're trying not to have anything be special. Think of it as a built-in convenience that service frameworks are free to use as a default :)

I'm assuming this error is modeled and associated to the operation. If the operation in the model does not define at least one error, code generation fails, right?

This would be a fine behavior to have. Another is to just use your normal error serialization logic, which should turn any exception that is not associated with the operation into an InternalFailure.

I am actually curious about the current behavior if the input does not parse at all.

@oddg by spec, here meaning protocol tests, unparseable input should result in a 400 with the error code of SerializationException. It's a synthetic, which as I outlined above, means non-modeled (and does not need to have a corresponding specialized type generated client-side).

[oddg]

Would that mapping be static per operation (i.e. two identical invalid requests resulting in the same GetCityError)? Would it be a simple mapping (i.e. no access to state)?

Yes. In my opinion, it makes sense that two identical invalid requests result in the same errors. I agree that it should not depend on any state. (Any error that is is state dependent should be generated in the core implementation of the handler for that operation.)

We could generate the operation registry to take in both the operation handler and the error mapping function per operation registration.

That makes a lot of sense to me.

by spec, here meaning protocol tests, unparseable input should result in a 400 with the error code of SerializationException. It's a synthetic, which as I outlined above, means non-modeled (and does not need to have a corresponding specialized type generated client-side).

Thanks.

[oddg]

Is it worth mentioning that the enum trait is also enforced by smithy-rs server?

[oddg]

you have to guarantee your validation failures result in modeled exceptions being rendered, and not unmodeled 400s.

I agree. Otherwise the server implementation breaks the Smithy contract. (I am actually curious about the current behavior if the input does not parse at all.)

As you mentioned, one approach to let the user choose which error the operation will return is to generate for each operation input a type for validation error and ask the user to provide a function mapping this type to the operation error type.

Taking the weather forecast example it would look like.

    enum GetCityInputError {
        CityId(CityIdValidationError)
    }

    enum CityIdValidationError {
        Pattern(String)
    }

A user would have to implement a mapping fn(GetCityInputError) -> GetCityError.

[oddg]

I also prefer the primary design as it fosters correctness.

[oddg]

Would it be possible to regard those "custom Rust features" as Smithy traits that live outside of the official Smithy implementation?

For example one could decide to create the following trait

@trait(selector: "integer")
structure rustInteger {
      @required
      int: String
}

@rustInteger("u8")
Integer MyInteger

and them modify smithy-rs to support that rustInteger trait?

That would allow a team using Rust to apply custom traits to a model shared with other team via the external trait application.

[jdisanti]

Will there be a way to hook these different failures for metrics so that service owners can discern between failure to parse and failure to validate in their metrics?

[david-perez]

Yes; the latter will be Deserialize rejections containing ConstraintViolation rejections, the former will contain other inner rejections, so it will be possible to distinguish.

[rcoh]

it's worth calling out that adding a constraint trait is now a breaking change for servers since it adds this type wrapper. I haven't thought about the trade off deeply, but it seems worth mentioning / considering since it will make it a bit more of a hassle to add constraints retroactively

[rcoh]

if you wanted to, you could impl an .unsigned() method when the model allowed to enable converting from i32 to u32 fearlessly

[david-perez]

I've updated the RFC to reflect what we ended up implementing.

[github-actions]

A new generated diff is ready to view.

• AWS SDK (ignoring whitespace)
• No codegen difference in the Server Test
• No codegen difference in the Server Test Python

A new doc preview is ready to view.

[github-actions]

A new generated diff is ready to view.

• AWS SDK (ignoring whitespace)
• No codegen difference in the Server Test
• No codegen difference in the Server Test Python

A new doc preview is ready to view.