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

feat!: add jackson annotations additionalProperties #2103

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

memdal
Copy link

@memdal memdal commented Sep 24, 2024

  • Adds @JsonAnySetter and @JsonAnyGetter annotations to additionalProperties

Description

Adds these annotations so that Jackson can marshall and unmarshall additional properties.

Checklist

  • The code follows the project's coding standards and is properly linted (npm run lint).
  • Tests have been added or updated to cover the changes.
  • Documentation has been updated to reflect the changes.
  • All tests pass successfully locally.(npm run test).

Copy link

netlify bot commented Sep 24, 2024

Deploy Preview for modelina canceled.

Name Link
🔨 Latest commit fa2cccc
🔍 Latest deploy log https://app.netlify.com/sites/modelina/deploys/6704dc742c7175000804c5d1

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

additionalProperties needs to be unwrapped, not simply serialized and deserialized 🙂

I dont think there is anything native in Jackson that supports that. Might be wrong though.

@memdal
Copy link
Author

memdal commented Sep 24, 2024

Hi, thanks for taking a look. What would unwrapped mean in this context?

In my tests any unknown key/value pair was given its own entry in the map, and any object value would become a map of its own.

@jonaslagoni
Copy link
Member

jonaslagoni commented Sep 25, 2024

serialized value {additionalProperties: {test: '123'}} should become {test: '123'}.

if it's not possible we should just ignore the property and add it to the documentation

@memdal
Copy link
Author

memdal commented Sep 25, 2024

In that case it does some wrapping/unwrapping. What happens is now (pseudocode):

Json to deserialize:

{
  "defined_property": "defined property value",
  "undefined_property": "undefined property value"
}

Java class to unmarshal the JSON into:

class JavaClassRepresentation {
// getters/setters/annotations and other methods omitted for brevity
  private String definedProperty = "defined property value";
  private Map<String, Object> additionalProperties = Map.of(
    Entry.of("undefined_property", "undefined property value")
  );
}

Json serialized from the Java class:

{
  "defined_property": "defined property value",
  "undefined_property": "undefined property value"
}

The one edge case I am uncertain about is if an additional property is actually named additionalProperties and has an object value. In that case it can either merge the maps and lose the wrapping/unwrapping, or it will have to lose data.

@memdal
Copy link
Author

memdal commented Oct 4, 2024

@jonaslagoni The @JsonAnyGetter annotation ensures that the properties from the additionalProperties map are unwrapped on serialization. Do you think this change could be added?

@jonaslagoni
Copy link
Member

jonaslagoni commented Oct 4, 2024

@memdal thats perfect yea: https://www.tutorialspoint.com/jackson_annotations/jackson_annotations_jsonanygetter.htm

My bad for not realizing it before ✌️ confused it with regular getter method..

src/generators/java/presets/JacksonPreset.ts Outdated Show resolved Hide resolved
src/generators/java/presets/JacksonPreset.ts Outdated Show resolved Hide resolved
* Adds @JsonAnySetter and @JsonAnyGetter annotations to
  additionalProperties.
@memdal memdal force-pushed the jackson-jsonanysetter-getter branch from f05e1bd to 3c2b25c Compare October 7, 2024 13:23
replace check for isAdditionalProperties with hasUnwrappedOptions when adding
@JsonAnyGetter annotation.
Copy link

sonarcloud bot commented Oct 8, 2024

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Nice 👍

@jonaslagoni jonaslagoni changed the title feat: add jackson annotations additionalProperties feat!: add jackson annotations additionalProperties Oct 8, 2024
@jonaslagoni jonaslagoni merged commit 07f39f1 into asyncapi:next Oct 8, 2024
18 checks passed
@jonaslagoni
Copy link
Member

@all-contributors please add @memdal for code, test

Copy link
Contributor

@jonaslagoni

I've put up a pull request to add @memdal! 🎉

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 4.0.0-next.62 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants