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

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

Open
5 of 6 tasks
notizklotz opened this issue Jan 5, 2021 · 14 comments
Open
5 of 6 tasks

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

notizklotz opened this issue Jan 5, 2021 · 14 comments

Comments

@notizklotz
Copy link

notizklotz commented Jan 5, 2021

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

The kotlin-spring generator generates multipart file upload attributes as org.springframework.core.io.Resource which actually cannot be handled by Spring Boot. org.springframework.web.multipart.MultipartFile should be generated instead: Uploading files guide

The spring generator handles this case correctly by applying some special case handling in the Mustache templates.

openapi-generator version

openapi-generator-maven-plugin:5.0.0

OpenAPI declaration file content or url

Multipart file upload sample spec

Steps to reproduce
  1. Generate server code using the kotlin-spring generator
  2. Generate server code using the spring generator
  3. Note that the spring generator generates the file paramter as List<MultipartFile> while the kotlin-spring generator generates List<Resource>
Related issues/PRs
Suggest a fix

Apply the same special case handling in the templates like the spring generator

@wing328
Copy link
Member

wing328 commented Jan 6, 2021

@notizklotz thanks for reporting the issue. Can you please file a PR with the suggested fix in the spring generator when you've time? We'll review accordingly

@notizklotz
Copy link
Author

Hi, yes I'll prepare a PR

@sylvainmoindron
Copy link
Contributor

