fix #5214: explicitly omitting nulls by default #5222

shawkins · 2023-06-08T11:50:50Z

Description

Resolves the discussion on #5214 - we'll treat the 6.6 / 6.7 behavior as a regression and switch to explicitly omitting nulls by default.

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

manusa · 2023-06-08T13:02:07Z

Unrelated to this change (I think), but worth noting and related to the general functionality, especially for patch scenarios.

Our Kubernetes Resource objects are currently initialized with empty arrays (I think we did add this behavior a while ago). This poses a problem when using builder to generate objects that will be sent to the API server in a JSON_MERGE (application/merge-patch+json) PATCH request. If the remote object has an entry in the array, since the sent patch JSON contains the field with an empty array, the remote field will be overwritten and cleared.

shawkins · 2023-06-08T13:46:51Z

Unrelated to this change (I think), but worth noting and related to the general functionality, especially for patch scenarios.

Our Kubernetes Resource objects are currently initialized with empty arrays (I think we did add this behavior a while ago). This poses a problem when using builder to generate objects that will be sent to the API server in a JSON_MERGE (application/merge-patch+json) PATCH request. If the remote object has an entry in the array, since the sent patch JSON contains the field with an empty array, the remote field will be overwritten and cleared.

@manusa for the built-in resources where we initialize empty collections there is also the annotation: @JsonInclude(JsonInclude.Include.NON_EMPTY) - the expectation moving forward then is that neither a null nor an empty collection will be included in the serialization by default. So I don't think the situation you are describing currently occurs with the built-in resources. However the opposite problem exists - this was discussed on #4606 but wasn't addressed.

andreaTP

Super late at this party, but thanks a lot for the changes @shawkins !

I agree this is the right direction, a couple of minor notes.

andreaTP · 2023-06-09T10:22:46Z

CHANGELOG.md

@@ -3,6 +3,7 @@
 ### 6.8-SNAPSHOT


Given the discussion, this fix should probably be included in a 6.7 patch release.

andreaTP · 2023-06-09T10:26:20Z

...tes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/KubernetesSerialization.java

