polishing

snicoll · snicoll · commit 6aa16b745fe2 · 2014-04-24T16:56:51.000+03:00
This commit removes the queue attribute of the JmsListener annotation
as this information should be provided by the container factory and not
by each individual listener endpoints.

There was a side effect that an annotation value cannot be null, which
was forcing the container to be a queue-based container by default.

Issue: SPR-9882
diff --git a/spring-jms/src/main/java/org/springframework/jms/annotation/JmsListener.java b/spring-jms/src/main/java/org/springframework/jms/annotation/JmsListener.java
@@ -96,12 +96,6 @@
 	 */
 	String destination();
 
-	/**
-	 * Specify if the {@link #destination()} refers to a queue or not. Refer to a
-	 * queue by default.
-	 */
-	boolean queue() default true;
-
 	/**
 	 * The name for the durable subscription, if any.
 	 */
diff --git a/spring-jms/src/main/java/org/springframework/jms/annotation/JmsListenerAnnotationBeanPostProcessor.java b/spring-jms/src/main/java/org/springframework/jms/annotation/JmsListenerAnnotationBeanPostProcessor.java
@@ -175,7 +175,6 @@ protected void processJmsListener(JmsListener jmsListener, Method method, Object
 		endpoint.setJmsHandlerMethodFactory(jmsHandlerMethodFactory);
 		endpoint.setId(getEndpointId(jmsListener));
 		endpoint.setDestination(jmsListener.destination());
-		endpoint.setQueue(jmsListener.queue());
 		if (StringUtils.hasText(jmsListener.selector())) {
 			endpoint.setSelector(jmsListener.selector());
 		}
diff --git a/spring-jms/src/main/java/org/springframework/jms/config/AbstractJmsListenerEndpoint.java b/spring-jms/src/main/java/org/springframework/jms/config/AbstractJmsListenerEndpoint.java
@@ -38,8 +38,6 @@ public abstract class AbstractJmsListenerEndpoint implements JmsListenerEndpoint
 
 	private String destination;
 
-	private boolean queue = true;
-
 	private String subscription;
 
 	private String selector;
@@ -68,20 +66,6 @@ public void setDestination(String destination) {
 		this.destination = destination;
 	}
 
-	/**
-	 * Return {@code true} if the destination is a queue.
-	 */
-	public boolean isQueue() {
-		return queue;
-	}
-
-	/**
-	 * Specify if the destination is a queue.
-	 */
-	public void setQueue(boolean queue) {
-		this.queue = queue;
-	}
-
 	/**
 	 * Return the name for the durable subscription, if any.
 	 */
@@ -136,7 +120,6 @@ protected void setupJmsMessageContainer(AbstractMessageListenerContainer contain
 		if (getSelector() != null) {
 			container.setMessageSelector(getSelector());
 		}
-		container.setPubSubDomain(!isQueue());
 		setupMessageListener(container);
 	}
 
@@ -156,7 +139,6 @@ protected void setupJcaMessageContainer(JmsMessageEndpointManager container) {
 		if (getSelector() != null) {
 			activationSpecConfig.setMessageSelector(getSelector());
 		}
-		activationSpecConfig.setPubSubDomain(!isQueue());
 		setupMessageListener(container);
 	}
 
@@ -183,8 +165,6 @@ protected StringBuilder getEndpointDescription() {
 				.append(this.id)
 				.append("] destination=")
 				.append(this.destination)
-				.append(" | queue='")
-				.append(this.queue)
 				.append("' | subscription='")
 				.append(this.subscription)
 				.append(" | selector='")
diff --git a/spring-jms/src/main/java/org/springframework/jms/config/MethodJmsListenerEndpoint.java b/spring-jms/src/main/java/org/springframework/jms/config/MethodJmsListenerEndpoint.java
@@ -85,11 +85,11 @@ protected MessagingMessageListenerAdapter createMessageListener(MessageListenerC
 		messageListener.setHandlerMethod(invocableHandlerMethod);
 		String responseDestination = getDefaultResponseDestination();
 		if (StringUtils.hasText(responseDestination)) {
-			if (isQueue()) {
-				messageListener.setDefaultResponseQueueName(responseDestination);
+			if (container.isPubSubDomain()) {
+				messageListener.setDefaultResponseTopicName(responseDestination);
 			}
 			else {
-				messageListener.setDefaultResponseTopicName(responseDestination);
+				messageListener.setDefaultResponseQueueName(responseDestination);
 			}
 		}
 		MessageConverter messageConverter = container.getMessageConverter();
diff --git a/spring-jms/src/main/java/org/springframework/jms/listener/MessageListenerContainer.java b/spring-jms/src/main/java/org/springframework/jms/listener/MessageListenerContainer.java
@@ -41,4 +41,10 @@ public interface MessageListenerContainer extends Lifecycle {
 	 */
 	MessageConverter getMessageConverter();
 
+	/**
+	 * Return whether the Publish/Subscribe domain ({@link javax.jms.Topic Topics}) is used.
+	 * Otherwise, the Point-to-Point domain ({@link javax.jms.Queue Queues}) is used.
+	 */
+	boolean isPubSubDomain();
+
 }
diff --git a/spring-jms/src/main/java/org/springframework/jms/listener/endpoint/JmsMessageEndpointManager.java b/spring-jms/src/main/java/org/springframework/jms/listener/endpoint/JmsMessageEndpointManager.java
@@ -156,6 +156,15 @@ public MessageConverter getMessageConverter() {
 		return null;
 	}
 
+	@Override
+	public boolean isPubSubDomain() {
+		JmsActivationSpecConfig config = getActivationSpecConfig();
+		if (config != null) {
+			return config.isPubSubDomain();
+		}
+		throw new IllegalStateException("could not determine pubSubDomain, no activation spec config is set");
+	}
+
 	/**
 	 * Set the name of this message endpoint. Populated with the bean name
 	 * automatically when defined within Spring's bean factory.
diff --git a/spring-jms/src/test/java/org/springframework/jms/annotation/AbstractJmsAnnotationDrivenTests.java b/spring-jms/src/test/java/org/springframework/jms/annotation/AbstractJmsAnnotationDrivenTests.java
@@ -105,7 +105,6 @@ public void testFullConfiguration(ApplicationContext context) {
 				simpleFactory.getContainers().get(0).getEndpoint();
 		assertEquals("listener1", endpoint.getId());
 		assertEquals("queueIn", endpoint.getDestination());
-		assertTrue(endpoint.isQueue());
 		assertEquals("mySelector", endpoint.getSelector());
 		assertEquals("mySubscription", endpoint.getSubscription());
 	}
diff --git a/spring-jms/src/test/java/org/springframework/jms/annotation/JmsListenerAnnotationBeanPostProcessorTests.java b/spring-jms/src/test/java/org/springframework/jms/annotation/JmsListenerAnnotationBeanPostProcessorTests.java
@@ -58,7 +58,6 @@ public void simpleMessageListener() {
 		MethodJmsListenerEndpoint methodEndpoint = (MethodJmsListenerEndpoint) endpoint;
 		assertNotNull(methodEndpoint.getBean());
 		assertNotNull(methodEndpoint.getMethod());
-		assertTrue(methodEndpoint.isQueue());
 		assertTrue("Should have been started " + container, container.isStarted());
 
 		SimpleMessageListenerContainer listenerContainer = new SimpleMessageListenerContainer();
diff --git a/spring-jms/src/test/java/org/springframework/jms/config/JmsListenerContainerFactoryTests.java b/spring-jms/src/test/java/org/springframework/jms/config/JmsListenerContainerFactoryTests.java
@@ -67,7 +67,6 @@ public void createSimpleContainer() {
 		MessageListener messageListener = new MessageListenerAdapter();
 		endpoint.setMessageListener(messageListener);
 		endpoint.setDestination("myQueue");
-		endpoint.setQueue(false); // See #setDefaultJmsConfig
 
 		SimpleMessageListenerContainer container = factory.createMessageListenerContainer(endpoint);
 
@@ -89,7 +88,6 @@ public void createJmsContainerFullConfig() {
 		MessageListener messageListener = new MessageListenerAdapter();
 		endpoint.setMessageListener(messageListener);
 		endpoint.setDestination("myQueue");
-		endpoint.setQueue(false); // See #setDefaultJmsConfig
 		DefaultMessageListenerContainer container = factory.createMessageListenerContainer(endpoint);
 
 		assertDefaultJmsConfig(container);
@@ -112,7 +110,6 @@ public void createJcaContainerFullConfig() {
 		MessageListener messageListener = new MessageListenerAdapter();
 		endpoint.setMessageListener(messageListener);
 		endpoint.setDestination("myQueue");
-		endpoint.setQueue(false); // See #setDefaultJmsConfig
 		JmsMessageEndpointManager container = factory.createMessageListenerContainer(endpoint);
 
 		assertDefaultJcaConfig(container);
@@ -121,19 +118,6 @@ public void createJcaContainerFullConfig() {
 		assertEquals("myQueue", container.getActivationSpecConfig().getDestinationName());
 	}
 
-	@Test
-	public void endpointCanOverrideConfig() {
-		DefaultJmsListenerContainerFactory factory = new DefaultJmsListenerContainerFactory();
-		factory.setPubSubDomain(true); // topic
-
-		SimpleJmsListenerEndpoint endpoint = new SimpleJmsListenerEndpoint();
-		endpoint.setMessageListener(new MessageListenerAdapter());
-		endpoint.setQueue(true); // queue
-
-		DefaultMessageListenerContainer container = factory.createMessageListenerContainer(endpoint);
-		assertEquals(false, container.isPubSubDomain()); // overridden by the endpoint config
-	}
-
 	@Test
 	public void jcaExclusiveProperties() {
 		DefaultJcaListenerContainerFactory factory = new DefaultJcaListenerContainerFactory();
diff --git a/spring-jms/src/test/java/org/springframework/jms/config/JmsListenerEndpointTests.java b/spring-jms/src/test/java/org/springframework/jms/config/JmsListenerEndpointTests.java
@@ -46,14 +46,12 @@ public void setupJmsMessageContainerFullConfig() {
 		MessageListener messageListener = new MessageListenerAdapter();
 		SimpleJmsListenerEndpoint endpoint = new SimpleJmsListenerEndpoint();
 		endpoint.setDestination("myQueue");
-		endpoint.setQueue(true);
 		endpoint.setSelector("foo = 'bar'");
 		endpoint.setSubscription("mySubscription");
 		endpoint.setMessageListener(messageListener);
 
 		endpoint.setupMessageContainer(container);
 		assertEquals("myQueue", container.getDestinationName());
-		assertFalse(container.isPubSubDomain());
 		assertEquals("foo = 'bar'", container.getMessageSelector());
 		assertEquals("mySubscription", container.getDurableSubscriptionName());
 		assertEquals(messageListener, container.getMessageListener());
@@ -65,15 +63,13 @@ public void setupJcaMessageContainerFullConfig() {
 		MessageListener messageListener = new MessageListenerAdapter();
 		SimpleJmsListenerEndpoint endpoint = new SimpleJmsListenerEndpoint();
 		endpoint.setDestination("myQueue");
-		endpoint.setQueue(true);
 		endpoint.setSelector("foo = 'bar'");
 		endpoint.setSubscription("mySubscription");
 		endpoint.setMessageListener(messageListener);
 
 		endpoint.setupMessageContainer(container);
 		JmsActivationSpecConfig config = container.getActivationSpecConfig();
 		assertEquals("myQueue", config.getDestinationName());
-		assertFalse(config.isPubSubDomain());
 		assertEquals("foo = 'bar'", config.getMessageSelector());
 		assertEquals("mySubscription", config.getDurableSubscriptionName());
 		assertEquals(messageListener, container.getMessageListener());
diff --git a/spring-jms/src/test/java/org/springframework/jms/config/MessageListenerTestContainer.java b/spring-jms/src/test/java/org/springframework/jms/config/MessageListenerTestContainer.java
@@ -86,6 +86,11 @@ public MessageConverter getMessageConverter() {
 		return null;
 	}
 
+	@Override
+	public boolean isPubSubDomain() {
+		return true;
+	}
+
 	@Override
 	public void afterPropertiesSet() {
 		if (!startInvoked) {
diff --git a/spring-jms/src/test/java/org/springframework/jms/config/MethodJmsListenerEndpointTests.java b/spring-jms/src/test/java/org/springframework/jms/config/MethodJmsListenerEndpointTests.java
@@ -44,6 +44,7 @@
 import org.springframework.context.support.StaticApplicationContext;
 import org.springframework.jms.StubTextMessage;
 import org.springframework.jms.listener.DefaultMessageListenerContainer;
+import org.springframework.jms.listener.MessageListenerContainer;
 import org.springframework.jms.listener.SimpleMessageListenerContainer;
 import org.springframework.jms.listener.adapter.ListenerExecutionFailedException;
 import org.springframework.jms.listener.adapter.MessagingMessageListenerAdapter;
@@ -220,8 +221,27 @@ public void processAndReply() throws JMSException {
 	}
 
 	@Test
-	public void processAndReplyWithSendTo() throws JMSException {
-		MessagingMessageListenerAdapter listener = createDefaultInstance(String.class);
+	public void processAndReplyWithSendToQueue() throws JMSException {
+		String methodName = "processAndReplyWithSendTo";
+		SimpleMessageListenerContainer container = new SimpleMessageListenerContainer();
+		MessagingMessageListenerAdapter listener = createInstance(this.factory,
+				getListenerMethod(methodName, String.class), container);
+		processAndReplyWithSendTo(listener, false);
+		assertListenerMethodInvocation(sample, methodName);
+	}
+
+	@Test
+	public void processAndReplyWithSendToTopic() throws JMSException {
+		String methodName = "processAndReplyWithSendTo";
+		SimpleMessageListenerContainer container = new SimpleMessageListenerContainer();
+		container.setPubSubDomain(true);
+		MessagingMessageListenerAdapter listener = createInstance(this.factory,
+				getListenerMethod(methodName, String.class), container);
+		processAndReplyWithSendTo(listener, true);
+		assertListenerMethodInvocation(sample, methodName);
+	}
+
+	private void processAndReplyWithSendTo(MessagingMessageListenerAdapter listener, boolean pubSubDomain) throws JMSException {
 		String body = "echo text";
 		String correlationId = "link-1234";
 		Destination replyDestination = new Destination() {};
@@ -231,7 +251,7 @@ public void processAndReplyWithSendTo() throws JMSException {
 		QueueSender queueSender = mock(QueueSender.class);
 		Session session = mock(Session.class);
 
-		given(destinationResolver.resolveDestinationName(session, "replyDestination", false))
+		given(destinationResolver.resolveDestinationName(session, "replyDestination", pubSubDomain))
 				.willReturn(replyDestination);
 		given(session.createTextMessage(body)).willReturn(reply);
 		given(session.createProducer(replyDestination)).willReturn(queueSender);
@@ -240,9 +260,8 @@ public void processAndReplyWithSendTo() throws JMSException {
 		StubTextMessage inputMessage = createSimpleJmsTextMessage(body);
 		inputMessage.setJMSCorrelationID(correlationId);
 		listener.onMessage(inputMessage, session);
-		assertDefaultListenerMethodInvocation();
 
-		verify(destinationResolver).resolveDestinationName(session, "replyDestination", false);
+		verify(destinationResolver).resolveDestinationName(session, "replyDestination", pubSubDomain);
 		verify(reply).setJMSCorrelationID(correlationId);
 		verify(queueSender).send(reply);
 		verify(queueSender).close();
@@ -322,12 +341,17 @@ public void invalidMessagePayloadType() throws JMSException {
 	}
 
 	private MessagingMessageListenerAdapter createInstance(
-			DefaultJmsHandlerMethodFactory factory, Method method) {
+			DefaultJmsHandlerMethodFactory factory, Method method, MessageListenerContainer container) {
 		MethodJmsListenerEndpoint endpoint = new MethodJmsListenerEndpoint();
 		endpoint.setBean(sample);
 		endpoint.setMethod(method);
 		endpoint.setJmsHandlerMethodFactory(factory);
-		return endpoint.createMessageListener(new SimpleMessageListenerContainer());
+		return endpoint.createMessageListener(container);
+	}
+
+	private MessagingMessageListenerAdapter createInstance(
+			DefaultJmsHandlerMethodFactory factory, Method method) {
+		return createInstance(factory, method, new SimpleMessageListenerContainer());
 	}
 
 	private MessagingMessageListenerAdapter createDefaultInstance(Class<?>... parameterTypes) {
diff --git a/spring-jms/src/test/java/org/springframework/jms/listener/endpoint/JmsMessageEndpointManagerTests.java b/spring-jms/src/test/java/org/springframework/jms/listener/endpoint/JmsMessageEndpointManagerTests.java
@@ -0,0 +1,64 @@
+/*
+ * Copyright 2002-2014 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.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.jms.listener.endpoint;
+
+import static org.junit.Assert.*;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
+/**
+ * @author Stephane Nicoll
+ */
+public class JmsMessageEndpointManagerTests {
+
+	@Rule
+	public final ExpectedException thrown = ExpectedException.none();
+
+	@Test
+	public void isPubSubDomainWithQueue() {
+		JmsMessageEndpointManager endpoint = new JmsMessageEndpointManager();
+		JmsActivationSpecConfig config = new JmsActivationSpecConfig();
+		config.setPubSubDomain(false);
+		endpoint.setActivationSpecConfig(config);
+		assertEquals(false, endpoint.isPubSubDomain());
+	}
+
+	@Test
+	public void isPubSubDomainWithTopic() {
+		JmsMessageEndpointManager endpoint = new JmsMessageEndpointManager();
+		JmsActivationSpecConfig config = new JmsActivationSpecConfig();
+		config.setPubSubDomain(true);
+		endpoint.setActivationSpecConfig(config);
+		assertEquals(true, endpoint.isPubSubDomain());
+	}
+
+	@Test
+	public void isPubSubDomainWithNoConfig() {
+		JmsMessageEndpointManager endpoint = new JmsMessageEndpointManager();
+
+		thrown.expect(IllegalStateException.class); // far from ideal
+		endpoint.isPubSubDomain();
+	}
+
+	@Test
+	public void getMessageConverterNoConfig() {
+		JmsMessageEndpointManager endpoint = new JmsMessageEndpointManager();
+		assertNull(endpoint.getMessageConverter());
+	}
+}

Original file line number	Diff line number	Diff line change
`@@ -175,7 +175,6 @@ protected void processJmsListener(JmsListener jmsListener, Method method, Object`
`175`	`175`	`endpoint.setJmsHandlerMethodFactory(jmsHandlerMethodFactory);`
`176`	`176`	`endpoint.setId(getEndpointId(jmsListener));`
`177`	`177`	`endpoint.setDestination(jmsListener.destination());`
`178`		`- endpoint.setQueue(jmsListener.queue());`
`179`	`178`	`if (StringUtils.hasText(jmsListener.selector())) {`
`180`	`179`	`endpoint.setSelector(jmsListener.selector());`
`181`	`180`	`}`
Original file line number	Diff line number	Diff line change
`@@ -85,11 +85,11 @@ protected MessagingMessageListenerAdapter createMessageListener(MessageListenerC`
`85`	`85`	`messageListener.setHandlerMethod(invocableHandlerMethod);`
`86`	`86`	`String responseDestination = getDefaultResponseDestination();`
`87`	`87`	`if (StringUtils.hasText(responseDestination)) {`
`88`		`- if (isQueue()) {`
`89`		`- messageListener.setDefaultResponseQueueName(responseDestination);`
	`88`	`+ if (container.isPubSubDomain()) {`
	`89`	`+ messageListener.setDefaultResponseTopicName(responseDestination);`
`90`	`90`	`}`
`91`	`91`	`else {`
`92`		`- messageListener.setDefaultResponseTopicName(responseDestination);`
	`92`	`+ messageListener.setDefaultResponseQueueName(responseDestination);`
`93`	`93`	`}`
`94`	`94`	`}`
`95`	`95`	`MessageConverter messageConverter = container.getMessageConverter();`
Original file line number	Diff line number	Diff line change
`@@ -105,7 +105,6 @@ public void testFullConfiguration(ApplicationContext context) {`
`105`	`105`	`simpleFactory.getContainers().get(0).getEndpoint();`
`106`	`106`	`assertEquals("listener1", endpoint.getId());`
`107`	`107`	`assertEquals("queueIn", endpoint.getDestination());`
`108`		`- assertTrue(endpoint.isQueue());`
`109`	`108`	`assertEquals("mySelector", endpoint.getSelector());`
`110`	`109`	`assertEquals("mySubscription", endpoint.getSubscription());`
`111`	`110`	`}`