Skip to content

[KOTLIN Spring] add interfaceOnly option #3050

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

Merged
merged 23 commits into from
Jun 27, 2019
Merged

[KOTLIN Spring] add interfaceOnly option #3050

merged 23 commits into from
Jun 27, 2019

Conversation

fj-roman
Copy link
Contributor

@fj-roman fj-roman commented Jun 1, 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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

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

Description of the PR

Add interfaceOnly option for kotlin-spring. Now you can write down you own Controller Classes and implement the API-Interfaces.
I'm not certain about the combination of the properties interfaceOnly and serviceImplementation. Maybe it's confusing.

@fj-roman fj-roman changed the title Feature/spring kotlin interface only [KOTLIN Spring] add interfaceOnly option Jun 1, 2019
@wing328 wing328 added this to the 4.0.2 milestone Jun 3, 2019
@wing328
Copy link
Member

wing328 commented Jun 3, 2019

Can you please resolve the merge conflicts when you've time?

fj-roman added 9 commits June 3, 2019 07:39
# Conflicts:
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinSpringServerCodegen.java
#	modules/openapi-generator/src/main/resources/kotlin-spring/api.mustache
# Conflicts:
#	samples/server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/PetApi.kt
#	samples/server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/StoreApi.kt
#	samples/server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/UserApi.kt
# Conflicts:
#	samples/server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/PetApi.kt
#	samples/server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/StoreApi.kt
#	samples/server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/UserApi.kt
Copy link
Member

@jimschubert jimschubert left a comment

Choose a reason for hiding this comment

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

Thank you for this implementation. I think we'll need to put the interface definition into a file other than api.mustache and change the default behavior back to using api.mustache. If not, this becomes a breaking change.

I'll have to look at how we might make serviceImplementation and interfaceOnly options less confusing.

@@ -57,19 +55,19 @@ import kotlin.collections.Map
@RequestMapping("\${api.base-path:<%contextPath%>}")
<%={{ }}=%>
{{#operations}}
class {{classname}}Controller({{#serviceInterface}}@Autowired(required = true) val service: {{classname}}Service{{/serviceInterface}}) {
interface {{classname}} {
Copy link
Member

Choose a reason for hiding this comment

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

Changing api.mustache to an interface class breaks convention we use elsewhere, and will cause anyone with custom templates to break (as the default interfaceOnly=false loads a file that didn't previously exist.

@jimschubert
Copy link
Member

I've tagged this as "Breaking change without fallback" since it will break custom templates as-is.
Let me know if you'd like assistance in removing the breaking change and I can open a PR against this branch in your fork.

@fj-roman
Copy link
Contributor Author

fj-roman commented Jun 6, 2019

@jimschubert @wing328 I think the Shippable check is broke at the moment. I don't see any mistake by the PR.

@jimschubert
Copy link
Member

@fj-roman shippable has been failing to download dependencies for the docker image today.

@fj-roman
Copy link
Contributor Author

fj-roman commented Jun 10, 2019

@jimschubert I removed the breaking change, can you please review the PR again.

@wing328 wing328 removed this from the 4.0.2 milestone Jun 20, 2019
@wing328 wing328 added this to the 4.0.3 milestone Jun 20, 2019
fj-roman added 2 commits June 25, 2019 20:57
# Conflicts:
#	samples/server/petstore/kotlin-springboot-reactive/.openapi-generator/VERSION
#	samples/server/petstore/kotlin-springboot-reactive/src/main/kotlin/org/openapitools/api/PetApi.kt
#	samples/server/petstore/kotlin-springboot-reactive/src/main/kotlin/org/openapitools/api/StoreApi.kt
#	samples/server/petstore/kotlin-springboot-reactive/src/main/kotlin/org/openapitools/api/UserApi.kt
#	samples/server/petstore/kotlin-springboot-reactive/src/test/kotlin/org/openapitools/api/PetApiTest.kt
#	samples/server/petstore/kotlin-springboot-reactive/src/test/kotlin/org/openapitools/api/StoreApiTest.kt
#	samples/server/petstore/kotlin-springboot-reactive/src/test/kotlin/org/openapitools/api/UserApiTest.kt
@jimschubert
Copy link
Member

The breaking change was removed. This looks good at quick glance, but I'll try it out tonight and merge if no other concerns.

fj-roman added 4 commits June 27, 2019 09:28
# Conflicts:
#	samples/client/petstore/perl/t/AnotherfakeApiTest.t
# Conflicts:
#	samples/client/petstore/perl/t/AnotherFakeApiTest.t
@jimschubert jimschubert merged commit 73966a0 into OpenAPITools:master Jun 27, 2019
@jimschubert jimschubert deleted the feature/spring_kotlin_interface_only branch June 27, 2019 16:26
@jimschubert
Copy link
Member

@fj-roman sorry it took a bit to evaluate after you pinged me on Gitter, but this has been merged as of a couple days ago. Just wanted to notify in case you didn't see. Thanks for the contribution!

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.

3 participants