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

Author: snicoll

spring-jms/src/main/java/org/springframework/jms/listener/adapter/AbstractAdaptableMessageListener.java

@@ -39,6 +39,7 @@
 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 @@ protected void postProcessProducer(MessageProducer producer, Message response) t
 
 
 	/**
-	 * 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 @@ protected Message createMessageForPayload(Object payload, Session session, Objec
 			}
 			return converter.toMessage(payload, session);
 		}
+
+		protected class LazyResolutionMessage implements org.springframework.messaging.Message<Object> {
+
+			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;
+			}
+		}
+
 	}
 
 

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.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 Object fromMessage(javax.jms.Message message) throws JMSException, Messag
 		if (message == null) {
 			return null;
 		}
-		return new LazyResolutionMessage(message);
+		Map<String, Object> mappedHeaders = extractHeaders(message);
+		Object convertedObject = extractPayload(message);
+		MessageBuilder<Object> builder = (convertedObject instanceof org.springframework.messaging.Message) ?
+				MessageBuilder.fromMessage((org.springframework.messaging.Message<Object>) convertedObject) :
+				MessageBuilder.withPayload(convertedObject);
+		return builder.copyHeadersIfAbsent(mappedHeaders).build();
 	}
 
 	/**
@@ -141,44 +148,11 @@ protected javax.jms.Message createMessageForPayload(Object payload, Session sess
 		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<Object> {
 
-		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;
-		}
-	}
 
 }

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.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 void setup() {
 
 	@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");
 

spring-jms/src/test/java/org/springframework/jms/listener/adapter/MessagingMessageListenerAdapterTests.java

@@ -29,7 +29,9 @@
 
 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 @@
  */
 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 void exceptionInInvocation() {
 		}
 	}
 
+	@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);

spring-jms/src/test/java/org/springframework/jms/support/converter/MessagingMessageConverterTests.java

@@ -74,30 +74,6 @@ public void customPayloadConverter() throws JMSException {
 		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;