Avoid matching multipart parameters annotated with @ModelAttribute #3277
Conversation
| if (parameter.getParameterAnnotation(ProjectedPayload.class) != null | ||
| || parameter.getParameterAnnotation(ModelAttribute.class) != null) { | ||
| // Annotated parameter (excluding multipart @ModelAttribute) | ||
| if (parameter.hasParameterAnnotation(ProjectedPayload.class) || |
There was a problem hiding this comment.
It seems that the proxy handler does not properly handle multipart arguments, but the request param method handler does properly support multipart params. Is this the proper fix or should the proxy handler also support multipart args properly? cc: @odrotbohm @mp911de
There was a problem hiding this comment.
We have various aspects here. We should deprecate unannotated proxying as that has proven to cause quite some issues. We also need to have a plan how to achieve this and how our migration path would look like.
I'm thinking about:
- (backport) 3.4.x: Introduce a warning log if a parameter is not annotated with
@ProjectedPayloadthat this style is deprecated and that we will drop support for projections if a parameter is not annotated with@ProjectedPayload - 3.5: stop supporting parameters that are not annotated with
@ProjectedPayloadbut keep the logging - 4.0: remove logging
Introducing warn logging can cause quite some fallout, so we should be mindful about the conditions and maybe once we have seen a parameter, that we do not repeat logging (caching?) for that particular occurence.
onobc
force-pushed
the
GH-3258-dont-match-multipart-modelattribute
branch
from
50ed578 to
b340597
Compare
|
Oops, wrong rebase branch. Fixing now... |
The ProxyHandlerMethodArgumentResolver now avoids matching multipart parameters annotated with @ModelAttribute. This allows multipart parameters to be handled by RequestParamMethodArgumentResolver which properly handles multipart arguments. Fixes spring-projects#3258 Related tickets spring-projects#2937 Signed-off-by: Chris Bono <[email protected]>
onobc
force-pushed
the
GH-3258-dont-match-multipart-modelattribute
branch
from
b340597 to
5467563
Compare
The `@ProjectedPayload` annotation can now be used on parameters. This prepares for the upcoming removal of support for non-annotated projections. Fixes spring-projects#3258 Related tickets spring-projects#2937 Signed-off-by: Chris Bono <[email protected]>
onobc
force-pushed
the
GH-3258-dont-match-multipart-modelattribute
branch
from
5467563 to
54a85b9
Compare
| @Retention(RUNTIME) | ||
| @Target(TYPE) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target({ ElementType.TYPE, ElementType.PARAMETER }) |
There was a problem hiding this comment.
The only place I see this documented is in spring-data-commons/src/main/antora/modules/ROOT/pages/repositories/core-extensions.adoc and was not sure if we wanted to add this ability in there as it is pretty high-level. Wdyt?
| if (parameter.getParameterAnnotation(ProjectedPayload.class) != null | ||
| || parameter.getParameterAnnotation(ModelAttribute.class) != null) { | ||
| // Annotated parameter (excluding multipart) | ||
| if ((parameter.hasParameterAnnotation(ProjectedPayload.class) || parameter.hasParameterAnnotation( |
There was a problem hiding this comment.
Long-term (in 4.0) we will further investigate why this resolver does not support multipart file parameters. Until then, now that we allow @ProjectedPayload at the parameter level, we need to be sure to not handle @ProjectedPayload MultipartFile nor @ModelAttribute MultipartFile.
The ProxyHandlerMethodArgumentResolver now avoids matching multipart parameters annotated with @ModelAttribute. This allows multipart parameters to be handled by RequestParamMethodArgumentResolver which properly handles multipart arguments.
In 3.3.3 the
ProxyingHandlerMethodArgumentResolverwould report that it does not support the@ModelAttribute MultipartFile fileparameter and then the next handler in the chain (RequestParamMethodArgumentResolver) was able to properly handle the multipart file arg.In 3.3.4 (commit) the
ProxyingHandlerMethodArgumentResolverreports that it does support the@ModelAttribute MultipartFile fileparameter and then it fails to properly handle the multipart file arg.Fixes #3258
Related tickets #2937