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(openapi-generator): Add optional logic to suppress faulty additionalProperties: true #689

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

newtork
Copy link
Contributor

@newtork newtork commented Jan 10, 2025

Context

https://github.tools.sap/AI/ai-sdk-java-backlog/issues/129

  • Generating with a component with additionalProperties: true will append extends HashMap<String,Object> to generated class signature. See intermediate commit 7bd61ed
  • ⚠️ During compilation this logs a warning due to missing serialVersionUID. See 7bd61ed#r151192960
  • ⚠️ During serialization Jackson default behavior is to ignore class fields and only iterate super#entrySet().

However

  • We have not been approached by any user complaining about this issue.

Feature scope:

  • Add optional feature toggle to fix this behavior (default: disabled)
  • Update sample project specification
    • Update serialization test coverage

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Error handling created / updated & covered by the tests above
  • Documentation updated
  • Release notes updated

@newtork newtork added please merge Request to merge a pull request please review Request to review a pull request labels Jan 10, 2025
""";
final Order order = Order.create().productId(100L).quantity(5).totalPrice(6.0f);
order.setCustomField("shoesize", 44);
assertThat(new ObjectMapper().writeValueAsString(order)).isEqualToIgnoringWhitespace(expected);
Copy link
Contributor Author

@newtork newtork Jan 10, 2025

Choose a reason for hiding this comment

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

(Comment)

Without the enabled feature toggle this test assertion fails:

Expecting actual:
  "{}"
to be equal to:
  "{
  "productId": 100,
  "quantity": 5,
  "totalPrice": 6.0,
  "packaging" : "bottle",
  "shoesize": 44
}

Copy link
Member

@MatKuhr MatKuhr left a comment

Choose a reason for hiding this comment

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

In case we consider this a fix, a release note would be good. Otherwise LGTM

@@ -75,6 +76,25 @@ void testJacksonSerialization()
assertThat(new ObjectMapper().writeValueAsString(obj)).isEqualToIgnoringWhitespace(expected);
}

@Test
void testJacksonSerializeOrder()
Copy link
Member

Choose a reason for hiding this comment

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

a deserialization test would also be helpful, right?

@@ -126,6 +126,7 @@
<pojoConstructorVisibility>protected</pojoConstructorVisibility>
<useOneOfInterfaces>true</useOneOfInterfaces>
<enumUnknownDefaultCase>true</enumUnknownDefaultCase>
<stopAdditionalProperties>true</stopAdditionalProperties>
Copy link
Member

Choose a reason for hiding this comment

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

considering this never worked, isn't this essentially a bugfix and thus doesn't require a feature flag?

* effectively disabling serialization. Jackson by default only serializes the map entries and will ignore all
* fields from the model class.
*/
STOP_ADDITIONAL_PROPERTIES("stopAdditionalProperties", "false");
Copy link
Member

Choose a reason for hiding this comment

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

+1 for keeping track of the flags, but then we should add all of our internal flags here for consistency, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please merge Request to merge a pull request please review Request to review a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants