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

[Spring] replace MultipartFile by Resource #18509

Merged
merged 8 commits into from
Jun 25, 2024

Conversation

martin-mfg
Copy link
Contributor

@martin-mfg martin-mfg commented Apr 26, 2024

fix #18345

To reproduce the problem, it's sufficient to use the spring generator with delegatePattern=true on this input:

openapi: 3.0.3
info:
  title: Swagger Petstore - OpenAPI 3.0
  description: ''
  version: 1.0.0
paths:
  /dummy:
    post:
      description: ''
      operationId: uploadFile
      requestBody:
        content:
          application/octet-stream:
            schema:
              type: string
              format: binary
      responses:
        '200':
          description: successful operation

The output doesn't compile.
(edit: added mention of delegatePattern and fixed the input spec above.)

As far as I can tell, the spring generator was meant to switch to using Resource instead of MultipartFile in all cases. But some places in the templates were not adjusted. I did this now in this PR. And it worked well in all scenarios I could test.
(The server generated here runs, but requests to the /multipart-mixed endpoint didn't work out of the box for me, due to "Unsupported Media Type". Both before and after my changes. So that's probably a different issue.)

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/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    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*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.1.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.
    @cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09) @martin-mfg (2023/08)

@martin-mfg
Copy link
Contributor Author

Should this not be a MultipartFileResource?

Good question. I used org.springframework.core.io.Resource because that's what other parts of the spring generator are using already for files. see e.g. here

I anyway wanted to try MultipartFileResource after reading your comment. But it turns out this is not possible/not intended, because MultipartFileResource is actually not a public class.

(Since Resource is more generic than MultipartFileResource, another consideration would be to keep our generated interfaces as generic as possible.)

So I guess I'll keep using Resource. Do you agree?

@martin-mfg martin-mfg marked this pull request as ready for review April 26, 2024 13:32
@MelleD
Copy link
Contributor

MelleD commented Apr 26, 2024

Hey @martin-mfg Iam unsure if really Resource is right for a Multipart. I don't know why the other API are using it.

We are using MultipartFile and there is method

	default Resource getResource() {
		return new MultipartFileResource(this);
	}

See: https://docs.spring.io/spring-framework/reference/web/webmvc/mvc-controller/ann-methods/multipart-forms.html

Did you try if a resource is really working as MultiPart? Or can you then cast the Resource to a MultiPartResource?

@martin-mfg
Copy link
Contributor Author

SpringCodegen briefly used MultipartFile in the past, but then switched (back) to Resource. see #9555 for the change itself, and the links there for people encountering problems with MultipartFile.

Did you try if a resource is really working as MultiPart?

Yes, the generated server works and can receive multipart requests.

Or can you then cast the Resource to a MultiPartResource?

The actual type of the Resource parameter was ByteArrayResource in my test. But I guess this depends on the concrete input spec. And Resource can not be cast to MultiPart(Resource).

Can you explain a bit more what problems you see with using Resource? It sounds like you already have a generated spring server which uses MultiPartFile. But as described in #18345, this shouldn't even compile in the first place.

Anyway, I do agree that MultiPartFile might actually be more useful than Resource, if the problems from #9555 can be avoided. But in that case I would like to keep the focus of this PR on fixing the compilation problem described in #18345. Making SpringCodegen generally use MultiPartFile instead of Resource should then be a separate PR. Does that sound good to you?

@MelleD
Copy link
Contributor

MelleD commented Apr 27, 2024

Currently we have Spring Api with MultiPart. I can check my settings and which version. We also use the generated Spring Client. So this will then break our current Api and client. That's my point. And we need for a bigger upload a multipart.

I will look into the issue on monday or tuesday and come back with my generated code + settings

@MelleD
Copy link
Contributor

MelleD commented Apr 29, 2024

That's the generated code with open api generator 7.5.0

Spring Java

    @RequestMapping(
        method = RequestMethod.POST,
        value = "/packages",
        produces = { "application/json" },
        consumes = { "multipart/form-data" }
    )
    
    ResponseEntity<MyDto> uploadPackage(
         @RequestPart(value = "file", required = true) MultipartFile file
    );

Open API yaml

  /packages:
    post:
      tags:
        - Package
      operationId: uploadPackage
      requestBody:
        required: true
        content:
          multipart/form-data:
            schema:
              required:
                - file
              properties:
                file:
                  type: string
                  format: binary

Maven setting

                  <configuration>
                     <inputSpec>${project.basedir}my-api.yaml</inputSpec>
                     <generatorName>spring</generatorName>
                     <configOptions>
                        <library>spring-cloud</library>
                        <useSpringBoot3>true</useSpringBoot3>
                        <dateLibrary>java8</dateLibrary>
                        <useOptional>true</useOptional>
                        <singleContentTypes>false</singleContentTypes>
                        <hateoas>true</hateoas>
                        <useSwaggerUI>false</useSwaggerUI>
                        <useTags>true</useTags>
                        <documentationProvider>none</documentationProvider>
                     </configOptions>
                  </configuration>

And here is the spring guide:

@martin-mfg martin-mfg changed the title [Spring] replace MultiPartFile by Resource [Spring] replace MultipartFile by Resource Apr 30, 2024
@martin-mfg
Copy link
Contributor Author

Thanks for the input. I didn't have time to take a closer look yet, and this week I won't be available. But this PR is still on my to-do-list.
Btw, I updated the PR description to mention that the problem is related to using delegatePattern=true.

@martin-mfg martin-mfg marked this pull request as draft May 21, 2024 14:52
@martin-mfg martin-mfg marked this pull request as ready for review May 22, 2024 06:30
@martin-mfg
Copy link
Contributor Author

Hi @MelleD, I understood now that my initial changes were too broad, affecting also users which didn't encounter the original problem reported in #18345, while the switch from MultipartFile to Resource was not exactly an improvement for these users. So I reworked the PR to narrow down the scope.

@martin-mfg
Copy link
Contributor Author

Hi @MelleD and @wing328, do you mind reviewing this PR?

@MelleD
Copy link
Contributor

MelleD commented Jun 24, 2024

Looks good for me

@wing328 wing328 merged commit 045b601 into OpenAPITools:master Jun 25, 2024
49 checks passed
@wing328 wing328 added this to the 7.7.0 milestone Jun 25, 2024
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.

org.springframework.core.io.Resource cannot be converted to org.springframework.web.multipart.MultipartFile
3 participants