I want to get an initial review on this before adding all fields in objectMeta here.

And if we end up setting these fields explicitly, do we still want to round trip through marshalling and unmarshalling or go field by field accumulating into the metadata object by iterating through the metadataMap?

ref #64267 (comment)

I don't think this is the right layer at which to do this. I think the unstructured objectmeta accessor implementations should be adjusted to produce an unstructured object that matches ObjectMeta omitempty semantics

For example, if SetClusterName("") is called, that should remove the clusterName element from the unstructured object

tricky one: do we need to do cascading element removal in unstructured accessor for omitempty semantics?

I think all the accessor methods are for top level fields in metadata, so I'm not sure that's an issue

updated.

I think the unstructured objectmeta accessor implementations should be adjusted to produce an unstructured object that matches ObjectMeta omitempty semantics

The accessors were already modified to support this for some of the fields. Added for the rest.

hi @nikhita, how about having an additional check to respect the field tag omitempty via reflect? we might only do the removal when it's tagged, amiright?

it's working tho, but a dynamic approach would be better for maintenance

Reflection is slow to use in the normal path. A reflection based unit test to catch drift might be better.

Reflection is slow to use in the normal path.

true. it's a heavy cost to do the reflection every time accessing normal paths. or we could simply have a thread-unsafe map like map[(path)string](isOmitemptyField)bool and put all things we need inside init() call

A reflection based unit test to catch drift might be better.

I added a very simple test and didn't use reflection because all fields in objectMeta are omitempty. Ptal.

how about having an additional check to respect the field tag omitempty via reflect

Given that reflection is costly and the metadata accessors are for metadata and we already know which fields are omitempty in it, I think it's overkill to add the reflection to the accessors.

or we could simply have a thread-unsafe map like map[(path)string](isOmitemptyField)bool

This could be incorporated into the generalized "setNestedField..` funcs in helpers.go. However, IMO if we want to introduce methods which look for the omitempty tag, they should be created as new functions and not be added into the existing helper functions.

Most of the existing ones are used heavily in apiextensions-apiserver where we already know if omitempty is needed or not. Adding reflection to the existing ones will make all these function calls slow.

shouldn't we set random values first?

Added another test for fuzzing. I let the original test for omitempty be as-is just to make sure we don't miss any cases for zero values (fuzzing should catch but better to be safe :))

I meant to do fuzzing before these to catch missing calls. If a field is added to ObjectMeta now, this list gets incomplete, but we don't notice. If we fuzz before, we would.

@sttts @liggitt @yue9944882 ptal.

Without this change,

    emptyMetadata := &metav1.ObjectMeta{}
    metadata, err := runtime.DefaultUnstructuredConverter.ToUnstructured(emptyMetadata)
    if err != nil {
        t.Fatal(err)
    }

will produce metadata as map[creationTimestamp:<nil>]. creationTimestamp has the omitempty tag as well, so this should not be present in the map.

CreationTimestamp is a metav1.Time. The conversion looks for a custom marshaller and gets the null value and then simply returns nil (without looking at the omitempty tag).

With this change, it looks for the omitempty tag again and does not store the value if it is nil.

Looks like this isnt' right. json.Marshal +Unmarshal will set creationTimestamp: nil and we always need to ensure that it satisfies json marshalling + unmarshalling.

This will fail the already existing round trip test for unstructured:

Interesting to note that we also have a round trip test specifically for creationTimestamp but it just checks if conversion is possible and not that the round trip values match.

What should be the intended behaviour here? Should creationTimestamp be present in the map or not? 🤔

What should be the intended behaviour here? Should creationTimestamp be present in the map or not? thinking

answering myself: from the docs

The "omitempty" option specifies that the field should be omitted from the encoding if the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string.

omitempty has no meaning for a struct, so creationTimestamp should be present in the map. It is weird that we set the value as nil but in any case, it should be present in the unstructured object/map.

shouldn't omitempty be removed from CreateTimestamp then? maybe in another pull? it's okay with or without it tho.

shouldn't omitempty be removed from CreateTimestamp then? maybe in another pull?

yeah, I think we should remove it too. But I thought it was suited for another PR (since it would involved schema changes too afaik and would be a breaking change).

Also, wanted to get feedback on the current changes before removing it. :)