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

Allow binding all MultipartFile instances as handler method argument [SPR-17405] #21938

Closed
spring-issuemaster opened this issue Oct 18, 2018 · 7 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Oct 18, 2018

Oliver Drotbohm opened SPR-17405 and commented

For cases in which the names of the files of a multipart request are not static but need to be dynamically calculated, it would be cool if there was a MultipartFiles object available for injection into a Spring MVC controller method similar to what we have with HttpHeaders.

The use case is to find a JSON document under a well known parameter name in a multipart request that then in turn contains the parameter names under which to lookup the files.

Using someMethod(@RequestParam("json") SomeModel model, MultipartHttpServletRequest request) already works but feels quite low level, especially if all you're interested in are the files.


Issue Links:

  • #18467 Part list/array gets resolved to all parts in current request
  • #18422 MultipartFile argument requires multipart request even when optional (and empty)

Referenced from: commits f0f1979

0 votes, 6 watchers

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 18, 2018

Rossen Stoyanchev commented

So basically support Map<String, Part> and/or Map<String, MultipartFile>.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 18, 2018

Oliver Drotbohm commented

I thought of a dedicated type as the reference documentation suggests that Map instances are considered the model.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 18, 2018

Rossen Stoyanchev commented

Yes but we do support Map arguments annotated with @RequestParam, @RequestHeader, and @PathVariable. In those cases the annotation is used to disambiguate among those various cases, but a Map with Part or MultipartFile values is pretty clear in terms of what it means. Likewise we already support Part and MultipartFile as collection or array. 

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 18, 2018

Oliver Drotbohm commented

Ah, that's interesting. Might be a good addition to the table of supported handler method types.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 18, 2018

Juergen Hoeller commented

Good point, a simple Map<String, MultipartFile> would go a long way here. Looking at this, it's arguably even an inconsistency between RequestParamMethodArgumentResolver and RequestParamMapMethodArgumentResolver in that the former supports multipart arguments but the latter doesn't. Also, MultipartHttpServletRequest supports programmatic retrieval of such a multipart map already, suggesting a corresponding injection option.

I'll try to sort this out for 5.1.2 still, probably as @RequestParam Map since that's an established pattern already, maybe also covering @RequestPart Map through a new RequestPartMapMethodArgumentResolver.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 19, 2018

Rossen Stoyanchev commented

@RequestParam Map sounds fine, but we still need to check for MultipartFile in the generic type, and if the Map is for MultipartFile values then @RequestParam could be optional. And we might as well check for Part too right? @RequestParam generally handles both. 

@RequestPart Map is another matter since for that we need to know the target the type to deserialize to which could vary for each part.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 24, 2018

Juergen Hoeller commented

I've added support for @RequestParam Map<String, MultipartFile> as well as @RequestParam Map<String, Part>. For the time being, I'm keeping this restricted to the dedicated annotation since that's also the only way to bind a Map of regular parameter values, and we don't react to specific map types in a non-annotated case elsewhere either. We may still relax this later on but preferably only as part of a wider revision in 5.2.

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.