Skip to content

Commit

Permalink
JAMES-2557 Improve RRT processor code quality
Browse files Browse the repository at this point in the history 
 - Avoid passing some parameters
 - Remove uneeded exceptions
 - Avoid knowledge duplication with Stream partitioning
  • Loading branch information
@chibenwa
chibenwa committed Oct 30, 2018
1 parent d29eec7 commit 4d2f085
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 34 deletions.
42 changes: 17 additions & 25 deletions ...lets/src/main/java/org/apache/james/transport/mailets/RecipientRewriteTableProcessor.java
View file
Expand Up @@ -19,12 +19,14 @@

package org.apache.james.transport.mailets;

import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collectors;

import javax.mail.MessagingException;
import javax.mail.internet.MimeMessage;

import org.apache.james.core.Domain;
import org.apache.james.core.MailAddress;
Expand All @@ -44,7 +46,6 @@
import org.slf4j.LoggerFactory;

import com.github.fge.lambdas.Throwing;
import com.github.steveash.guavate.Guavate;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -163,52 +164,43 @@ private RrtExecutionResult executeRrtForRecipient(Mail mail, MailAddress recipie
Mappings mappings = virtualTableStore.getMappings(recipient.getLocalPart(), recipient.getDomain());

if (mappings != null && !mappings.isEmpty()) {
List<MailAddress> newMailAddresses = handleMappings(mappings, mail, recipient, mail.getMessage());
List<MailAddress> newMailAddresses = handleMappings(mappings, mail, recipient);
return RrtExecutionResult.success(newMailAddresses);
}
return RrtExecutionResult.success(recipient);
} catch (ErrorMappingException | RecipientRewriteTableException | MessagingException e) {
} catch (ErrorMappingException | RecipientRewriteTableException e) {
LOGGER.warn("Could not rewrite recipient {}", recipient, e);
return RrtExecutionResult.error(recipient);
}
}

@VisibleForTesting
List<MailAddress> handleMappings(Mappings mappings, Mail mail, MailAddress recipient, MimeMessage message) throws MessagingException {
ImmutableList<MailAddress> mailAddresses = mappings.asStream()
List<MailAddress> handleMappings(Mappings mappings, Mail mail, MailAddress recipient) {
boolean isLocal = true;
Map<Boolean, List<MailAddress>> mailAddressSplit = mappings.asStream()
.map(mapping -> mapping.appendDomainIfNone(defaultDomainSupplier))
.map(Mapping::asMailAddress)
.flatMap(OptionalUtils::toStream)
.collect(Guavate.toImmutableList());
.collect(Collectors.partitioningBy(mailAddress -> mailetContext.isLocalServer(mailAddress.getDomain())));

forwardToRemoteAddress(mail, recipient, message, mailAddresses);
forwardToRemoteAddress(mail, recipient, mailAddressSplit.get(!isLocal));

return getLocalAddresses(mailAddresses);
return mailAddressSplit.get(isLocal);
}

private ImmutableList<MailAddress> getLocalAddresses(ImmutableList<MailAddress> mailAddresses) {
return mailAddresses.stream()
.filter(mailAddress -> mailetContext.isLocalServer(mailAddress.getDomain()))
.collect(Guavate.toImmutableList());
}

private void forwardToRemoteAddress(Mail mail, MailAddress recipient, MimeMessage message, ImmutableList<MailAddress> mailAddresses) {
ImmutableList<MailAddress> remoteAddresses = mailAddresses.stream()
.filter(mailAddress -> !mailetContext.isLocalServer(mailAddress.getDomain()))
.collect(Guavate.toImmutableList());

if (!remoteAddresses.isEmpty()) {
private void forwardToRemoteAddress(Mail mail, MailAddress recipient, Collection<MailAddress> remoteRecipients) {
if (!remoteRecipients.isEmpty()) {
try {
mailetContext.sendMail(
MailImpl.builder()
.name(mail.getName())
.sender(mail.getMaybeSender())
.recipients(remoteAddresses)
.mimeMessage(message)
.recipients(ImmutableList.copyOf(remoteRecipients))
.mimeMessage(mail.getMessage())
.build());
LOGGER.info("Mail for {} forwarded to {}", recipient, remoteAddresses);
LOGGER.info("Mail for {} forwarded to {}", recipient, remoteRecipients);
} catch (MessagingException ex) {
LOGGER.warn("Error forwarding mail to {}", remoteAddresses);
LOGGER.warn("Error forwarding mail to {}", remoteRecipients);
}
}
}
Expand Down
18 changes: 9 additions & 9 deletions .../src/test/java/org/apache/james/transport/mailets/RecipientRewriteTableProcessorTest.java
View file
Expand Up @@ -86,7 +86,7 @@ public void handleMappingsShouldThrowExceptionWhenMappingsContainAtLeastOneNoneD
.add(MailAddressFixture.OTHER_AT_JAMES.toString())
.build();

processor.handleMappings(mappings, FakeMail.builder().sender(MailAddressFixture.ANY_AT_JAMES).build(), MailAddressFixture.OTHER_AT_JAMES, message);
processor.handleMappings(mappings, FakeMail.builder().sender(MailAddressFixture.ANY_AT_JAMES).build(), MailAddressFixture.OTHER_AT_JAMES);
}

@Test
Expand All @@ -97,7 +97,7 @@ public void handleMappingsShouldDoNotCareDefaultDomainWhenMappingsDoesNotContain
.add(MailAddressFixture.OTHER_AT_JAMES.toString())
.build();

processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES, message);
processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES);
}

