Affects: 6.1.6
This is a slight variation of SPR-17340 (#21874) that I've run into.
I have a Payload
class that derives from Map<String, Object>
:
public class Payload extends HashMap<String, Object> {
// some convenience methods here
}
And a method argument resolver for it:
public class PayloadMethodArgumentResolver extends RequestResponseBodyMethodProcessor {
...
@Override
public boolean supportsParameter(MethodParameter parameter) {
return parameter.getParameterType().equals(Payload.class);
}
...
@Override
public Object resolveArgument(MethodParameter parameter, ModelAndViewContainer mavContainer, NativeWebRequest webRequest, WebDataBinderFactory binderFactory) throws Exception {
Payload payload = new Payload();
...
return payload;
}
}
I want to use it like this:
@PostMapping("/host")
public ResponseEntity<?> createHost(Payload values) {
...
}
Note that there is no annotation before Payload
(it should not be necessary, since we have a custom type with a custom argument resolver. But, since there is no way to add my resolver to the top of the resolver list (org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.getDefaultArgumentResolvers()
), the MapMethodProcessor
will be used, returning an empty BindingAwareModelMap
. Since this does not match the required type of createHost
, an IllegalArgumentException
will be thrown in org.springframework.web.method.support.InvocableHandlerMethod.doInvoke(Object...)
.
I think the problem lies in org.springframework.web.method.annotation.MapMethodProcessor.supportsParameter(MethodParameter)
, which should not claim support for every subtype of Map (as it currently does). If the type check would be flipped like this, I think it should work correctly:
@Override
public boolean supportsParameter(MethodParameter parameter) {
return (parameter.getParameterType().isAssignableFrom(Map.class) &&
parameter.getParameterAnnotations().length == 0);
}
Only if the type of the parameter can be assigned a Map
should the MapMethodProcessor
be applied.
Related Issues
-
21874
-
23043
Comment From: sbrannen
Hi @effad,
Congratulations on submitting your first issue for the Spring Framework! 👍
I think the problem lies in
org.springframework.web.method.annotation.MapMethodProcessor.supportsParameter(MethodParameter)
, which should not claim support for every subtype of Map (as it currently does).
That seems like a reasonable assumption, and we'll investigate that approach.
If the type check would be flipped like this, I think it should work correctly:
Doing that alone unfortunately breaks existing support for ModelMap
as a controller method argument.
Thus, if we "flip the type check" in MapMethodProcessor
, we'll need to add back support for ModelMap
-- for example, in either MapMethodProcessor
or ModelMethodProcessor
.
If we decide to make such changes, we'll also want to make sure that use cases like your Payload
are supported in both Web MVC and WebFlux.
Comment From: sbrannen
The following change in MapMethodProcessor
is one possible solution for the above (for Web MVC).
@Override
public boolean supportsParameter(MethodParameter parameter) {
return ((parameter.getParameterType().isAssignableFrom(Map.class) ||
ModelMap.class.isAssignableFrom(parameter.getParameterType())) &&
parameter.getParameterAnnotations().length == 0);
}
Comment From: effad
FWIW I have worked around the problem for the moment, by putting my own PayloadMethodArgumentResolver
to the top of the list by overriding afterPropertiesSet
in RequestMappingHandlerAdapter
, like this:
@Override
protected RequestMappingHandlerAdapter createRequestMappingHandlerAdapter() {
return new RequestMappingHandlerAdapter() {
@Override
public void afterPropertiesSet() {
super.afterPropertiesSet();
List<HandlerMethodArgumentResolver> resolvers = new ArrayList<>(getArgumentResolvers());
resolvers.add(0, payloadMethodArgumentResolver());
setArgumentResolvers(resolvers);
}
};
}
This feels quite hacky, though :-).
Comment From: rstoyanchev
@sbrannen I think this would be fine https://github.com/spring-projects/spring-framework/issues/33160#issuecomment-2213512738. The goal is to be able to inject Map
or ModelMap
. Not sure there is a good reason to inject anything more specific, and is not guaranteed to work.
Comment From: jhoeller
Moving this to 6.2.x since we are not likely to resolve it for this week anymore, and there is a significant risk for side effects.