Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[java-generator] Fix additionalProperties ser/deser #4561

Merged
merged 2 commits into from Nov 14, 2022

Conversation

andreaTP
Copy link
Member

@andreaTP andreaTP commented Nov 4, 2022

Description

Fix #4543

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

@andreaTP
Copy link
Member Author

andreaTP commented Nov 4, 2022

cc @mteubner

@@ -82,4 +82,9 @@ public class Auth implements io.fabric8.kubernetes.api.model.KubernetesResource
public void setAdditionalProperties(java.util.Map<String, Object> additionalProperties) {
this.additionalProperties = additionalProperties;
}

@com.fasterxml.jackson.annotation.JsonAnySetter()
public void setAdditionalProperties(String key, Object value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be setAdditionalProperty singular?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprisingly not, the integration test fails if I make it singular.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So probably some jackson magic :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, at least now is properly tested 🙂

@@ -280,6 +282,13 @@ public GeneratorResult generateJava() {

objField.createGetter().addAnnotation("com.fasterxml.jackson.annotation.JsonAnyGetter");
objField.createSetter().addAnnotation("com.fasterxml.jackson.annotation.JsonAnySetter");

MethodDeclaration additionalSetter = clz.addMethod("setAdditionalProperties", Modifier.Keyword.PUBLIC);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MethodDeclaration additionalSetter = clz.addMethod("setAdditionalProperties", Modifier.Keyword.PUBLIC);
MethodDeclaration additionalSetter = clz.addMethod("setAdditionalProperty", Modifier.Keyword.PUBLIC);

Sorry, I may not be being clear. I would expect both the test and this logic to say setAdditionalProperty singular. That lines up with the model generation logic in the main project - Pod.setAdditionalProperty.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that's what I did, changing both breaks the functionality I'm fixing here as demonstrated by the additional test included in this PR.

If you are still in doubt I propose to have a quick sync on it to clarify somewhere next week ;-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it would probably be best to compare to what's already in use. The core model classes are generated with:

    @JsonIgnore
    private Map<String, Object> additionalProperties = new HashMap<String, Object>();

   @JsonAnyGetter
    public Map<String, Object> getAdditionalProperties() {
        return this.additionalProperties;
    }

    @JsonAnySetter
    public void setAdditionalProperty(String name, Object value) {
        this.additionalProperties.put(name, value);
    }

due to a limitation in the jsonschema2pojo,

@JsonIgnore
void setAdditionalProperties(Map<String, Object> properties)

being generated by the class annotations:

@Setter
@Accessors(prefix = {
    "_",
    ""
})

As far as I know that handles all of the issues we have seen with serdes and builder logic.

Could you try aligning to that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shawkins sorry for the long forth and back, the integration test failure was due to a different reason ... fixed now 👍

@sonarcloud
Copy link

sonarcloud bot commented Nov 14, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@shawkins shawkins self-requested a review November 14, 2022 13:53
Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@manusa manusa added this to the 6.3.0 milestone Nov 14, 2022
@manusa manusa merged commit 9690a70 into fabric8io:master Nov 14, 2022
@manusa manusa added the changelog missing A line to changelog.md regarding the change is not added label Nov 14, 2022
manusa added a commit that referenced this pull request Nov 14, 2022
Signed-off-by: Marc Nuri <marc@marcnuri.com>
@manusa manusa removed the changelog missing A line to changelog.md regarding the change is not added label Nov 14, 2022
@mteubner
Copy link

mteubner commented Nov 14, 2022

@andreaTP Do I do something wrong on that? When I check out the master branch, und try to build this test, I still get

package io.dapr.v1alpha1.componentspec;

@com.fasterxml.jackson.annotation.JsonInclude(com.fasterxml.jackson.annotation.JsonInclude.Include.NON_NULL)
@com.fasterxml.jackson.annotation.JsonPropertyOrder({})
@com.fasterxml.jackson.databind.annotation.JsonDeserialize(using = com.fasterxml.jackson.databind.JsonDeserializer.None.class)
public class Test implements io.fabric8.kubernetes.api.model.KubernetesResource {

@com.fasterxml.jackson.annotation.JsonIgnore()
private java.util.Map<String, Object> additionalProperties = new java.util.HashMap<>();

@com.fasterxml.jackson.annotation.JsonAnyGetter()
public java.util.Map<String, Object> getAdditionalProperties() {
    return additionalProperties;
}

@com.fasterxml.jackson.annotation.JsonAnySetter()
public void setAdditionalProperties(java.util.Map<String, Object> additionalProperties) {
    this.additionalProperties = additionalProperties;
}

}

@mteubner you should execute mvn install, if you want to use the new code, not verify :(

@andreaTP workgs fine for me, thx a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional Attributes are generated to be ignored, because of wrong setting of JsonAnySetter annotation
4 participants