InvocableHandlerMethod incorrectly resolves arguments by type

[garyrussell]

getMethodArgumentValues() performs early argument resolution by type; even when parameters are annotated and should be resolved by the resolvers.

@Test
void testInvocableHandlerMethod() throws Exception {
	TestClass bean = new TestClass();
	InvocableHandlerMethod handler = messageHandlerFactory().createInvocableHandlerMethod(bean,
			TestClass.class.getDeclaredMethod("defMethod", Object.class, String.class));
	handler.invoke(new GenericMessage<>("foo", Collections.singletonMap("bar", "baz")), "foo");
	assertThat(bean.object).isEqualTo("foo");
	assertThat(bean.bar).isEqualTo("baz");
}

private MessageHandlerMethodFactory messageHandlerFactory() {
	DefaultMessageHandlerMethodFactory defaultFactory = new DefaultMessageHandlerMethodFactory();
	DefaultFormattingConversionService cs = new DefaultFormattingConversionService();
	defaultFactory.setConversionService(cs);
	GenericMessageConverter messageConverter = new GenericMessageConverter(cs);
	defaultFactory.setMessageConverter(messageConverter);
	defaultFactory.afterPropertiesSet();
	return defaultFactory;
}

public static class TestClass {

	Object object;

	String bar;

	public void defMethod(Object obj, @Header("bar") String bar) {
		this.object = obj;
		this.bar = bar;
	}

}

Both parameters are resolved via the providedArgs (foo, foo).

Manifested in this Spring for Apache Kafka issue.

I can probably work around it in spring-kafka by subclassing and overriding findProvidedArgument() to ignore parameters with @Header annotations, but thought I should raise it here because the same issue would apply to other messaging apps.

Oops - findProvidedArgument is static, and I can't override getMethodArgumentValues because it uses private fields.

[rstoyanchev]

@garyrussell, indeed the providedArgs are matched by type and before argument resolvers. That much is mentioned in the javadoc:

* @param providedArgs "given" arguments matched by type, not resolved

Typical usages within the Spring Framework are for strong matches by type like HandlerMethod, javax.jms.Message, or Exception. Using something like String values in providedArgs is expected to lead to such side effects.

I don't have any ideas how to improve this. We can't move providedArgs after argument resolvers, because we already have default assumptions for unresolved arguments, e.g. treating them as @Payload. We could try and reduce the possibility for collision by checking that the argument isn't annotated, but some arguments don't have annotations.

What do these String values represent? Could there be another approach, e.g. to wrap them in some container object in order to be passed in safely as providedArgs, or could they be represented as headers?

If all else fails we could make findProvidedArgument an instance method.

[garyrussell]

@rstoyanchev Yes; I figured it would be hard to change; but on the other hand, given that this HandlerMethod is in spring-messaging and the subclass always calls it in the context of messaging (it has a Message<?> in addition to the providedArgs), it seems to me that findProvidedArgument() could skip any MethodParameter explicitly annotated with @Header. Maybe in 5.3 only, perhaps.

But, I can certainly work around it by providing the Kafka ConsumerRecord metadata in a container object instead of in discrete headers.

[rstoyanchev]

given that this HandlerMethod is in spring-messaging and the subclass always calls it in the context of messaging (it has a Message<?> in addition to the providedArgs), it seems to me that findProvidedArgument() could skip any MethodParameter explicitly annotated with @Header

Yes checking for a very specific annotation is certainly be possible. Do you still need it given the change to add ConsumerRecordMetadata? I was merely pointing out that String could lead to other collisions.