JAMES-1877 Improve Bouncer : - Stop failing on empty exception error message - Remove extra space in message - Move methods of MessageComposer used only by Bouncer
Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/6a2fe8a8 Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/6a2fe8a8 Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/6a2fe8a8 Branch: refs/heads/master Commit: 6a2fe8a883e37afaebcd832dcee1082314b65d2d Parents: 9b59d55 Author: Benoit Tellier <[email protected]> Authored: Fri Dec 2 13:57:11 2016 +0700 Committer: Benoit Tellier <[email protected]> Committed: Tue Jan 10 15:12:52 2017 +0700 ---------------------------------------------------------------------- .../mailets/remoteDelivery/Bouncer.java | 69 +++++++++++++------- .../mailets/remoteDelivery/MessageComposer.java | 23 ------- .../mailets/remoteDelivery/BouncerTest.java | 62 +++++++++++++----- 3 files changed, 93 insertions(+), 61 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/6a2fe8a8/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/Bouncer.java ---------------------------------------------------------------------- diff --git a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/Bouncer.java b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/Bouncer.java index da90b08..fa60c23 100644 --- a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/Bouncer.java +++ b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/Bouncer.java @@ -37,28 +37,26 @@ public class Bouncer { public static final String DELIVERY_ERROR = "delivery-error"; private final RemoteDeliveryConfiguration configuration; - private final MessageComposer messageComposer; private final MailetContext mailetContext; private final Logger logger; public Bouncer(RemoteDeliveryConfiguration configuration, MailetContext mailetContext, Logger logger) { this.configuration = configuration; - this.messageComposer = new MessageComposer(configuration); this.mailetContext = mailetContext; this.logger = logger; } public void bounce(Mail mail, Exception ex) { if (mail.getSender() == null) { - logger.debug("Null Sender: no bounce will be generated for " + mail.getName()); + logger.debug("Null Sender: no bounce will be generated for {}", mail.getName()); } else { if (configuration.getBounceProcessor() != null) { - mail.setAttribute(DELIVERY_ERROR, messageComposer.getErrorMsg(ex)); + mail.setAttribute(DELIVERY_ERROR, getErrorMsg(ex)); mail.setState(configuration.getBounceProcessor()); try { mailetContext.sendMail(mail); } catch (MessagingException e) { - logger.debug("Exception re-inserting failed mail: ", e); + logger.warn("Exception re-inserting failed mail: ", e); } } else { bounceWithMailetContext(mail, ex); @@ -72,26 +70,18 @@ public class Bouncer { try { mailetContext.bounce(mail, explanationText(mail, ex)); } catch (MessagingException me) { - logger.debug("Encountered unexpected messaging exception while bouncing message: " + me.getMessage()); + logger.warn("Encountered unexpected messaging exception while bouncing message: {}", me.getMessage()); } catch (Exception e) { - logger.debug("Encountered unexpected exception while bouncing message: " + e.getMessage()); + logger.warn("Encountered unexpected exception while bouncing message: {}", e.getMessage()); } } public String explanationText(Mail mail, Exception ex) { StringWriter sout = new StringWriter(); PrintWriter out = new PrintWriter(sout, true); - String machine; - try { - machine = configuration.getHeloNameProvider().getHeloName(); - - } catch (Exception e) { - machine = "[address unknown]"; - } - String bounceBuffer = "Hi. This is the James mail server at " + machine + "."; - out.println(bounceBuffer); + out.println("Hi. This is the James mail server at " + resolveMachineName() + "."); out.println("I'm afraid I wasn't able to deliver your message to the following addresses."); - out.println("This is a permanent error; I've given up. Sorry it didn't work out. Below"); + out.println("This is a permanent error; I've given up. Sorry it didn't work out. Below"); out.println("I include the list of recipients and the reason why I was unable to deliver"); out.println("your message."); out.println(); @@ -100,25 +90,58 @@ public class Bouncer { } if (ex instanceof MessagingException) { if (((MessagingException) ex).getNextException() == null) { - out.println(ex.getMessage().trim()); + out.println(sanitizeExceptionMessage(ex)); } else { Exception ex1 = ((MessagingException) ex).getNextException(); if (ex1 instanceof SendFailedException) { - out.println("Remote mail server told me: " + ex1.getMessage().trim()); + out.println("Remote mail server told me: " + sanitizeExceptionMessage(ex1)); } else if (ex1 instanceof UnknownHostException) { - out.println("Unknown host: " + ex1.getMessage().trim()); + out.println("Unknown host: " + sanitizeExceptionMessage(ex1)); out.println("This could be a DNS server error, a typo, or a problem with the recipient's mail server."); } else if (ex1 instanceof ConnectException) { // Already formatted as "Connection timed out: connect" - out.println(ex1.getMessage().trim()); + out.println(sanitizeExceptionMessage(ex1)); } else if (ex1 instanceof SocketException) { - out.println("Socket exception: " + ex1.getMessage().trim()); + out.println("Socket exception: " + sanitizeExceptionMessage(ex1)); } else { - out.println(ex1.getMessage().trim()); + out.println(sanitizeExceptionMessage(ex1)); } } } out.println(); return sout.toString(); } + + private String sanitizeExceptionMessage(Exception e) { + if (e.getMessage() == null) { + return "null"; + } else { + return e.getMessage().trim(); + } + } + + private String resolveMachineName() { + try { + return configuration.getHeloNameProvider().getHeloName(); + } catch (Exception e) { + return "[address unknown]"; + } + } + + public String getErrorMsg(Exception ex) { + if (ex instanceof MessagingException) { + return getNestedExceptionMessage((MessagingException) ex); + } else { + return sanitizeExceptionMessage(ex); + } + } + + public String getNestedExceptionMessage(MessagingException me) { + if (me.getNextException() == null) { + return sanitizeExceptionMessage(me); + } else { + Exception ex1 = me.getNextException(); + return sanitizeExceptionMessage(ex1); + } + } } http://git-wip-us.apache.org/repos/asf/james-project/blob/6a2fe8a8/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/MessageComposer.java ---------------------------------------------------------------------- diff --git a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/MessageComposer.java b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/MessageComposer.java index 3e6d861..74852e9 100644 --- a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/MessageComposer.java +++ b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/MessageComposer.java @@ -99,29 +99,6 @@ public class MessageComposer { return null; } - /** - * Utility method for getting the error message from the (nested) exception. - * - * @param me MessagingException - * @return error message - */ - public String getErrorMsg(Exception ex) { - if (ex instanceof MessagingException) { - return getNestedExceptionMessage((MessagingException) ex); - } else { - return ex.getMessage(); - } - } - - public String getNestedExceptionMessage(MessagingException me) { - if (me.getNextException() == null) { - return me.getMessage().trim(); - } else { - Exception ex1 = me.getNextException(); - return ex1.getMessage().trim(); - } - } - public String composeFailLogMessage(Mail mail, ExecutionResult executionResult) { StringWriter sout = new StringWriter(); PrintWriter out = new PrintWriter(sout, true); http://git-wip-us.apache.org/repos/asf/james-project/blob/6a2fe8a8/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remoteDelivery/BouncerTest.java ---------------------------------------------------------------------- diff --git a/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remoteDelivery/BouncerTest.java b/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remoteDelivery/BouncerTest.java index de683c0..cc600ad 100644 --- a/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remoteDelivery/BouncerTest.java +++ b/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remoteDelivery/BouncerTest.java @@ -74,7 +74,7 @@ public class BouncerTest { FakeMailContext.BouncedMail expected = new FakeMailContext.BouncedMail(FakeMailContext.fromMail(mail), "Hi. This is the James mail server at " + HELLO_NAME + ".\n" + "I'm afraid I wasn't able to deliver your message to the following addresses.\n" + - "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + + "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + "I include the list of recipients and the reason why I was unable to deliver\n" + "your message.\n\n\n", Optional.<MailAddress>absent()); @@ -101,7 +101,7 @@ public class BouncerTest { FakeMailContext.BouncedMail expected = new FakeMailContext.BouncedMail(FakeMailContext.fromMail(mail), "Hi. This is the James mail server at " + HELLO_NAME + ".\n" + "I'm afraid I wasn't able to deliver your message to the following addresses.\n" + - "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + + "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + "I include the list of recipients and the reason why I was unable to deliver\n" + "your message.\n" + "\n" + @@ -130,7 +130,7 @@ public class BouncerTest { FakeMailContext.BouncedMail expected = new FakeMailContext.BouncedMail(FakeMailContext.fromMail(mail), "Hi. This is the James mail server at " + HELLO_NAME + ".\n" + "I'm afraid I wasn't able to deliver your message to the following addresses.\n" + - "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + + "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + "I include the list of recipients and the reason why I was unable to deliver\n" + "your message.\n" + "\n" + @@ -159,7 +159,7 @@ public class BouncerTest { FakeMailContext.BouncedMail expected = new FakeMailContext.BouncedMail(FakeMailContext.fromMail(mail), "Hi. This is the James mail server at " + HELLO_NAME + ".\n" + "I'm afraid I wasn't able to deliver your message to the following addresses.\n" + - "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + + "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + "I include the list of recipients and the reason why I was unable to deliver\n" + "your message.\n" + "\n" + @@ -189,7 +189,7 @@ public class BouncerTest { FakeMailContext.BouncedMail expected = new FakeMailContext.BouncedMail(FakeMailContext.fromMail(mail), "Hi. This is the James mail server at " + HELLO_NAME + ".\n" + "I'm afraid I wasn't able to deliver your message to the following addresses.\n" + - "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + + "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + "I include the list of recipients and the reason why I was unable to deliver\n" + "your message.\n" + "\n" + @@ -218,7 +218,7 @@ public class BouncerTest { FakeMailContext.BouncedMail expected = new FakeMailContext.BouncedMail(FakeMailContext.fromMail(mail), "Hi. This is the James mail server at " + HELLO_NAME + ".\n" + "I'm afraid I wasn't able to deliver your message to the following addresses.\n" + - "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + + "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + "I include the list of recipients and the reason why I was unable to deliver\n" + "your message.\n" + "\n" + @@ -247,7 +247,7 @@ public class BouncerTest { FakeMailContext.BouncedMail expected = new FakeMailContext.BouncedMail(FakeMailContext.fromMail(mail), "Hi. This is the James mail server at " + HELLO_NAME + ".\n" + "I'm afraid I wasn't able to deliver your message to the following addresses.\n" + - "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + + "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + "I include the list of recipients and the reason why I was unable to deliver\n" + "your message.\n" + "\n" + @@ -277,7 +277,7 @@ public class BouncerTest { } @Test - public void bounceShouldSupportExceptionWithoutMessagesByDefaultByDefault() throws Exception { + public void bounceShouldSupportExceptionWithoutMessagesByDefault() throws Exception { RemoteDeliveryConfiguration configuration = new RemoteDeliveryConfiguration( FakeMailetConfig.builder() .setProperty(RemoteDeliveryConfiguration.DELIVERY_THREADS, "1") @@ -294,7 +294,7 @@ public class BouncerTest { FakeMailContext.BouncedMail expected = new FakeMailContext.BouncedMail(FakeMailContext.fromMail(mail), "Hi. This is the James mail server at " + HELLO_NAME + ".\n" + "I'm afraid I wasn't able to deliver your message to the following addresses.\n" + - "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + + "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + "I include the list of recipients and the reason why I was unable to deliver\n" + "your message.\n\n\n", Optional.<MailAddress>absent()); @@ -303,7 +303,7 @@ public class BouncerTest { } @Test - public void bounceShouldNotSupportMessagingExceptionWithoutMessagesByDefaultByDefault() throws Exception { + public void bounceShouldNotSupportMessagingExceptionWithoutMessagesByDefault() throws Exception { RemoteDeliveryConfiguration configuration = new RemoteDeliveryConfiguration( FakeMailetConfig.builder() .setProperty(RemoteDeliveryConfiguration.DELIVERY_THREADS, "1") @@ -317,8 +317,15 @@ public class BouncerTest { .build(); testee.bounce(mail, new MessagingException()); + FakeMailContext.BouncedMail expected = new FakeMailContext.BouncedMail(FakeMailContext.fromMail(mail), + "Hi. This is the James mail server at " + HELLO_NAME + ".\n" + + "I'm afraid I wasn't able to deliver your message to the following addresses.\n" + + "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + + "I include the list of recipients and the reason why I was unable to deliver\n" + + "your message.\n\nnull\n\n", + Optional.<MailAddress>absent()); assertThat(mailetContext.getSentMails()).isEmpty(); - assertThat(mailetContext.getBouncedMails()).isEmpty(); + assertThat(mailetContext.getBouncedMails()).containsOnly(expected); } @Test @@ -367,7 +374,7 @@ public class BouncerTest { } @Test - public void bounceShouldDisplayAddressByDefaultByDefault() throws Exception { + public void bounceShouldDisplayAddressByDefault() throws Exception { RemoteDeliveryConfiguration configuration = new RemoteDeliveryConfiguration( FakeMailetConfig.builder() .setProperty(RemoteDeliveryConfiguration.DELIVERY_THREADS, "1") @@ -385,7 +392,7 @@ public class BouncerTest { FakeMailContext.BouncedMail expected = new FakeMailContext.BouncedMail(FakeMailContext.fromMail(mail), "Hi. This is the James mail server at " + HELLO_NAME + ".\n" + "I'm afraid I wasn't able to deliver your message to the following addresses.\n" + - "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + + "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + "I include the list of recipients and the reason why I was unable to deliver\n" + "your message.\n\n" + MailAddressFixture.ANY_AT_JAMES2.asString() + "\n\n", @@ -395,7 +402,7 @@ public class BouncerTest { } @Test - public void bounceShouldDisplayAddressesByDefaultByDefault() throws Exception { + public void bounceShouldDisplayAddressesByDefault() throws Exception { RemoteDeliveryConfiguration configuration = new RemoteDeliveryConfiguration( FakeMailetConfig.builder() .setProperty(RemoteDeliveryConfiguration.DELIVERY_THREADS, "1") @@ -413,7 +420,7 @@ public class BouncerTest { FakeMailContext.BouncedMail expected = new FakeMailContext.BouncedMail(FakeMailContext.fromMail(mail), "Hi. This is the James mail server at " + HELLO_NAME + ".\n" + "I'm afraid I wasn't able to deliver your message to the following addresses.\n" + - "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + + "This is a permanent error; I've given up. Sorry it didn't work out. Below\n" + "I include the list of recipients and the reason why I was unable to deliver\n" + "your message.\n\n" + MailAddressFixture.ANY_AT_JAMES2.asString() + "\n" + @@ -422,4 +429,29 @@ public class BouncerTest { assertThat(mailetContext.getSentMails()).isEmpty(); assertThat(mailetContext.getBouncedMails()).containsOnly(expected); } + + @Test + public void bounceShouldWorkWhenProcessorSpecifiedAndNoExceptionMessage() throws Exception { + RemoteDeliveryConfiguration configuration = new RemoteDeliveryConfiguration( + FakeMailetConfig.builder() + .setProperty(RemoteDeliveryConfiguration.DELIVERY_THREADS, "1") + .setProperty(RemoteDeliveryConfiguration.HELO_NAME, HELLO_NAME) + .setProperty(RemoteDeliveryConfiguration.BOUNCE_PROCESSOR, BOUNCE_PROCESSOR) + .build(), + mock(DomainList.class)); + Bouncer testee = new Bouncer(configuration, mailetContext, LOGGER); + + Mail mail = FakeMail.builder().state(Mail.DEFAULT) + .sender(MailAddressFixture.ANY_AT_JAMES) + .build(); + testee.bounce(mail, new MessagingException()); + + FakeMailContext.SentMail expected = FakeMailContext.sentMailBuilder() + .sender(MailAddressFixture.ANY_AT_JAMES) + .attribute(DELIVERY_ERROR, "null") + .state(BOUNCE_PROCESSOR) + .build(); + assertThat(mailetContext.getSentMails()).containsOnly(expected); + assertThat(mailetContext.getBouncedMails()).isEmpty(); + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
