JAMES-1877 Refactor decision making upon SFE exception
Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/1c6d1d06 Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/1c6d1d06 Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/1c6d1d06 Branch: refs/heads/master Commit: 1c6d1d06c60c911a59f43df50e233e1319796587 Parents: 6a2fe8a Author: Benoit Tellier <btell...@linagora.com> Authored: Wed Dec 7 09:16:44 2016 +0700 Committer: Benoit Tellier <btell...@linagora.com> Committed: Tue Jan 10 15:12:52 2017 +0700 ---------------------------------------------------------------------- .../mailets/remoteDelivery/ExecutionResult.java | 16 ++ .../mailets/remoteDelivery/MailDelivrer.java | 125 ++++--------- .../mailets/remoteDelivery/SFEHelper.java | 66 +++++++ .../remoteDelivery/MailDelivrerTest.java | 183 +++++++++++++++++++ 4 files changed, 297 insertions(+), 93 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/1c6d1d06/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/ExecutionResult.java ---------------------------------------------------------------------- diff --git a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/ExecutionResult.java b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/ExecutionResult.java index f984fd1..b408159 100644 --- a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/ExecutionResult.java +++ b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/ExecutionResult.java @@ -19,6 +19,7 @@ package org.apache.james.transport.mailets.remoteDelivery; +import com.google.common.base.Objects; import com.google.common.base.Optional; public class ExecutionResult { @@ -72,5 +73,20 @@ public class ExecutionResult { public boolean isPermanent() { return executionState == ExecutionState.PERMANENT_FAILURE; } + + @Override + public boolean equals(Object o) { + if (o instanceof ExecutionResult) { + ExecutionResult that = (ExecutionResult) o; + return Objects.equal(this.executionState, that.executionState) + && Objects.equal(this.exception, that.exception); + } + return false; + } + + @Override + public int hashCode() { + return Objects.hashCode(executionState, exception); + } } http://git-wip-us.apache.org/repos/asf/james-project/blob/1c6d1d06/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/MailDelivrer.java ---------------------------------------------------------------------- diff --git a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/MailDelivrer.java b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/MailDelivrer.java index df77eb8..c89a2a0 100644 --- a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/MailDelivrer.java +++ b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/MailDelivrer.java @@ -20,17 +20,15 @@ package org.apache.james.transport.mailets.remoteDelivery; import java.io.IOException; -import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Iterator; +import java.util.List; import javax.mail.Address; import javax.mail.MessagingException; import javax.mail.SendFailedException; import javax.mail.internet.InternetAddress; import javax.mail.internet.MimeMessage; -import javax.mail.internet.ParseException; import org.apache.james.dnsservice.api.DNSService; import org.apache.james.dnsservice.api.TemporaryResolutionException; @@ -39,7 +37,7 @@ import org.apache.mailet.Mail; import org.apache.mailet.MailAddress; import org.slf4j.Logger; -import com.google.common.base.Optional; +import com.google.common.annotations.VisibleForTesting; public class MailDelivrer { @@ -177,105 +175,46 @@ public class MailDelivrer { } } - private ExecutionResult handleSenderFailedException(Mail mail, SendFailedException sfe) { + @VisibleForTesting + ExecutionResult handleSenderFailedException(Mail mail, SendFailedException sfe) { logSendFailedException(sfe); - - // Copy the recipients as direct modification may not be possible - Collection<MailAddress> recipients = new ArrayList<MailAddress>(mail.getRecipients()); - - ExecutionResult deleteMessage = ExecutionResult.temporaryFailure(); EnhancedMessagingException enhancedMessagingException = new EnhancedMessagingException(sfe); - - /* - * If you send a message that has multiple invalid addresses, you'll - * get a top-level SendFailedException that that has the valid, - * valid-unsent, and invalid address lists, with all of the server - * response messages will be contained within the nested exceptions. - * [Note: the content of the nested exceptions is implementation - * dependent.] - * - * sfe.getInvalidAddresses() should be considered permanent. - * sfe.getValidUnsentAddresses() should be considered temporary. - * - * JavaMail v1.3 properly populates those collections based upon the - * 4xx and 5xx response codes to RCPT TO. Some servers, such as - * Yahoo! don't respond to the RCPT TO, and provide a 5xx reply - * after DATA. In that case, we will pick up the failure from - * SMTPSendFailedException. - */ - - /* - * SMTPSendFailedException introduced in JavaMail 1.3.2, and - * provides detailed protocol reply code for the operation - */ - if (enhancedMessagingException.hasReturnCode() || enhancedMessagingException.hasNestedReturnCode()) { - if (enhancedMessagingException.isServerError()) { - deleteMessage = ExecutionResult.permanentFailure(sfe); - } else { - deleteMessage = ExecutionResult.temporaryFailure(sfe); + List<MailAddress> invalidAddresses = SFEHelper.getAddressesAsMailAddress(sfe.getInvalidAddresses(), logger); + List<MailAddress> validUnsentAddresses = SFEHelper.getAddressesAsMailAddress(sfe.getValidUnsentAddresses(), logger); + if (configuration.isDebug()) { + logger.debug("Mail " + mail.getName() + " has initially recipients: " + mail.getRecipients()); + if (!invalidAddresses.isEmpty()) { + logger.debug("Invalid recipients: " + invalidAddresses); + } + if (!validUnsentAddresses.isEmpty()) { + logger.debug("Unsent recipients: " + validUnsentAddresses); } } - - // log the original set of intended recipients - if (configuration.isDebug()) - logger.debug("Recipients: " + recipients); - - if (sfe.getInvalidAddresses() != null) { - Address[] address = sfe.getInvalidAddresses(); - if (address.length > 0) { - recipients.clear(); - for (Address addres : address) { - try { - recipients.add(new MailAddress(addres.toString())); - } catch (ParseException pe) { - // this should never happen ... we should have - // caught malformed addresses long before we - // got to this code. - logger.debug("Can't parse invalid address: " + pe.getMessage()); - } - } - // Set the recipients for the mail - mail.setRecipients(recipients); - - if (configuration.isDebug()) - logger.debug("Invalid recipients: " + recipients); - deleteMessage = ExecutionResult.permanentFailure(sfe); - logger.debug(messageComposer.composeFailLogMessage(mail, deleteMessage)); + if (!validUnsentAddresses.isEmpty()) { + mail.setRecipients(validUnsentAddresses); + if (enhancedMessagingException.hasReturnCode()) { + boolean isPermanent = enhancedMessagingException.isServerError(); + return logAndReturn(mail, ExecutionResult.onFailure(isPermanent, sfe)); + } else { + return logAndReturn(mail, ExecutionResult.temporaryFailure(sfe)); } } + if (!invalidAddresses.isEmpty()) { + mail.setRecipients(invalidAddresses); + return logAndReturn(mail, ExecutionResult.permanentFailure(sfe)); + } - if (sfe.getValidUnsentAddresses() != null) { - Address[] address = sfe.getValidUnsentAddresses(); - if (address.length > 0) { - recipients.clear(); - for (Address addres : address) { - try { - recipients.add(new MailAddress(addres.toString())); - } catch (ParseException pe) { - // this should never happen ... we should have - // caught malformed addresses long before we - // got to this code. - logger.debug("Can't parse unsent address: " + pe.getMessage()); - } - } - // Set the recipients for the mail - mail.setRecipients(recipients); - if (configuration.isDebug()) - logger.debug("Unsent recipients: " + recipients); - - if (enhancedMessagingException.hasReturnCode()) { - boolean isPermanent = enhancedMessagingException.isServerError(); - deleteMessage = ExecutionResult.onFailure(isPermanent, sfe); - logger.debug(messageComposer.composeFailLogMessage(mail, deleteMessage)); - } else { - deleteMessage = ExecutionResult.temporaryFailure(sfe); - logger.debug(messageComposer.composeFailLogMessage(mail, deleteMessage)); - } + if (enhancedMessagingException.hasReturnCode() || enhancedMessagingException.hasNestedReturnCode()) { + if (enhancedMessagingException.isServerError()) { + return ExecutionResult.permanentFailure(sfe); } } + return ExecutionResult.temporaryFailure(sfe); + } - - return deleteMessage; + private ExecutionResult logAndReturn(Mail mail, ExecutionResult executionResult) { + logger.debug(messageComposer.composeFailLogMessage(mail, executionResult)); + return executionResult; } private MessagingException handleSendFailException(Mail mail, SendFailedException sfe) throws SendFailedException { http://git-wip-us.apache.org/repos/asf/james-project/blob/1c6d1d06/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/SFEHelper.java ---------------------------------------------------------------------- diff --git a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/SFEHelper.java b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/SFEHelper.java new file mode 100644 index 0000000..a9650d3 --- /dev/null +++ b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remoteDelivery/SFEHelper.java @@ -0,0 +1,66 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you 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.apache.james.transport.mailets.remoteDelivery; + +import java.util.Arrays; +import java.util.List; + +import javax.mail.Address; +import javax.mail.internet.AddressException; + +import org.apache.mailet.MailAddress; +import org.slf4j.Logger; + +import com.google.common.base.Function; +import com.google.common.base.Optional; +import com.google.common.base.Predicate; +import com.google.common.collect.FluentIterable; +import com.google.common.collect.ImmutableList; + +public class SFEHelper { + + public static List<MailAddress> getAddressesAsMailAddress(Address[] addresses, final Logger logger) { + if (addresses == null) { + return ImmutableList.of(); + } + return FluentIterable.from(Arrays.asList(addresses)).transform(new Function<Address, Optional<MailAddress>>() { + @Override + public Optional<MailAddress> apply(Address input) { + try { + return Optional.of(new MailAddress(input.toString())); + } catch (AddressException e) { + logger.debug("Can't parse unsent address: " + e.getMessage()); + return Optional.absent(); + } + } + }).filter(new Predicate<Optional<MailAddress>>() { + @Override + public boolean apply(Optional<MailAddress> input) { + return input.isPresent(); + } + }).transform(new Function<Optional<MailAddress>, MailAddress>() { + @Override + public MailAddress apply(Optional<MailAddress> input) { + return input.get(); + } + }).toList(); + } + +} http://git-wip-us.apache.org/repos/asf/james-project/blob/1c6d1d06/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remoteDelivery/MailDelivrerTest.java ---------------------------------------------------------------------- diff --git a/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remoteDelivery/MailDelivrerTest.java b/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remoteDelivery/MailDelivrerTest.java new file mode 100644 index 0000000..9c5fe69 --- /dev/null +++ b/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remoteDelivery/MailDelivrerTest.java @@ -0,0 +1,183 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you 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.apache.james.transport.mailets.remoteDelivery; + +import static org.mockito.Mockito.mock; +import static org.assertj.core.api.Assertions.assertThat; + +import javax.mail.Address; +import javax.mail.SendFailedException; +import javax.mail.internet.InternetAddress; + +import org.apache.james.dnsservice.api.DNSService; +import org.apache.mailet.Mail; +import org.apache.mailet.base.MailAddressFixture; +import org.apache.mailet.base.test.FakeMail; +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.sun.mail.smtp.SMTPSenderFailedException; + +public class MailDelivrerTest { + private static final Logger LOGGER = LoggerFactory.getLogger(MailDelivrerTest.class); + + private MailDelivrer testee; + + @Before + public void setUp() { + testee = new MailDelivrer(mock(RemoteDeliveryConfiguration.class), mock(MailDelivrerToHost.class), mock(DNSService.class), LOGGER); + } + + @Test + public void handleSenderFailedExceptionShouldReturnTemporaryFailureByDefault() throws Exception { + Mail mail = FakeMail.builder().recipients(MailAddressFixture.ANY_AT_JAMES, MailAddressFixture.OTHER_AT_JAMES).build(); + + SendFailedException sfe = new SendFailedException(); + ExecutionResult executionResult = testee.handleSenderFailedException(mail, sfe); + + assertThat(executionResult).isEqualTo(ExecutionResult.temporaryFailure(sfe)); + } + + @Test + public void handleSenderFailedExceptionShouldReturnTemporaryFailureWhenNotServerException() throws Exception { + Mail mail = FakeMail.builder().recipients(MailAddressFixture.ANY_AT_JAMES, MailAddressFixture.OTHER_AT_JAMES).build(); + + SendFailedException sfe = new SMTPSenderFailedException(new InternetAddress(MailAddressFixture.OTHER_AT_JAMES.asString()), "Comand", 400, "An temporary error"); + ExecutionResult executionResult = testee.handleSenderFailedException(mail, sfe); + + assertThat(executionResult).isEqualTo(ExecutionResult.temporaryFailure(sfe)); + } + + @Ignore("Return code is always ignored") + @Test + public void handleSenderFailedExceptionShouldReturnPermanentFailureWhenServerException() throws Exception { + Mail mail = FakeMail.builder().recipients(MailAddressFixture.ANY_AT_JAMES, MailAddressFixture.OTHER_AT_JAMES).build(); + + SendFailedException sfe = new SMTPSenderFailedException(new InternetAddress(MailAddressFixture.OTHER_AT_JAMES.asString()), "Comand", 505, "An temporary error"); + ExecutionResult executionResult = testee.handleSenderFailedException(mail, sfe); + + assertThat(executionResult).isEqualTo(ExecutionResult.permanentFailure(sfe)); + } + + @Test + public void handleSenderFailedExceptionShouldReturnPermanentFailureWhenInvalidAndNotValidUnsent() throws Exception { + Mail mail = FakeMail.builder().recipients(MailAddressFixture.ANY_AT_JAMES, MailAddressFixture.OTHER_AT_JAMES).build(); + + Address[] validSent = {}; + Address[] validUnsent = {}; + Address[] invalid = {new InternetAddress(MailAddressFixture.ANY_AT_JAMES.asString())}; + SendFailedException sfe = new SendFailedException("Message", + new Exception(), + validSent, + validUnsent, + invalid); + ExecutionResult executionResult = testee.handleSenderFailedException(mail, sfe); + + assertThat(executionResult).isEqualTo(ExecutionResult.permanentFailure(sfe)); + } + + @Test + public void handleSenderFailedExceptionShouldReturnTemporaryFailureWhenValidUnsent() throws Exception { + Mail mail = FakeMail.builder().recipients(MailAddressFixture.ANY_AT_JAMES, MailAddressFixture.OTHER_AT_JAMES).build(); + + Address[] validSent = {}; + Address[] validUnsent = {new InternetAddress(MailAddressFixture.OTHER_AT_JAMES.asString())}; + Address[] invalid = {}; + SendFailedException sfe = new SendFailedException("Message", + new Exception(), + validSent, + validUnsent, + invalid); + ExecutionResult executionResult = testee.handleSenderFailedException(mail, sfe); + + assertThat(executionResult).isEqualTo(ExecutionResult.temporaryFailure(sfe)); + } + + @Test + public void handleSenderFailedExceptionShouldReturnTemporaryFailureWhenInvalidAndValidUnsent() throws Exception { + Mail mail = FakeMail.builder().recipients(MailAddressFixture.ANY_AT_JAMES, MailAddressFixture.OTHER_AT_JAMES).build(); + + Address[] validSent = {}; + Address[] validUnsent = {new InternetAddress(MailAddressFixture.OTHER_AT_JAMES.asString())}; + Address[] invalid = {new InternetAddress(MailAddressFixture.ANY_AT_JAMES.asString())}; + SendFailedException sfe = new SendFailedException("Message", + new Exception(), + validSent, + validUnsent, + invalid); + ExecutionResult executionResult = testee.handleSenderFailedException(mail, sfe); + + assertThat(executionResult).isEqualTo(ExecutionResult.temporaryFailure(sfe)); + } + + @Test + public void handleSenderFailedExceptionShouldSetRecipientToInvalidWhenOnlyInvalid() throws Exception { + Mail mail = FakeMail.builder().recipients(MailAddressFixture.ANY_AT_JAMES, MailAddressFixture.OTHER_AT_JAMES).build(); + + Address[] validSent = {}; + Address[] validUnsent = {}; + Address[] invalid = {new InternetAddress(MailAddressFixture.ANY_AT_JAMES.asString())}; + SendFailedException sfe = new SendFailedException("Message", + new Exception(), + validSent, + validUnsent, + invalid); + testee.handleSenderFailedException(mail, sfe); + + assertThat(mail.getRecipients()).containsOnly(MailAddressFixture.ANY_AT_JAMES); + } + + @Test + public void handleSenderFailedExceptionShouldSetRecipientToValidUnsentWhenOnlyValidUnsent() throws Exception { + Mail mail = FakeMail.builder().recipients(MailAddressFixture.ANY_AT_JAMES, MailAddressFixture.OTHER_AT_JAMES).build(); + + Address[] validSent = {}; + Address[] validUnsent = {new InternetAddress(MailAddressFixture.OTHER_AT_JAMES.asString())}; + Address[] invalid = {}; + SendFailedException sfe = new SendFailedException("Message", + new Exception(), + validSent, + validUnsent, + invalid); + testee.handleSenderFailedException(mail, sfe); + + assertThat(mail.getRecipients()).containsOnly(MailAddressFixture.OTHER_AT_JAMES); + } + + @Test + public void handleSenderFailedExceptionShouldSetRecipientToValidUnsentWhenValidUnsentAndInvalid() throws Exception { + Mail mail = FakeMail.builder().recipients(MailAddressFixture.ANY_AT_JAMES, MailAddressFixture.OTHER_AT_JAMES).build(); + + Address[] validSent = {}; + Address[] validUnsent = {new InternetAddress(MailAddressFixture.OTHER_AT_JAMES.asString())}; + Address[] invalid = {new InternetAddress(MailAddressFixture.ANY_AT_JAMES.asString())}; + SendFailedException sfe = new SendFailedException("Message", + new Exception(), + validSent, + validUnsent, + invalid); + testee.handleSenderFailedException(mail, sfe); + + assertThat(mail.getRecipients()).containsOnly(MailAddressFixture.OTHER_AT_JAMES); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org