back in 2019 the générator switched to Resources (my bad #2455) Since MultipartFile don't exist in reactive spring.

we should use MultipartFile whith spring web and FilePart with webflux

@koscejev
Copy link
Contributor

koscejev commented Mar 28, 2022

I stumbled upon the same problem for reactive. Indeed, FilePart can be provided by Spring's org.springframework.http.codec.multipart.DefaultPartHttpMessageReader, but this reader actually implements HttpMessageReader<Part> and it provides FilePart only if the Content-Disposition header contains filename. So the API must use Part, not FilePart and actual implementation logic (outside of the generator scope) can check whether it's FilePart.

Next, the problem is with the mentioned type - Part instead of Resource is already used correctly by Java Spring generator (except for arrays of files, where it uses List<Flux<Part>> for some reason - but that's a separate bug). But Kotlin Spring generator currently just uses Resource, which has no way of working. Because of suspend usage, the type should be Part for a single and Flow<Part> for multiple. But Flow<Part> doesn't work for some strange reason (it fails on some internal Spring lookup of the correct Reader and I don't understand the underlying problem in detail unfortunately), while Flux<Part> actually works (verified on our project via customizing templates).

So I propose changing it (at least for now) to Flux<Part> (even though Kotlin's Flow would normally be used here) when dealing with an array of file parts, and just Part when dealing with single file parts. There are still some unknowns - I haven't verified some more advanced usages, such as multiple multipart file fields (I've only verified single array file field).

@wing328
If this is OK, I can maybe try introducing the proper fix (not just template change) that would change Resource to Part and use Flux<Part> for an array of files - and submit PR to 6.0.x (since it'll be a breaking change).

@m-i-k-e-e
Copy link

Hey Guys,

It works with resources but you have to adapt the RequestPart parameter. By default it generates @RequestPart("file") but using @RequesPart("my_param_name") worked for me.

@moehringn
Copy link

I have the same issue, trying to upload multiple files within the same request. kotlin-spring generator does not respect the property name for the file:

      CreateMediaRequest:
        type: object
        properties:
          name:
            type: string
          type:
            $ref: '#/components/schemas/MediaType'
          file:
            type: string
            format: binary
          thumbnail:
            type: string
            format: binary

  fun createMediaPost(
    @Parameter(
      description = ""
    ) @RequestParam(value = "name", required = true) name: kotlin.String,
    @Parameter(
      description = "",
    ) @RequestParam(value = "type", required = true) type: MediaType,
    @Parameter(
      description = "file detail"
    ) @Valid @RequestPart("file") thumbnail: org.springframework.core.io.Resource,        
    @Parameter(
      description = "file detail"
    ) @Valid @RequestPart("file") file: org.springframework.core.io.Resource?
  ): ResponseEntity<MediaView> {
    return getDelegate().createMediaPost(name, type, thumbnail, file)
  }

As you can see, both @RequestPart("file") have the value "file" instead of "file" and "thumbnail"

@m-i-k-e-e how did you manage to set "my_param_name" in there?

@pbabu0192
Copy link

I ran into the same issue with multipart and kotlin-spring generator
@moehringn did you manage to fix this issue?
@m-i-k-e-e Could you please elaborate your solution?

@dds-pereihug
Copy link

one suggestion as a fix, could be:

  • on formParams.mustache
    {{#isFormParam}} {{^isFile}}{{#swagger2AnnotationLibrary}}@Parameter(description = "{{{description}}}"{{#required}}, required = true{{/required}}{{#allowableValues}}{{#defaultValue}}, schema = Schema(allowableValues = [{{#values}}"{{{.}}}"{{^-last}}, {{/-last}}{{/values}}]{{^isContainer}}, defaultValue = {{^isString}}"{{/isString}}{{#isString}}{{#isEnum}}"{{/isEnum}}{{/isString}}{{{defaultValue}}}{{^isString}}"{{/isString}}{{#isString}}{{#isEnum}}"{{/isEnum}}{{/isString}}{{/isContainer}}){{/defaultValue}}{{/allowableValues}}{{#allowableValues}}{{^defaultValue}}, schema = Schema(allowableValues = [{{#values}}"{{{.}}}"{{^-last}}, {{/-last}}{{/values}}]){{/defaultValue}}{{/allowableValues}}{{^allowableValues}}{{#defaultValue}}{{^isContainer}}, schema = Schema(defaultValue = {{^isString}}"{{/isString}}{{#isString}}{{#isEnum}}"{{/isEnum}}{{/isString}}{{{defaultValue}}}{{^isString}}"{{/isString}}{{#isString}}{{#isEnum}}"{{/isEnum}}{{/isString}}){{/isContainer}}{{/defaultValue}}{{/allowableValues}}){{/swagger2AnnotationLibrary}}{{#swagger1AnnotationLibrary}}@ApiParam(value = "{{{description}}}"{{#required}}, required = true{{/required}}{{#allowableValues}}, allowableValues = "{{#values}}{{{.}}}{{^-last}}, {{/-last}}{{/values}}"{{/allowableValues}}{{#defaultValue}}, defaultValue = "{{{.}}}"{{/defaultValue}}){{/swagger1AnnotationLibrary}} @RequestParam(value = "{{baseName}}"{{#required}}, required = true{{/required}}{{^required}}, required = false{{/required}}) {{{paramName}}}: {{>optionalDataType}} {{/isFile}} {{#isFile}}{{#swagger2AnnotationLibrary}}@Parameter(description = "{{{description}}}"){{/swagger2AnnotationLibrary}}{{#swagger1AnnotationLibrary}}@ApiParam(value = "file detail"){{/swagger1AnnotationLibrary}} {{#useBeanValidation}}@Valid{{/useBeanValidation}} @RequestPart("{{baseName}}"{{#required}}, required = true{{/required}}{{^required}}, required = false{{/required}}) {{{paramName}}}: MultipartFile{{/isFile}} {{/isFormParam}}
  • on apiInterface.mustache, just add the following import
    import org.springframework.web.multipart.MultipartFile

@OlgaErmolaeva
Copy link

@dds-pereihug Thank you so much for your solution. It works.
But I didn't add import org.springframework.web.multipart.MultipartFile in apiInterface.mustache. Instead I used the fully qualified class name in the formParams.mustache.

{{#isFormParam}} {{^isFile}}{{#swagger2AnnotationLibrary}}@Parameter(description = "{{{description}}}"{{#required}}, required = true{{/required}}{{#allowableValues}}{{#defaultValue}}, schema = Schema(allowableValues = [{{#values}}"{{{.}}}"{{^-last}}, {{/-last}}{{/values}}]{{^isContainer}}, defaultValue = {{^isString}}"{{/isString}}{{#isString}}{{#isEnum}}"{{/isEnum}}{{/isString}}{{{defaultValue}}}{{^isString}}"{{/isString}}{{#isString}}{{#isEnum}}"{{/isEnum}}{{/isString}}{{/isContainer}}){{/defaultValue}}{{/allowableValues}}{{#allowableValues}}{{^defaultValue}}, schema = Schema(allowableValues = [{{#values}}"{{{.}}}"{{^-last}}, {{/-last}}{{/values}}]){{/defaultValue}}{{/allowableValues}}{{^allowableValues}}{{#defaultValue}}{{^isContainer}}, schema = Schema(defaultValue = {{^isString}}"{{/isString}}{{#isString}}{{#isEnum}}"{{/isEnum}}{{/isString}}{{{defaultValue}}}{{^isString}}"{{/isString}}{{#isString}}{{#isEnum}}"{{/isEnum}}{{/isString}}){{/isContainer}}{{/defaultValue}}{{/allowableValues}}){{/swagger2AnnotationLibrary}}{{#swagger1AnnotationLibrary}}@ApiParam(value = "{{{description}}}"{{#required}}, required = true{{/required}}{{#allowableValues}}, allowableValues = "{{#values}}{{{.}}}{{^-last}}, {{/-last}}{{/values}}"{{/allowableValues}}{{#defaultValue}}, defaultValue = "{{{.}}}"{{/defaultValue}}){{/swagger1AnnotationLibrary}} @RequestParam(value = "{{baseName}}"{{#required}}, required = true{{/required}}{{^required}}, required = false{{/required}}) {{{paramName}}}: {{>optionalDataType}} {{/isFile}} {{#isFile}}{{#swagger2AnnotationLibrary}}@Parameter(description = "{{{description}}}"){{/swagger2AnnotationLibrary}}{{#swagger1AnnotationLibrary}}@ApiParam(value = "file detail"){{/swagger1AnnotationLibrary}} {{#useBeanValidation}}@Valid{{/useBeanValidation}} @RequestPart("{{baseName}}"{{#required}}, required = true{{/required}}{{^required}}, required = false{{/required}}) {{{paramName}}}: org.springframework.web.multipart.MultipartFile{{/isFile}} {{/isFormParam}}

@dds-pereihug
Copy link

Glad i could help @OlgaErmolaeva.

@wing328 dont know if there was a pr for this, but if u want i can provide my suggestion as form as PR.

@mkeller75
Copy link

mkeller75 commented Sep 2, 2024

Hi @wing328
I would also be very interested in a solution for the faulty multipart generation. Is it already foreseeable when the proposed fix from @dds-pereihug / @OlgaErmolaeva will be integrated?

@wing328
Copy link
Member

wing328 commented Sep 2, 2024

please kindly submit a PR if none has been submitted so far

we will review accordingly and try to get it merged before next release

@varminas
Copy link

varminas commented Sep 2, 2024

I truly appreciate the time and effort you've already put into addressing this matter.
This solution is very important to us.

@tomasbudnikov-ext90569
Copy link

Can you please let me know when the next release with this fix will be available? 🤞 Thank you for your effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests