JAMES-2182 StoreMailboxManager should avoid throwing exception in `.filterWhen

We observed a few `MailboxExistsException` error logs since the change 91f50b0#diff-51e67dcd44af864286be49fbe7d5474bff07b8fe6cd0536121b4f2898dbbda9e.

Actually, the errors are still caught by the main reactor stream therefore no impact to users. However, it seems errors emitted by asynchronous operators like `.filterWhen` can be propagated through their own internal asynchronous paths and result in unhandled exceptions being logged (e.g., "onErrorDropped").

Author: quantranhong1999

mailbox/api/src/test/java/org/apache/james/mailbox/MailboxManagerTest.java

@@ -1873,6 +1873,23 @@ void renameMailboxShouldRenamedChildMailboxesWithRenameOption() throws MailboxEx
                 MailboxPath.forUser(USER_1, "mbx9.mbx2.mbx3"));
         }
 
+        @Test
+        void renameMailboxToAnExistingMailboxShouldThrowMailboxExistsException() throws MailboxException {
+            MailboxSession session = mailboxManager.createSystemSession(USER_1);
+
+            MailboxPath existingPath = MailboxPath.forUser(USER_1, "mbx1");
+            MailboxPath toBeRenamed = MailboxPath.forUser(USER_1, "mbx2");
+
+            mailboxManager.createMailbox(existingPath, session);
+            subscriptionManager.subscribe(session, existingPath);
+
+            mailboxManager.createMailbox(toBeRenamed, session);
+            subscriptionManager.subscribe(session, toBeRenamed);
+
+            assertThatThrownBy(() -> mailboxManager.renameMailbox(toBeRenamed, existingPath, RENAME_SUBSCRIPTIONS, session))
+                .isInstanceOf(MailboxExistsException.class);
+        }
+
         @Test
         void renameMailboxShouldRenameSubscriptionWhenCalledWithRenameSubscriptionOption() throws MailboxException {
             MailboxSession session = mailboxManager.createSystemSession(USER_1);

mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java

@@ -434,15 +434,20 @@ private Mono<MailboxId> performConcurrentMailboxCreation(MailboxSession mailboxS
             .flatMap(any -> inheritRightsReactive(mailboxSession, mailboxPath).thenReturn(any));
     }
 
-
-    private Mono<Void> assertCanCreateReactive(MailboxSession session, MailboxPath path) {
+    private Mono<Boolean> canCreateReactive(MailboxSession session, MailboxPath path) {
         if (path.belongsTo(session)) {
-            return Mono.empty();
+            return Mono.just(true);
         }
 
         return nearestExistingParent(session, path)
             .filterWhen(parent -> hasRightReactive(parent, Right.CreateMailbox, session))
-            .switchIfEmpty(Mono.error(new InsufficientRightsException("user '" + session.getUser().asString() + "' is not allowed to create the mailbox '" + path.asString() + "'")))
+            .hasElement();
+    }
+
+    private Mono<Void> assertCanCreateReactive(MailboxSession session, MailboxPath path) {
+        return canCreateReactive(session, path)
+            .filter(canCreate -> canCreate)
+            .switchIfEmpty(Mono.error(() -> new InsufficientRightsException("user '" + session.getUser().asString() + "' is not allowed to create the mailbox '" + path.asString() + "'")))
             .then();
     }
 
@@ -469,7 +474,7 @@ public void deleteMailbox(final MailboxPath mailboxPath, final MailboxSession se
         MailboxMapper mailboxMapper = mailboxSessionMapperFactory.getMailboxMapper(session);
 
         mailboxMapper.execute(() -> block(mailboxMapper.findMailboxByPath(mailboxPath)
-            .filterWhen(mailbox -> assertCanDeleteReactive(session, mailbox.generateAssociatedPath()).thenReturn(true))
+            .flatMap(mailbox -> assertCanDeleteReactive(session, mailbox))
             .flatMap(mailbox -> doDeleteMailbox(mailboxMapper, mailbox, session))
             .switchIfEmpty(Mono.error(() -> new MailboxNotFoundException(mailboxPath)))));
     }
@@ -480,7 +485,7 @@ public Mailbox deleteMailbox(MailboxId mailboxId, MailboxSession session) throws
         MailboxMapper mailboxMapper = mailboxSessionMapperFactory.getMailboxMapper(session);
 
         return mailboxMapper.execute(() -> block(mailboxMapper.findMailboxById(mailboxId)
-            .filterWhen(mailbox -> assertCanDeleteReactive(session, mailbox.generateAssociatedPath()).thenReturn(true))
+            .flatMap(mailbox -> assertCanDeleteReactive(session, mailbox))
             .flatMap(mailbox -> doDeleteMailbox(mailboxMapper, mailbox, session))));
     }
 
@@ -490,7 +495,7 @@ public Mono<Mailbox> deleteMailboxReactive(MailboxId mailboxId, MailboxSession s
         MailboxMapper mailboxMapper = mailboxSessionMapperFactory.getMailboxMapper(session);
 
         return mailboxMapper.executeReactive(mailboxMapper.findMailboxById(mailboxId)
-            .filterWhen(mailbox -> assertCanDeleteReactive(session, mailbox.generateAssociatedPath()).thenReturn(true))
+            .flatMap(mailbox -> assertCanDeleteReactive(session, mailbox))
             .flatMap(mailbox -> doDeleteMailbox(mailboxMapper, mailbox, session)));
     }
 
@@ -500,33 +505,34 @@ public Mono<Void> deleteMailboxReactive(MailboxPath mailboxPath, MailboxSession
         MailboxMapper mailboxMapper = mailboxSessionMapperFactory.getMailboxMapper(session);
 
         return mailboxMapper.executeReactive(mailboxMapper.findMailboxByPath(mailboxPath)
-            .filterWhen(mailbox -> assertCanDeleteReactive(session, mailbox.generateAssociatedPath()).thenReturn(true))
+            .flatMap(mailbox -> assertCanDeleteReactive(session, mailbox))
             .flatMap(mailbox -> doDeleteMailbox(mailboxMapper, mailbox, session))
             .switchIfEmpty(Mono.error(() -> new MailboxNotFoundException(mailboxPath))))
             .then();
     }
 
-    private Mono<Void> assertCanDeleteReactive(MailboxSession session, MailboxPath path) {
+    private Mono<Mailbox> assertCanDeleteReactive(MailboxSession session, Mailbox mailbox) {
+        MailboxPath path = mailbox.generateAssociatedPath();
         if (path.belongsTo(session)) {
-            return Mono.empty();
+            return Mono.just(mailbox);
         }
         return Mono.from(hasRightReactive(path, Right.DeleteMailbox, session))
             .flatMap(hasRight -> {
                 if (hasRight) {
-                    return Mono.empty();
+                    return Mono.just(mailbox);
                 }
                 return Mono.error(new InsufficientRightsException("user '" + session.getUser().asString() + "' is not allowed to delete the mailbox '" + path.asString() + "'"));
             });
     }
 
-    private Mono<Void> assertCanDeleteWhenRename(MailboxSession session, MailboxPath path) {
+    private Mono<MailboxPath> assertCanDeleteWhenRename(MailboxSession session, MailboxPath path) {
         if (path.belongsTo(session)) {
-            return Mono.empty();
+            return Mono.just(path);
         }
         return Mono.from(myRightsReactive(path, session))
             .flatMap(rights -> {
                 if (rights.contains(Right.DeleteMailbox)) {
-                    return Mono.empty();
+                    return Mono.just(path);
                 } else if (!rights.contains(Right.Lookup)) {
                     return Mono.error(new MailboxNotFoundException(path));
                 } else {
@@ -633,7 +639,8 @@ public Mono<List<MailboxRenamedResult>> renameMailboxReactive(MailboxId mailboxI
         MailboxMapper mapper = mailboxSessionMapperFactory.getMailboxMapper(session);
 
         Mono<Mailbox> fromMailboxPublisher = mapper.findMailboxById(mailboxId)
-            .filterWhen(mailbox -> assertCanDeleteWhenRename(session, mailbox.generateAssociatedPath()).thenReturn(true))
+            .flatMap(mailbox -> assertCanDeleteWhenRename(session, mailbox.generateAssociatedPath())
+                .thenReturn(mailbox))
             .switchIfEmpty(Mono.error(() -> new MailboxNotFoundException(mailboxId)));
 
         return sanitizedMailboxPath(newMailboxPath, session)
@@ -642,16 +649,15 @@ public Mono<List<MailboxRenamedResult>> renameMailboxReactive(MailboxId mailboxI
 
     private Mono<MailboxPath> sanitizedMailboxPath(MailboxPath mailboxPath, MailboxSession session) {
         Function<MailboxPath, Mono<Boolean>> assertNewMailboxPathDoesNotExist = newMailboxPath -> mailboxExists(mailboxPath, session)
-            .filter(FunctionalUtils.identityPredicate().negate())
-            .switchIfEmpty(Mono.error(new MailboxExistsException(mailboxPath.toString())))
-            .thenReturn(true);
+            .map(FunctionalUtils.negate());
 
-        Function<MailboxPath, Mono<Boolean>> assertRightToCreate = newMailboxPath -> assertCanCreateReactive(session, mailboxPath)
-            .thenReturn(true);
+        Function<MailboxPath, Mono<Boolean>> assertRightToCreate = newMailboxPath -> canCreateReactive(session, mailboxPath);
 
         return Mono.fromCallable(() -> mailboxPath.sanitize(session.getPathDelimiter()))
             .filterWhen(assertNewMailboxPathDoesNotExist)
+            .switchIfEmpty(Mono.error(() -> new MailboxExistsException(mailboxPath.toString())))
             .filterWhen(assertRightToCreate)
+            .switchIfEmpty(Mono.error(() -> new InsufficientRightsException("user '" + session.getUser().asString() + "' is not allowed to create the mailbox '" + mailboxPath.asString() + "'")))
             .flatMap(newMailboxPath -> Mono.fromCallable(() -> newMailboxPath.assertAcceptable(session.getPathDelimiter())));
     }
 

server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerTest.java

@@ -89,6 +89,7 @@
 import org.apache.james.mailbox.ModSeq;
 import org.apache.james.mailbox.inmemory.InMemoryMailboxManager;
 import org.apache.james.mailbox.inmemory.manager.InMemoryIntegrationResources;
+import org.apache.james.mailbox.model.MailboxACL;
 import org.apache.james.mailbox.model.MailboxPath;
 import org.apache.james.mailbox.model.MessageRange;
 import org.apache.james.mailbox.model.UidValidity;
@@ -3498,4 +3499,82 @@ void concurrentIdCommandsInTheSameSessionShouldSucceed() throws Exception {
         }
     }
 
+    @Nested
+    class RenameMailboxTest {
+        IMAPServer imapServer;
+        private int port;
+
+        @BeforeEach
+        void beforeEach() throws Exception {
+            imapServer = createImapServer("imapServer.xml");
+            port = imapServer.getListenAddresses().get(0).getPort();
+            MailboxSession mailboxSession = memoryIntegrationResources.getMailboxManager().createSystemSession(USER);
+            memoryIntegrationResources.getMailboxManager()
+                .createMailbox(MailboxPath.forUser(USER, "mailbox1"), mailboxSession);
+            memoryIntegrationResources.getMailboxManager()
+                .createMailbox(MailboxPath.forUser(USER, "mailbox2"), mailboxSession);
+        }
+
+        @AfterEach
+        void tearDown() {
+            imapServer.destroy();
+        }
+
+        @Test
+        void renameShouldFailWhenTargetMailboxAlreadyExists() throws Exception {
+            testIMAPClient.connect("127.0.0.1", port)
+                .login(USER.asString(), USER_PASS);
+
+            String response = testIMAPClient.sendCommand("RENAME mailbox1 mailbox2");
+
+            assertThat(response).contains("NO RENAME failed. Mailbox already exists.");
+        }
+
+        @Test
+        void renameShouldFailWhenRequestedMailboxDoesNotExist() throws Exception {
+            testIMAPClient.connect("127.0.0.1", port)
+                .login(USER.asString(), USER_PASS);
+
+            String response = testIMAPClient.sendCommand("RENAME nonExistingMailbox newMailboxName");
+
+            assertThat(response).contains("NO RENAME failed. Mailbox not found.");
+        }
+
+        @Test
+        void renameShouldFailWhenInsufficientRightsOnSharedMailbox() throws Exception {
+            // Create a mailbox for another user
+            memoryIntegrationResources.getMailboxManager()
+                .createMailbox(MailboxPath.forUser(USER2, "sharedMailbox.child1"),
+                    memoryIntegrationResources.getMailboxManager().createSystemSession(USER2));
+
+            // Ensure the current user does not have the "delete mailbox" right on the shared mailbox
+            memoryIntegrationResources.getMailboxManager()
+                .applyRightsCommand(MailboxPath.forUser(USER2, "sharedMailbox"),
+                    MailboxACL.command()
+                        .forUser(USER)
+                        .rights(MailboxACL.Right.Lookup, MailboxACL.Right.Read, MailboxACL.Right.Insert,
+                            MailboxACL.Right.CreateMailbox,
+                            MailboxACL.Right.Administer, MailboxACL.Right.Write)
+                        .asAddition(),
+                    memoryIntegrationResources.getMailboxManager().createSystemSession(USER2));
+            memoryIntegrationResources.getMailboxManager()
+                .applyRightsCommand(MailboxPath.forUser(USER2, "sharedMailbox.child1"),
+                    MailboxACL.command()
+                        .forUser(USER)
+                        .rights(MailboxACL.Right.Lookup, MailboxACL.Right.Read, MailboxACL.Right.Insert,
+                            MailboxACL.Right.CreateMailbox,
+                            MailboxACL.Right.Administer, MailboxACL.Right.Write)
+                        .asAddition(),
+                    memoryIntegrationResources.getMailboxManager().createSystemSession(USER2));
+
+            // Connect and attempt to rename the shared mailbox
+            testIMAPClient.connect("127.0.0.1", port)
+                .login(USER.asString(), USER_PASS);
+            String response = testIMAPClient.sendCommand("RENAME #user.bobo.sharedMailbox.child1 #user.bobo.sharedMailbox.newChild");
+
+            // Assert that the operation fails due to insufficient rights
+            assertThat(response).contains("NO RENAME processing failed.");
+        }
+    }
+
 }