Skip to content

INT-4571 Make MessageHandlerMethodFactory injectable #2688

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

INT-4571 Make MessageHandlerMethodFactory injectable #2688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion ...n-core/src/main/java/org/springframework/integration/context/IntegrationContextUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2018 the original author or authors.
* Copyright 2002-2019 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -36,6 +36,7 @@
* @author Josh Long
* @author Artem Bilan
* @author Gary Russell
* @author Oleg Zhurakousky
*/
public abstract class IntegrationContextUtils {

Expand Down Expand Up @@ -105,6 +106,8 @@ public abstract class IntegrationContextUtils {

public static final String DISPOSABLES_BEAN_NAME = "integrationDisposableAutoCreatedBeans";

public static final String MESSAGE_HANDLER_FACTORY_BEAN_NAME = "integrationMessageHandlerMethodFactory";

/**
* @param beanFactory BeanFactory for lookup, must not be null.
* @return The {@link MetadataStore} bean whose name is "metadataStore".
Expand Down
5 changes: 3 additions & 2 deletions ...org/springframework/integration/handler/support/HandlerMethodArgumentResolversHolder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2017-2018 the original author or authors.
* Copyright 2017-2019 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -28,8 +28,9 @@
* @author Gary Russell
*
* @since 5.0
*
* @deprecated as of 5.1.2. Instead simply configure your own MessageHandlerMethodFactory as a bean.
*/
@Deprecated
public class HandlerMethodArgumentResolversHolder {

private final List<HandlerMethodArgumentResolver> resolvers;
Expand Down
92 changes: 56 additions & 36 deletions ...in/java/org/springframework/integration/handler/support/MessagingMethodInvokerHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@
import org.springframework.messaging.handler.annotation.Headers;
import org.springframework.messaging.handler.annotation.Payload;
import org.springframework.messaging.handler.annotation.support.DefaultMessageHandlerMethodFactory;
import org.springframework.messaging.handler.annotation.support.MessageHandlerMethodFactory;
import org.springframework.messaging.handler.invocation.HandlerMethodArgumentResolver;
import org.springframework.messaging.handler.invocation.InvocableHandlerMethod;
import org.springframework.messaging.handler.invocation.MethodArgumentResolutionException;
Expand Down Expand Up @@ -167,7 +168,7 @@ public class MessagingMethodInvokerHelper<T> extends AbstractExpressionEvaluator
SPEL_COMPILERS.put(SpelCompilerMode.MIXED, EXPRESSION_PARSER_MIXED);
}

private final DefaultMessageHandlerMethodFactory messageHandlerMethodFactory =
private MessageHandlerMethodFactory messageHandlerMethodFactory =
new DefaultMessageHandlerMethodFactory();

private final Object targetObject;
Expand All @@ -184,7 +185,7 @@ public class MessagingMethodInvokerHelper<T> extends AbstractExpressionEvaluator

private final List<Map<Class<?>, HandlerMethod>> handlerMethodsList;

private final HandlerMethod handlerMethod;
private HandlerMethod handlerMethod;

private final TypeDescriptor expectedType;

Expand Down Expand Up @@ -257,16 +258,7 @@ private MessagingMethodInvokerHelper(Object targetObject, Class<? extends Annota

Assert.notNull(targetObject, "targetObject must not be null");
this.targetObject = targetObject;
try {
InvocableHandlerMethod invocableHandlerMethod =
this.messageHandlerMethodFactory.createInvocableHandlerMethod(targetObject, method);
this.handlerMethod = new HandlerMethod(invocableHandlerMethod, canProcessMessageList);
this.defaultHandlerMethod = null;
checkSpelInvokerRequired(getTargetClass(targetObject), method, this.handlerMethod);
}
catch (IneligibleMethodException e) {
throw new IllegalArgumentException(e);
}
createHandlerMethod();
this.handlerMethods = null;
this.handlerMessageMethods = null;
this.handlerMethodsList = null;
Expand Down Expand Up @@ -295,7 +287,8 @@ public void setUseSpelInvoker(boolean useSpelInvoker) {
@Override
public void setBeanFactory(@NonNull BeanFactory beanFactory) {
super.setBeanFactory(beanFactory);
this.messageHandlerMethodFactory.setBeanFactory(beanFactory);
((DefaultMessageHandlerMethodFactory) this.messageHandlerMethodFactory).setBeanFactory(beanFactory);

if (beanFactory instanceof ConfigurableListableBeanFactory) {
BeanExpressionResolver beanExpressionResolver = ((ConfigurableListableBeanFactory) beanFactory)
.getBeanExpressionResolver();
Expand All @@ -310,7 +303,7 @@ public void setBeanFactory(@NonNull BeanFactory beanFactory) {
public void setConversionService(ConversionService conversionService) {
super.setConversionService(conversionService);
if (conversionService != null) {
this.messageHandlerMethodFactory.setConversionService(conversionService);
((DefaultMessageHandlerMethodFactory) this.messageHandlerMethodFactory).setConversionService(conversionService);
}
}

Expand Down Expand Up @@ -403,6 +396,24 @@ private MessagingMethodInvokerHelper(Object targetObject, Class<? extends Annota
this.jsonObjectMapper = mapper;
}

private boolean isProvidedMessageHandlerFactoryBean() {
return getBeanFactory() != null
&& getBeanFactory().containsBean(IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME);
}

private void createHandlerMethod() {
try {
InvocableHandlerMethod invocableHandlerMethod =
this.messageHandlerMethodFactory.createInvocableHandlerMethod(this.targetObject, this.method);
this.handlerMethod = new HandlerMethod(invocableHandlerMethod, this.canProcessMessageList);
this.defaultHandlerMethod = null;
checkSpelInvokerRequired(getTargetClass(this.targetObject), this.method, this.handlerMethod);
}
catch (IneligibleMethodException e) {
throw new IllegalArgumentException(e);
}
}

private void setDisplayString(Object targetObject, Object targetMethod) {
StringBuilder sb = new StringBuilder(targetObject.getClass().getName());
if (targetMethod instanceof Method) {
Expand Down Expand Up @@ -503,37 +514,46 @@ private void initializeHandler(HandlerMethod candidate) {
candidate.initialized = true;
}

@SuppressWarnings("deprecation")
private synchronized void initialize() throws Exception {
if (!this.initialized) {
BeanFactory beanFactory = getBeanFactory();
if (beanFactory != null &&
beanFactory.containsBean(IntegrationContextUtils.ARGUMENT_RESOLVER_MESSAGE_CONVERTER_BEAN_NAME)) {
if (isProvidedMessageHandlerFactoryBean()) {
logger.info("Overriding default instance of MessageHandlerMethodFactory with provided one.");
this.messageHandlerMethodFactory = getBeanFactory()
.getBean(IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME, MessageHandlerMethodFactory.class);
createHandlerMethod();
}
else {
BeanFactory beanFactory = getBeanFactory();
if (beanFactory != null &&
beanFactory.containsBean(IntegrationContextUtils.ARGUMENT_RESOLVER_MESSAGE_CONVERTER_BEAN_NAME)) {

try {
MessageConverter messageConverter =
beanFactory.getBean(IntegrationContextUtils.ARGUMENT_RESOLVER_MESSAGE_CONVERTER_BEAN_NAME,
MessageConverter.class);
try {
MessageConverter messageConverter =
beanFactory.getBean(IntegrationContextUtils.ARGUMENT_RESOLVER_MESSAGE_CONVERTER_BEAN_NAME,
MessageConverter.class);

this.messageHandlerMethodFactory.setMessageConverter(messageConverter);
((DefaultMessageHandlerMethodFactory) this.messageHandlerMethodFactory).setMessageConverter(messageConverter);

HandlerMethodArgumentResolversHolder handlerMethodArgumentResolversHolder =
beanFactory.getBean(this.canProcessMessageList
? IntegrationContextUtils.LIST_ARGUMENT_RESOLVERS_BEAN_NAME
: IntegrationContextUtils.ARGUMENT_RESOLVERS_BEAN_NAME,
HandlerMethodArgumentResolversHolder.class);
HandlerMethodArgumentResolversHolder handlerMethodArgumentResolversHolder =
beanFactory.getBean(this.canProcessMessageList
? IntegrationContextUtils.LIST_ARGUMENT_RESOLVERS_BEAN_NAME
: IntegrationContextUtils.ARGUMENT_RESOLVERS_BEAN_NAME,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not OK do not have an out-of-the-box bean on the matter. And that would simplify a code in here, but at the same time pay attention, please, that we would like to use different argument resolvers for list case and non-list.

With the centralized single IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME we don't have such a distribution and proper logic here in the in this class.

So, do we need to introduce an IntegrationContextUtils.LIST_MESSAGE_HANDLER_FACTORY_BEAN_NAME as well to cover this situation?

If I'm alone in my "brain swamp", feel free to merge it and let's raise an issue to revise it in the next version!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure i follow. . .
We do not need OOTB bean. As I said in the notes it's status quo unless. . and the unless part is where you decide to override at which point you register a bean of type and name specified.

Yes, there are quite a lot of additional improvements that can/should be done in this specific class and I've mentioned some of them, but the impact of introducing it now (minor release) may be more then we can/should allow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that with an explicit IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME bean in your target application you apply it for all the MessagingMethodInvokerHelper instances where some of them really should rely on the this.canProcessMessageList and the choice of argument resolves in your bean might be wrong or doesn't fit the list processing logic. I mean that with single global bean you are going to break something in your application without a way to reinstate the proper behavior.

I might agree if you only apply such a global bean for those MessagingMethodInvokerHelper which are not this.canProcessMessageList though...

Copy link
Contributor Author

@olegz olegz Jan 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, if I override MessageHandlerMethodFactory I am taking full control. I have to take care of everything - as actually shown in test where there is only single resolver which accepts everything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more time: we have a special case for the this.canProcessMessageList.
See DefaultConfiguringBeanFactoryPostProcessor.internalArgumentResolversBuilder() and its logic around listCapable parameter.
I'm not sure that a single global bean can address that use-case. And that's definitely might be a case why we have fine-grained it into a separate IntegrationContextUtils.LIST_ARGUMENT_RESOLVERS_BEAN_NAME.

Might the case that I just can't find the proper words to explain the issue...

@garyrussell , WDYT?

Copy link
Contributor

@garyrussell garyrussell Jan 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it would be better to pre-define standard beans for list and not-list case, and allow the user to override either one of them, or both; however, given the timing, and the fact we have to get 10 releases out today, I would tend to go with this compromise (SCSt doesn't need the list-capable invocation - that is generally for aggregation processors).

We can add an issue to the backlog to clean it up properly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Then merging with simple code style polishing.

Copy link
Contributor Author

@olegz olegz Jan 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also make sure we revisit the lifecycle of handleMethod so it's only created when the actual instance of MessageHandlerMethodFactory is known.

Now, with regard to standard beans for list and not-list case, I still believe it is out of scope for this issue and is the responsibility of the user who configures/overrides MessageHandlerMethodFactory to inject and configure the appropriate resolvers. IMHO it must be all or nothing. As an example, I as a user who provides my own instance of MessageHandlerMethodFactory would never expect framework doing anything to it (injecting additional resolvers, etc) other then just using it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done #2690

HandlerMethodArgumentResolversHolder.class);

this.messageHandlerMethodFactory.setCustomArgumentResolvers(
handlerMethodArgumentResolversHolder.getResolvers());
((DefaultMessageHandlerMethodFactory) this.messageHandlerMethodFactory).setCustomArgumentResolvers(
handlerMethodArgumentResolversHolder.getResolvers());
}
catch (NoSuchBeanDefinitionException e) {
configureLocalMessageHandlerFactory();
}
}
catch (NoSuchBeanDefinitionException e) {
else {
configureLocalMessageHandlerFactory();
}
}
else {
configureLocalMessageHandlerFactory();
((DefaultMessageHandlerMethodFactory) this.messageHandlerMethodFactory).afterPropertiesSet();
}

this.messageHandlerMethodFactory.afterPropertiesSet();
prepareEvaluationContext();
this.initialized = true;
}
Expand All @@ -551,7 +571,7 @@ private void configureLocalMessageHandlerFactory() {
messageConverter = getBeanFactory()
.getBean(IntegrationContextUtils.ARGUMENT_RESOLVER_MESSAGE_CONVERTER_BEAN_NAME,
MessageConverter.class);
this.messageHandlerMethodFactory.setMessageConverter(messageConverter);
((DefaultMessageHandlerMethodFactory) this.messageHandlerMethodFactory).setMessageConverter(messageConverter);
}
else {
messageConverter = new ConfigurableCompositeMessageConverter();
Expand Down Expand Up @@ -579,7 +599,7 @@ private void configureLocalMessageHandlerFactory() {

customArgumentResolvers.add(mapArgumentResolver);

this.messageHandlerMethodFactory.setCustomArgumentResolvers(customArgumentResolvers);
((DefaultMessageHandlerMethodFactory) this.messageHandlerMethodFactory).setCustomArgumentResolvers(customArgumentResolvers);
}

@SuppressWarnings("unchecked")
Expand Down
65 changes: 64 additions & 1 deletion ...est/java/org/springframework/integration/handler/MethodInvokingMessageProcessorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2018 the original author or authors.
* Copyright 2002-2019 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -61,7 +61,9 @@
import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
import org.springframework.context.ApplicationContext;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.core.MethodParameter;
import org.springframework.expression.Expression;
import org.springframework.expression.spel.SpelCompilerMode;
import org.springframework.expression.spel.SpelEvaluationException;
Expand All @@ -70,19 +72,26 @@
import org.springframework.expression.spel.support.StandardEvaluationContext;
import org.springframework.integration.annotation.ServiceActivator;
import org.springframework.integration.annotation.UseSpelInvoker;
import org.springframework.integration.channel.QueueChannel;
import org.springframework.integration.config.EnableIntegration;
import org.springframework.integration.context.IntegrationContextUtils;
import org.springframework.integration.gateway.GatewayProxyFactoryBean;
import org.springframework.integration.gateway.RequestReplyExchanger;
import org.springframework.integration.handler.support.MessagingMethodInvokerHelper;
import org.springframework.integration.support.MessageBuilder;
import org.springframework.integration.test.util.TestUtils;
import org.springframework.messaging.Message;
import org.springframework.messaging.MessageChannel;
import org.springframework.messaging.MessageHandler;
import org.springframework.messaging.MessageHandlingException;
import org.springframework.messaging.MessageHeaders;
import org.springframework.messaging.MessagingException;
import org.springframework.messaging.PollableChannel;
import org.springframework.messaging.handler.annotation.Header;
import org.springframework.messaging.handler.annotation.Payload;
import org.springframework.messaging.handler.annotation.support.DefaultMessageHandlerMethodFactory;
import org.springframework.messaging.handler.annotation.support.MessageHandlerMethodFactory;
import org.springframework.messaging.handler.invocation.HandlerMethodArgumentResolver;
import org.springframework.messaging.support.GenericMessage;
import org.springframework.util.StopWatch;

Expand All @@ -107,6 +116,60 @@ public class MethodInvokingMessageProcessorTests {
@Rule
public ExpectedException expected = ExpectedException.none();

@Test
public void testMessageHandlerMethodFactoryOverride() {
try (AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(MyConfiguration.class)) {
MessageChannel channel = context.getBean("foo", MessageChannel.class);
channel.send(MessageBuilder.withPayload("Bob Smith").build());
PollableChannel out = context.getBean("out", PollableChannel.class);
assertEquals("Person: Bob Smith", out.receive().getPayload());
}
}

@EnableIntegration
public static class MyConfiguration {
@ServiceActivator(inputChannel = "foo", outputChannel = "out")
public String foo(Person person) {
return person.toString();
}

@Bean
public PollableChannel out() {
return new QueueChannel();
}

@Bean(IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME)
public MessageHandlerMethodFactory messageHandlerMethodFactory() {
DefaultMessageHandlerMethodFactory f = new DefaultMessageHandlerMethodFactory();
HandlerMethodArgumentResolver resolver = new HandlerMethodArgumentResolver() {

@Override
public boolean supportsParameter(MethodParameter parameter) {
return true;
}

@Override
public Object resolveArgument(MethodParameter parameter, Message<?> message) throws Exception {
String[] names = ((String) message.getPayload()).split(" ");
return new Person(names[0], names[1]);
}
};
f.setArgumentResolvers(Collections.singletonList(resolver));
f.afterPropertiesSet();
return f;
}

public static class Person {
private final String name;
public Person(String fname, String lname) {
this.name = fname + " " + lname;
}
public String toString() {
return "Person: " + name;
}
}
}

@Test
public void testHandlerInheritanceMethodImplInSuper() {
class A {
Expand Down