This is an automated email from the ASF dual-hosted git repository. btellier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit b1976f6d64e8c996804b11e586179952d7ebea0c Author: Benoit Tellier <[email protected]> AuthorDate: Wed Sep 4 13:36:09 2019 +0700 JAMES-2097 Avoid retrying already succeeded recipients when sendPartial --- .../james/mailets/RemoteDeliveryErrorTest.java | 6 ++-- .../remote/delivery/InternetAddressConverter.java | 10 +++--- .../mailets/remote/delivery/MailDelivrer.java | 24 ++++++++++++-- .../remote/delivery/MailDelivrerToHost.java | 11 +++++-- .../delivery/InternetAddressConverterTest.java | 4 +-- .../mailets/remote/delivery/MailDelivrerTest.java | 37 +++++++++++----------- 6 files changed, 58 insertions(+), 34 deletions(-) diff --git a/server/mailet/integration-testing/src/test/java/org/apache/james/mailets/RemoteDeliveryErrorTest.java b/server/mailet/integration-testing/src/test/java/org/apache/james/mailets/RemoteDeliveryErrorTest.java index 0eda97c..5b29992 100644 --- a/server/mailet/integration-testing/src/test/java/org/apache/james/mailets/RemoteDeliveryErrorTest.java +++ b/server/mailet/integration-testing/src/test/java/org/apache/james/mailets/RemoteDeliveryErrorTest.java @@ -61,7 +61,6 @@ import org.apache.james.utils.SMTPMessageSender; import org.junit.After; import org.junit.Before; import org.junit.ClassRule; -import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -431,7 +430,6 @@ public class RemoteDeliveryErrorTest { "]"); } - @Ignore("JAMES-2097 Using full recipients for following MX iteration when partial fails on delivering") @Test public void remoteDeliveryShouldNotDuplicateContentWhenSendPartialWhenFailover() throws Exception { ImmutableList<InetAddress> addresses = ImmutableList.of(InetAddress.getByName(mockSmtp.getContainerIp())); @@ -476,7 +474,7 @@ public class RemoteDeliveryErrorTest { .body("", hasSize(1)) .body("[0].from", is(FROM)) .body("[0].recipients", hasSize(1)) - .body("[0].recipients[0]", is(RECIPIENT2)) + .body("[0].recipients[0]", is(RECIPIENT1)) .body("[0].message", containsString("subject: test")); given(requestSpecificationForMockSMTP2, RESPONSE_SPECIFICATION) @@ -485,7 +483,7 @@ public class RemoteDeliveryErrorTest { .body("", hasSize(1)) .body("[0].from", is(FROM)) .body("[0].recipients", hasSize(1)) - .body("[0].recipients[0]", is(RECIPIENT1)) + .body("[0].recipients[0]", is(RECIPIENT2)) .body("[0].message", containsString("subject: test")); } diff --git a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remote/delivery/InternetAddressConverter.java b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remote/delivery/InternetAddressConverter.java index c68b9ec..bbc71fd 100644 --- a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remote/delivery/InternetAddressConverter.java +++ b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remote/delivery/InternetAddressConverter.java @@ -20,20 +20,20 @@ package org.apache.james.transport.mailets.remote.delivery; import java.util.Collection; + import javax.mail.internet.InternetAddress; import org.apache.james.core.MailAddress; +import com.github.steveash.guavate.Guavate; import com.google.common.base.Preconditions; - +import com.google.common.collect.ImmutableSet; public class InternetAddressConverter { - - public static InternetAddress[] convert(Collection<MailAddress> recipients) { + public static ImmutableSet<InternetAddress> convert(Collection<MailAddress> recipients) { Preconditions.checkNotNull(recipients); return recipients.stream() .map(MailAddress::toInternetAddress) - .toArray(InternetAddress[]::new); + .collect(Guavate.toImmutableSet()); } - } diff --git a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrer.java b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrer.java index 64ca447..f3abe26 100644 --- a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrer.java +++ b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrer.java @@ -20,8 +20,13 @@ package org.apache.james.transport.mailets.remote.delivery; import java.io.IOException; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.Optional; +import java.util.Set; import javax.mail.Address; import javax.mail.MessagingException; @@ -37,8 +42,10 @@ import org.apache.mailet.Mail; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.github.steveash.guavate.Guavate; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; @SuppressWarnings("deprecation") @@ -118,14 +125,18 @@ public class MailDelivrer { return rcpt.getDomain(); } - private ExecutionResult doDeliver(Mail mail, InternetAddress[] addr, Iterator<HostAddress> targetServers) throws MessagingException { + private ExecutionResult doDeliver(Mail mail, Set<InternetAddress> addr, Iterator<HostAddress> targetServers) throws MessagingException { MessagingException lastError = null; + Set<InternetAddress> targetAddresses = new HashSet<>(addr); + while (targetServers.hasNext()) { try { - return mailDelivrerToHost.tryDeliveryToHost(mail, addr, targetServers.next()); + return mailDelivrerToHost.tryDeliveryToHost(mail, targetAddresses, targetServers.next()); } catch (SendFailedException sfe) { lastError = handleSendFailExceptionOnMxIteration(mail, sfe); + + targetAddresses.removeAll(listDeliveredAddresses(sfe)); } catch (MessagingException me) { lastError = handleMessagingException(mail, me); if (configuration.isDebug()) { @@ -147,6 +158,15 @@ public class MailDelivrer { return ExecutionResult.temporaryFailure(); } + private Collection<InternetAddress> listDeliveredAddresses(SendFailedException sfe) { + return Optional.ofNullable(sfe.getValidSentAddresses()) + .map(addresses -> + Arrays.stream(addresses) + .map(InternetAddress.class::cast) + .collect(Guavate.toImmutableList())) + .orElse(ImmutableList.of()); + } + private MessagingException handleMessagingException(Mail mail, MessagingException me) throws MessagingException { LOGGER.debug("Exception delivering message ({}) - {}", mail.getName(), me.getMessage()); if ((me.getNextException() != null) && (me.getNextException() instanceof IOException)) { diff --git a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrerToHost.java b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrerToHost.java index d8a3b91..d51af6c 100644 --- a/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrerToHost.java +++ b/server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrerToHost.java @@ -20,6 +20,7 @@ package org.apache.james.transport.mailets.remote.delivery; import java.io.IOException; +import java.util.Collection; import java.util.Properties; import javax.mail.MessagingException; @@ -50,7 +51,7 @@ public class MailDelivrerToHost { this.session = Session.getInstance(configuration.createFinalJavaxProperties()); } - public ExecutionResult tryDeliveryToHost(Mail mail, InternetAddress[] addr, HostAddress outgoingMailServer) throws MessagingException { + public ExecutionResult tryDeliveryToHost(Mail mail, Collection<InternetAddress> addr, HostAddress outgoingMailServer) throws MessagingException { Properties props = getPropertiesForMail(mail); LOGGER.debug("Attempting delivery of {} to host {} at {} from {}", mail.getName(), outgoingMailServer.getHostName(), outgoingMailServer.getHost(), props.get("mail.smtp.from")); @@ -66,7 +67,7 @@ public class MailDelivrerToHost { transport = (SMTPTransport) session.getTransport(outgoingMailServer); transport.setLocalHost(props.getProperty("mail.smtp.localhost", configuration.getHeloNameProvider().getHeloName())); connect(outgoingMailServer, transport); - transport.sendMessage(adaptToTransport(mail.getMessage(), transport), addr); + transport.sendMessage(adaptToTransport(mail.getMessage(), transport), toArray(addr)); LOGGER.debug("Mail ({}) sent successfully to {} at {} from {} for {}", mail.getName(), outgoingMailServer.getHostName(), outgoingMailServer.getHost(), props.get("mail.smtp.from"), mail.getRecipients()); } finally { @@ -75,6 +76,12 @@ public class MailDelivrerToHost { return ExecutionResult.success(); } + private InternetAddress[] toArray(Collection<InternetAddress> addr) { + InternetAddress[] addresses = new InternetAddress[addr.size()]; + addr.toArray(addresses); + return addresses; + } + private Properties getPropertiesForMail(Mail mail) { Properties props = session.getProperties(); props.put("mail.smtp.from", mail.getMaybeSender().asString()); diff --git a/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remote/delivery/InternetAddressConverterTest.java b/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remote/delivery/InternetAddressConverterTest.java index ef1308c..d4283b3 100644 --- a/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remote/delivery/InternetAddressConverterTest.java +++ b/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remote/delivery/InternetAddressConverterTest.java @@ -23,8 +23,6 @@ import static org.assertj.core.api.Assertions.assertThat; import javax.mail.internet.InternetAddress; -import org.apache.james.core.MailAddress; -import org.apache.james.transport.mailets.remote.delivery.InternetAddressConverter; import org.apache.mailet.base.MailAddressFixture; import org.junit.Rule; import org.junit.Test; @@ -39,7 +37,7 @@ public class InternetAddressConverterTest { @Test public void convertShouldWorkWithEmptyAddressList() { - assertThat(InternetAddressConverter.convert(ImmutableList.<MailAddress>of())).isEmpty(); + assertThat(InternetAddressConverter.convert(ImmutableList.of())).isEmpty(); } @Test diff --git a/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrerTest.java b/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrerTest.java index 6c3e88f..4f76eca 100644 --- a/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrerTest.java +++ b/server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/remote/delivery/MailDelivrerTest.java @@ -29,6 +29,7 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import java.io.IOException; +import java.util.Collection; import javax.mail.Address; import javax.mail.MessagingException; @@ -302,11 +303,11 @@ public class MailDelivrerTest { HOST_ADDRESS_1, HOST_ADDRESS_2).iterator(); when(dnsHelper.retrieveHostAddressIterator(MailAddressFixture.JAMES_APACHE_ORG)).thenReturn(dnsEntries); - when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class))) + when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class))) .thenReturn(ExecutionResult.success()); ExecutionResult executionResult = testee.deliver(mail); - verify(mailDelivrerToHost, times(1)).tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class)); + verify(mailDelivrerToHost, times(1)).tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class)); assertThat(executionResult.getExecutionState()).isEqualTo(ExecutionResult.ExecutionState.SUCCESS); } @@ -318,11 +319,11 @@ public class MailDelivrerTest { HOST_ADDRESS_1, HOST_ADDRESS_2).iterator(); when(dnsHelper.retrieveHostAddressIterator(MailAddressFixture.JAMES_APACHE_ORG)).thenReturn(dnsEntries); - when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class))) + when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class))) .thenThrow(new MessagingException("500 : Horrible way to manage Server Return code")); ExecutionResult executionResult = testee.deliver(mail); - verify(mailDelivrerToHost, times(1)).tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class)); + verify(mailDelivrerToHost, times(1)).tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class)); assertThat(executionResult.getExecutionState()).isEqualTo(ExecutionResult.ExecutionState.PERMANENT_FAILURE); } @@ -334,11 +335,11 @@ public class MailDelivrerTest { HOST_ADDRESS_1, HOST_ADDRESS_2).iterator(); when(dnsHelper.retrieveHostAddressIterator(MailAddressFixture.JAMES_APACHE_ORG)).thenReturn(dnsEntries); - when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class))) + when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class))) .thenThrow(new MessagingException("400 : Horrible way to manage Server Return code")); ExecutionResult executionResult = testee.deliver(mail); - verify(mailDelivrerToHost, times(1)).tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class)); + verify(mailDelivrerToHost, times(1)).tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class)); assertThat(executionResult.getExecutionState()).isEqualTo(ExecutionResult.ExecutionState.TEMPORARY_FAILURE); } @@ -350,13 +351,13 @@ public class MailDelivrerTest { HOST_ADDRESS_1, HOST_ADDRESS_2).iterator(); when(dnsHelper.retrieveHostAddressIterator(MailAddressFixture.JAMES_APACHE_ORG)).thenReturn(dnsEntries); - when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), eq(HOST_ADDRESS_1))) + when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(Collection.class), eq(HOST_ADDRESS_1))) .thenThrow(new MessagingException("400 : Horrible way to manage Server Return code", new IOException())); - when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), eq(HOST_ADDRESS_2))) + when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(Collection.class), eq(HOST_ADDRESS_2))) .thenReturn(ExecutionResult.success()); ExecutionResult executionResult = testee.deliver(mail); - verify(mailDelivrerToHost, times(2)).tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class)); + verify(mailDelivrerToHost, times(2)).tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class)); assertThat(executionResult.getExecutionState()).isEqualTo(ExecutionResult.ExecutionState.SUCCESS); } @@ -368,11 +369,11 @@ public class MailDelivrerTest { HOST_ADDRESS_1, HOST_ADDRESS_2).iterator(); when(dnsHelper.retrieveHostAddressIterator(MailAddressFixture.JAMES_APACHE_ORG)).thenReturn(dnsEntries); - when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class))) + when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class))) .thenThrow(new SMTPSenderFailedException(new InternetAddress(MailAddressFixture.ANY_AT_JAMES.toString()), "command", 505, "Big failure")); ExecutionResult executionResult = testee.deliver(mail); - verify(mailDelivrerToHost, times(1)).tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class)); + verify(mailDelivrerToHost, times(1)).tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class)); assertThat(executionResult.getExecutionState()).isEqualTo(ExecutionResult.ExecutionState.PERMANENT_FAILURE); } @@ -384,11 +385,11 @@ public class MailDelivrerTest { HOST_ADDRESS_1, HOST_ADDRESS_2).iterator(); when(dnsHelper.retrieveHostAddressIterator(MailAddressFixture.JAMES_APACHE_ORG)).thenReturn(dnsEntries); - when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class))) + when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class))) .thenThrow(new SendFailedException()); ExecutionResult executionResult = testee.deliver(mail); - verify(mailDelivrerToHost, times(1)).tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class)); + verify(mailDelivrerToHost, times(1)).tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class)); assertThat(executionResult.getExecutionState()).isEqualTo(ExecutionResult.ExecutionState.TEMPORARY_FAILURE); } @@ -408,11 +409,11 @@ public class MailDelivrerTest { HOST_ADDRESS_1, HOST_ADDRESS_2).iterator(); when(dnsHelper.retrieveHostAddressIterator(MailAddressFixture.JAMES_APACHE_ORG)).thenReturn(dnsEntries); - when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class))) + when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class))) .thenThrow(sfe); ExecutionResult executionResult = testee.deliver(mail); - verify(mailDelivrerToHost, times(2)).tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class)); + verify(mailDelivrerToHost, times(2)).tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class)); assertThat(executionResult.getExecutionState()).isEqualTo(ExecutionResult.ExecutionState.TEMPORARY_FAILURE); } @@ -432,13 +433,13 @@ public class MailDelivrerTest { HOST_ADDRESS_1, HOST_ADDRESS_2).iterator(); when(dnsHelper.retrieveHostAddressIterator(MailAddressFixture.JAMES_APACHE_ORG)).thenReturn(dnsEntries); - when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), eq(HOST_ADDRESS_1))) + when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(Collection.class), eq(HOST_ADDRESS_1))) .thenThrow(sfe); - when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), eq(HOST_ADDRESS_2))) + when(mailDelivrerToHost.tryDeliveryToHost(any(Mail.class), any(Collection.class), eq(HOST_ADDRESS_2))) .thenReturn(ExecutionResult.success()); ExecutionResult executionResult = testee.deliver(mail); - verify(mailDelivrerToHost, times(2)).tryDeliveryToHost(any(Mail.class), any(InternetAddress[].class), any(HostAddress.class)); + verify(mailDelivrerToHost, times(2)).tryDeliveryToHost(any(Mail.class), any(Collection.class), any(HostAddress.class)); assertThat(executionResult.getExecutionState()).isEqualTo(ExecutionResult.ExecutionState.SUCCESS); } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
