Question about input trait that implies clientOptional on required fields

[timocov]

Hi team,

Sorry for a bit weird title and it is probably just very stupid question, please bear with me.

Based on https://smithy.io/2.0/spec/type-refinement-traits.html#input-trait:

Required members of a structure marked with the @input trait are implicitly considered clientOptional

clientOptional on the other hand:

Requires that non-authoritative generators like clients treat a structure member as optional regardless of if the member is also marked with the required trait or default trait.

So if I have the following model:

operation GetItem {
    input := {
        @required
        itemId: String
    }
}

Based on the documentation above, itemId shouldn't be considered to be required from the client side, but it should be treated as such from the server side.

I understand that intention is to prevent breaking changes, but if later I decide to add another input:

operation GetItem {
    input := {
        @required
        itemId: String

        @required
        category: String
    }
}

And if I run smithy diff tool for this model against the one above - it won't report any breaking changes because category: String is implicitly marked as clientOptional, but if you make a call without this field from the client side - it will result in a request rejection because the server requires this field (it doesn't have a default value nor it is optional). To me it feels like implicitly adding clientOptional to all required fields hides the backward compatibility issue that could have been detected with diff tool, but it wasn't because of clientOptional.

I understand that if you re-use types between inputs and outputs, then having clientOptional on required fields makes sense - a field always returned/generated by the server can be optional for the client to provide (or even ignored by the server if provided). But @input explicitly disallows using the type it is attached to as an output or in other operations.

So I'm a bit confused why @input should imply clientOptional and what potential problems it prevents (if you can give some examples that would be awesome).

Thank you!

[mtdowling]

wrt clientOptonal, we should update the guidance to instead say that the member remains required when validation is performed but is “optional” in generated types regardless of client or server. This is works better for sharing types across client, server, types, etc. (only have time atm to respond to part of the question)

[joewyz]

Hi,
Regarding the smithy diff part, the smithy diff tool would report the backward-incompatible change for adding a new required member in your operation input because we do not explicitly attach @clientOptional to the input members by default.

The design doc written by @mtdowling may explain the question why @input imply @clientOptional and the problems it prevents:

Members of a structure marked with the @input trait are implicitly considered to be marked with @clientOptional. The @input trait special-cases a structure as the input of a single operation that cannot be referenced in any structures marked with the @input have more relaxed backward compatibility guarantees. It is backward compatible to remove the @required trait from top-level members of structures marked with the @input trait, and the @required trait does not need to be replaced with the @default trait (though this is allowed as well). This gives service teams the ability to remove the @required trait from top-level input members and loosen requirements without risking breaking previously generated clients.

The practical implication of this backward compatibility affordance is that code generated types for members of an @input structure SHOULD all be considered optional regardless of the use of @required or @default. Generators MAY special-case members that serve as resource identifiers to be non-nullable because those members can never remove the @required trait. Not observing these optionality affordances runs the risk of previously generated code breaking when a model is updated in the future.

Here are some usage in SDKs during the code generation:

• Smithy-Rust Smithy Rust uses smithy's nullableindex to check nullability, and it will check if the structure is an operation input.
• Smithy-Kotlin: Smithy Kotlin uses it the same way.

Hope these can help you.

[timocov]

Thanks @joewyz for a detailed response!

Regarding the smithy diff part, the smithy diff tool would report the backward-incompatible change for adding a new required member in your operation input because we do not explicitly attach @clientOptional to the input members by default.

I guess this is my confusion, because in my tests with the diff tool it didn't report adding a required member to an input as a breaking chance because it uses NullableIndex which respects implicit adding of clientOptional to input members if runs in a client mode, which is the default if not specified (which ChangedNullability doesn't). Does this look like a bug?

[timocov]

the member remains required when validation is performed but is “optional” in generated types regardless of client or server

I think this adds more clarity (at least for me) @mtdowling, thanks!

[joewyz]

@timocov Can you please share more details how you test with diff tool? I tried the model you provided and ran smithy diff command. An error AddedRequiredMember fired for member category. The AddedRequiredMember evaluator does not use NullableIndex for validation.

[timocov]

@joewyz my bad! You're right, adding a new required member does report as a breaking change, but marking existing member as required is not (this is what I tested originally). To me this felt the same, that's why I didn't differentiate these cases when posted previous message.

[joewyz]

I hope Michael's answer or my anwer resolves your confusion. Please let us know if you still find it confusing.

[timocov]

@joewyz not quite. So I understand that @input makes codegen fields not required and but these fields still should be validated as required - this is clear now. But what I am still confused with is the diff - why adding a required field is considered a breaking change, but marking an existing field as required in an input is not. From the design doc it is clear what @input means for the removal:

It is backward compatible to remove the @required trait from top-level members of structures marked with the @input trait, and the @required trait does not need to be replaced with the @default trait (though this is allowed as well).

But it doesn't specify the anything for "adding", which I would assume "adding a required field to an input is a breaking change" (which is confirmed by adding a new field), and "marking an existing field as "required" is kinda the same, which I would assume should trigger a breaking change detection, but it doesn't.

So say we have the following input shape:

@input
structure OperationInput {
    id: String
}

The field id already exists, and it is not required, it is optional from any point of view (codegen, diff, etc). If I mark this field as required now:

@input
structure OperationInput {
    @required
    id: String
}

This field has existed before, but now from the contract point of view it is considered required. The client codegen still should consider this field as "optional", but the server, when performs validation, should reject any input that doesn't this field set - to me this feels like a breaking change, because none of the previous clients will be able to make successful calls if they don't provide this field. But the diff tool doesn't report that.

On the other hand, if we add a new required field:

@input
structure OperationInput {
    id: String

    @required
    type: String
}

The diff tool will successfully report this as a breaking change, which is expected as it is not possible to make a call to the server without providing this new field.

So what I'm confused about - why adding an optional field and then marking it as required is not a breaking change, but adding the same field as required from the beginning is. I'd expect it to be breaking changes in both cases and this feels like a bug to me, but I must be missing something obvious here 🙁