JAMES-2529 ActionApplier should handle MailboxNotFound This case can happen frequently. Example:
- Bob create the 'toto' mailbox - Bob set up a rule to redirect mails from Fred to the 'toto' mailbox - Bob deletes the 'toto' mailbox - Fred sends a mail to Bob Note that this is implemented by applying all actions (and thus relaxing the 'first applied' tests) Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/8b1c70ab Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/8b1c70ab Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/8b1c70ab Branch: refs/heads/master Commit: 8b1c70aba0032173c21ed377820eba4932c811fb Parents: 97ba650 Author: Benoit Tellier <[email protected]> Authored: Thu Aug 30 11:33:40 2018 +0700 Committer: Antoine Duprat <[email protected]> Committed: Thu Aug 30 15:07:02 2018 +0200 ---------------------------------------------------------------------- .../james/jmap/mailet/filter/ActionApplier.java | 33 +++-- .../james/jmap/mailet/filter/JMAPFiltering.java | 9 +- .../james/jmap/mailet/filter/RuleMatcher.java | 7 +- .../mailet/filter/JMAPFilteringExtension.java | 4 +- .../jmap/mailet/filter/JMAPFilteringTest.java | 143 +++++++++++++++++-- 5 files changed, 160 insertions(+), 36 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/8b1c70ab/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/ActionApplier.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/ActionApplier.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/ActionApplier.java index fdb55ac..7006801 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/ActionApplier.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/ActionApplier.java @@ -19,6 +19,8 @@ package org.apache.james.jmap.mailet.filter; +import java.util.stream.Stream; + import javax.inject.Inject; import org.apache.james.core.User; @@ -27,14 +29,18 @@ import org.apache.james.mailbox.MailboxManager; import org.apache.james.mailbox.MailboxSession; import org.apache.james.mailbox.MessageManager; import org.apache.james.mailbox.exception.MailboxException; +import org.apache.james.mailbox.exception.MailboxNotFoundException; import org.apache.james.mailbox.model.MailboxId; import org.apache.mailet.Mail; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import com.github.fge.lambdas.Throwing; import com.google.common.annotations.VisibleForTesting; public class ActionApplier { static final String DELIVERY_PATH_PREFIX = "DeliveryPath_"; + public static final Logger LOGGER = LoggerFactory.getLogger(ActionApplier.class); @VisibleForTesting static class Factory { @@ -81,21 +87,22 @@ public class ActionApplier { this.user = user; } - public void apply(Rule.Action action) { - action.getAppendInMailboxes() - .getMailboxIds() - .stream() - .findFirst() - .map(mailboxIdFactory::fromString) - .ifPresent(Throwing.consumer(this::addStorageDirective)); + public void apply(Stream<Rule.Action> actions) { + actions.flatMap(action -> action.getAppendInMailboxes().getMailboxIds().stream()) + .map(mailboxIdFactory::fromString) + .forEach(Throwing.consumer(this::addStorageDirective)); } private void addStorageDirective(MailboxId mailboxId) throws MailboxException { - MailboxSession mailboxSession = mailboxManager.createSystemSession(user.asString()); - MessageManager messageManager = mailboxManager.getMailbox(mailboxId, mailboxSession); - - String mailboxName = messageManager.getMailboxPath().getName(); - String attributeNameForUser = DELIVERY_PATH_PREFIX + user.asString(); - mail.setAttribute(attributeNameForUser, mailboxName); + try { + MailboxSession mailboxSession = mailboxManager.createSystemSession(user.asString()); + MessageManager messageManager = mailboxManager.getMailbox(mailboxId, mailboxSession); + + String mailboxName = messageManager.getMailboxPath().getName(); + String attributeNameForUser = DELIVERY_PATH_PREFIX + user.asString(); + mail.setAttribute(attributeNameForUser, mailboxName); + } catch (MailboxNotFoundException e) { + LOGGER.info("Mailbox {} does not exist, but it was mentioned in a JMAP filtering rule", mailboxId, e); + } } } http://git-wip-us.apache.org/repos/asf/james-project/blob/8b1c70ab/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/JMAPFiltering.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/JMAPFiltering.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/JMAPFiltering.java index 388fabe..a68193a 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/JMAPFiltering.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/JMAPFiltering.java @@ -21,6 +21,7 @@ package org.apache.james.jmap.mailet.filter; import java.util.List; import java.util.Optional; +import java.util.stream.Stream; import javax.inject.Inject; @@ -67,11 +68,11 @@ public class JMAPFiltering extends GenericMailet { private void findFirstApplicableRule(User user, Mail mail) { List<Rule> filteringRules = filteringManagement.listRulesForUser(user); RuleMatcher ruleMatcher = new RuleMatcher(filteringRules); - Optional<Rule> maybeMatchingRule = ruleMatcher.findApplicableRule(mail); + Stream<Rule> matchingRules = ruleMatcher.findApplicableRules(mail); - maybeMatchingRule.ifPresent(rule -> actionApplierFactory.forMail(mail) - .forUser(user) - .apply(rule.getAction())); + actionApplierFactory.forMail(mail) + .forUser(user) + .apply(matchingRules.map(Rule::getAction)); } private Optional<User> retrieveUser(MailAddress recipient) { http://git-wip-us.apache.org/repos/asf/james-project/blob/8b1c70ab/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/RuleMatcher.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/RuleMatcher.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/RuleMatcher.java index abc1da1..07f0371 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/RuleMatcher.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/mailet/filter/RuleMatcher.java @@ -20,7 +20,7 @@ package org.apache.james.jmap.mailet.filter; import java.util.List; -import java.util.Optional; +import java.util.stream.Stream; import org.apache.james.jmap.api.filtering.Rule; import org.apache.mailet.Mail; @@ -36,9 +36,8 @@ class RuleMatcher { this.filteringRules = filteringRules; } - Optional<Rule> findApplicableRule(Mail mail) { + Stream<Rule> findApplicableRules(Mail mail) { return filteringRules.stream() - .filter(rule -> MailMatcher.from(rule).match(mail)) - .findFirst(); + .filter(rule -> MailMatcher.from(rule).match(mail)); } } http://git-wip-us.apache.org/repos/asf/james-project/blob/8b1c70ab/server/protocols/jmap/src/test/java/org/apache/james/jmap/mailet/filter/JMAPFilteringExtension.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/test/java/org/apache/james/jmap/mailet/filter/JMAPFilteringExtension.java b/server/protocols/jmap/src/test/java/org/apache/james/jmap/mailet/filter/JMAPFilteringExtension.java index 05dc317..526c94b 100644 --- a/server/protocols/jmap/src/test/java/org/apache/james/jmap/mailet/filter/JMAPFilteringExtension.java +++ b/server/protocols/jmap/src/test/java/org/apache/james/jmap/mailet/filter/JMAPFilteringExtension.java @@ -68,7 +68,7 @@ public class JMAPFilteringExtension implements BeforeEachCallback, ParameterReso this.filteringManagement = filteringManagement; this.mailboxManager = mailboxManager; try { - this.recipient1Mailbox = createMailbox(mailboxManager, RECIPIENT_1_USERNAME, RECIPIENT_1_MAILBOX_1); + this.recipient1Mailbox = createMailbox(RECIPIENT_1_USERNAME, RECIPIENT_1_MAILBOX_1); } catch (Exception e) { throw new RuntimeException(e); } @@ -90,7 +90,7 @@ public class JMAPFilteringExtension implements BeforeEachCallback, ParameterReso return recipient1Mailbox; } - public MailboxId createMailbox(InMemoryMailboxManager mailboxManager, String username, String mailboxName) throws Exception { + public MailboxId createMailbox(String username, String mailboxName) throws Exception { MailboxSession mailboxSession = mailboxManager.createSystemSession(username); return mailboxManager .createMailbox(MailboxPath.forUser(username, mailboxName), mailboxSession) http://git-wip-us.apache.org/repos/asf/james-project/blob/8b1c70ab/server/protocols/jmap/src/test/java/org/apache/james/jmap/mailet/filter/JMAPFilteringTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/test/java/org/apache/james/jmap/mailet/filter/JMAPFilteringTest.java b/server/protocols/jmap/src/test/java/org/apache/james/jmap/mailet/filter/JMAPFilteringTest.java index 15ba523..29d6bca 100644 --- a/server/protocols/jmap/src/test/java/org/apache/james/jmap/mailet/filter/JMAPFilteringTest.java +++ b/server/protocols/jmap/src/test/java/org/apache/james/jmap/mailet/filter/JMAPFilteringTest.java @@ -54,6 +54,7 @@ import static org.apache.james.jmap.mailet.filter.JMAPFilteringFixture.USER_3_FU import static org.apache.james.jmap.mailet.filter.JMAPFilteringFixture.USER_3_USERNAME; import static org.apache.james.jmap.mailet.filter.JMAPFilteringFixture.USER_4_FULL_ADDRESS; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; import java.util.Locale; import java.util.Optional; @@ -63,7 +64,6 @@ import org.apache.james.core.User; import org.apache.james.core.builder.MimeMessageBuilder; import org.apache.james.jmap.api.filtering.Rule; import org.apache.james.jmap.mailet.filter.JMAPFilteringExtension.JMAPFilteringTestSystem; -import org.apache.james.mailbox.inmemory.InMemoryMailboxManager; import org.apache.james.mailbox.model.MailboxId; import org.apache.james.util.StreamUtils; import org.apache.mailet.base.test.FakeMail; @@ -677,11 +677,10 @@ class JMAPFilteringTest { @Nested class MultiRuleBehaviourTest { @Test - void mailDirectiveShouldSetFirstMatchedRuleWhenMultipleRules(JMAPFilteringTestSystem testSystem) throws Exception { - InMemoryMailboxManager mailboxManager = testSystem.getMailboxManager(); - MailboxId mailbox1Id = testSystem.createMailbox(mailboxManager, RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_1"); - MailboxId mailbox2Id = testSystem.createMailbox(mailboxManager, RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_2"); - MailboxId mailbox3Id = testSystem.createMailbox(mailboxManager, RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_3"); + void mailDirectiveShouldSetLastMatchedRuleWhenMultipleRules(JMAPFilteringTestSystem testSystem) throws Exception { + MailboxId mailbox1Id = testSystem.createMailbox(RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_1"); + MailboxId mailbox2Id = testSystem.createMailbox(RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_2"); + MailboxId mailbox3Id = testSystem.createMailbox(RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_3"); testSystem.getFilteringManagement().defineRulesForUser(User.fromUsername(RECIPIENT_1_USERNAME), Rule.builder() @@ -711,15 +710,14 @@ class JMAPFilteringTest { testSystem.getJmapFiltering().service(mail); assertThat(mail.getAttribute(DELIVERY_PATH_PREFIX + RECIPIENT_1_USERNAME)) - .isEqualTo("RECIPIENT_1_MAILBOX_1"); + .isEqualTo("RECIPIENT_1_MAILBOX_3"); } @Test - void mailDirectiveShouldSetFirstMatchedMailboxWhenMultipleMailboxes(JMAPFilteringTestSystem testSystem) throws Exception { - InMemoryMailboxManager mailboxManager = testSystem.getMailboxManager(); - MailboxId mailbox1Id = testSystem.createMailbox(mailboxManager, RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_1"); - MailboxId mailbox2Id = testSystem.createMailbox(mailboxManager, RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_2"); - MailboxId mailbox3Id = testSystem.createMailbox(mailboxManager, RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_3"); + void mailDirectiveShouldSetLastMatchedMailboxWhenMultipleMailboxes(JMAPFilteringTestSystem testSystem) throws Exception { + MailboxId mailbox1Id = testSystem.createMailbox(RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_1"); + MailboxId mailbox2Id = testSystem.createMailbox(RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_2"); + MailboxId mailbox3Id = testSystem.createMailbox(RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_3"); testSystem.getFilteringManagement().defineRulesForUser(User.fromUsername(RECIPIENT_1_USERNAME), Rule.builder() @@ -738,7 +736,35 @@ class JMAPFilteringTest { testSystem.getJmapFiltering().service(mail); assertThat(mail.getAttribute(DELIVERY_PATH_PREFIX + RECIPIENT_1_USERNAME)) - .isEqualTo("RECIPIENT_1_MAILBOX_3"); + .isEqualTo("RECIPIENT_1_MAILBOX_1"); + } + + @Test + void rulesWithEmptyMailboxIdsShouldBeSkept(JMAPFilteringTestSystem testSystem) throws Exception { + MailboxId mailbox1Id = testSystem.createMailbox(RECIPIENT_1_USERNAME, "RECIPIENT_1_MAILBOX_1"); + + testSystem.getFilteringManagement().defineRulesForUser(User.fromUsername(RECIPIENT_1_USERNAME), + Rule.builder() + .id(Rule.Id.of("1")) + .name("rule 1") + .condition(Rule.Condition.of(SUBJECT, CONTAINS, UNSCRAMBLED_SUBJECT)) + .action(Rule.Action.of(Rule.Action.AppendInMailboxes.withMailboxIds(ImmutableList.of()))) + .build(), + Rule.builder() + .id(Rule.Id.of("2")) + .name("rule 2") + .condition(Rule.Condition.of(SUBJECT, CONTAINS, UNSCRAMBLED_SUBJECT)) + .action(Rule.Action.of(Rule.Action.AppendInMailboxes.withMailboxIds(ImmutableList.of( + mailbox1Id.serialize())))) + .build()); + + FakeMail mail = testSystem.asMail(mimeMessageBuilder() + .setSubject(UNSCRAMBLED_SUBJECT)); + + testSystem.getJmapFiltering().service(mail); + + assertThat(mail.getAttribute(DELIVERY_PATH_PREFIX + RECIPIENT_1_USERNAME)) + .isEqualTo("RECIPIENT_1_MAILBOX_1"); } @Test @@ -780,4 +806,95 @@ class JMAPFilteringTest { .isNull(); } } + + @Nested + class UnknownMailboxIds { + @Test + void serviceShouldNotThrowWhenUnknownMailboxId(JMAPFilteringTestSystem testSystem) throws Exception { + String unknownMailboxId = "4242"; + testSystem.getFilteringManagement().defineRulesForUser(User.fromUsername(RECIPIENT_1_USERNAME), + Rule.builder() + .id(Rule.Id.of("1")) + .name("rule 1") + .condition(Rule.Condition.of(FROM, CONTAINS, FRED_MARTIN_FULLNAME)) + .action(Rule.Action.of(Rule.Action.AppendInMailboxes.withMailboxIds(unknownMailboxId))) + .build()); + + FakeMail mail = testSystem.asMail(mimeMessageBuilder() + .addFrom(FRED_MARTIN_FULL_SCRAMBLED_ADDRESS)); + + assertThatCode(() -> testSystem.getJmapFiltering().service(mail)) + .doesNotThrowAnyException(); + } + + @Test + void mailDirectiveShouldNotBeSetWhenUnknownMailboxId(JMAPFilteringTestSystem testSystem) throws Exception { + String unknownMailboxId = "4242"; + testSystem.getFilteringManagement().defineRulesForUser(User.fromUsername(RECIPIENT_1_USERNAME), + Rule.builder() + .id(Rule.Id.of("1")) + .name("rule 1") + .condition(Rule.Condition.of(FROM, CONTAINS, FRED_MARTIN_FULLNAME)) + .action(Rule.Action.of(Rule.Action.AppendInMailboxes.withMailboxIds(unknownMailboxId))) + .build()); + + FakeMail mail = testSystem.asMail(mimeMessageBuilder() + .addFrom(FRED_MARTIN_FULL_SCRAMBLED_ADDRESS)); + + testSystem.getJmapFiltering().service(mail); + + assertThat(mail.getAttribute(DELIVERY_PATH_PREFIX + RECIPIENT_1_USERNAME)) + .isNull(); + } + + @Test + void rulesWithInvalidMailboxIdsShouldBeSkept(JMAPFilteringTestSystem testSystem) throws Exception { + String unknownMailboxId = "4242"; + testSystem.getFilteringManagement().defineRulesForUser(User.fromUsername(RECIPIENT_1_USERNAME), + Rule.builder() + .id(Rule.Id.of("1")) + .name("rule 1") + .condition(Rule.Condition.of(FROM, CONTAINS, FRED_MARTIN_FULLNAME)) + .action(Rule.Action.of(Rule.Action.AppendInMailboxes.withMailboxIds(unknownMailboxId))) + .build(), + Rule.builder() + .id(Rule.Id.of("2")) + .name("rule 2") + .condition(Rule.Condition.of(FROM, CONTAINS, FRED_MARTIN_FULLNAME)) + .action(Rule.Action.of(Rule.Action.AppendInMailboxes.withMailboxIds( + testSystem.getRecipient1MailboxId().serialize()))) + .build()); + + FakeMail mail = testSystem.asMail(mimeMessageBuilder() + .addFrom(FRED_MARTIN_FULL_SCRAMBLED_ADDRESS)); + + testSystem.getJmapFiltering().service(mail); + + assertThat(mail.getAttribute(DELIVERY_PATH_PREFIX + RECIPIENT_1_USERNAME)) + .isEqualTo(RECIPIENT_1_MAILBOX_1); + } + + @Test + void rulesWithMultipleMailboxIdsShouldFallbackWhenInvalidFirstMailboxId(JMAPFilteringTestSystem testSystem) throws Exception { + String unknownMailboxId = "4242"; + + testSystem.getFilteringManagement().defineRulesForUser(User.fromUsername(RECIPIENT_1_USERNAME), + Rule.builder() + .id(Rule.Id.of("1")) + .name("rule 1") + .condition(Rule.Condition.of(FROM, CONTAINS, FRED_MARTIN_FULLNAME)) + .action(Rule.Action.of(Rule.Action.AppendInMailboxes.withMailboxIds( + unknownMailboxId, + testSystem.getRecipient1MailboxId().serialize()))) + .build()); + + FakeMail mail = testSystem.asMail(mimeMessageBuilder() + .addFrom(FRED_MARTIN_FULL_SCRAMBLED_ADDRESS)); + + testSystem.getJmapFiltering().service(mail); + + assertThat(mail.getAttribute(DELIVERY_PATH_PREFIX + RECIPIENT_1_USERNAME)) + .isEqualTo(RECIPIENT_1_MAILBOX_1); + } + } } \ No newline at end of file --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
