Closed ctron closed 4 years ago
Do you happen to know if metadata needs to be optional in the first place?
ObjectMeta
was added in https://github.com/kubernetes/kubernetes/commit/55163a7df1b50824155560216b1edd9f4aec5365#diff-9ff06ad723720f9428a65da3710cf436R423 and it was already json:"omitempty"
at the time. Then every json:"omitempty"
field gained the +optional
annotation in https://github.com/kubernetes/kubernetes/commit/7414cafbeb378dcc4f6fe6ef402c2d54c0ee5633#diff-9ff06ad723720f9428a65da3710cf436R1926 So on the face of it having it be optional is intentional by upstream, but I'm not sure there is ever any situation where either the client's request or the server's response can omit it.
I don't think metadata is optional. Reading it back, you will always have at least things like the selflink or the uid. When creating initially, I think in every case you need to put the name
field.
Also I think that omitempty
doesn't necessarily translate to Option
.
e.g. a string
in go, which is ""
(empty) would not be serialized with omitempty
. Having a *string
( pointer to string) would serialize ""
with omitempty
. See: https://play.golang.org/p/Ip17iRwOudA
This works only with primitive types. Types which can be "empty". e.g. false' for boolean,
""for string,
0for integers, or
nilfor pointers. However a struct cannot be "empty" (unless it is a pointer). So even if a struct has only "empty" fields, it is serialized: https://play.golang.org/p/HP9A86s_e-C (
threeis missing, but
two` is not).
However, when deserializing, and not using pointers, you get the full tree deserialized, without the case of having nil
s.
Translating this to Rust, I would propose to use e.g. #[serde(skip_serializing_if = "Map::is_empty")]
for things that can be empty and are marked omitempty
. And only use Option
when a pointer value is used: string
-> String
and *string
-> Option<String>
.
Also I think that
omitempty
doesn't necessarily translate toOption
.
That annotation was removed from the golang code later and only +optional
remains. But anyway it doesn't matter; the codegen is generated from the swagger spec, which says that the metadata
field is not required, so representing it as Option
is correct.
Also I think that
omitempty
doesn't necessarily translate toOption
.That annotation was removed from the golang code later and only
+optional
remains. But anyway it doesn't matter; the codegen is generated from the swagger spec, which says that themetadata
field is not required, so representing it asOption
is correct.
Sounds reasonable. So do you think this change is ok? Or do you want any changes?
Well, the question still stands about whether metadata needs to be optional or not. It would be nice to resolve that so that we don't need both metadata_mut
and set_metadata
in the trait. But I'm not sure we can resolve it between the two of us. (I'd previously asked this of the folks at kube
too but they also did not opine on the matter.)
Anyway, can you review https://github.com/Arnavion/k8s-openapi/compare/fix-66 ? It's your PR + some whitespace changes (added spaces around the =
in set_metadata
's expr, removed one extra trailing newline from the impl Metadata
generated code) + rebased on latest master.
Anyway, can you review https://github.com/Arnavion/k8s-openapi/compare/fix-66 ? It's your PR + some whitespace changes (added spaces around the
=
inset_metadata
's expr, removed one extra trailing newline from theimpl Metadata
generated code) + rebased on latest master.
:+1: looks good to me, thanks!
I had to create two new methods (but maybe there is a better way):
The first allows to get an Option, holding a mutable reference to the metadata. However when using
Default::default()
, you get empty metadata and need a way to set it. I would have usedfn metadata_mut(&mut self) -> &mut Option<<Self as Metadata>::Ty>
instead, but in some cases the value is not backed by an Option, but rather by the actual value.So I added a second
set_metedata
function, which allows to set the value. As in some cases the value may be backed by the actual value, I don't think a "clear" method is possible.