-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
[spring] Invalid default values for arrays and maps on API method declarations #1552
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
Conversation
…efault values in Spring MVC annotations for array and map parameters. Run the ./bin/spring-all-petstore.sh script to re-generate the Spring samples.
|
||
return super.toDefaultValue(p); | ||
} | ||
|
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.
@rubms thanks for the fix. What about overriding toDefaultValue
instead?
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 what I first tried, but it breaks the generated code: the default initialization of variables gets broken, causing the generated code not to compile. For instance, overriding toDefaultValue
, if no default value is specified in the spec for an array it will be initialized to nothing (instead of new ArrayList<>()), generating a non-compiling code:
List<String> listProperty = ;
This is why I overrode only the behavior for operation parameters, and not toDefaultValue
.
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 explanation. I think I know what issue you encountered.
I just fix a similar issue in another Java-related generator. Let me file a PR and have you to review my approach before deciding on which one to use.
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've filed #1725. Please review when you've time.
Moving forward, we may need to introduce toParameterDefaultValue and toPropertyDefaultValue for better control of default value in different scenarios.
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.
Sorry it took so long, I was on holidays. I agree, we need to introduce toParameterDefaultValue and toPropertyDefaultValue to better control both scenarios. I have reviewed the #1725 PR and it looks good to me!
Closed via #1725 |
@rubms thanks for the contribution, which has been included in the 4.0.0-beta release: https://twitter.com/oas_generator/status/1079727020374806529. Happy New Year and looking forward to more collaboration and contributions in 2019! |
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
and./bin/security/{LANG}-petstore.sh
if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\
.master
,3.4.x
,4.0.x
. Default:master
.C.C.: @bbdouglas (2017/07) @JFCote (2017/08) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01)
Description of the PR
Fixed issue #1551 that caused the generation of invalid default values in Spring MVC annotations for array and map parameters.
In Spring there are 2 different cases with respect to default values:
However the Spring code generator uses the same value (the default variable initializer, in my opinion) for both: initializing variables and specifying Spring MVC annotations.
This PR includes an overwrite of
fromParameter
for the SpringCodegen class in which the default value for parameters is taken from the one specified by the player. The default value for other types of properties (model properties, for instance) is not modified.