[kotlin-spring] use spring resource for file handling #2455

sylvainmoindron · 2019-03-19T22:22:26Z

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\.
Filed the PR against the correct branch: master~~, 3.4.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 of the PR

Using spring ResponseEntity<Resource> for handling file when generating an API who return a file with

schema:  
  type: string
  format: binary

before it was ResponseEntity<File> which only responded filename

@jimschubert @dr4ke616

…dling

auto-labeler · 2019-03-19T22:22:30Z

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

jimschubert

I haven't used spring resource in this way. What's in the PR looks good side from the import of that full type.

Let me know if you need a hand looking into fixing it.

.../server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/PetApiServiceImpl.kt

sylvainmoindron · 2019-03-21T18:07:42Z

with the correct sample the CI check are OK

wing328 · 2019-03-22T02:56:38Z

samples/server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/PetApi.kt

@@ -56,7 +56,7 @@ class PetApiController(@Autowired(required = true) val service: PetApiService) {
    @RequestMapping(
            value = ["/pet/{petId}"],
            method = [RequestMethod.DELETE])
-    fun deletePet(@ApiParam(value = "Pet id to delete", required=true) @PathVariable("petId") petId: Long,@ApiParam(value = "" ) @RequestHeader(value="api_key", required=false) apiKey: String?): ResponseEntity<Unit> {
+    fun deletePet(@ApiParam(value = "Pet id to delete", required=true, defaultValue="null") @PathVariable("petId") petId: Long,@ApiParam(value = "" , defaultValue="null") @RequestHeader(value="api_key", required=false) apiKey: String): ResponseEntity<Unit> {


Looks like the default should be null (without double quotes) or should be omitted.

I don't think it's related to this change so we will need to fix it later.

I saw lot of change in the gérérated code (last time it was générated was 3.3.9 on Sep 2018).
i alsa saw a change in the body parameter name
(@Valid @RequestBody pet: Pet
changed to
@Valid @RequestBody body: Pet

@wing328 This looks like it's probably an issue with the spec document? An id path parameter, by definition, can't be null otherwise the request won't match the path definition.

there is another PR for this problem i think
#2487

sylvainmoindron · 2019-03-22T15:10:56Z

there is a problem at the moment for a binary body, the API use Resource, the interface ApiService use multipart. i'm looking for the problem

sylvainmoindron · 2019-03-22T16:22:27Z

i fixed the multipart problem

jimschubert · 2019-03-20T12:50:06Z

samples/server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/UserApi.kt

@@ -78,7 +78,7 @@ class UserApiController(@Autowired(required = true) val service: UserApiService)
    @RequestMapping(
            value = ["/user/{username}"],
            method = [RequestMethod.DELETE])
-    fun deleteUser(@ApiParam(value = "The name that needs to be deleted", required=true) @PathVariable("username") username: String): ResponseEntity<Unit> {
+    fun deleteUser(@ApiParam(value = "The name that needs to be deleted", required=true, defaultValue="null") @PathVariable("username") username: String): ResponseEntity<Unit> {


This looks unrelated to this PR, but I don't think passing a null from Spring's internals to a Kotlin String will work. We may be missing nullable type work on the input parameter.

jimschubert · 2019-03-25T02:40:40Z

samples/server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/PetApi.kt

@@ -56,7 +56,7 @@ class PetApiController(@Autowired(required = true) val service: PetApiService) {
    @RequestMapping(
            value = ["/pet/{petId}"],
            method = [RequestMethod.DELETE])
-    fun deletePet(@ApiParam(value = "Pet id to delete", required=true) @PathVariable("petId") petId: Long,@ApiParam(value = "" ) @RequestHeader(value="api_key", required=false) apiKey: String?): ResponseEntity<Unit> {
+    fun deletePet(@ApiParam(value = "Pet id to delete", required=true, defaultValue="null") @PathVariable("petId") petId: Long,@ApiParam(value = "" , defaultValue="null") @RequestHeader(value="api_key", required=false) apiKey: String): ResponseEntity<Unit> {


@wing328 This looks like it's probably an issue with the spec document? An id path parameter, by definition, can't be null otherwise the request won't match the path definition.

jimschubert · 2019-03-25T03:11:13Z

modules/openapi-generator/src/main/resources/kotlin-spring/serviceImpl.mustache

 @Service
 {{#operations}}
 class {{classname}}ServiceImpl : {{classname}}Service {
 {{#operation}}

-    override fun {{operationId}}({{#allParams}}{{paramName}}: {{^isFile}}{{>optionalDataType}}{{/isFile}}{{#isFile}}org.springframework.web.multipart.MultipartFile{{/isFile}}{{#hasMore}},{{/hasMore}}{{/allParams}}): {{>returnTypes}} {
+    override fun {{operationId}}({{#allParams}}{{paramName}}: {{#isFormParam}}{{#isFile}}org.springframework.web.multipart.MultipartFile{{/isFile}}{{^isFile}}{{{dataType}}}{{/isFile}}{{/isFormParam}}{{^isFormParam}}{{{dataType}}}{{/isFormParam}}{{#hasMore}},{{/hasMore}}{{/allParams}}): {{>returnTypes}} {


I think this needs to change org.springframework.web.multipart.MultipartFile to {{{dataType}}} to use Resource as well, otherwise it's hard-coded and the file type mapping will only work for models. I could be wrong as I've not used Resource, and jcenter is giving me 502 bad gateways so I can't download plugins required to build a sample.

Same issue in service.mustache and formParams.mustache.

At first I wanted to treat all file as Resource, but after watching other générator I thought that keeping Multipart for form was a better idea (i was inspired by the spring-java générator).
Maybe i'm wrong ,it's my first job on openapi-genrator.

i'm doing a POC where I remove all instance of MultipartFile; if i'm confident i break nothing, i will commit it.

I followed your recommendations and did a push

# 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

jimschubert · 2019-03-26T02:14:02Z

I tested this against the petstore simple yaml and looks like Resource is working as expected for file input.

I restarted Shippable (failed on Elm client tests), but I think this is good to go once checks have completed.

wing328 · 2019-03-27T09:13:59Z

Resolved the merge conflicts via 4f8a555

I'll update the Kotlin server samples later after merging the PR into master.

wing328 · 2019-03-27T14:17:15Z

Samples updated via 1313cff

sylvainmoindron added 2 commits March 19, 2019 19:10

[kotlin-spring] use org.springframework.core.io.Resource for file han…

0a51378

…dling

run ./bin/kotlin-springboot-petstore-server.sh

fa0aaab

auto-labeler bot added the Server: Kotlin label Mar 19, 2019

jimschubert reviewed Mar 19, 2019

View reviewed changes

.../server/petstore/kotlin-springboot/src/main/kotlin/org/openapitools/api/PetApiServiceImpl.kt Outdated Show resolved Hide resolved

run ./bin/kotlin-springboot-petstore-server.sh after building the CLI

d198be8

wing328 reviewed Mar 22, 2019

View reviewed changes

use MultipartFile for file in form and the specified type otherwise

d3d100c

remplace tab with space

69a2359

wing328 added the Issue: Bug label Mar 25, 2019

wing328 added this to the 4.0.0 milestone Mar 25, 2019

jimschubert reviewed Mar 25, 2019

View reviewed changes

sylvainmoindron added 3 commits March 25, 2019 10:18

[kotlin-springboot] replace all instance of MultipartFile by Resource

7dab7e8

run ./bin/kotlin-springboot-petstore-server.sh

b282f18

jimschubert self-assigned this Mar 26, 2019

Merge branch 'master' into master

4f8a555

wing328 merged commit 74fbd34 into OpenAPITools:master Mar 27, 2019

sylvainmoindron mentioned this pull request Feb 15, 2021

[BUG][KOTLIN-SPRING] Multipart file upload generate wrong type #8333

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kotlin-spring] use spring resource for file handling #2455

[kotlin-spring] use spring resource for file handling #2455

sylvainmoindron commented Mar 19, 2019

auto-labeler bot commented Mar 19, 2019

jimschubert left a comment

sylvainmoindron commented Mar 21, 2019

wing328 Mar 22, 2019

sylvainmoindron Mar 22, 2019

jimschubert Mar 25, 2019

sylvainmoindron Mar 25, 2019

sylvainmoindron commented Mar 22, 2019

sylvainmoindron commented Mar 22, 2019

jimschubert Mar 20, 2019

jimschubert Mar 25, 2019

jimschubert Mar 25, 2019

sylvainmoindron Mar 25, 2019

sylvainmoindron Mar 25, 2019

sylvainmoindron Mar 25, 2019

jimschubert commented Mar 26, 2019

wing328 commented Mar 27, 2019

wing328 commented Mar 27, 2019

[kotlin-spring] use spring resource for file handling #2455

[kotlin-spring] use spring resource for file handling #2455

Conversation

sylvainmoindron commented Mar 19, 2019

PR checklist

Description of the PR

auto-labeler bot commented Mar 19, 2019

jimschubert left a comment

Choose a reason for hiding this comment

sylvainmoindron commented Mar 21, 2019

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

sylvainmoindron commented Mar 22, 2019

sylvainmoindron commented Mar 22, 2019

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

Choose a reason for hiding this comment

jimschubert commented Mar 26, 2019

wing328 commented Mar 27, 2019

wing328 commented Mar 27, 2019