-
Notifications
You must be signed in to change notification settings - Fork 14
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
base: main
Are you sure you want to change the base?
Conversation
"""; | ||
final Order order = Order.create().productId(100L).quantity(5).totalPrice(6.0f); | ||
order.setCustomField("shoesize", 44); | ||
assertThat(new ObjectMapper().writeValueAsString(order)).isEqualToIgnoringWhitespace(expected); |
There was a problem hiding this comment.
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
}
There was a problem hiding this 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() |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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?
Context
https://github.tools.sap/AI/ai-sdk-java-backlog/issues/129
additionalProperties: true
will appendextends HashMap<String,Object>
to generated class signature. See intermediate commit 7bd61edserialVersionUID
. See 7bd61ed#r151192960super#entrySet()
.However
Feature scope:
Definition of Done
Error handling created / updated & covered by the tests aboveDocumentation updatedRelease notes updated