From e1d1dd29c08bfbe2051f6e9c5019544157abfe41 Mon Sep 17 00:00:00 2001 From: Gary Russell Date: Mon, 13 Feb 2017 21:40:53 -0500 Subject: [PATCH] AMQP-710: Fix Exception Return From Void Method JIRA: https://jira.spring.io/browse/AMQP-710 Throw the original exception if `returnException` is set for a method with void return, with no return address. Polishing --- .../adapter/DelegatingInvocableHandler.java | 13 ++- .../listener/adapter/HandlerAdapter.java | 20 +++- .../MessagingMessageListenerAdapter.java | 28 +++-- .../MessagingMessageListenerAdapterTests.java | 103 +++++++++++++++++- 4 files changed, 148 insertions(+), 16 deletions(-) diff --git a/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/adapter/DelegatingInvocableHandler.java b/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/adapter/DelegatingInvocableHandler.java index c48075db..a2253c92 100644 --- a/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/adapter/DelegatingInvocableHandler.java +++ b/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/adapter/DelegatingInvocableHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2015-2016 the original author or authors. + * Copyright 2015-2017 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. @@ -225,4 +225,15 @@ public class DelegatingInvocableHandler { return handlerForPayload == null ? "no match" : handlerForPayload.getMethod().toGenericString(); //NOSONAR } + /** + * Return the method that will be invoked for this payload. + * @param payload the payload. + * @return the method. + * @since 2.0 + */ + public Method getMethodFor(Object payload) { + InvocableHandlerMethod handlerForPayload = getHandlerForPayload(payload.getClass()); + return handlerForPayload == null ? null : handlerForPayload.getMethod(); + } + } diff --git a/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/adapter/HandlerAdapter.java b/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/adapter/HandlerAdapter.java index 0bf5424f..f90a4dbe 100644 --- a/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/adapter/HandlerAdapter.java +++ b/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/adapter/HandlerAdapter.java @@ -1,5 +1,5 @@ /* - * Copyright 2015-2016 the original author or authors. + * Copyright 2015-2017 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. @@ -16,6 +16,8 @@ package org.springframework.amqp.rabbit.listener.adapter; +import java.lang.reflect.Method; + import org.springframework.messaging.Message; import org.springframework.messaging.handler.invocation.InvocableHandlerMethod; @@ -62,6 +64,22 @@ public class HandlerAdapter { } } + /** + * Return the return type for the method that will be chosen for this payload. + * @param payload the payload. + * @return the return type, or null if no handler found. + * @since 2.0 + */ + public Object getReturnType(Object payload) { + if (this.invokerHandlerMethod != null) { + return this.invokerHandlerMethod.getMethod().getReturnType(); + } + else { + Method method = this.delegatingHandler.getMethodFor(payload); + return method == null ? null : method.getReturnType(); + } + } + public Object getBean() { if (this.invokerHandlerMethod != null) { return this.invokerHandlerMethod.getBean(); diff --git a/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/adapter/MessagingMessageListenerAdapter.java b/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/adapter/MessagingMessageListenerAdapter.java index a9cb7c9e..a63c8c02 100644 --- a/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/adapter/MessagingMessageListenerAdapter.java +++ b/spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/adapter/MessagingMessageListenerAdapter.java @@ -137,17 +137,29 @@ public class MessagingMessageListenerAdapter extends AbstractAdaptableMessageLis } } catch (Exception ex) { - if (!this.returnExceptions) { - throw ex; - } - handleResult(new RemoteInvocationResult(ex), amqpMessage, channel, message); + returnOrThrow(amqpMessage, channel, message, ex, ex); } } else { - if (!this.returnExceptions) { - throw e; - } - handleResult(new RemoteInvocationResult(e.getCause()), amqpMessage, channel, message); + returnOrThrow(amqpMessage, channel, message, e.getCause(), e); + } + } + } + + private void returnOrThrow(org.springframework.amqp.core.Message amqpMessage, Channel channel, Message message, + Throwable throwableToReturn, Exception exceptionToThrow) throws Exception { + if (!this.returnExceptions) { + throw exceptionToThrow; + } + try { + handleResult(new RemoteInvocationResult(throwableToReturn), amqpMessage, channel, message); + } + catch (ReplyFailureException rfe) { + if (void.class.equals(this.handlerMethod.getReturnType(message.getPayload()))) { + throw exceptionToThrow; + } + else { + throw rfe; } } } diff --git a/spring-rabbit/src/test/java/org/springframework/amqp/rabbit/listener/adapter/MessagingMessageListenerAdapterTests.java b/spring-rabbit/src/test/java/org/springframework/amqp/rabbit/listener/adapter/MessagingMessageListenerAdapterTests.java index 2db329d7..1b422326 100644 --- a/spring-rabbit/src/test/java/org/springframework/amqp/rabbit/listener/adapter/MessagingMessageListenerAdapterTests.java +++ b/spring-rabbit/src/test/java/org/springframework/amqp/rabbit/listener/adapter/MessagingMessageListenerAdapterTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2016 the original author or authors. + * Copyright 2014-2017 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. @@ -16,27 +16,38 @@ package org.springframework.amqp.rabbit.listener.adapter; +import static org.hamcrest.CoreMatchers.containsString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import java.lang.reflect.Method; +import java.util.ArrayList; import java.util.LinkedHashMap; +import java.util.List; import org.junit.Before; import org.junit.Test; +import org.springframework.amqp.core.MessageProperties; import org.springframework.amqp.rabbit.listener.exception.ListenerExecutionFailedException; import org.springframework.amqp.rabbit.test.MessageTestUtils; import org.springframework.amqp.support.AmqpHeaders; import org.springframework.amqp.support.converter.Jackson2JsonMessageConverter; +import org.springframework.amqp.support.converter.SimpleMessageConverter; import org.springframework.beans.factory.support.StaticListableBeanFactory; import org.springframework.messaging.Message; import org.springframework.messaging.handler.annotation.support.DefaultMessageHandlerMethodFactory; +import org.springframework.messaging.handler.invocation.InvocableHandlerMethod; import org.springframework.messaging.support.MessageBuilder; import org.springframework.util.ReflectionUtils; +import com.rabbitmq.client.AMQP.BasicProperties; import com.rabbitmq.client.Channel; /** @@ -90,10 +101,62 @@ public class MessagingMessageListenerAdapterTests { assertEquals("Expected test exception", ex.getCause().getMessage()); } catch (Exception ex) { - fail("Should not have thrown another exception"); + fail("Should not have thrown an " + ex.getClass().getSimpleName()); } } + @Test + public void exceptionInListenerBadReturnExceptionSetting() { + org.springframework.amqp.core.Message message = MessageTestUtils.createTextMessage("foo"); + Channel channel = mock(Channel.class); + MessagingMessageListenerAdapter listener = getSimpleInstance("fail", true, String.class); + try { + listener.onMessage(message, channel); + fail("Should have thrown an exception"); + } + catch (ListenerExecutionFailedException ex) { + assertEquals(IllegalArgumentException.class, ex.getCause().getClass()); + assertEquals("Expected test exception", ex.getCause().getMessage()); + } + catch (Exception ex) { + fail("Should not have thrown an " + ex.getClass().getSimpleName()); + } + } + + @Test + public void exceptionInMultiListenerReturnException() throws Exception { + org.springframework.amqp.core.Message message = MessageTestUtils.createTextMessage("foo"); + Channel channel = mock(Channel.class); + MessagingMessageListenerAdapter listener = getMultiInstance("fail", "failWithReturn", true, String.class, + Integer.class); + try { + listener.onMessage(message, channel); + fail("Should have thrown an exception"); + } + catch (ListenerExecutionFailedException ex) { + assertEquals(IllegalArgumentException.class, ex.getCause().getClass()); + assertEquals("Expected test exception", ex.getCause().getMessage()); + } + catch (Exception ex) { + ex.printStackTrace(); + fail("Should not have thrown an " + ex.getClass().getSimpleName()); + } + message = new SimpleMessageConverter().toMessage(42, new MessageProperties()); + try { + listener.onMessage(message, channel); + fail("Should have thrown an exception"); + } + catch (ReplyFailureException ex) { + assertThat(ex.getMessage(), containsString("Failed to send reply")); + } + catch (Exception ex) { + fail("Should not have thrown an " + ex.getClass().getSimpleName()); + } + message.getMessageProperties().setReplyTo("foo/bar"); + listener.onMessage(message, channel); + verify(channel).basicPublish(eq("foo"), eq("bar"), eq(false), any(BasicProperties.class), any(byte[].class)); + } + @Test public void exceptionInInvocation() { org.springframework.amqp.core.Message message = MessageTestUtils.createTextMessage("foo"); @@ -109,7 +172,7 @@ public class MessagingMessageListenerAdapterTests { ex.getCause().getClass()); } catch (Exception ex) { - fail("Should not have thrown another exception"); + fail("Should not have thrown an " + ex.getClass().getSimpleName()); } } @@ -155,15 +218,38 @@ public class MessagingMessageListenerAdapterTests { protected MessagingMessageListenerAdapter getSimpleInstance(String methodName, Class... parameterTypes) { Method m = ReflectionUtils.findMethod(SampleBean.class, methodName, parameterTypes); - return createInstance(m); + return createInstance(m, false); } - protected MessagingMessageListenerAdapter createInstance(Method m) { - MessagingMessageListenerAdapter adapter = new MessagingMessageListenerAdapter(null, m); + protected MessagingMessageListenerAdapter getSimpleInstance(String methodName, boolean returnExceptions, + Class... parameterTypes) { + Method m = ReflectionUtils.findMethod(SampleBean.class, methodName, parameterTypes); + return createInstance(m, returnExceptions); + } + + protected MessagingMessageListenerAdapter createInstance(Method m, boolean returnExceptions) { + MessagingMessageListenerAdapter adapter = new MessagingMessageListenerAdapter(null, m, returnExceptions, null); adapter.setHandlerMethod(new HandlerAdapter(factory.createInvocableHandlerMethod(sample, m))); return adapter; } + protected MessagingMessageListenerAdapter getMultiInstance(String methodName1, String methodName2, + boolean returnExceptions, Class m1ParameterType, Class m2ParameterType) { + Method m1 = ReflectionUtils.findMethod(SampleBean.class, methodName1, m1ParameterType); + Method m2 = ReflectionUtils.findMethod(SampleBean.class, methodName2, m2ParameterType); + return createMultiInstance(m1, m2, returnExceptions); + } + + protected MessagingMessageListenerAdapter createMultiInstance(Method m1, Method m2, boolean returnExceptions) { + MessagingMessageListenerAdapter adapter = new MessagingMessageListenerAdapter(null, null, returnExceptions, null); + List methods = new ArrayList<>(); + methods.add(this.factory.createInvocableHandlerMethod(sample, m1)); + methods.add(this.factory.createInvocableHandlerMethod(sample, m2)); + DelegatingInvocableHandler handler = new DelegatingInvocableHandler(methods, this.sample, null, null); + adapter.setHandlerMethod(new HandlerAdapter(handler)); + return adapter; + } + private void initializeFactory(DefaultMessageHandlerMethodFactory factory) { factory.setBeanFactory(new StaticListableBeanFactory()); factory.afterPropertiesSet(); @@ -214,6 +300,11 @@ public class MessagingMessageListenerAdapterTests { this.payload = message.getPayload(); } + @SuppressWarnings("unused") + public String failWithReturn(Integer input) { + throw new IllegalArgumentException("Expected test exception"); + } + } private static class Foo {