@@ -89,6 +91,8 @@ public KubernetesSerialization(ObjectMapper mapper, boolean searchClassloaders)
  protected void configureMapper(ObjectMapper mapper) {
    mapper.registerModules(new JavaTimeModule(), unmatchedFieldTypeModule);
    mapper.disable(DeserializationFeature.FAIL_ON_INVALID_SUBTYPE);
+    // omit null fields, but keep null map values
+    mapper.setDefaultPropertyInclusion(JsonInclude.Value.construct(Include.NON_NULL, Include.ALWAYS));


Note for the future, is it possible to let the user customize those options completely?

Only in an advanced usage scenario. The configureMapper method is protected and may be overriden if someone wants a create a custom KubernetesSerialization subclass. However I wanted to work through the quarkus integration and especially a plan to minimize the number of classes referenced by the kubernetesdeserializer - I'm thinking we need provide a way to lazily load - before open things up even more.

Great idea!

manusa · 2023-06-09T11:07:18Z

So I don't think the situation you are describing currently occurs with the built-in resources

It does occur (for sure at least on 6.6, but I guess this is the same on master and earlier versions too) for DeploymentConfig, for example.

I think this has to do with the javaOmitEmpty JSON Schema configuration that's computed using the getPropertyDescriptor function:

kubernetes-client/kubernetes-model-generator/pkg/schemagen/generate.go

Line 313 in 6023042

    
           func (g *schemaGenerator) getPropertyDescriptor(t reflect.Type, desc string, omitEmpty bool) JSONPropertyDescriptor {

And the fact that builders are initialized with empty collections.

The following link contains a concrete example of why this is a problem:
https://github.com/manusa/yakd/blob/6d942632ee3ad5e738ba31c0b2fe45bac99f3c99/src/main/java/com/marcnuri/yakd/quickstarts/dashboard/deploymentconfigs/DeploymentConfigService.java#L90-L97

Basically when sending the patch unless the workaround is applied, the JSON sent to the server contains something like:

{"spec": {"replicas": 1, "triggers": []}}

Since this is a JSON_MERGE patch, the triggers are wiped out from the server resource.

For this use-case, the desired JSON content should be like:

{"spec": {"replicas": 1}}

shawkins · 2023-06-09T11:56:40Z

It does occur (for sure at least on 6.6, but I guess this is the same on master and earlier versions too) for DeploymentConfig, for example.

So yes that would be a bug with the omitEmpty logic. I recall specifically adding a workaround for all maps in Fabric8DefaultRule but wasn't aware we had an inconsistency with some lists (others lists of either simple or complex types like objectmeta.finalizers or ownerreferences do end up annotated appropriately). I'd be fine with another override in Fabric8DefaultRule to act as a catch all.

And the fact that builders are initialized with empty collections.

Builders make quite a few assumptions about null / empty - the initialization, no null map values, and no null complex type collection values, and probably more... At least the builder behavior to default to an empty collection was part of the motivation of the change to initialize all of pojos with empty collections.

shawkins · 2023-06-10T12:54:14Z

@manusa the rationale for the inconsistency in the deploymentconfig spec triggers is that they are not marked as omitempty upstream: https://github.com/openshift/api/blob/master/apps/v1/types.go#L63

As you point out our builders are opionated about treating an empty and null collection the same - and we're already initializing the trigger list to empty, so I would be fine to just force the annotation. That can either be done in the generator or via the default rule. Do you want that as part of this pr?

manusa · 2023-06-12T09:06:42Z

That can either be done in the generator or via the default rule. Do you want that as part of this pr?

No,I think we should defer that to a latter PR. It can also be treated as a bugfix (I'm still unsure if we'll have to release yet another 6.7 Z version).

sonarqubecloud · 2023-06-14T14:08:19Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

shawkins requested review from manusa, oscerd, rohanKanojia and sunix as code owners June 8, 2023 11:50

shawkins force-pushed the iss5214 branch from f76dd57 to 3a4f67f Compare June 8, 2023 12:20

shawkins force-pushed the iss5214 branch from 3a4f67f to 953c899 Compare June 8, 2023 13:34

andreaTP approved these changes Jun 9, 2023

View reviewed changes

manusa added this to the 6.7.2 milestone Jun 14, 2023

manusa approved these changes Jun 14, 2023

View reviewed changes

fix fabric8io#5214: explicitly omitting nulls by default

dd38370

manusa force-pushed the iss5214 branch from 953c899 to dd38370 Compare June 14, 2023 13:31

manusa merged commit 4dba8cf into fabric8io:master Jun 14, 2023

shawkins mentioned this pull request Jun 16, 2023

Determine if all built-in collections should omit empty #5262

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #5214: explicitly omitting nulls by default #5222

fix #5214: explicitly omitting nulls by default #5222

shawkins commented Jun 8, 2023 •

edited by manusa

Loading

manusa commented Jun 8, 2023

shawkins commented Jun 8, 2023 •

edited

Loading

andreaTP left a comment

andreaTP Jun 9, 2023

andreaTP Jun 9, 2023

shawkins Jun 9, 2023 •

edited

Loading

andreaTP Jun 9, 2023

manusa commented Jun 9, 2023 •

edited

Loading

shawkins commented Jun 9, 2023

shawkins commented Jun 10, 2023

manusa commented Jun 12, 2023

sonarqubecloud bot commented Jun 14, 2023

fix #5214: explicitly omitting nulls by default #5222

fix #5214: explicitly omitting nulls by default #5222

Conversation

shawkins commented Jun 8, 2023 • edited by manusa Loading

Description

Type of change

Checklist

manusa commented Jun 8, 2023

shawkins commented Jun 8, 2023 • edited Loading

andreaTP left a comment

Choose a reason for hiding this comment

andreaTP Jun 9, 2023

Choose a reason for hiding this comment

andreaTP Jun 9, 2023

Choose a reason for hiding this comment

shawkins Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

andreaTP Jun 9, 2023

Choose a reason for hiding this comment

manusa commented Jun 9, 2023 • edited Loading

shawkins commented Jun 9, 2023

shawkins commented Jun 10, 2023

manusa commented Jun 12, 2023

sonarqubecloud bot commented Jun 14, 2023

shawkins commented Jun 8, 2023 •

edited by manusa

Loading

shawkins commented Jun 8, 2023 •

edited

Loading

shawkins Jun 9, 2023 •

edited

Loading

manusa commented Jun 9, 2023 •

edited

Loading