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

Model getters have same annotations as fields (breaks native) #6086

Merged
merged 3 commits into from
Jul 1, 2024

Conversation

manusa
Copy link
Member

@manusa manusa commented Jun 27, 2024

Description

Fix #6085 annotate getters with same Jackson annotations as fields.

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 manusa added this to the 7.0.0 milestone Jun 27, 2024
@manusa manusa force-pushed the fix/native branch 2 times, most recently from 29dee24 to 7111d31 Compare June 27, 2024 13:51
@manusa manusa changed the title Model getters/setters not annotated with @JsonProperty (breaks native) Model getters have same annotations as fields (breaks native) Jun 27, 2024
Copy link

sonarcloud bot commented Jun 27, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@manusa
Copy link
Member Author

manusa commented Jun 27, 2024

@shawkins @galderz

This is passing now.

The new approach is to add the JsonInclude and JsonUnwrapped annotations to getter methods too so that they share the same configuration.

If the issue in native mode was that there was divergent configuration for serialization properties and field configuration took precedence, then this should address it.

Let me know if I should merge this and include it in 6.13.1 or defer it to a possible 6.13.2 or alternative workaround.

@manusa
Copy link
Member Author

manusa commented Jul 1, 2024

I'm going to merge this since I don't think it causes any harm and might fix the downstream issue.

I'll try and release 6.13.1 tomorrow with this included, if you think this shouldn't go in or we should revert, please let me know today.

@manusa manusa merged commit f650777 into fabric8io:main Jul 1, 2024
20 of 21 checks passed
@manusa manusa deleted the fix/native branch July 1, 2024 10:18
@jorsol
Copy link
Contributor

jorsol commented Jul 2, 2024

Hi @manusa ,

There are still some methods that are not annotated like in io.fabric8.kubernetes.api.model.ObjectMeta (annotations, labels):

    @JsonProperty("annotations")
    @JsonInclude(JsonInclude.Include.NON_EMPTY)
    private Map<String, String> annotations = new LinkedHashMap<String, String>();

    @JsonProperty("annotations")
    public Map<String, String> getAnnotations() {
        return annotations;
    }

    @JsonProperty("annotations")
    public void setAnnotations(Map<String, String> annotations) {
        this.annotations = annotations;
    }

@manusa
Copy link
Member Author

manusa commented Jul 2, 2024

🤦 😮‍💨 too much logic scattered around.

I created another issue #6098 to fix this.

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.

JsonProperty annotation in fields and methods breaks Jackson behavior in native
3 participants