Skip to content
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

Various fix for Kotlin templates #3504

Merged
merged 4 commits into from
Aug 11, 2019

Conversation

gimlet2
Copy link
Contributor

@gimlet2 gimlet2 commented Jul 31, 2019

Fix escaping of dataType field.
Fix handling of arrays/lists of enums.
Update okhttp to 4.0.1
main issue was:
val `data`: ApiResponseListString.`Data`? = null, ... enum class `Data`(val value: kotlin.collection.List<kotlin.String>)
instead of
val `data`: kotlin.collection.List<ApiResponseListString.`Data`>? = null,` `enum class `Data`(val value: kotlin.String)

Andrei Chernyshev added 2 commits July 31, 2019 11:55
Fix escaping of dataType field.
Fix handling of arrays/lists of enums.
@@ -176,6 +176,7 @@ public AbstractKotlinCodegen() {
typeMapping.put("binary", "kotlin.Array<kotlin.Byte>");
typeMapping.put("Date", "java.time.LocalDate");
typeMapping.put("DateTime", "java.time.LocalDateTime");
typeMapping.put("ref", "kotlin.Any");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gimlet2 I don't think there's a type called "ref".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @wing328! Yes, there is no such type. But in spec I've got i see { "name": "request", "in": "formData", "description": "request", "required": false, "type": "ref" }
And that was generated by springfox swagger. I think that can be fixed some other way and will be happy to do it if you can recommend me how.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's bug in springfox based on the information you've provided as type: ref is not defined in the official openapi/swagger spec.

Given that it's a form parameter, what exactly is that? string? float?

What does the HTTP request payload look like?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref is not defined in the OpenAPI/Swagger spec. Please use a validator to validate the API spec/document first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should include this in the generator, and rather require those who need it to pass this as a type mapping at generation time (see CLI options for example.

A ref in OpenAPI means something completely different than a top-level object type. Adding this means we need to support it through a deprecation cycle, and it may cause problems for others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Removed

@wing328
Copy link
Member

wing328 commented Aug 1, 2019

cc @jimschubert (2017/09), @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04)

@gimlet2
Copy link
Contributor Author

gimlet2 commented Aug 8, 2019

Dear reviewers, can you give me please a hint what is wrong with Shippable ci job and how to fix it?
@wing328 @jimschubert

@wing328
Copy link
Member

wing328 commented Aug 8, 2019

I've restarted the shippable job. Let's see how it goes.

@gimlet2
Copy link
Contributor Author

gimlet2 commented Aug 9, 2019

@wing328 Thank you for rerunning shippable. Looks like all checks are ok now. Should I fix something else, or it would be fine to merge at some point?

@jimschubert
Copy link
Member

jimschubert commented Aug 11, 2019

@gimlet2 I noticed there's an additional isList property being added which doesn't seem to be used and will cause confusion with isListContainer. Could you verify that this isn't being used, or that I'm not mistaken and missed it?

Sorry for the delay, we had PR merges on hold for the 4.1.0 release.

edit: After the explanation in your comment, I looked again and saw the usage in the templates. I just overlooked it previously.

@jimschubert
Copy link
Member

Thanks for the contribution! My previous question was a matter of my overlooking the value in the template.

@jimschubert jimschubert merged commit 07381e7 into OpenAPITools:master Aug 11, 2019
@jimschubert jimschubert added this to the 4.1.1 milestone Aug 11, 2019
@gimlet2
Copy link
Contributor Author

gimlet2 commented Aug 12, 2019

@jimschubert @wing328 Thank you for help with fixing it! 👍

@wing328 wing328 changed the title Fix kotlin templates Various fix for Kotlin templates Aug 26, 2019
@wing328
Copy link
Member

wing328 commented Aug 26, 2019

@gimlet2 thanks for the PR, which has been included in the v4.1.1 release: https://twitter.com/oas_generator/status/1165944867391860737

@4brunu
Copy link
Contributor

4brunu commented Sep 30, 2019

I'm not sure if this is the right place or if I should open an issue, but in this PR, the OkHttp version was bumped from 3.14.2 to 4.0.1.

This has a side effect that the Kotlin-client no longer supports Android 4.X version, it only supports Android 5.0 and upwards, because OkHttp 4.X only supports Android 5.0 and upwards.
https://github.com/square/okhttp/blob/master/README.md#requirements

But the Android 4.X version still represents 10% of the global Android market share, and some companies still need to support those devices.
https://developer.android.com/about/dashboards/

I'm not sure if this change was an accident, but it would be great if Kotlin-client supports Android 4.X.

So we have three possibilites:

  • supports OkHttp 4.X and 3.Y in parallel
  • revert this change and only support OkHttp 3.Y
  • only support OkHttp 4.X

What are your thoughts on this? Should I create an issue with this?

EDIT: I created an issue #4007

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.

5 participants