Question if `@required` trait check should be part of validation or deserialization

[timocov]

Currently, if a member is marked with @required trait and you run deserialization on a object without such field, the deserialization fails with software.amazon.smithy.java.core.serde.SerializationException:

Caused by: software.amazon.smithy.java.core.serde.SerializationException: Missing required members: [result]
	at software.amazon.smithy.java.core.schema.PresenceTracker.validate(PresenceTracker.java:57)

This creates a couple of issues:

• it feels like this behavior is different from other traits (but I understand that the nature of other traits is different as well) - for them you have either to run a Validator or validate themselves
• if you want to treat such situation as "validation error", then it is not quite possible because SerializationException is generic and 1) it doesn't have a list of fields that are missing (the list above is just a string created here), 2) if a field is missing in a sub-object, then it is even harder to distinguish because for instance Caused by: software.amazon.smithy.java.core.serde.SerializationException: Missing required members: [id] doesn't tell where exactly id is missing
• there is a errorCorrection method on builder that in theory can be useful, but it feels like its purpose is completely different

I understand that I guess it is rather a philosophical question whether to treat the result of @required as "malformed input" (e.g. similar to when an input is not valid json) or "validation error" (when the structure and types are valid, but values aren't aligned to validation) so seeking for your thoughts on this.

Essentially:

• if you think that @required is an indicator of "malformed input", then can we at least change the exception being thrown to 1) be specific rather a generic 2) contain a list of all field paths which are missing, instead of just one field somewhere
• if you think that it can be/is "validation error" then have an option to @required tracking while deserializing an object and add its validation to the validator

Thanks!

[mtdowling]

It's both, really -- if something is required, and you're deserializing a primitive integer, you can either do error correction or throw an exception.

Adding paths to the missing member names sounds interesting, though I don't know if we can do that in a way that doesn't tank deserialization performance. Presence tracking is done per/structure. We'd have to do lots of profiling.

[timocov]

you're deserializing a primitive integer, you can either do error correction or throw an exception

I was thinking about this. Wouldn't it be possible to use boxed types for primitives in such case? I understand that it might have performance implications (yet to confirm how much exactly), but at least it can be opt-in if it does? Not sure how it will look like from a configuration point of view tho (it feels like generating boxed types over primitives is one thing, but not failing deser for required fields is another).

[timocov]

Another option I was thinking about if the validation could be run against the builder - at least in this case the builder could have a boxed type but the actual type would have primitives. Not sure if it makes sense.

[mtdowling]

It would be possible to use boxed types, sure. But Smithy and its predecessors it came from differentiate between primitive values and boxed values. Error correction simply sets a missing value to the zero value. IIRC builders also don’t use boxed types, rather they use a bitset for presence tracking validation to avoid allocations. Validation can’t really be run against builders since validation is built around serialization primitives that builders don’t implement.

[timocov]

But Smithy and its predecessors it came from differentiate between primitive values and boxed values

Can you elaborate on this please?

I just had a play with the codegen and it generates boxed types for primitives if they aren't required already so it is not something new I guess. Not sure what would be the best option here, but do you think we can add a configuration flag to the codegen something like to disable presence tracker that will result in 1) no presence tracker added 2) all required primitives are boxed (even tho I think this can be controlled via overriding symbols provider already IIRC)? Or maybe not exposing it to the public config but allow custom integrations to modify it (similar to traitInitializers method something like presenceTrackerEnabled or so). I guess another implication will be to to add a check to the validator, but it looks like it is already there 👍

[mtdowling]

I just meant that we codegen int and Integer based on the model. Changing that would be a breaking change.

There are a lot of avenues here, but to clarify: is the root of this request basically to add paths to required error messages?

[timocov]

Not quite. The ultimate goal of what I'm working on right now is to integrate smithy into a (mostly existing) spring boot application (btw if you're considering adding support for this, I'm happy to share/contribute). One of the aspects is obviously input validation. In my case, the validation in most of the parts is done via jakarta (and this is what SB is integrated out of the box) and that's what I want to replicate with smithy. Currently, I'm able successfully generate Jakarta annotations attached to getters and get the validation running on smithy-generated objects, but one of the issues is that because the required trait fails the deserialisation, it kinda separates the process into 2 places - one where the deserialisation happens and then Jakarta validation part. And on top of that, if I want to generate a list of validation errors for the whole input object, because the deserialisation fails at early stage it is simply impossible to do.

One of the possible solutions I see right now is not to use smithy's constraint traits and re-create jakarta ones instead, but I'm not sure if this is going to work well considering that the codegen might change based on built-in traits.

Also, as a long-term solution I think I'd prefer to go with smithy-java validation and migrate all services off jakarta to get more controlled and predictable results (and potentially more performant solution), but I'm not sure when and how it will happen.