openapi: simplify API consumer interface, make output fields required

[chirino]

Keep input fields optional.

[ctron]

I am not a fan of this. I think a rule of "if it's absent, it's empty" is easy to understand.

Now we are introducing the idea of "input" and "output" models. Having no clear indication of which is which, but expecting different policies around that. Even removing the attributes for deserializing defaults.

[chirino]

The problem is NOT that it's hard to understand. It is harder to use. All API clients need to null-check every field access that is not required since it could be missing. It's the same reason you use field Vec<X> declarations instead of field Option<Vec<T>> declarations for all of these. It's much easier to deal with empty values than null values. You avoid an additional "does it exist check".

The only valid reason to make these not-required fields is if it's an experimental field that may be removed. If that's the case we should be documenting that fact too.

Could we keep the input and output models in different files to distinguish them?

[ctron]

Hm, maybe I just don't see the issue outside of Rust. That might be case.

I would expect that the benefit overall is, that empty stuff doesn't get serialize and reduces output. Meaning: less traffic. Maybe not worth optimizing for that. 🤷

Could we keep the input and output models in different files to distinguish them?

I guess having different modules might make sense then. I am still not a fan, but at least that would make it transparent why some group of structs follow one pattern, while others follow another one.

[bobmcwhirter]

@chirino would it be troublesome if we emit empty vectors, but skip any Option::None? Does that only solve half the problem?

For me, seeing:

average_score: null,

seems not much better than just omitting it, and you've still got to null-check.

While I do see the benefit of

list_of_things: [],

avoids a null-check.

[chirino]

@chirino would it be troublesome if we emit empty vectors, but skip any Option::None? Does that only solve half the problem?

That would be fine. Consumer code still has to do similar existence checks. So ease of use is about the same.

But I still question why we don't want the openapi models to match what our Rust models are. We are actively trying to make them different. Saying fields could be missing is like saying our fields are being held in HashMap and so they may not be there when you read them back out.

Like why are we making extra effort to make things more complex?

[chirino]

Seems like Utopia does not make the Option fields required (I think it's a bug), so there is there is actually no difference.

BTW if Utopia was doing the right thing. It would impact client-generated types.. field names become average_score? vs average_score to denote that the field may be undefined as well as null.

[chirino]

Updated PR to only apply this change to Vecs and Labels.

[chirino]

Ah look like we COULD make options required with utopia. see: juhaku/utoipa#530

[bobmcwhirter]

@chirino would it be troublesome if we emit empty vectors, but skip any Option::None? Does that only solve half the problem?

That would be fine. Consumer code still has to do similar existence checks. So ease of use is about the same.

But I still question why we don't want the openapi models to match what our Rust models are. We are actively trying to make them different. Saying fields could be missing is like saying our fields are being held in HashMap and so they may not be there when you read them back out.

Like why are we making extra effort to make things more complex?

I think that's a fair assessment also. I don't have a strong opinion really.

[chirino]

Latest commit to the PR adds back making the optional fields required. This time, adding the right utopia config so that it shows up in the openapi doc.

[bobmcwhirter]

Right on.

[bobmcwhirter]

Merge at will.