-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[Java] add missing nullable judgement when required property is true #18518
base: master
Are you sure you want to change the base?
Conversation
cb92534
to
fcc8bc4
Compare
@@ -398,7 +398,7 @@ public class {{classname}} {{#parent}}extends {{{.}}} {{/parent}}{{#vendorExtens | |||
{{#items.isModel}} | |||
{{#required}} | |||
// ensure the json data is an array | |||
if (!jsonObj.get("{{{baseName}}}").isJsonArray()) { | |||
if (!jsonObj.get("{{{baseName}}}").isJsonArray(){{#isNullable}} && !jsonObj.get("{{baseName}}").isJsonNull(){{/isNullable}}) { |
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.
thanks for the fix and the test.
shall the new condition {{#isNullable}} && !jsonObj.get("{{baseName}}").isJsonNull(){{/isNullable}}
be put in its own code block instead of adding to the JSON array type check as the error message right below only mentions expecting the field to be an array?
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.
thanks for the fix and the test.
shall the new condition
{{#isNullable}} && !jsonObj.get("{{baseName}}").isJsonNull(){{/isNullable}}
be put in its own code block instead of adding to the JSON array type check as the error message right below only mentions expecting the field to be an array?
Hi, I don't quite understand. It doesn't seem to make sense to bring up the nullable judgment alone, because nullable is just an auxiliary judgment. If we put in its own code block, the nullable judgment is set outside the original judgment. From the code point of view, the logic is the same. If you write a judgment to nullable alone to throw an exception, the whole logic will seem even more strange.
{{^isNullable}}
if (jsonObj.get("{{baseName}}").isJsonNull()) {
// throw exception here
}
{{/isNullable}}
// we will get an exception in the next judgement if json element is null
// ensure the json data is an array
if (!jsonObj.get("{{{baseName}}}").isJsonArray()) {
throw new IllegalArgumentException(String.format("Expected the field `{{{baseName}}}` to be an array in the JSON string but got `%s`", jsonObj.get("{{{baseName}}}").toString()));
}
Or this is the change you want.
{{#isNullable}}
if (!jsonObj.get("{{baseName}}").isJsonNull()) {
// ensure the json data is an array
if (!jsonObj.get("{{{baseName}}}").isJsonArray()) {
throw new IllegalArgumentException(String.format("Expected the field `{{{baseName}}}` to be an array in the JSON string but got `%s`", jsonObj.get("{{{baseName}}}").toString()));
}
}
{{/isNullable}}
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.
can you please PM me via Slack when you've time?
https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g
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.
ok👌
6f6a2b0
to
9dd27c9
Compare
06bb421
to
6375bc9
Compare
a4c531e
to
0de51eb
Compare
0de51eb
to
42a572b
Compare
fix #18516
Tests:
openapi.yaml
run following command to generate:
openapi-generator-cli generate -i openapi.yaml -o test -g java
And the code compiled successfully.
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.1.0 minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)