openapi-generator
openapi-generator copied to clipboard
[Spring] replace MultipartFile by Resource
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
- [x] Read the contribution guidelines.
- [x] 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.
- [x] Run the following to build the project and update samples:
(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./mvnw clean package ./bin/generate-samples.sh ./bin/configs/*.yaml ./bin/utils/export_docs_generators.sh
./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. - [x] 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) - [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. @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)
Should this not be a MultipartFileResource? https://docs.spring.io/spring-framework/docs/5.1.8.RELEASE_to_5.1.9.RELEASE/Spring%20Framework%205.1.9.RELEASE/org/springframework/web/multipart/MultipartFileResource.html
See source: https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/web/multipart/MultipartFileResource.java
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?
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?
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?
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
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:
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
.
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.
Hi @MelleD and @wing328, do you mind reviewing this PR?
Looks good for me