feat: add annotation @PreserveUnknownFields for field #4398

wineway · 2022-09-07T03:48:58Z

Signed-off-by: wineway wangyuweihx@gmail.com

Description

like #4313, we need to use a sealed interface as a field type and offer multi implementations for this interface, it seems we can't and don't need to validate its fields in this situation, and we don't want to add something like @JsonAnyGetter might change its serialize behavior. so we think we can add a new annotation @PreserveUnknownFields for this scenario.

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

Signed-off-by: wineway <wangyuweihx@gmail.com>

andreaTP · 2022-09-07T09:34:48Z

As much as I'm unhappy adding yet another way to emit x-kubernetes-preserve-unknown-fields, I do understand the reasoning and I don't want to oppose it.

Just one more question: have you considered extending JsonNode instead of adding AnyGetter or similar?

andreaTP · 2022-09-07T09:35:37Z

side note: you can run mvn spotless:apply to get the formatting right.

Signed-off-by: wineway <wangyuweihx@gmail.com>

wineway · 2022-09-07T10:17:38Z

Just one more question: have you considered extending JsonNode instead of adding AnyGetter or similar?

Hmmm. I think this could be just a not so good workaround, it destroyed the inheritance and did not work for the sealed interface.

https://github.com/fabric8io/kubernetes-client/blob/master/crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractJsonSchema.java#L266

    boolean preserveUnknownFields = (definition.getFullyQualifiedName() != null &&
        definition.getFullyQualifiedName().equals(JSON_NODE_TYPE));

More than this, it seems that it won't check if the type of the field extends JsonNode, it just checks field type name is equal to JsonNode

andreaTP · 2022-09-07T10:28:50Z

@wineway thanks for the argument, you can use SchemaFrom to get JsonNode type for the annotation processor purposes ... but I understand this workaround might not be good enough for some use-cases

andreaTP

A few minor comments and one question, have you attempted to refactor the code such as we can setXKubernetesPreserveUnknownFields in only one place instead of two in the JsonSchema classes?

Thanks a lot!

andreaTP · 2022-09-08T09:03:48Z

CHANGELOG.md

@@ -11,6 +11,7 @@
 * Fix #4383: bump snakeyaml from 1.30 to 1.31

 #### New Features
+* Feat: add annotation @PreserveUnknownFields for generation field


can you please expand a little this sentence, for example saying that it affects the crd-generator

andreaTP · 2022-09-08T09:05:42Z

crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractJsonSchema.java

@@ -352,6 +362,7 @@ private static class PropertyOrAccessor {
    private boolean required;
    private boolean ignored;
    private boolean preserveUnknownFields;
+    private boolean preserveSelfUnknownFields;


Do we need an extra field here?
Is it possible to rely on the already existing preserveUnknownFields ?

andreaTP · 2022-09-08T09:07:17Z

crd-generator/api/src/main/java/io/fabric8/crd/generator/annotation/PreserveUnknownFields.java

+import java.lang.annotation.*;
+
+/*
+ * Used to tweak the behavior of the crd-generator


something like to emit 'x-kubernetes-preserve-unknown-fields'

andreaTP · 2022-09-08T09:15:22Z

doc/CRD-generator.md

@@ -357,6 +357,28 @@ Corresponding `x-kubernetes-preserve-unknown-fields: true` will be generated in
            x-kubernetes-preserve-unknown-fields: true
 ```

+You can also annotation a field with @PreserveUnknownFields:


Can you, please, add the annotation to the cheatsheet below.

fabiobrz · 2022-09-12T13:55:35Z

doc/CRD-generator.md

@@ -357,6 +357,28 @@ Corresponding `x-kubernetes-preserve-unknown-fields: true` will be generated in
            x-kubernetes-preserve-unknown-fields: true
 ```

+You can also annotation a field with @PreserveUnknownFields:


Also s/annotation/annotate/ ?

Also s/annotation/annotate/ ?

sorry I can't find where it is, can I ask for help😂

Please replace annotation with annotate so that the sentence reads:

You can also annotate a field with @PreserveUnknownFields:

Please replace annotation with annotate so that the sentence reads:

sorry😂……fixed

wineway · 2022-09-14T07:51:03Z

have you attempted to refactor the code such as we can setXKubernetesPreserveUnknownFields in only one place instead of two in the JsonSchema classes?

I have tried, but I'm afraid I can't refactor the two fields to one in limited code change. if I just separate facade.process to two-stage, or make the type of preserveUnknownFields from boolean to enum, Hmmm, I think adding a new field will be more clear.

Signed-off-by: wineway <wangyuweihx@gmail.com>

sonarcloud · 2022-09-15T10:21:09Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

85.7% Coverage
12.7% Duplication

add annotation @PreserveUnknownFields for field

6425e3e

Signed-off-by: wineway <wangyuweihx@gmail.com>

wineway changed the title ~~add annotation @PreserveUnknownFields for field~~ feat: add annotation @PreserveUnknownFields for field Sep 7, 2022

add changelog & fixed copyright

33240aa

Signed-off-by: wineway <wangyuweihx@gmail.com>

rohanKanojia requested review from metacosm and andreaTP September 8, 2022 07:30

andreaTP requested changes Sep 8, 2022

View reviewed changes

fabiobrz reviewed Sep 12, 2022

View reviewed changes

wineway added 2 commits September 14, 2022 16:48

add more explain for changelog & doc

328b5bc

Signed-off-by: wineway <wangyuweihx@gmail.com>

fixed typo

5a3708e

andreaTP approved these changes Sep 15, 2022

View reviewed changes

manusa approved these changes Sep 16, 2022

View reviewed changes

manusa added this to the 6.2.0 milestone Sep 16, 2022

manusa merged commit 66ca429 into fabric8io:master Sep 16, 2022

manusa mentioned this pull request Sep 16, 2022

refactor: JsonSchema remove preserveSelfUnknownFields #4425

Merged

11 tasks

euberseder-hubspot mentioned this pull request Dec 21, 2023

back port annotation support for defaults and preserve-unknown-fields HubSpot/kubernetes-client#129

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add annotation @PreserveUnknownFields for field #4398

feat: add annotation @PreserveUnknownFields for field #4398

wineway commented Sep 7, 2022 •

edited

andreaTP commented Sep 7, 2022

andreaTP commented Sep 7, 2022

wineway commented Sep 7, 2022 •

edited

andreaTP commented Sep 7, 2022

andreaTP left a comment

andreaTP Sep 8, 2022

andreaTP Sep 8, 2022

andreaTP Sep 8, 2022

andreaTP Sep 8, 2022

fabiobrz Sep 12, 2022

wineway Sep 14, 2022

andreaTP Sep 14, 2022

wineway Sep 14, 2022

wineway commented Sep 14, 2022

sonarcloud bot commented Sep 15, 2022

feat: add annotation @PreserveUnknownFields for field #4398

feat: add annotation @PreserveUnknownFields for field #4398

Conversation

wineway commented Sep 7, 2022 • edited

Description

Type of change

Checklist

andreaTP commented Sep 7, 2022

andreaTP commented Sep 7, 2022

wineway commented Sep 7, 2022 • edited

andreaTP commented Sep 7, 2022

andreaTP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wineway commented Sep 14, 2022

sonarcloud bot commented Sep 15, 2022

wineway commented Sep 7, 2022 •

edited

wineway commented Sep 7, 2022 •

edited