[Kotlin] Use mutable container types when 'modelMutable' is enabled by rm3l · Pull Request #11154 · OpenAPITools/openapi-generator

rm3l · 2021-12-19T23:52:15Z

This should close #11088.

Basically, when the Kotlin generators are run with modelMutable set to true, the changes here allow to generate read-write Mutable Kotlin container types (like MutableList or MutableMap) for OpenAPI arrays and dictionaries, instead of read-only types (like List or Map).
This way, it makes it possible to mutate such containers right away, without performing potentially expensive copies.

See #11088 for more context and details.

Maybe I missed something, but I noticed that unlike the other Kotlin generators, the kotlin-client one uses the raw templates rather than the Java source files to determine the different data types.
Please do let me know if things should be done differently.

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master (5.3.0), 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@jimschubert @dr4ke616 @karismann @Zomzog @andrewemery @4brunu @yutaka0m Could anyone of you please take a look at this ? Thanks.

4brunu · 2022-01-26T09:15:29Z

@rm3l could you please explain what this does and why it's needed? Thanks

Similar reasons as in my other comment below.

For example, given the following input operation:

/store/inventory: get: tags: - store summary: Returns pet inventories by status description: Returns a map of status codes to quantities operationId: getInventory produces: - application/json parameters: [] responses: '200': description: successful operation schema: type: object additionalProperties: type: integer format: int32

Without the changes here, this resulted in the following code generated by the kotlin-spring generator:

StoreApiController.kt : the Response field in the ApiResponse annotation has changed to MutableMap, which, I guess, is not valid.

diff --git a/samples/server/petstore/kotlin-springboot-modelMutable/src/main/kotlin/org/openapitools/api/StoreApiController.kt b/samples/server/petstore/kotlin-springboot-modelMutable/src/main/kotlin/org/openapitools/api/StoreApiController.kt index e2be73b4ec3..ad63efeae2d 100644 --- a/samples/server/petstore/kotlin-springboot-modelMutable/src/main/kotlin/org/openapitools/api/StoreApiController.kt +++ b/samples/server/petstore/kotlin-springboot-modelMutable/src/main/kotlin/org/openapitools/api/StoreApiController.kt @@ -58,7 +58,7 @@ class StoreApiController(@Autowired(required = true) val service: StoreApiServic responseContainer = "Map", authorizations = [Authorization(value = "api_key")]) @ApiResponses( value = [ApiResponse(code = 200, message = "successful operation", - response = kotlin.collections.Map::class, responseContainer = "Map")]) value = [ApiResponse(code = 200, message = "successful operation", + response = kotlin.collections.MutableMap::class, responseContainer = "Map")]) @RequestMapping( method = [RequestMethod.GET], value = ["/store/inventory"],

Again, maybe this is not the right approach here or maybe I missed something, but I found no clear way to distinguish between the type mappings used for generating models (Kotlin data classes) and response types in the kotlin-spring generator. Also, since the original issue I reported was more related to model mutability, I thought it would be ok not to change the code generated for the Spring Controllers.

Thanks for explaining, this way we have an explanation in case we need it 👍

4brunu · 2022-01-26T09:15:36Z

@rm3l could you please explain what this does and why it's needed? Thanks

Sure. While testing, I noticed that changing the type mappings to their Mutable types resulted in all types being changed as well in the generated Spring Request handlers methods.
This included the types for all query parameters. On the other side, the generated tests would still use the non-mutable types, resulting in the generated test code not compiling at all.

For example, given the following operation in an OpenAPI definition:

/pet/findByStatus: get: operationId: findPetsByStatus parameters: - name: status in: query required: true type: array items: type: string enum: - available - pending - sold

Without the changes here, this resulted in the following code generated by the kotlin-spring generator:

PetApiController.kt (notice that status is generated as a MutableList):

@RequestMapping( method = [RequestMethod.GET], value = ["/pet/findByStatus"] ) fun findPetsByStatus(... @RequestParam(value = "status", required = true) status: kotlin.collections.MutableList<kotlin.String> ): ResponseEntity<List<Pet>> { return ResponseEntity(service.findPetsByStatus(status), HttpStatus.valueOf(200)) }

PetApiTest.kt (which would not compile because api.findPetsByStatus expects a MutableList, but status is suprisingly generated as a List)

@Test fun findPetsByStatusTest() { val status:kotlin.collections.List<kotlin.String> = TODO() val response: ResponseEntity<List<Pet>> = api.findPetsByStatus(status) // TODO: test validations }

Maybe this is not the right approach here or maybe I missed something, but I found no clear way to distinguish between the type mappings used for generating models (Kotlin data classes) and request parameters in the kotlin-spring generator. Also, since the original issue I reported was more related to model mutability, I thought it would be ok not to change the code generated for the Spring Controllers.
Another possible approach I thought of that would fix the compilation issue was to make sure the test code generates request params as mutable containers instead.
Let me know your thoughts.

Thanks for explaining, this way we have an explanation in case we need it 👍

FYI, I added few comments in the code for reference.

4brunu · 2022-01-27T09:07:45Z

@rm3l As far as I can tell the mutable models are disabled by default, and since there is not sample project with it active, this change shouldn't happen, or am I wrong?

This file got changed right after I launched the bin/generate-samples.sh script, as recommended in the PR checklist.
From what I understood after reading the script, it generates samples for all configs under the bin/configs folder. It therefore picks the kotlin-spring-boot-modelMutable.yaml file, which generates a sample project with mutable models.
So I guess mutable models are indeed disabled by default, except in this sample project.

Thanks for explaining 👍

4brunu · 2022-01-27T09:09:32Z

@rm3l Could you please enable the mutable models in a sample project for this feature to be testes in every commit please?
In one of the kotlin config files in the following directory.
https://github.com/OpenAPITools/openapi-generator/tree/master/bin/configs

rm3l · 2022-01-27T21:19:49Z

@rm3l Could you please enable the mutable models in a sample project for this feature to be testes in every commit please? In one of the kotlin config files in the following directory. https://github.com/OpenAPITools/openapi-generator/tree/master/bin/configs

Sure. Indeed, since the existing kotlin-spring-boot-modelMutable.yaml config already enables model mutability for the kotlin-spring generator, I will add similar configs for the other Kotlin generators impacted by this feature. Thanks for spotting this.

rm3l · 2022-01-27T23:12:12Z

@rm3l Could you please enable the mutable models in a sample project for this feature to be testes in every commit please? In one of the kotlin config files in the following directory. https://github.com/OpenAPITools/openapi-generator/tree/master/bin/configs

Sure. Indeed, since the existing kotlin-spring-boot-modelMutable.yaml config already enables model mutability for the kotlin-spring generator, I will add similar configs for the other Kotlin generators impacted by this feature. Thanks for spotting this.

@4brunu I added additional configs to generate samples with model mutability enabled for all the other Kotlin generators impacted by this PR:

kotlin
kotlin-server
korlin-vertx
ktorm-schema

Could you please take a look when you get a chance? Thanks.

4brunu

Thanks for making the changes.
Looks good to me 👍

…del_mutable_is_true

rm3l · 2022-01-30T20:27:31Z

Great - thanks for your time and support, @4brunu!

martin-regis · 2022-03-25T11:49:25Z

Hi, it works great for MutableLists with default values, but unfortunately not for MutableSets with default values. As example, this is what currently will be generated when modelMutable is set to true:

@field:JsonProperty("foo", required = true) var foo: kotlin.collections.MutableList<kotlin.String> = arrayListOf("foo"),
        
@field:JsonProperty("bar", required = true) var bar: kotlin.collections.MutableSet<kotlin.String> = setOf("bar"), // should be mutableSetOf("bar")

My OAS:

foo:
  type: array
  items:
    type: string
  default: [ 'foo' ]
bar:          
  type: array
  items:
    type: string
  uniqueItems: true
  default: [ 'bar' ]

martin-regis · 2022-03-25T12:07:27Z

+                dataTypeAssigner.setReturnType(returnType.substring("kotlin.collections.MutableMap<".length(), end).split(",")[1].trim());
+                dataTypeAssigner.setReturnContainer("Map");
+            }
        }


