Skip to content

[Kotlin][Spring] optional types for required=false without defaultValue#2487

Closed
markus-wa wants to merge 4 commits intoOpenAPITools:masterfrom
markus-wa:kotlin-spring-optional-params
Closed

[Kotlin][Spring] optional types for required=false without defaultValue#2487
markus-wa wants to merge 4 commits intoOpenAPITools:masterfrom
markus-wa:kotlin-spring-optional-params

Conversation

@markus-wa
Copy link
Contributor

@markus-wa markus-wa commented Mar 23, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{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\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

cc: @jimschubert (2017/09) @dr4ke616 (2018/08)

Description of the PR

This fixes a regression introduced (presumeably) by #1107 / #1106

The parameter types should only be non-nullable if they are either required or have a default value. Since defaultValue is "null" (a string) by default, mustache treats it as a value (not-false or unset) the {{#defaultValue}}bla{{/defaultValue}} branch was alway hit, never {{^defaultValue}}.....

This change follows a similar pattern of the C# generator which uses the helper property isNullable for this instead.

It also fixes Failed to convert value of type 'java.lang.String' to required type 'java.lang.Integer'; nested exception is java.lang.NumberFormatException: For input string: "null" if the optional parameter isn't provided in the request by removing the defaultValue parameter if it's "null".

@markus-wa
Copy link
Contributor Author

I'm personally not quite happy with this https://github.com/OpenAPITools/openapi-generator/pull/2487/files#diff-333e74d43089b01287cc3b8a0b0f6986R509

but I've put it as ready for review so we can gather feedback on it.

I've noticed that the regression actually seems to be coming from some change that changed the default value of defaultValue to "null" (string) instead of null. The reason I believe this is because the old sample code didn't have the , defaultValue="null" here https://github.com/OpenAPITools/openapi-generator/pull/2487/files#diff-f4ba6d632a20f27265f9bcc7c462e79fR59

Unfortunately I can't find the change, the only things I found around that area seem to be from 2016 or earlier.

@markus-wa markus-wa marked this pull request as ready for review March 23, 2019 22:26
@wing328
Copy link
Member

wing328 commented Mar 24, 2019

Unfortunately I can't find the change, the only things I found around that area seem to be from 2016 or earlier.

It could be related to a change I made as I did fix default values in some other generators. My apologies in advance.

Is it correct to say that the issue should be fix by changing the default value of defaultValue to null instead of "null" (string)? I think I can file a PR based on similar fix I did for some other generators?

(I'm aware of your workaround in https://github.com/OpenAPITools/openapi-generator/pull/2487/files#diff-333e74d43089b01287cc3b8a0b0f6986R509)

@markus-wa
Copy link
Contributor Author

Yeah changing it to null should do the trick. isNullable wouldn't be required either (I just left it in as a relic from my previous change c2f7148).

I think I can file a PR based on similar fix I did for some other generators?

That would be great, would probably be much cleaner than this here 😄!

@wing328
Copy link
Member

wing328 commented Mar 26, 2019

@markus-wa I've filed #2513 to fix the default value. Please review when you've time.

@markus-wa markus-wa mentioned this pull request Mar 26, 2019
4 tasks
@markus-wa
Copy link
Contributor Author

Closing in favour of #2513

@markus-wa markus-wa closed this Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments