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

[KOTLIN] Spring Boot Server Generator #820

Merged
merged 39 commits into from
Aug 27, 2018

Conversation

dr4ke616
Copy link
Contributor

@dr4ke616 dr4ke616 commented Aug 15, 2018

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 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\.
  • Filed the PR against the correct branch: master, 3.3.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description

This PR aims to add support for a Spring Boot Kotlin sever generator.

Sample Generation Scripts

  • bin/kotlin-springboot-petstore-server.sh
  • bin/openapi3/kotlin-springboot-petstore-server.sh
  • bin/windows/kotlin-springboot-petstore-server.bat
  • bin/windows/openapi3/kotlin-springboot-petstore-server.bat

Outstanding Tasks Related To This PR:

  • Additional Configuration options:
    • Swagger annotations
    • Service interfaces
    • Service implementations
    • Default exception handlers
  • Generate README.md
  • Enable optional bean validation using javax.validation.Valid
  • Use RestController instead of Controller annotation (see PR [Java][Spring] do webflux controllers the right way #571 for more info with regard to how it is achieved using the Spring Java generator) Edit: Outside of scope to this PR, will add in later PR.

CCing relevant contributors:

@wing328
Copy link
Member

wing328 commented Aug 15, 2018

@dr4ke616 thanks for the new generator 👍 . I'll review tomorrow and let you know if I've any feedback.

cc @jimschubert

@wing328
Copy link
Member

wing328 commented Aug 16, 2018

Linked to #819

@wing328
Copy link
Member

wing328 commented Aug 16, 2018

@dr4ke616 when you've time, please add samples/server/petstore/kotlin-springboot in the CircleCI pom.xml:

so that the samples will be tested with JDK7, JDK8

UPDATE: saw this line about targeting JDK8

@dr4ke616
Copy link
Contributor Author

@wing328 yup, this PR will only support JDK8 for now. I may submit another PR later with support for JDK7.

.gitignore Outdated
# Kotlin Spring Boot
samples/server/petstore/kotlin-springboot
samples/server/openapi3/petstore/kotlin-springboot

Copy link
Member

Choose a reason for hiding this comment

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

@dr4ke616 Can you please remove these lines so that the project will include these files to be tested by the CI?

Copy link
Member

Choose a reason for hiding this comment

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

I was going to ask you about this, then I saw that you commented on it.

Putting these directories into .gitignore is a great way to reduce noise while doing local development and awaiting feedback, though!

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.

This looks fantastic.

There are a few smaller questions that I don't feel are blocking merge, and which I didn't make any attempt to change (e.g. questions about commenting, intended workflow, and dependency versions).

There's one major question that I'd like feedback from others before merge, namely the use of kotlin-spring rather than adding this as a library under kotlin-server. @wing328 @jmini any thoughts about this? I'd obviously love to get a kotlin spring generator in ASAP, but I don't want to cause confusion especially when we're working toward standardizing and cleaning up the generator naming structure.

Some of my more minor comments about formatting were for my own tracking purposes and community documentation (people often review original implementation PRs as a starting point for implementing new generators).

I've opened a PR against your branch at dr4ke616#1 to address those minor fixes.

mvn clean package
fi

# if you've executed sbt assembly previously it will use that instead.
Copy link
Member

Choose a reason for hiding this comment

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

This comment doesn't make sense, as we don't use sbt to build this project (or kotlin)

mvn clean package
fi

# if you've executed sbt assembly previously it will use that instead.
Copy link
Member

Choose a reason for hiding this comment

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

Again, sbt comment doesn't make sense here. Seems like a copy/paste that we'll eventually want to clean up elsewhere as well.

name = name.substring(0, 2).toLowerCase() + name.substring(2);

// If name contains special chars -> replace them.
if ((((CharSequence) name).chars().anyMatch(character -> specialCharReplacements.keySet().contains("" + ((char) character))))) {
Copy link
Member

Choose a reason for hiding this comment

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

(CharSequence) cast is redundant here.

name = sanitizeName(name, "\\W-[\\$]");

if (name.toLowerCase().matches("^_*class$"))
return "propertyClass";
Copy link
Member

Choose a reason for hiding this comment

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

these ifs should probably have braces around their blocks to avoid early return issues and potential bugs in the event that we ever have an odd merge.

modelPackage = "org.openapitools.model";

// spring uses the jackson lib
additionalProperties.put("jackson", "true");
Copy link
Member

Choose a reason for hiding this comment

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

If this is the case, this will need to be done in processOpts otherwise a user can change this (intentionally or unintentionally) via options.

@@ -0,0 +1 @@
{{#required}}@NotNull {{/required}}{{>beanValidationCore}}
Copy link
Member

Choose a reason for hiding this comment

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

beanValidationCore template is missing in this PR, and causes generation to fail.

compile("org.jetbrains.kotlin:kotlin-reflect")
compile("org.springframework.boot:spring-boot-starter-web")
{{#swaggerAnnotations}}
compile("io.swagger:swagger-annotations:1.5.14")
Copy link
Member

Choose a reason for hiding this comment

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

Is this old swagger-annotations version due to compatibility issues with Spring Boot? Can it be bumped to latest pre-2.0 version (1.5.21)?

}

plugins {
val kotlinVersion = "1.2.41"
Copy link
Member

Choose a reason for hiding this comment

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

Are newer versions of kotlin (1.2.{50,51,60}) unsupported by Spring Boot?

@@ -0,0 +1 @@
{{contextPath}}
Copy link
Member

Choose a reason for hiding this comment

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

Is defaultBasePath.mustache here for compatibility or extension reasons? If not, can {{contextPath}} just be inserted where {{>defaultbasePath}} exists, to simplify template maintenance and customization?

I don't see {{>defaultbasePath}} anywhere, and I haven't looked too in-depth into the template inclusions to understand if it's potentially in the missing file.


# if you've executed sbt assembly previously it will use that instead.
export JAVA_OPTS="${JAVA_OPTS} -XX:MaxPermSize=256M -Xmx1024M -DloggerPath=conf/log4j.properties"
ags="$@ generate -i modules/openapi-generator/src/test/resources/2_0/petstore.yaml -g kotlin-spring -o samples/server/petstore/kotlin-springboot --additional-properties=library=spring-boot"
Copy link
Member

Choose a reason for hiding this comment

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

target -t with the templates under resource, and you don't need to rebuild the jar each time only a template is changed.

@dr4ke616
Copy link
Contributor Author

@jimschubert I've merged your suggested changes

@jimschubert
Copy link
Member

@dr4ke616 thanks. I think the only remaining big question is about the missing beanValidationCore.mustache. The inclusion causes generate to fail for me, and my fix was just to remove that. I didn't attempt compiling the generated server, and will look into it further a little later today or tomorrow.

I asked @wing328 about the kotlin-spring generator name and we don't think this needs to change at the moment. The Java server has a fully separate spring generator name, and from that perspective I think kotlin-spring is the preferred name.

We have an open feature request to split options for locating generators into framework, language, CodegenType. At that point, I think the kotlin-spring would change.

@dr4ke616
Copy link
Contributor Author

@jimschubert great, RE beanValidationCore.mustache I've a fix for that on the way, plus some minor changes regarding other comments made. Will push later today or tomorrow and let you know.

Regarding library naming, i will leave as kotlin-spring for the time being.

@dr4ke616
Copy link
Contributor Author

@jimschubert suggested changes made in ae9d383

@jimschubert
Copy link
Member

Tested against the new changes, and things look good. I added a stub object in the generated getPetById and verified the response. So the PR looks good to me. Thanks @dr4ke616 for the awesome contribution!

I have a few non-blocking questions, though…

I don't work with Spring, personally, and found it weird that it fails on finding an appropriate response type when making a GET request from the browser. The browser sends the following Accept header:

Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8

So, Spring should be falling back to application/json for the final */* option. Instead, it returns the following any time xml/xhtml+xml are defined:

<html>
    <body>
        <h1>Whitelabel Error Page</h1>
        <p>This application has no explicit mapping for /error, so you are seeing this as a fallback.</p>
        <div id='created'>Mon Aug 20 22:20:16 EDT 2018</div>
        <div>There was an unexpected error (type=Not Acceptable, status=406).</div>
        <div>Could not find acceptable representation</div>
    </body>
</html>

Is this because the spring starter dependency includes Thymeleaf or whatever the Spring templating for HTML is?

Also, I've found that the implementation won't construct a valid response unless I have exactly application/json or application/json; charset=utf-8. If I have application/xml as documented in the RequestMapping attribute, it also fails. Does this indicate that we'll need some additional configuration based on the xml template property?

/cc @wing328 or @jmini if one of you are familiar with Spring Boot would you mind looking this over to make sure I haven't missed something, and then merging?

@wing328 wing328 added this to the 3.2.2 milestone Aug 21, 2018
@wing328
Copy link
Member

wing328 commented Aug 21, 2018

/cc @wing328 or @jmini if one of you are familiar with Spring Boot would you mind looking this over to make sure I haven't missed something, and then merging?

@cbornet would you please review this PR when you've time?

@dr4ke616
Copy link
Contributor Author

dr4ke616 commented Aug 21, 2018

Ive made one final commit to this PR which I think it may be worth mentioning (see 530ab3f). Initially the mapping from date-time was to LocalDateTime, I updated to OffsetDateTime, similarly date was also mapped to LocalDateTime, now it is mapped to LocalDate.

I made this change to the AbstractKotlinCodegen, therefore i feel it worth mentioning in the case it has any negative impact on some of the existing Kotlin code generators which I may have overlooked. If there are issues, i can revert the commit and add the type mappings straight to KotlinSpringServerCodegen.

@dr4ke616
Copy link
Contributor Author

@jimschubert regarding your first question, I believe spring is responding with a HTML like response because within your Accept header is text/html. This is handled with Spring default error handler, which behaves differently depending on what that is in your Accept header. Some more info available here.

Regarding your second question, this is due to a missing jackson-dataformat-xml dependency (Fixed here - ba20ef7).

@wing328
Copy link
Member

wing328 commented Aug 22, 2018

Ive made one final commit to this PR which I think it may be worth mentioning (see 530ab3f). Initially the mapping from date-time was to LocalDateTime, I updated to OffsetDateTime, similarly date was also mapped to LocalDateTime, now it is mapped to LocalDate.

I suggest not to change the mapping in the abstract Kotlin class as it will impact thekotlin and kotlin-server generators. This is not to say OffsetDateTime is not appropriate.

For kotlin client to support OffsetDateTime, we can add another dateLibrary option in addition to what we provide today:

	dateLibrary
	    Option. Date library to use
	        string - String
	        java8 - Java 8 native JSR310
	        threetenbp - Threetenbp

For kotlin spring boot generator, I would suggest overriding the mapping locally instead of updating the default mapping in the abstract Kotlin class.

@jimschubert any concern to change the default mapping in the abstract Kotlin class, which will impact the Kotlin server generator you created?

@wing328 wing328 modified the milestones: 3.2.2, 3.2.3 Aug 22, 2018
@dr4ke616
Copy link
Contributor Author

@wing328 That's fair enough, I will revert the change and apply the mappings locally to the KotlinSpringServerCodegen.

@dr4ke616
Copy link
Contributor Author

Should be ready for final review.

  • Rebased changes on top of master
  • Reverted date time mappings in AbstractKotlinCodegen and added them locally to KotlinSpringServerCodegen (as @wing328 suggested)
  • Regenerated all samples

@dr4ke616 dr4ke616 force-pushed the kotlin-spring branch 3 times, most recently from b608e38 to be09af8 Compare August 22, 2018 16:16
dr4ke616 and others added 13 commits August 22, 2018 17:21
* Minor fixes from PR comments for user submission

* Puts braces around conditional block bodies with one-liner bodies.
* Modifies README.mustache to use artifact id and version supplied by
user (or default configuration)
* Targets templates under resource directory explicitly to prevent the
need to rebuild for evaluation of  template-only changes.

* [kotlin-spring] Remove comments referencing sbt in bash scripts
* Additional comments around how we set the title based off the open api spec
* Fixed missing `beanValidationCore` template
* Put the lambdas into the lambda object as other generators do (Ktor, C#, cpp)
* Bump swagger-annotations version to latest pre-2.0 version (1.5.21)
* Set kotlin version to 1.2.60
* Updated README to set port based on template
* Added more additional properties to build bash scripts
* Removed `defaultBasePath.mustache` in place of using {{contextPath}} directly
* Log warning for when `serviceImplementation` is set t o true
…t correctly catch the error and return bad request. Handling other exceptions a litle better
This reverts commit 4152dc78b4813da71c675272ca90fb31a333aea1.
@jimschubert
Copy link
Member

jimschubert commented Aug 23, 2018

OffsetDateTime is part of Java8, and isn't another date library.

It was originally implemented as LocalDateTime, if I remember correctly, to match how other generators work. It looks like maybe all JVM generators default to LocalDateTime. I agree that it should probably be OffsetDateTime because OAS defines date-time as targeting
RFC 3339 (https://xml2rfc.tools.ietf.org/public/rfc/html/rfc3339.html#anchor14).

The issue is that LocalDateTime#parse fails on standard ISO 8601 formats whereas OffsetDateTime does not. And while OAS requires rfc3339 for date-time, in reality most APIs support ISO 8601 in which zone information is optional.

An example is when the value is provided by JavaScript:

var d = new Date();
"2018-08-23T03:00:33.254Z"

Try this in a REPL, and you get failures on default parsing with LocalDateTime#parse:


scala> import java.time._
import java.time._

scala> import java.time.format._
import java.time.format._

scala> val input = "2018-08-23T03:00:33.254Z"
input: String = 2018-08-23T03:00:33.254Z

scala> val local = LocalDateTime.parse(input)
java.time.format.DateTimeParseException: Text '2018-08-23T03:00:33.254Z' could not be parsed, unparsed text found at index 23
  at java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:1952)
  at java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1851)
  at java.time.LocalDateTime.parse(LocalDateTime.java:492)
  at java.time.LocalDateTime.parse(LocalDateTime.java:477)
  ... 28 elided

scala> val off = OffsetDateTime.parse(input)
off: java.time.OffsetDateTime = 2018-08-23T03:00:33.254Z

Parse the same with formatting options and you get consistent parsing:

scala> val off = OffsetDateTime.parse(input, DateTimeFormatter.ISO_OFFSET_DATE_TIME)
off: java.time.OffsetDateTime = 2018-08-23T03:00:33.254Z

scala> val local = LocalDateTime.parse(input, DateTimeFormatter.ISO_OFFSET_DATE_TIME)
local: java.time.LocalDateTime = 2018-08-23T03:00:33.254

Now, suppose your input date time is the following (valid ISO 8601 without zone information):

scala> val input = "2018-08-23T03:00:33"
input: String = 2018-08-23T03:00:33

This fails to parse for everything except LocalDateTime#parse:

scala> val local = LocalDateTime.parse(input, DateTimeFormatter.ISO_OFFSET_DATE_TIME)
java.time.format.DateTimeParseException: Text '2018-08-23T03:00:33' could not be parsed at index 19
  at java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:1949)
  at java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1851)
  at java.time.LocalDateTime.parse(LocalDateTime.java:492)
  ... 28 elided

scala> val off = OffsetDateTime.parse(input, DateTimeFormatter.ISO_OFFSET_DATE_TIME)
java.time.format.DateTimeParseException: Text '2018-08-23T03:00:33' could not be parsed at index 19
  at java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:1949)
  at java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1851)
  at java.time.OffsetDateTime.parse(OffsetDateTime.java:402)
  ... 28 elided

scala> val local = LocalDateTime.parse(input)
local: java.time.LocalDateTime = 2018-08-23T03:00:33

scala> val off = OffsetDateTime.parse(input)
java.time.format.DateTimeParseException: Text '2018-08-23T03:00:33' could not be parsed at index 19
  at java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:1949)
  at java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1851)
  at java.time.OffsetDateTime.parse(OffsetDateTime.java:402)
  at java.time.OffsetDateTime.parse(OffsetDateTime.java:387)
  ... 28 elided

As far as I can tell, the assumption across JVM languages that this should be LocalDateTime may have been due to misreading the ABNF

date-time = full-date "T" full-time

We do allow for modifying these types at generation time, so I don't think it's a huge deal to replace how this is done across all generators. After all, clients should understand the date-time formats being used in their targeted APIs.

If anything, I think all JVM languages should be updated to support both parse methods with DateTimeFormatter.ISO_OFFSET_DATE_TIME as this is more consistent and works in both cases when inputs have time zone suffixes.

@wing328 do you have any historical context as to why the default for date-time was chosen as LocalDateTime over OffsetDateTime? Would you be opposed to changing this for all generators in the 4.0.0 release?

Edit: looks like it's Joda time's LocalDateTime in DefaultCodegen, and AbstractJavaCodegen as a weird java8-localdatetime which encodes date library options into the date library name. Some generators like JavaPlayFrameworkCodegen fully disallow the use of LocalDateTime by resetting the type/import mappings at the end of processOpts. I know it would be a major breaking change, but I think such cleaning up date-time across generators would ultimately benefit the code.

@cbornet
Copy link
Member

cbornet commented Aug 23, 2018

To give more info on this: the Java client and Spring generators are using OffsetDateTime since a long time now (I did the change from LocalDateTime to OffsetDateTime at the time because I fully agree with @jimschubert ). That's why there is a java8-localdatetime dateLibrary option : it was to keep a fallback for people that were using the older type. I guess the same could be done with all JVM generators.

@wing328
Copy link
Member

wing328 commented Aug 23, 2018

@wing328 do you have any historical context as to why the default for date-time was chosen as LocalDateTime over OffsetDateTime? Would you be opposed to changing this for all generators in the 4.0.0 release?

Sorry, I don't recall the history/reason of why LocalDateTime was used instead.

If no one has further feedback on this PR, I'll merge it tomorrow (Friday)

@cbornet
Copy link
Member

cbornet commented Aug 23, 2018

Some history : swagger-api/swagger-codegen#2138

@cbornet
Copy link
Member

cbornet commented Aug 23, 2018

Before that the use of LocalDateTime had been a bug, not a deliberate choice

@wing328 wing328 merged commit 8689227 into OpenAPITools:master Aug 27, 2018
@wing328
Copy link
Member

wing328 commented Aug 28, 2018

FYI. I've filed #918 with some minor improvements (license, README, etc)

@wing328
Copy link
Member

wing328 commented Aug 30, 2018

@dr4ke616 thanks again for the new generator, which has been included in the v3.2.3 release: https://twitter.com/oas_generator/status/1035200785066254336

@dr4ke616 dr4ke616 deleted the kotlin-spring branch August 30, 2018 19:52
ndbroadbent added a commit to DocSpring/openapi-generator that referenced this pull request Sep 6, 2018
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* Kotlin Spring initial bootstrap

* Basic configuration construction for Kotlin Spring

* Wired up with comand line client

* Initial kotlin spring boot application generated using gradle kotlin-dsl

* Added basic support for generating models

* Basic controllers generated without endpoints generated

* Basic spring boot app generated with models and controllers

* Added fix for type mapping in AbstractKotlinCodegen. Originally it was mapping list o kotlin.Array instead of kotlin.collections.List

* Fixed return type mapping

* Sorted bash springboot petstore generator script

* Implemented toVarName in AbstractKotlinCodegen to better handle some edgecases

* Checking for reserved words or numerical starting class names in AbstractKotlinCodegen

* Implemented toOperationId in AbstractKotlinCodegen

* Fixed types that were not correctly being mapped to primitives (byte / arrayOf / mapOf)

* Escaping dollar symbols in function names

* Added support for outter enum classes

* Added basic support for generating services

* Removed option for generated config package. Added option to enable/disable generated global exception handler

* Added configuration option to generate gradle. Generated maven pom.xml file as default

* Fixed up bash scripts for generating test sample code

* Added configurable option for Swagger Annotations

* Added configurable option for generating service interfaces and service implementations

* Added README generation

* Enable optional bean validation

* Added kotlin spring sample to CircleCI pom.xml

* Removed kotlin spring boot from .gitignore

* Minor fixes from PR comments for user submission (#1)

* Minor fixes from PR comments for user submission

* Puts braces around conditional block bodies with one-liner bodies.
* Modifies README.mustache to use artifact id and version supplied by
user (or default configuration)
* Targets templates under resource directory explicitly to prevent the
need to rebuild for evaluation of  template-only changes.

* [kotlin-spring] Remove comments referencing sbt in bash scripts

* List of changes based upon code review:
* Additional comments around how we set the title based off the open api spec
* Fixed missing `beanValidationCore` template
* Put the lambdas into the lambda object as other generators do (Ktor, C#, cpp)
* Bump swagger-annotations version to latest pre-2.0 version (1.5.21)
* Set kotlin version to 1.2.60
* Updated README to set port based on template
* Added more additional properties to build bash scripts
* Removed `defaultBasePath.mustache` in place of using {{contextPath}} directly
* Log warning for when `serviceImplementation` is set t o true

* Updated samples

* Generating ConstraintViolation Exception Handler, as Springboot doesnt correctly catch the error and return bad request. Handling other exceptions a litle better

* Small fix for date time mappings (plus sample re-gen)

* Minor fix in README template, where port was using wrong variable

* Fix missing jackson-dataformat-xml dependency

* Fix build - needed to re-run kotlin-server-petstore.sh

* Fixes after merge with master

* Revert "Small fix for date time mappings (plus sample re-gen)"

This reverts commit 4152dc78b4813da71c675272ca90fb31a333aea1.

* Moved type mappings to Kotlin Spring generator

* Regenerated samples

* Regenerated samples
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.

None yet

4 participants