-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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 spring server codegen improvements #1070
Kotlin spring server codegen improvements #1070
Conversation
3a1e75b
to
a41730d
Compare
Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors. Let me know if you need help fixing it. |
@@ -173,7 +173,7 @@ public AbstractKotlinCodegen() { | |||
typeMapping.put("DateTime", "java.time.LocalDateTime"); | |||
|
|||
instantiationTypes.put("array", "kotlin.arrayOf"); | |||
instantiationTypes.put("list", "kotlin.arrayOf"); | |||
instantiationTypes.put("list", "kotlin.listOf"); |
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 also impact other Kotline generators (e.g. client).
@jimschubert @dr4ke616, are you ok with that?
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 think for the spring boot code gen it should be fine, it does seem like an obvious mistake.
There are no diffs in other kotlin code generators when re-building the samples, so I'm guessing its ok. @jimschubert any thoughts?
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.
Array was being used over List because of the serializer. That's since been changed, so this shouldn't be an issue as far as language support is concerned. We could always just generate the client to hit a service created by the Springboot generator to verify.
However, this will break client consumers who have coded to array.
Another thing to consider is that Kotlin arrays are compiled to Java arrays. These are more lightweight than List. So this change may also have an impact on resource limited consumers (Android).
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.
@jimschubert Ah ok, I'll change it back in that case. Out of interest though is list being used as a type in OpenApi specs? I thought this map was from openapi types but I don't see list anywhere
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.
@herojan these types represent openapi data types as well as other common types that have been normalized in DefaultCodegen
. I'm on mobile, so it's hard to search GitHub, but if I remember correctly the DefaultCodegen logic (originally targeting mainly Java) treated OpenAPI array types as array of they were primitives or list if not. It's been about a year since I looked through that code, so that's pretty off memory. But, for example, you could have a type File
and instantiate it with some custom infrastructure type like MyFileMaker
.
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.
@jimschubert Ah right that makes sense, good to know. Thanks!
efc5500
to
de4ffd6
Compare
…w people to get and easily put back.
de4ffd6
to
34711dc
Compare
34711dc
to
be0d5cb
Compare
|
||
@get:NotNull | ||
@get:NotNull |
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.
Formatting here looks a little inconsistent
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.
Fixed that now
* If required and readonly, a field should be optional. This is to allow people to get and easily put back. * Use list instead of array, use String instead of kotlin.String etc * Update samples * code review: fix annotation formatting * code review: revert change to use listOf
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.3.x
,4.0.x
. Default:master
.@dr4ke616
Description of the PR