I think there is a case missing for "MutableSet"

Hi @martin-regis could you please open a PR with a fix please?

wing328 added Client: Kotlin Enhancement: General Server: Kotlin labels Jan 2, 2022

wing328 added this to the 5.4.0 milestone Jan 2, 2022

rm3l force-pushed the use_specific_kotlin_container_type_when_model_mutable_is_true branch 2 times, most recently from 3f56d5f to d569dfb Compare January 10, 2022 23:49

4brunu reviewed Jan 14, 2022

View reviewed changes

Comment thread modules/openapi-generator/src/main/resources/kotlin-client/data_class_opt_var.mustache Outdated

rm3l force-pushed the use_specific_kotlin_container_type_when_model_mutable_is_true branch 2 times, most recently from 2570e60 to 1e3423f Compare January 25, 2022 21:16

4brunu reviewed Jan 26, 2022

View reviewed changes

4brunu reviewed Jan 27, 2022

View reviewed changes

rm3l added 5 commits January 27, 2022 23:54

[Kotlin] Use Mutable container types when 'modelMutable' is enabled

e1250a9

Generate sample 'kotlin' project with mutable models

10d0ad9

Generate sample 'kotlin-server' project with mutable models

c9d2482

Generate sample 'kotlin-vertx' project with mutable models

3d61382

Generate sample 'ktorm-schema' project with mutable models

de65993

rm3l force-pushed the use_specific_kotlin_container_type_when_model_mutable_is_true branch from 1e3423f to de65993 Compare January 27, 2022 23:03

4brunu approved these changes Jan 29, 2022

View reviewed changes

Merge branch 'master' into use_specific_kotlin_container_type_when_mo…

bdc9263

…del_mutable_is_true

4brunu merged commit ed14ce4 into OpenAPITools:master Jan 30, 2022

rm3l deleted the use_specific_kotlin_container_type_when_model_mutable_is_true branch January 30, 2022 20:27

martin-regis reviewed Mar 25, 2022

View reviewed changes

Uh oh!

Conversation

rm3l commented Dec 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rm3l Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rm3l Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

4brunu commented Jan 27, 2022

Uh oh!

rm3l commented Jan 27, 2022

Uh oh!

rm3l commented Jan 27, 2022

Uh oh!

4brunu left a comment

Choose a reason for hiding this comment

Uh oh!

rm3l commented Jan 30, 2022

Uh oh!

martin-regis commented Mar 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rm3l commented Dec 19, 2021 •

edited

Loading

rm3l Jan 26, 2022 •

edited

Loading

rm3l Jan 26, 2022 •

edited

Loading