search

Arnavion / k8s-openapi

Rust definitions of the resource types in the Kubernetes client API
Apache License 2.0
383 stars 41 forks source link

Optional vs empty listables #72

Closed clux closed 3 years ago

clux commented 4 years ago

As an example; ObjectMeta has a bunch of common listables:

https://github.com/kubernetes/apimachinery/blob/945d4ebf362b3bbbc070e89371e69f9394737676/pkg/apis/meta/v1/types.go#L226-L264

These are marked as +optional and also with omitempty. This implies they do not distinguish between the empty list/map case and the missing list/map case.

However, here we end up wrapping these objects in an Option<SomeListable<..>>:

    pub labels: Option<std::collections::BTreeMap<String, String>>,

So my question is; is it possible to match serde default annotations on listables when there's a omitempty annnotation? It would be a lot more ergonomic if we saw structs with that looking like:

    #[serde(default, skip_serializing_if = "BTreeMap::is_empty")]
    pub labels: std::collections::BTreeMap<String, String>,

so we didn't have to unpack an any of these. We already have to check for presence of the item we want in the collection everywhere anyway.

Arnavion commented 4 years ago

Yes, looks like it would be fine. I searched for to nil and is nil to see if any fields have comments that would indicate they differentiate between empty lists/maps and nil, and it doesn't seem like there are any.

waynr commented 3 months ago

@Arnavion this seems to be happening again since your fix in 6cff214. I'm currently trying out 0.22.0 and seeing the exact example given in this issue (ObjectMeta.labels) being of type Option<BTreeMap<...>>

Arnavion commented 3 months ago

https://github.com/Arnavion/k8s-openapi/commit/f0fda041e401e7d3bbb39b19ff305e258688728d