-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
[Kotlin] Add a new additional property to configure Jackson's failOnUnknownProperties
#19506
[Kotlin] Add a new additional property to configure Jackson's failOnUnknownProperties
#19506
Conversation
…UnknownProperties` Default to false
@fistons thanks for the PR let us know if you need help to complete this PR cc |
You are welcome! I still need to work on it, I will let you know if I need help |
…ializationFeature`
Should be good now! |
if (!additionalProperties.containsKey(FAIL_ON_UNKNOWN_PROPERTIES)) { | ||
additionalProperties.put(FAIL_ON_UNKNOWN_PROPERTIES, 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.
does this allow setting FAIL_ON_UNKNOWN_PROPERTIES
to true?
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.
As far as I understand the generator, FAIL_ON_UNKNOWN_PROPERTIES
will be true if and only if the we specify the failOnUnknownProperties
option to true.
Am I right?
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.
I'm new to the repository, so I may be wrong here, but reading these lines, it looks as though we only update the property value (and to false) if the property isn't included by the user configuration.
The other properties are in this format:
if (additionalProperties.containsKey(KEY)) {
setKey(additionalProperties.get(KEY).toString());
}
such as:
if (additionalProperties.containsKey(REQUEST_DATE_CONVERTER)) {
setRequestDateConverter(additionalProperties.get(REQUEST_DATE_CONVERTER).toString());
}
Which reads as though - if provided, we will override the value based on the provided value. I'm sure a unit test will make this clear, though.
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.
Indeed, you are right, thanks!
I refactored this part and added a unit test for this.
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.
Should the changes in this file have a new unit test in KotlinClientCodegenModelTest?
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.
Sure, I will add one
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.
Done!
@wing328 can we get additional reviews on this? When is the cutoff to include this change in the 7.9.0 release on 2024-09-23? |
thanks for the PR, which has been merged |
This PR intents to fix #19408 and to have the same default behavior as the Java clients' i.e. being able to configure the Jackson's ObjectMapper FAIL_ON_UNKNOWN_PROPERTIES.
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master
(upcoming 7.6.0 minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)