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.79k stars 215 forks source link

Confusion with structure member optionality in IDL v2 in clients #1478

Closed david-perez closed 1 year ago

david-perez commented 2 years ago

The spec says that a client (i.e. a non-authoritative model consumer) should render a structure member shape with @default as:

Present unless also @clientOptional or part of an @input structure.

My reading of that is that the bar member of the Foo structure in this model:

operation MyOperation {
    input: Foo
}

structure Foo {
    @default("hello")
    bar: String
}

should be rendered in Rust as not optional, i.e. a plain std::string::String:

struct Foo {
    bar: String
}

But I don't see the purpose of @default in this particular example then. The user still has to always provide a value for bar.

Should the phrase be reworded to "Present unless also @clientOptional or reachable via an operation's input"?

Note that adding @input is not listed as a backwards incompatible change, so it feels odd that @input appears in that table: if I were to add @input to the Foo structure shape in the model above, the spec would dictate that bar be of type Option<T> then, and that is backwards incompatible in Rust.

mtdowling commented 2 years ago

But what's the purpose of @default then? The user still has to always provide a value for bar.

I think this is something you need to answer yourself when building a generator. It sounds like more of a general question on how to use default values with a struct in Rust. I know in languages with constructors, builders, etc, you have an opportunity to set defaults when they aren't provided.

Should the phrase be reworded to "Present unless also @clientOptional or reachable via an operation's input"?

I think you're inferring that because it requires a value in the example Rust type, then all inputs should have optional members for anything that has a default value so that caller doesn't have to set the default themselves. I think that's the same question as above though. Plenty of types are shared between inputs and outputs though, and it doesn't seem easy to manage for a team modeling their service because you can at any time go from referencing a shape only in input to referencing it in both input and output.

david-perez commented 2 years ago

Plenty of types are shared between inputs and outputs though, and it doesn't seem easy to manage for a team modeling their service because you can at any time go from referencing a shape only in input to referencing it in both input and output.

Yeah that's fair. I think I'm now inclined to keep the spec as-is in this regard, because changing the type of a struct member when it ceases to be transitively reachable in operation input is likely to cause confusion, whereas adding/removing @input is a more conscious act, and the type change only affects the direct members of the structure shape that has @input.

It sounds like more of a general question on how to use default values with a struct in Rust. I know in languages with constructors, builders, etc, you have an opportunity to set defaults when they aren't provided.

Yes, in Rust we will use builders to enact the benefits of @default. This is the way it will work:

let foo = Foo::builder().build();
let foo2 = Foo::builder().bar("hello").build();
let foo3 = Foo {
    bar: String::from("hello"),
}
assert_eq!(foo, foo2);
assert_eq!(foo2, foo3);
david-perez commented 2 years ago

I think all that remains to be cleared up is whether adding and removing @input is a backwards compatible change. You can see from my example that if I add @input, bar would become Option<String>: code using the builder API would not break, but code instantiating the struct directly would.

// Works, same code with same semantics.
let foo = Foo::builder().build();
// Still works because the builder method for `.bar()` wraps `"hello"` in `Some`
// in its implementation.
let foo2 = Foo::builder().bar("hello").build();
// Breaking change.
let foo2 = Foo {
    bar: Some(String::from("hello")),
}

You get a similar situation when considering an example where you remove @required from a structure member when the structure is marked with the @input trait. I agree with the spec's sentiment that "relaxing a constraint" should not break client code, but in Rust going from T to Option<T> (or viceversa) is, unfortunately, a breaking change. These are situations that are only starting to surface in IDL v2 because in IDL v1 we render everything as Option<T> in clients.

I don't see a way to mitigate this breakage by modifying the spec, so this most likely should be addressed in the Rust code generator. I don't know how though; perhaps @rcoh has some trick up his sleeve.

mtdowling commented 2 years ago

Yeah, adding the @input trait is interesting for clients. Most client generators create dedicated synthetic input shapes if the model doesn't already mark an operation input structure with the @input trait. So for client generators in practice, it's fine. But it's probably more straightforward to just call this a breaking change in the spec.