-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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] better default value handling #14130
Conversation
.../openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java
Outdated
Show resolved
Hide resolved
...ore/java/apache-httpclient/src/test/java/org/openapitools/client/model/DefaultValueTest.java
Outdated
Show resolved
Hide resolved
@leonard84 thanks for the feedback. Will definitely add tests before marking this PR ready for review. |
@leonard84 I've pushed some updates and added some tests. Note that there's new project (echo API client) with the newly added tests for default values of array property: https://github.com/OpenAPITools/openapi-generator/pull/14130/files#diff-43b8a464f4da55b9453d495bd235c5091b9a0de7a0d4ab0cfbc9fc66386cbe00 In order for the API tests to pass, you will need to run the following locally:
Please kindly review and let me know if you've further feedback. |
.../openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java
Outdated
Show resolved
Hide resolved
.../openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java
Outdated
Show resolved
Hide resolved
.../openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java
Outdated
Show resolved
Hide resolved
…en/languages/AbstractJavaCodegen.java Co-authored-by: Leonard Brünings <lord_damokles@gmx.net>
…en/languages/AbstractJavaCodegen.java Co-authored-by: Leonard Brünings <lord_damokles@gmx.net>
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 couldn't find any tests, that check the behavior of the new default values with regards to serialization/deserialization, either as json or query parameters.
I would expect at least the following scenarios:
- serialization/deserialization with no explicit values present
- serialization/deserialization with non-default values
@@ -572,7 +568,9 @@ public void toDefaultValueDateTimeLegacyTest() { | |||
defaultValue = codegen.toDefaultValue(dateSchema); | |||
|
|||
// dateLibrary <> java8 | |||
Assert.assertNull(defaultValue); | |||
if (defaultValue.contains("HKT")) { |
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.
❌ This will only work when the local ZoneId.systemDefault()
happens to be HKT
which I imagine is not the case for every developer. I'd suggest to test with a named ZoneId
, so that it is portable.
❗ Furthermore, I'd try to avoid conditionals in tests as much as possible. This condition can be false when it should actually be true and nothing would be failing.
@@ -827,7 +827,7 @@ public void maplikeDefaultValueForModelWithStringToStringMapping() { | |||
Assert.assertNull(cm.defaultValue, "Expected no defined default value in spec"); | |||
|
|||
String defaultValue = codegen.toDefaultValue(schema); | |||
Assert.assertEquals(defaultValue, "new HashMap<>()", "Expected string-string map aliased model to default to new HashMap<String, String>()"); | |||
Assert.assertEquals(defaultValue, "null", "Expected string-string map aliased model to default to null since nullable is not set 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.
❓ why is is set to null
, when nullable
is 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.
That's the old toDefaultValue(Schema schema)
not used by java generators any more, which use toDefaultValue(CodegenProperty cp, Schema schema)
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
@@ -81,7 +81,7 @@ public class NullableClass extends HashMap<String, Object> { | |||
private JsonNullable<List<Object>> arrayAndItemsNullableProp = JsonNullable.<List<Object>>undefined(); | |||
|
|||
public static final String JSON_PROPERTY_ARRAY_ITEMS_NULLABLE = "array_items_nullable"; | |||
private List<Object> arrayItemsNullable = null; | |||
private List<Object> arrayItemsNullable = new ArrayList<>(); |
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.
❓ If the field is nullable
shouldn't it be null
here?
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.
the array item is nullable, not the array itself:
array_items_nullable:
type: array
items:
type: object
nullable: 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.
got it
nullable
attribute to determine the default value of container (e.g.null
vsnew ArrayList<>()
)containerDefaultToNull
(false by default) to allow fallback to previous behaviourPR checklist
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*
.For Windows users, please run the script in Git BASH.
master
(6.3.0) (minor release - breaking changes with fallbacks),7.0.x
(breaking changes without fallbacks)cc @bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10)