Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
david-perez opened this issue Oct 31, 2022 · 4 comments · Fixed by #1519
Closed

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

david-perez opened this issue Oct 31, 2022 · 4 comments · Fixed by #1519

Comments

@david-perez
Copy link
Contributor

david-perez commented Oct 31, 2022

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants