Initialize labels and annotations on ObjectMeta

[amydentremont]

Description

Initialize the labels and annotations maps on ObjectMeta so they're empty Maps instead of null if there are no labels/annotations on a resource.
The additionalProperties map is already initialized in ObjectMeta, as are all the List fields, so I don't see a reason to not initialize the labels and annotations too.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change
• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Checklist

• Code contributed by me aligns with current project license: Apache 2.0
• I Added CHANGELOG entry regarding this change
• I have implemented unit tests to cover my changes
• I have added/updated the javadocs and other documentation accordingly
• No new bugs, code smells, etc. in SonarCloud report
• I tested my code in Kubernetes
• I tested my code in OpenShift

It says to open the PR as a draft if you haven't completed the checklist, but most of this doesn't seem warranted for such a simple change, and I'm afraid this won't be looked at if I don't just open for review

[shawkins]

Linking to #3625 - The issue is that ObjectMetadata, labels, and annotations are all nullable (even with this change as the setters or getters are not guarding against null) and that complicates a lot of basic logic. One option is to go further with this change and enforce they are all non-null. The issue with that is that the serialization will change - that is instead of omitting metadata, you'll see metadata: {}, and similar changes for annotations and labels. Changing the generated code to add @JsonInclude(JsonInclude.Include.NON_EMPTY) on annotations and labels could help.

Another option is to have convenience methods - HasMetadata.getOrCreateMetadata(), etc. that will guarantee non-null results.

Another issue with this pr is that these modifications are to a generated class - which will get undone the next time the code is generated.

[manusa]

What's the end purpose of this?

Is it to be able to resource.getMetedata().getAnnotations().put("annotation", "is-safe-to-out").

Considering this use-case, there's what Steven already mentioned.

But there's also the immutable approach where you new ResourceBuilder(resource).editOrNewMetadata().addToAnnotations("annotation", "this-is-safe-too").endMetadata().build()

[shawkins]

What's the end purpose of this?

It could be for reads or writes. For writes a builder can make sense. For reads it's an awfully heavy weight approach.

[manusa]

For reads it's an awfully heavy weight approach.

#3625 (getters/ getOrDefault) should fix reads.

This more or less highlights the point of my question, can Amy's use-case be fixed/improved by #3625, or is there something else.

[shawkins]

This more or less highlights the point of my question, can Amy's use-case be fixed/improved by #3625, or is there something else.

Yes it can. These are all driving at the same things. Here's the options:

• convenience methods such as HasMetadata.getAnnotations, HasMetadata.getLabels that would be documented as to the behavior of auto-creating the ObjectMetadata, annotations, and labels. Other methods like HasMetadata.getAnnotation wouldn't have to create anything.

• change the generation logic so that ObjectMetadata, annotations, and labels are non-null by default. That especially makes sense for annotations and labels because it's done for other collections (arrays/lists), but would be inconsistent for ObjectMetadata as we don't do that for other nested object state. If we don't do it for ObjectMetadata, you still would likely add a similar set of convenience as above.

• Doing HasMetadata should have convenience methods for annotations and labels #3625 (comment) for reads is syntactically shorter that using a builder, but would be even more heavy weight. I'll respond with some more thoughts over there.

[amydentremont]

Sorry I missed these replies! And yeah I totally missed that this was a generated class 😅

My purpose is for reading. I submitted this PR after fixing a bug where we got a NPE when no labels existed on a resource.
The code has been:

Optional<String> deployGroup = Optional.ofNullable(
   javaApp.getMetadata().getLabels().get("deployGroup")
);

And I needed to update it to first check that labels weren't null:

Optional<String> deployGroup;
if (javaApp.getMetadata().getLabels() == null) {
    deployGroup = Optional.empty();
} else {
    deployGroup =
       Optional.ofNullable(javaApp.getMetadata().getLabels().get("deployGroup"));
}

Having the convenience methods for getting labels and annotations would definitely help; I could use just getLabel for this example. Though personally I still just wouldn't expect getLabels and getAnnotations to return null instead of empty maps, especially when additionalProperties is initialized to an empty map in the same class.

[shawkins]

Closing this pr, I'll introduce another (#4160) based upon updating the generation logic.