Kotlin generator migration #26

tcslater · 2018-02-28T00:24:26Z

Migration of kotlin generator files from swagger-codegen.
Updated for compatability with 3.0 changes and template migration from mustache to handlebars as per https://github.com/swagger-api/swagger-codegen/wiki/Swagger-Codegen-migration-from-Mustache-and-Handlebars-templates

Related PR for cleanup in swaggen-codegen 3.0.0 swagger-api/swagger-codegen#7739

…0.0 [WIP]

…lpers

…ator

…rs String helpers instead

webron · 2018-02-28T01:31:12Z

Thanks, @tcslater!

@jimschubert - do you have time to help us review this PR?

jmini · 2018-03-02T06:24:24Z

This looks good. Thank you!

I have checked all the points of my migration guide:
https://github.com/swagger-api/swagger-codegen/wiki/Swagger-Codegen-migration-(swagger-codegen-generators-repository)

Here my review points:

I asked myself: will you not use the occasion of a major version change (Swagger v3) to update the name to kotlin-client instead of just kotlin ?
This way it would be clear in the language list that there is a client and a server.
I have tried your generators on the petstore example (the version I have used is here: petstore.json)
I get following error (with the client and with the server):

WARN io.swagger.codegen.languages.kotlin.AbstractKotlinCodegen - No Type defined for Property body
java.lang.NullPointerException
	at io.swagger.codegen.languages.kotlin.AbstractKotlinCodegen.toModelName(AbstractKotlinCodegen.java:473)
	at io.swagger.codegen.languages.kotlin.AbstractKotlinCodegen.getSchemaType(AbstractKotlinCodegen.java:300)
	at io.swagger.codegen.languages.DefaultCodegenConfig.fromModel(DefaultCodegenConfig.java:1285)
	at io.swagger.codegen.languages.DefaultCodegenConfig.fromRequestBody(DefaultCodegenConfig.java:2369)
	at io.swagger.codegen.languages.DefaultCodegenConfig.fromOperation(DefaultCodegenConfig.java:1943)
	at io.swagger.codegen.DefaultGenerator.processOperation(DefaultGenerator.java:855)
	at io.swagger.codegen.DefaultGenerator.processPaths(DefaultGenerator.java:782)
	at io.swagger.codegen.DefaultGenerator.generateApis(DefaultGenerator.java:421)
	at io.swagger.codegen.DefaultGenerator.generate(DefaultGenerator.java:729)
	at <my code>.convert(Swagger3ConverterMain.java:111)
Exception in thread "main" java.lang.RuntimeException: Could not process operation:
  Tag: class Tag {
    name: pet
    description: null
    externalDocs: null
}
  Operation: updatePetWithForm
  Resource: post /pet/{petId}
  Exception: null
	at io.swagger.codegen.DefaultGenerator.processOperation(DefaultGenerator.java:880)
	at io.swagger.codegen.DefaultGenerator.processPaths(DefaultGenerator.java:782)
	at io.swagger.codegen.DefaultGenerator.generateApis(DefaultGenerator.java:421)
	at io.swagger.codegen.DefaultGenerator.generate(DefaultGenerator.java:729)
	at <my code>.convert(Swagger3ConverterMain.java:111)
Caused by: java.lang.NullPointerException
	at io.swagger.codegen.languages.kotlin.AbstractKotlinCodegen.toModelName(AbstractKotlinCodegen.java:473)
	at io.swagger.codegen.languages.kotlin.AbstractKotlinCodegen.getSchemaType(AbstractKotlinCodegen.java:300)
	at io.swagger.codegen.languages.DefaultCodegenConfig.fromModel(DefaultCodegenConfig.java:1285)
	at io.swagger.codegen.languages.DefaultCodegenConfig.fromRequestBody(DefaultCodegenConfig.java:2369)
	at io.swagger.codegen.languages.DefaultCodegenConfig.fromOperation(DefaultCodegenConfig.java:1943)
	at io.swagger.codegen.DefaultGenerator.processOperation(DefaultGenerator.java:855)
	... 6 more

Can you have a look at it?

jmini

We have started to remove the guava dependency. See #5.

jmini · 2018-03-06T05:03:12Z

src/main/java/io/swagger/codegen/languages/kotlin/KotlinServerCodegen.java

