Skip to content

INT-4571 Made MessageHandlerMethodFactory injectable #2686

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 Made MessageHandlerMethodFactory injectable #2686

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";
Copy link
Member

Choose a reason for hiding this comment

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

Wait. And where is the bean for this name?
What is the point then to deprecate that HandlerMethodArgumentResolversHolder if we don't provide a valuable alternative out-of-the-box?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we do by allowing injection of integrationMessageHandlerMethodFactory (see HandlerMethodArgumentResolversHolder deprecation comment)

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.

What do you mean by "good argument about integrationMessageHandlerMethodFactory bean definition."?

Copy link
Member

Choose a reason for hiding this comment

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

My point is that you just don't register the bean for the integrationMessageHandlerMethodFactory as out-of-the-box, default solution.
This way it doesn't sound reasonable to deprecate another feature since we really don't replace it.


/**
* @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
77 changes: 46 additions & 31 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 Down Expand Up @@ -295,7 +296,14 @@ public void setUseSpelInvoker(boolean useSpelInvoker) {
@Override
public void setBeanFactory(@NonNull BeanFactory beanFactory) {
super.setBeanFactory(beanFactory);
this.messageHandlerMethodFactory.setBeanFactory(beanFactory);
if (isProvidedMessageHandlerFactoryBean()) {
this.messageHandlerMethodFactory = beanFactory.getBean(IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME,
MessageHandlerMethodFactory.class);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can do this here; the beanFactory might not have the bean yet.

Move to initialize?

Copy link
Contributor Author

@olegz olegz Jan 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I thought about it and was struggling as well, but i guess you're right.
In all fairness I think this class needs a bit more surgery. Look at line 262 where messageHandlerMethodFactory is used which means we will createInvocableHandlerMethod always using the default one, yet may still inject one for the actual invocation. But I stop short of trying to make a big change. Perhaps "Consider revisiting..." type issue?

Copy link
Member

Choose a reason for hiding this comment

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

Definitely better to move this getBean() into the initialize() below. This is not a "one-stop-shop" bean and it really may have a lot of dependencies, so calling it from the setBeanFactory() is a potential bug.

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'll move it but keep in mind that it is still broken given what I said above. Ideally the logic that uses messageHandlerMethodFactory needs to be revisited/refactored so in the end it is a binary choice (default or injected). Right now it is kind of "not-really" a binary choice and it will be even more "not-really" if pushed to the initialize() method, since by that time you will execute at least two operations on the messageHandlerMethodFactory which in the end may not be used at all.

else {
((DefaultMessageHandlerMethodFactory) this.messageHandlerMethodFactory).setBeanFactory(beanFactory);
}

if (beanFactory instanceof ConfigurableListableBeanFactory) {
BeanExpressionResolver beanExpressionResolver = ((ConfigurableListableBeanFactory) beanFactory)
.getBeanExpressionResolver();
Expand All @@ -309,8 +317,8 @@ public void setBeanFactory(@NonNull BeanFactory beanFactory) {
@Override
public void setConversionService(ConversionService conversionService) {
super.setConversionService(conversionService);
if (conversionService != null) {
this.messageHandlerMethodFactory.setConversionService(conversionService);
if (conversionService != null && !isProvidedMessageHandlerFactoryBean()) {
((DefaultMessageHandlerMethodFactory) this.messageHandlerMethodFactory).setConversionService(conversionService);
}
}

Expand Down Expand Up @@ -403,6 +411,11 @@ 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 setDisplayString(Object targetObject, Object targetMethod) {
StringBuilder sb = new StringBuilder(targetObject.getClass().getName());
if (targetMethod instanceof Method) {
Expand Down Expand Up @@ -503,37 +516,39 @@ 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)) {

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

this.messageHandlerMethodFactory.setMessageConverter(messageConverter);

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

this.messageHandlerMethodFactory.setCustomArgumentResolvers(
handlerMethodArgumentResolversHolder.getResolvers());
if (!isProvidedMessageHandlerFactoryBean()) {
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);

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

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

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

this.messageHandlerMethodFactory.afterPropertiesSet();
prepareEvaluationContext();
this.initialized = true;
}
Expand All @@ -551,7 +566,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 +594,7 @@ private void configureLocalMessageHandlerFactory() {

customArgumentResolvers.add(mapArgumentResolver);

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

@SuppressWarnings("unchecked")
Expand Down
27 changes: 26 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 @@ -62,6 +62,7 @@
import org.springframework.context.ApplicationContext;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.support.GenericApplicationContext;
import org.springframework.expression.Expression;
import org.springframework.expression.spel.SpelCompilerMode;
import org.springframework.expression.spel.SpelEvaluationException;
Expand All @@ -71,6 +72,7 @@
import org.springframework.integration.annotation.ServiceActivator;
import org.springframework.integration.annotation.UseSpelInvoker;
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;
Expand All @@ -83,6 +85,8 @@
import org.springframework.messaging.MessagingException;
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.support.GenericMessage;
import org.springframework.util.StopWatch;

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

@Test
public void testHandlerMethodFactoryInjection() throws Exception {
SingleMethodJsonWithSpELMessageWildBean bean = new SingleMethodJsonWithSpELMessageWildBean();
MessagingMethodInvokerHelper<?> helper = new MessagingMethodInvokerHelper<>(bean,
SingleMethodJsonWithSpELMessageWildBean.class.getDeclaredMethod("foo", Message.class), false);
GenericApplicationContext context = new GenericApplicationContext();
DefaultMessageHandlerMethodFactory handlerMethodFactory = new DefaultMessageHandlerMethodFactory();
context.registerBean(IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME,
MessageHandlerMethodFactory.class, () -> handlerMethodFactory);
context.refresh();
Copy link
Member

Choose a reason for hiding this comment

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

The context has to be closed in the end of the test.

helper.setBeanFactory(context);

Object injectedHandlerMethodFactory = TestUtils.getPropertyValue(helper, "messageHandlerMethodFactory");
assertThat(injectedHandlerMethodFactory, equalTo(handlerMethodFactory));

Message<?> message = new GenericMessage<>("baz",
Collections.singletonMap(MessageHeaders.CONTENT_TYPE, "application/json"));
helper.process(message);
assertThat(bean.foo.getPayload(), equalTo("baz"));
}

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