From eba873067c27dd3c64b95a38583ef4a45ea0c759 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Wed, 29 Jun 2016 18:10:34 +0200 Subject: [PATCH] Better hide lazy resolution of JMS payloads This commit fixes MessagingMessageConverter to no longer expose the lazy message resolution algorithm. This restores proper behaviour for converters used outside of that context. Instead, such arrangement is now private to AbstractAdaptableMessageListener (as it should). Issue: SPR-14389 --- .../AbstractAdaptableMessageListener.java | 66 ++++++++++++++++++- .../converter/MessagingMessageConverter.java | 44 +++---------- ...tenerContainerFactoryIntegrationTests.java | 18 ++++- .../MessagingMessageListenerAdapterTests.java | 30 +++++++++ .../MessagingMessageConverterTests.java | 24 ------- 5 files changed, 118 insertions(+), 64 deletions(-) diff --git a/spring-jms/src/main/java/org/springframework/jms/listener/adapter/AbstractAdaptableMessageListener.java b/spring-jms/src/main/java/org/springframework/jms/listener/adapter/AbstractAdaptableMessageListener.java index 1ed430e1f6..2f53f57e95 100644 --- a/spring-jms/src/main/java/org/springframework/jms/listener/adapter/AbstractAdaptableMessageListener.java +++ b/spring-jms/src/main/java/org/springframework/jms/listener/adapter/AbstractAdaptableMessageListener.java @@ -39,6 +39,7 @@ import org.springframework.jms.support.converter.SimpleMessageConverter; import org.springframework.jms.support.converter.SmartMessageConverter; import org.springframework.jms.support.destination.DestinationResolver; import org.springframework.jms.support.destination.DynamicDestinationResolver; +import org.springframework.messaging.MessageHeaders; import org.springframework.util.Assert; /** @@ -417,11 +418,21 @@ public abstract class AbstractAdaptableMessageListener /** - * Delegates payload extraction to {@link #extractMessage(javax.jms.Message)} to - * enforce backward compatibility. + * A {@link MessagingMessageConverter} that lazily invoke payload extraction and + * delegate it to {@link #extractMessage(javax.jms.Message)} in order to enforce + * backward compatibility. */ private class MessagingMessageConverterAdapter extends MessagingMessageConverter { + @SuppressWarnings("unchecked") + @Override + public Object fromMessage(javax.jms.Message message) throws JMSException, MessageConversionException { + if (message == null) { + return null; + } + return new LazyResolutionMessage(message); + } + @Override protected Object extractPayload(Message message) throws JMSException { Object payload = extractMessage(message); @@ -452,6 +463,57 @@ public abstract class AbstractAdaptableMessageListener } return converter.toMessage(payload, session); } + + protected class LazyResolutionMessage implements org.springframework.messaging.Message { + + private final javax.jms.Message message; + + private Object payload; + + private MessageHeaders headers; + + public LazyResolutionMessage(javax.jms.Message message) { + this.message = message; + } + + @Override + public Object getPayload() { + if (this.payload == null) { + try { + this.payload = unwrapPayload(); + } + catch (JMSException ex) { + throw new MessageConversionException( + "Failed to extract payload from [" + this.message + "]", ex); + } + } + // + return this.payload; + } + + /** + * Extract the payload of the current message. Since we deferred the resolution + * of the payload, a custom converter may still return a full message for it. In + * this case, its payload is returned. + * @return the payload of the message + */ + private Object unwrapPayload() throws JMSException { + Object payload = extractPayload(this.message); + if (payload instanceof org.springframework.messaging.Message) { + return ((org.springframework.messaging.Message) payload).getPayload(); + } + return payload; + } + + @Override + public MessageHeaders getHeaders() { + if (this.headers == null) { + this.headers = extractHeaders(this.message); + } + return this.headers; + } + } + } diff --git a/spring-jms/src/main/java/org/springframework/jms/support/converter/MessagingMessageConverter.java b/spring-jms/src/main/java/org/springframework/jms/support/converter/MessagingMessageConverter.java index e6c308e7c8..c791d289ee 100644 --- a/spring-jms/src/main/java/org/springframework/jms/support/converter/MessagingMessageConverter.java +++ b/spring-jms/src/main/java/org/springframework/jms/support/converter/MessagingMessageConverter.java @@ -16,6 +16,7 @@ package org.springframework.jms.support.converter; +import java.util.Map; import javax.jms.JMSException; import javax.jms.Session; @@ -25,6 +26,7 @@ import org.springframework.jms.support.SimpleJmsHeaderMapper; import org.springframework.messaging.Message; import org.springframework.messaging.MessageHeaders; import org.springframework.messaging.core.AbstractMessagingTemplate; +import org.springframework.messaging.support.MessageBuilder; import org.springframework.util.Assert; /** @@ -107,7 +109,12 @@ public class MessagingMessageConverter implements MessageConverter, Initializing if (message == null) { return null; } - return new LazyResolutionMessage(message); + Map mappedHeaders = extractHeaders(message); + Object convertedObject = extractPayload(message); + MessageBuilder builder = (convertedObject instanceof org.springframework.messaging.Message) ? + MessageBuilder.fromMessage((org.springframework.messaging.Message) convertedObject) : + MessageBuilder.withPayload(convertedObject); + return builder.copyHeadersIfAbsent(mappedHeaders).build(); } /** @@ -141,44 +148,11 @@ public class MessagingMessageConverter implements MessageConverter, Initializing return createMessageForPayload(payload, session); } - private MessageHeaders extractHeaders(javax.jms.Message message) { + protected final MessageHeaders extractHeaders(javax.jms.Message message) { return this.headerMapper.toHeaders(message); } - private class LazyResolutionMessage implements Message { - private final javax.jms.Message message; - - private Object payload; - - private MessageHeaders headers; - - public LazyResolutionMessage(javax.jms.Message message) { - this.message = message; - } - - @Override - public Object getPayload() { - if (this.payload == null) { - try { - this.payload = extractPayload(this.message); - } - catch (JMSException ex) { - throw new MessageConversionException( - "Failed to extract payload from [" + this.message + "]", ex); - } - } - return this.payload; - } - - @Override - public MessageHeaders getHeaders() { - if (this.headers == null) { - this.headers = extractHeaders(this.message); - } - return this.headers; - } - } } diff --git a/spring-jms/src/test/java/org/springframework/jms/config/JmsListenerContainerFactoryIntegrationTests.java b/spring-jms/src/test/java/org/springframework/jms/config/JmsListenerContainerFactoryIntegrationTests.java index 33247a7be5..dcfceb8265 100644 --- a/spring-jms/src/test/java/org/springframework/jms/config/JmsListenerContainerFactoryIntegrationTests.java +++ b/spring-jms/src/test/java/org/springframework/jms/config/JmsListenerContainerFactoryIntegrationTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2016 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. @@ -35,6 +35,7 @@ import org.springframework.jms.listener.DefaultMessageListenerContainer; import org.springframework.jms.listener.SessionAwareMessageListener; import org.springframework.jms.support.converter.MessageConversionException; import org.springframework.jms.support.converter.MessageConverter; +import org.springframework.jms.support.converter.MessagingMessageConverter; import org.springframework.messaging.handler.annotation.Header; import org.springframework.messaging.handler.annotation.Payload; import org.springframework.messaging.handler.annotation.support.DefaultMessageHandlerMethodFactory; @@ -65,10 +66,21 @@ public class JmsListenerContainerFactoryIntegrationTests { @Test public void messageConverterUsedIfSet() throws JMSException { - containerFactory.setMessageConverter(new UpperCaseMessageConverter()); + this.containerFactory.setMessageConverter(new UpperCaseMessageConverter()); + testMessageConverterIsUsed(); + } + @Test + public void messagingMessageConverterCanBeUsed() throws JMSException { + MessagingMessageConverter converter = new MessagingMessageConverter(); + converter.setPayloadConverter(new UpperCaseMessageConverter()); + this.containerFactory.setMessageConverter(converter); + testMessageConverterIsUsed(); + } + + private void testMessageConverterIsUsed() throws JMSException { MethodJmsListenerEndpoint endpoint = createDefaultMethodJmsEndpoint( - listener.getClass(), "handleIt", String.class, String.class); + this.listener.getClass(), "handleIt", String.class, String.class); Message message = new StubTextMessage("foo-bar"); message.setStringProperty("my-header", "my-value"); diff --git a/spring-jms/src/test/java/org/springframework/jms/listener/adapter/MessagingMessageListenerAdapterTests.java b/spring-jms/src/test/java/org/springframework/jms/listener/adapter/MessagingMessageListenerAdapterTests.java index 0b574b36f3..0daf92c08c 100644 --- a/spring-jms/src/test/java/org/springframework/jms/listener/adapter/MessagingMessageListenerAdapterTests.java +++ b/spring-jms/src/test/java/org/springframework/jms/listener/adapter/MessagingMessageListenerAdapterTests.java @@ -29,7 +29,9 @@ import javax.jms.Topic; import com.fasterxml.jackson.annotation.JsonView; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.springframework.beans.factory.support.StaticListableBeanFactory; import org.springframework.jms.StubTextMessage; @@ -51,6 +53,9 @@ import static org.mockito.BDDMockito.*; */ public class MessagingMessageListenerAdapterTests { + @Rule + public final ExpectedException thrown = ExpectedException.none(); + private static final Destination sharedReplyDestination = mock(Destination.class); private final DefaultMessageHandlerMethodFactory factory = new DefaultMessageHandlerMethodFactory(); @@ -122,6 +127,31 @@ public class MessagingMessageListenerAdapterTests { } } + @Test + public void payloadConversionLazilyInvoked() throws JMSException { + javax.jms.Message jmsMessage = mock(javax.jms.Message.class); + MessageConverter messageConverter = mock(MessageConverter.class); + given(messageConverter.fromMessage(jmsMessage)).willReturn("FooBar"); + MessagingMessageListenerAdapter listener = getSimpleInstance("simple", Message.class); + listener.setMessageConverter(messageConverter); + Message message = listener.toMessagingMessage(jmsMessage); + verify(messageConverter, never()).fromMessage(jmsMessage); + assertEquals("FooBar", message.getPayload()); + verify(messageConverter, times(1)).fromMessage(jmsMessage); + } + + @Test + public void headerConversionLazilyInvoked() throws JMSException { + javax.jms.Message jmsMessage = mock(javax.jms.Message.class); + when(jmsMessage.getPropertyNames()).thenThrow(new IllegalArgumentException("Header failure")); + MessagingMessageListenerAdapter listener = getSimpleInstance("simple", Message.class); + Message message = listener.toMessagingMessage(jmsMessage); + + this.thrown.expect(IllegalArgumentException.class); + this.thrown.expectMessage("Header failure"); + message.getHeaders(); // Triggers headers resolution + } + @Test public void incomingMessageUsesMessageConverter() throws JMSException { javax.jms.Message jmsMessage = mock(javax.jms.Message.class); diff --git a/spring-jms/src/test/java/org/springframework/jms/support/converter/MessagingMessageConverterTests.java b/spring-jms/src/test/java/org/springframework/jms/support/converter/MessagingMessageConverterTests.java index e390676bd4..ab023afd36 100644 --- a/spring-jms/src/test/java/org/springframework/jms/support/converter/MessagingMessageConverterTests.java +++ b/spring-jms/src/test/java/org/springframework/jms/support/converter/MessagingMessageConverterTests.java @@ -74,30 +74,6 @@ public class MessagingMessageConverterTests { assertEquals(1224L, msg.getPayload()); } - @Test - public void payloadConversionLazilyInvoked() throws JMSException { - TextMessage jmsMsg = new StubTextMessage("1224"); - TestMessageConverter converter = new TestMessageConverter(); - this.converter.setPayloadConverter(converter); - Message msg = (Message) this.converter.fromMessage(jmsMsg); - assertEquals("Converter should not have been called yet", false, converter.called); - assertEquals(1224L, msg.getPayload()); - assertEquals("Converter should have been called", true, converter.called); - } - - @Test - public void headerConversionLazilyInvoked() throws JMSException { - javax.jms.Message jmsMsg = mock(javax.jms.Message.class); - when(jmsMsg.getPropertyNames()).thenThrow(new IllegalArgumentException("Header failure")); - - Message msg = (Message) this.converter.fromMessage(jmsMsg); - - this.thrown.expect(IllegalArgumentException.class); - this.thrown.expectMessage("Header failure"); - msg.getHeaders(); // Triggers headers resolution - } - - static class TestMessageConverter extends SimpleMessageConverter { private boolean called;