+
+    // This is here to potentially warn the user when an option is not supoprted by the target framework.
+    private Map<String, List<String>> optionsSupportedPerFramework = new ImmutableMap.Builder<String, List<String>>()
+            .put(Constants.KTOR, Arrays.asList(


Please use Collections.singletonMap(Constants.KTOR, Arrays.asList(..)); instead of guava.

jmini · 2018-03-06T05:06:05Z

Nice! Thank you for the update.

By reviewing your code, I have noticed, tha you are using ImmutableMap from guava.
In this project, we have migrated guava logic to plain java elements. See #5.

I think you should do it too.

KotlinServerCodegen.<init>(KotlinServerCodegen.java:29)

jmini · 2018-03-06T07:32:14Z

An other point:

When I have tried to generate the kotlin-server I got this error:

Exception in thread "main" java.lang.RuntimeException: Could not generate api file for 'Pet'
	at io.swagger.codegen.DefaultGenerator.generateApis(DefaultGenerator.java:524)
	at io.swagger.codegen.DefaultGenerator.generate(DefaultGenerator.java:728)
	at <my code>.Swagger3ConverterMain.convert(Swagger3ConverterMain.java:132)
Caused by: java.io.FileNotFoundException: /v2/kotlin-server/api.mustache
	at com.github.jknack.handlebars.io.URLTemplateLoader.sourceAt(URLTemplateLoader.java:70)
	at com.github.jknack.handlebars.Handlebars.compile(Handlebars.java:357)
	at com.github.jknack.handlebars.Handlebars.compile(Handlebars.java:343)
	at io.swagger.codegen.DefaultGenerator.getHandlebars(DefaultGenerator.java:1018)
	at io.swagger.codegen.DefaultGenerator.processTemplateToFile(DefaultGenerator.java:741)
	at io.swagger.codegen.DefaultGenerator.generateApis(DefaultGenerator.java:483)
	... 4 more

I needed to add:

config.additionalProperties().put(CodegenConstants.LIBRARY, KotlinServerCodegen.DEFAULT_LIBRARY);

Maybe you could set the default library (when noting is set).

I am not using the CLI, I use the swagger-codegen-generators artifact and its dependencies directly.
I did not try kotlin with the maven plugin.

tcslater · 2018-03-06T23:05:43Z

Thanks. I have removed the guava dependency and fixed the default library option.

jimschubert · 2018-03-07T13:40:35Z

src/main/java/io/swagger/codegen/languages/kotlin/AbstractKotlinCodegen.java

+     * @return sanitized string
+     */
+    private String sanitizeKotlinSpecificNames(final String name) {
+        String word = removeNonNameElementToCamelCase(name);


This might cause problems. Whatever removeNonNameElementToCamelCase does, if it changes name at all, it may make enums stop working.

For instance, if an enum is last-call, this method previously would have converted this to last_call. If removeNonNameElementToCamelCase changes name to lastCall or lastcall, that expectation won't be met. I couldn't find the implementation of removeNonNameElementToCamelCase, so I can't say for certain.

This change was to fix an issue I was having where path parameters that were not in a valid kotlin format were not being converted for use as attributes in the generated data classes in Paths.kt. I'll look into it some more to make sure enums still work correctly.

jimschubert · 2018-03-07T13:48:40Z

src/main/java/io/swagger/codegen/languages/kotlin/KotlinClientCodegen.java

+    }
+
+    public String getName() {
+        return "kotlin-client";


Not really part of this PR, but this seems as good a place as any to comment.

I think it's weird that this value is still used as an option to "language" when generating code. It makes sense that this generator's name is "kotlin-client" (more sense than when I generate scala client code at work using custom templates with -l scalatra -- which is a server generator and not a language).

I had suggested in an issue in the swagger-codegen repo that there are multiple levels of generation that I think should be addressed:

language (e.g. Kotlin)

generator type (e.g. Client)

framework (e.g. Default, OkHttp, Retrofit)

Options: language version, framework version, etc.

To support a lot of this cleanly, you'd want shared templates that can be reused across framework options and across language/framework versions.

Great comment, @jimschubert. This might help with another issue we're looking to solve with regard to versioning. @HugoMario can you track that please?

I agree regarding duplication. All of the model templates for kotlin-client and kotlin-server are currently duplicated. These should be shared. I was going to try and refactor this but the only way I could think of getting it to work at the moment was to make the client generator a library option... so "-l kotlin --library=client" and "-l kotlin --library=ktor-server". This seemed a little weird so I opted not to pursue it in this PR

jimschubert · 2018-03-07T13:54:45Z

Sorry for the late response. Being from a repository I don't watch, this was sorted into a mail folder I hardly ever look at.

jmini · 2018-03-07T14:19:10Z

@jimschubert:
Thank you for this answer!

I think it's weird that this value is still used as an option to "language" when generating code. It makes sense that this generator's name is "kotlin-client" (more sense than when I generate scala client code at work using custom templates with -l scalatra -- which is a server generator and not a language).

I had suggested in an issue in the swagger-codegen repo that there are multiple levels of generation that I think should be addressed:

language (e.g. Kotlin)

generator type (e.g. Client)

framework (e.g. Default, OkHttp, Retrofit)

Options: language version, framework version, etc.

To support a lot of this cleanly, you'd want shared templates that can be reused across framework options and across language/framework versions.

I totally share this vision!

Redundancy in templates:

I have discovered this issue: swagger-api/swagger-codegen#4937 => not so much activity there.

Generator names:

For the moment -l is used to select the generator. This is the only switch that exists.
This PR introduces a new generator: kotlin-server. For the client generator, I prefer kotlin-client than kotlin.

webron · 2018-03-07T22:32:09Z

@jimschubert thanks for the review, no worries about the delay. If you don't think it's too much overload, maybe star this repo now. In the long run, the goal is to move all generators here.

…meters as described in open-api definition

Fixed Ktor Path template to generate nullable value for optional para…

jmini

With #40 I have proposed to avoid fields declaration that hides fields or variables of a parent class. It would be nice if you could adopt this convention for kotlin.

declare the LOGGER as private static in your 3 classes
do not redeclare enumPropertyNaming in KotlinClientCodegen.

Thank you in advance.

jmini · 2018-03-15T20:58:37Z

src/main/java/io/swagger/codegen/languages/kotlin/KotlinClientCodegen.java

+
+    public static final String DATE_LIBRARY = "dateLibrary";
+    protected CodegenConstants.ENUM_PROPERTY_NAMING_TYPE enumPropertyNaming = CodegenConstants.ENUM_PROPERTY_NAMING_TYPE.camelCase;
+    static Logger LOGGER = LoggerFactory.getLogger(KotlinClientCodegen.class);


LOGGER declaration should be private static

jmini · 2018-03-15T20:59:26Z

src/main/java/io/swagger/codegen/languages/kotlin/KotlinClientCodegen.java

+public class KotlinClientCodegen extends AbstractKotlinCodegen {
+
+    public static final String DATE_LIBRARY = "dateLibrary";
+    protected CodegenConstants.ENUM_PROPERTY_NAMING_TYPE enumPropertyNaming = CodegenConstants.ENUM_PROPERTY_NAMING_TYPE.camelCase;


enumPropertyNaming is already declared in AbstractKotlinCodegen. You just need to change the value (in the constructor for example) in the child class.

jmini · 2018-03-15T21:00:31Z

src/main/java/io/swagger/codegen/languages/kotlin/AbstractKotlinCodegen.java

+import java.util.Map;
+
+public abstract class AbstractKotlinCodegen extends DefaultCodegenConfig  {
+    static Logger LOGGER = LoggerFactory.getLogger(AbstractKotlinCodegen.class);


LOGGER declaration should be private static

jmini · 2018-03-15T21:00:53Z

src/main/java/io/swagger/codegen/languages/kotlin/KotlinServerCodegen.java

+    public static final String DEFAULT_LIBRARY = Constants.KTOR;
+    public static final String GENERATE_APIS = "generateApis";
+
+    static Logger LOGGER = LoggerFactory.getLogger(KotlinServerCodegen.class);


LOGGER declaration should be private static

…g declaration in child class

jmini · 2018-04-05T06:37:45Z

Is there something to do here? I think this could be merged.

…rror for Array Schema

SWCRS-83:

tcslater · 2018-04-09T01:58:45Z

Nothing more to be done that I'm aware of. I'd be happy for it to be merged.

HugoMario · 2018-04-10T00:58:48Z

thanks a lot @tcslater !!!

tcslater added 24 commits February 21, 2018 08:43

Migrating kotlin generator into swagger-codegen-genertors repo for 3.…

8fa4dd9

…0.0 [WIP]

Updated templates to work with handlebars

25e1447

Use v2 template path

d482bd0

Updating tests [WIP]

9a6b4df

Updated references to is*, isNot*, has*, hasNot* to use new is/has he…

7d5130b

…lpers

Merge branch 'master' into kotlin

4bb270b

Added kotlin codegen classes to service registry

f08aff7

Generate oneOf/anyOf/allOf/not attributes as kotlin.Any type

48f38f3

Initial implementation of type aliases support in kotlin generator

454e202

Initial implementation of type aliases support in kotlin-server gener…

745227c

…ator

Migrating kotlin-server ktor templates

4e402db

Migrating kotlin-server ktor templates

9ee0cec

Migrating kotlin-server ktor templates

8f75ab4

Further fixes to template migration

1264aa2

Remove custom httpMethodLowerCase property. Register and use Handleba…

9abfda7

…rs String helpers instead

Merge branch 'master' into kotlin

1d01ace

Removed commented out code

8dd95bc

Fixing constants in options test

9dcd190

Fixed array model test

e364587

Fix map generation

4adf9a3

Update map generation test to use schemas

3d8c13b

Fix complex model test

285d415

Cleaning up whitespace changes due to mustache/handlebars differences

bf01914

Merge branch 'master' into kotlin

a0df0d7

tcslater mentioned this pull request Feb 28, 2018

Kotlin generator migration/cleanup swagger-api/swagger-codegen#7739

Merged

HugoMario self-requested a review February 28, 2018 00:48

tcslater added 2 commits March 3, 2018 15:57

Merge branch 'master' into kotlin

425e84b

Fix scope iteration issue from mustache->handlerbars migration

9c03ffe

tcslater added 2 commits March 5, 2018 21:46

Merge branch 'master' into kotlin

34cd359

Implement getArgumentsLocation

22a5cdf

jmini suggested changes Mar 6, 2018

View reviewed changes

Remove guava dependency. Set default library option if not specified

a8e1ff1

tcslater added 2 commits March 7, 2018 10:31

Default generateApis option to true

416e199

Ensure that the parameter names in the path are valid kotlin names

af15071

jimschubert approved these changes Mar 7, 2018

View reviewed changes

tcslater and others added 5 commits March 8, 2018 14:45

Merge and fix conflicts in CodegenConfig

987b919

Merge branch 'master' into kotlin

4d45da2

Fixed Ktor Path template to generate nullable value for optional para…

8397e8d

…meters as described in open-api definition

Merge pull request #1 from claricepoh/kotlin-issues-46

9850e49

Fixed Ktor Path template to generate nullable value for optional para…

Add .vscode to .gitignore

2ec6d65

jmini suggested changes Mar 15, 2018

View reviewed changes

tcslater added 4 commits March 16, 2018 08:23

Made all LOGGERs private static. Removed duplicated enumPropertyNamin…

899d97e

…g declaration in child class

update ktor version to 0.9.1

c18a0ce

Merge branch 'master' into kotlin

73c00bf

Merge branch 'master' into kotlin

d5054e3

Clarice Poh and others added 3 commits April 6, 2018 14:54

SWCRS-83: Fixed Kotlin Generator to resolve typealiases compilation e…

0d502db

…rror for Array Schema

Merge branch 'kotlin' into kotlin-issues-46

0bb7f98

Merge pull request #3 from claricepoh/kotlin-issues-46

f15dc1b

SWCRS-83:

HugoMario merged commit c1b806c into swagger-api:master Apr 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kotlin generator migration #26

Kotlin generator migration #26

tcslater commented Feb 28, 2018 •

edited

Loading

webron commented Feb 28, 2018

jmini commented Mar 2, 2018

jmini left a comment

jmini Mar 6, 2018

jmini commented Mar 6, 2018

jmini commented Mar 6, 2018

tcslater commented Mar 6, 2018

jimschubert Mar 7, 2018

tcslater Mar 8, 2018

jimschubert Mar 7, 2018

webron Mar 7, 2018

tcslater Mar 8, 2018

jimschubert commented Mar 7, 2018

jmini commented Mar 7, 2018

webron commented Mar 7, 2018

jmini left a comment •

edited

Loading

jmini Mar 15, 2018

jmini Mar 15, 2018

jmini Mar 15, 2018

jmini Mar 15, 2018

jmini commented Apr 5, 2018

tcslater commented Apr 9, 2018

HugoMario commented Apr 10, 2018

Kotlin generator migration #26

Kotlin generator migration #26

Conversation

tcslater commented Feb 28, 2018 • edited Loading

webron commented Feb 28, 2018

jmini commented Mar 2, 2018

jmini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmini commented Mar 6, 2018

jmini commented Mar 6, 2018

tcslater commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimschubert commented Mar 7, 2018

jmini commented Mar 7, 2018

webron commented Mar 7, 2018

jmini left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmini commented Apr 5, 2018

tcslater commented Apr 9, 2018

HugoMario commented Apr 10, 2018

tcslater commented Feb 28, 2018 •

edited

Loading

jmini left a comment •

edited

Loading