MapMethodProcessor should only resolve arguments of type Map or the ModelMap hierarchy

[effad]

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

• MapMethodProcessor precludes custom argument resolvers with Map and custom annotation [SPR-17340] #21874
• Spring MVC custom argument resolvers order #23043

[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.

[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);
}

[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 :-).

[rstoyanchev]

@sbrannen I think this would be fine #33160 (comment). 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.

[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.