@Test
Expand All @@ -109,7 +109,7 @@ public void handleMappingsShouldReturnTheMailAddressBelongToLocalServer() throws
.add(MailAddressFixture.OTHER_AT_JAMES.toString())
.build();

Collection<MailAddress> result = processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES, message);
Collection<MailAddress> result = processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES);

assertThat(result).containsOnly(nonDomainWithDefaultLocal);
}
Expand All @@ -125,7 +125,7 @@ public void handleMappingsShouldReturnTheOnlyMailAddressBelongToLocalServer() th
.add(MailAddressFixture.OTHER_AT_JAMES.toString())
.build();

Collection<MailAddress> result = processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES, message);
Collection<MailAddress> result = processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES);

assertThat(result).containsOnly(MailAddressFixture.ANY_AT_LOCAL);
}
Expand All @@ -140,7 +140,7 @@ public void handleMappingsShouldRemoveMappingElementWhenCannotCreateMailAddress(
.add(MailAddressFixture.OTHER_AT_JAMES.toString())
.build();

Collection<MailAddress> result = processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES, message);
Collection<MailAddress> result = processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES);

assertThat(result).containsOnly(nonDomainWithDefaultLocal);
}
Expand All @@ -156,7 +156,7 @@ public void handleMappingsShouldForwardEmailToRemoteServer() throws Exception {
.add(MailAddressFixture.OTHER_AT_JAMES.toString())
.build();

processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES, message);
processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES);

FakeMailContext.SentMail expected = FakeMailContext.sentMailBuilder()
.sender(MailAddressFixture.ANY_AT_JAMES)
Expand All @@ -177,7 +177,7 @@ public void handleMappingsShouldNotForwardAnyEmailToRemoteServerWhenNoMoreReomot
.add(INVALID_MAIL_ADDRESS)
.build();

processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES, message);
processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES);

assertThat(mailetContext.getSentMails()).isEmpty();
}
Expand All @@ -192,7 +192,7 @@ public void handleMappingWithOnlyLocalRecipient() throws Exception {
.add(MailAddressFixture.ANY_AT_LOCAL.toString())
.build();

Collection<MailAddress> result = processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES, message);
Collection<MailAddress> result = processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES);

assertThat(result).containsOnly(nonDomainWithDefaultLocal, MailAddressFixture.ANY_AT_LOCAL);
}
Expand All @@ -206,7 +206,7 @@ public void handleMappingWithOnlyRemoteRecipient() throws Exception {
.add(MailAddressFixture.OTHER_AT_JAMES.toString())
.build();

Collection<MailAddress> result = processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES, message);
Collection<MailAddress> result = processor.handleMappings(mappings, mail, MailAddressFixture.OTHER_AT_JAMES);

FakeMailContext.SentMail expected = FakeMailContext.sentMailBuilder()
.sender(MailAddressFixture.ANY_AT_JAMES)
Expand Down

0 comments on commit 4d2f085

Please sign in to comment.