This is an automated email from the ASF dual-hosted git repository. chibenwa pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 2e6ab37916a28116a1eeb88332246b528ac57c1a Author: Benoit TELLIER <[email protected]> AuthorDate: Tue Jun 16 22:03:41 2026 +0200 [FIX] User rename: handle submailbox edge cases when mailbox exist --- .../mailbox/MailboxUsernameChangeTaskStep.java | 21 ++- .../mailbox/MailboxUsernameChangeTaskStepTest.java | 177 +++++++++++++++++++++ 2 files changed, 192 insertions(+), 6 deletions(-) diff --git a/server/container/mailbox-adapter/src/main/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStep.java b/server/container/mailbox-adapter/src/main/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStep.java index b23334c786..016e662c93 100644 --- a/server/container/mailbox-adapter/src/main/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStep.java +++ b/server/container/mailbox-adapter/src/main/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStep.java @@ -19,6 +19,7 @@ package org.apache.james.adapter.mailbox; +import java.util.Comparator; import java.util.concurrent.ThreadLocalRandom; import jakarta.inject.Inject; @@ -77,14 +78,20 @@ public class MailboxUsernameChangeTaskStep implements UsernameChangeTaskStep { int renameOperationId = ThreadLocalRandom.current().nextInt(1000); return mailboxManager.search(queryUser, MailboxManager.MailboxSearchFetchType.Minimal, fromSession) - // Only keep top level, rename takes care of sub mailboxes - .filter(mailbox -> mailbox.getPath().getHierarchyLevels(fromSession.getPathDelimiter()).size() == 1) + // Process the deepest mailboxes first. As renameMailbox is recursive (it also renames + // sub mailboxes), handling a mailbox only once all of its descendants have already been + // migrated guarantees it is effectively a leaf at that point: the rename then carries no + // sub mailbox along, which lets us migrate every node individually - including when the + // destination mailbox already exists and a plain rename is therefore not possible. + .sort(Comparator.comparingInt( + (MailboxMetaData mailbox) -> mailbox.getPath().getHierarchyLevels(fromSession.getPathDelimiter()).size()) + .reversed()) .concatMap(mailbox -> migrateMailbox(fromSession, toSession, mailbox, renameOperationId)) .doFinally(any -> mailboxManager.endProcessingRequest(fromSession)) .doFinally(any -> mailboxManager.endProcessingRequest(toSession)); } - private Mono<Void> migrateMailbox(MailboxSession fromSession, MailboxSession toSession, org.apache.james.mailbox.model.MailboxMetaData mailbox, int renameOperationId) { + private Mono<Void> migrateMailbox(MailboxSession fromSession, MailboxSession toSession, MailboxMetaData mailbox, int renameOperationId) { MailboxPath renamedPath = mailbox.getPath().withUser(toSession.getUser()); return mailboxManager.mailboxExists(renamedPath, toSession) .flatMap(exist -> { @@ -104,9 +111,11 @@ public class MailboxUsernameChangeTaskStep implements UsernameChangeTaskStep { .then(); } - // The destination mailbox already exists: bring the source mailbox into the destination - // account under a temporary name, MOVE its messages into the destination, then drop the - // emptied temporary mailbox. + // The destination mailbox already exists: a plain rename is impossible, so we bring the source + // mailbox into the destination account under a temporary name, MOVE its messages into the + // destination, then drop the emptied temporary mailbox. Sub mailboxes are not handled here: as + // mailboxes are migrated deepest first, the source mailbox no longer has any descendant when we + // reach this point. private Mono<Void> moveWhenMailboxExist(MailboxSession fromSession, MailboxSession toSession, MailboxMetaData mailbox, MailboxPath renamedPath, int renameOperationId) { MailboxPath temporaryPath = new MailboxPath(renamedPath.getNamespace(), renamedPath.getUser(), renamedPath.getName() + "-tmp-" + renameOperationId); return mailboxManager.renameMailboxReactive(mailbox.getPath(), temporaryPath, diff --git a/server/container/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStepTest.java b/server/container/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStepTest.java index 18c5187116..439f77d02b 100644 --- a/server/container/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStepTest.java +++ b/server/container/mailbox-adapter/src/test/java/org/apache/james/adapter/mailbox/MailboxUsernameChangeTaskStepTest.java @@ -38,6 +38,7 @@ import org.apache.james.mailbox.model.search.MailboxQuery; import org.apache.james.mailbox.store.StoreSubscriptionManager; import org.apache.james.mime4j.dom.Message; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import reactor.core.publisher.Mono; @@ -198,4 +199,180 @@ class MailboxUsernameChangeTaskStepTest { MailboxPath.forUser(BOB, "test.child"), bobInbox); } + + private void appendMessage(MailboxPath path, MailboxSession session, String body) throws Exception { + mailboxManager.getMailbox(path, session) + .appendMessage(MessageManager.AppendCommand.from(Message.Builder.of() + .setSubject("subject") + .setBody(body, StandardCharsets.UTF_8) + .build()), session); + } + + private long messageCount(MailboxPath path, MailboxSession session) throws Exception { + return mailboxManager.getMailbox(path, session).getMessageCount(session); + } + + @Nested + class WhenDestinationParentMailboxAlreadyExists { + @Test + void shouldMigrateSubMailboxesWhenDestinationParentAlreadyExists() throws Exception { + MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE); + MailboxSession bobSession = mailboxManager.createSystemSession(BOB); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test.child"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(BOB, "test"), MailboxManager.CreateOption.NONE, bobSession); + + Mono.from(testee.changeUsername(ALICE, BOB)).block(); + + assertThat(mailboxManager.list(mailboxManager.createSystemSession(BOB))) + .containsOnly(MailboxPath.forUser(BOB, "test"), + MailboxPath.forUser(BOB, "test.child")); + } + + @Test + void shouldNotLeaveTemporaryMailboxesWhenDestinationParentAlreadyExists() throws Exception { + MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE); + MailboxSession bobSession = mailboxManager.createSystemSession(BOB); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test.child"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(BOB, "test"), MailboxManager.CreateOption.NONE, bobSession); + + Mono.from(testee.changeUsername(ALICE, BOB)).block(); + + assertThat(mailboxManager.list(mailboxManager.createSystemSession(BOB))) + .noneMatch(path -> path.getName().contains("-tmp-")); + } + + @Test + void shouldNotLooseMessagesOfSourceOnlyTopLevelMailboxWhenDestinationParentAlreadyExists() throws Exception { + MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE); + MailboxSession bobSession = mailboxManager.createSystemSession(BOB); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(BOB, "test"), MailboxManager.CreateOption.NONE, bobSession); + appendMessage(MailboxPath.forUser(ALICE, "test"), aliceSession, "alice"); + appendMessage(MailboxPath.forUser(BOB, "test"), bobSession, "bob"); + + Mono.from(testee.changeUsername(ALICE, BOB)).block(); + + assertThat(messageCount(MailboxPath.forUser(BOB, "test"), bobSession)).isEqualTo(2); + } + + @Test + void shouldMoveSubMailboxMessagesWhenDestinationParentAlreadyExists() throws Exception { + MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE); + MailboxSession bobSession = mailboxManager.createSystemSession(BOB); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test.child"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(BOB, "test"), MailboxManager.CreateOption.NONE, bobSession); + appendMessage(MailboxPath.forUser(ALICE, "test.child"), aliceSession, "alice-child"); + + Mono.from(testee.changeUsername(ALICE, BOB)).block(); + + assertThat(messageCount(MailboxPath.forUser(BOB, "test.child"), bobSession)).isEqualTo(1); + } + + @Test + void shouldMergeSubMailboxMessagesWhenBothUsersHaveTheSameSubMailbox() throws Exception { + MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE); + MailboxSession bobSession = mailboxManager.createSystemSession(BOB); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test.child"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(BOB, "test"), MailboxManager.CreateOption.NONE, bobSession); + mailboxManager.createMailbox(MailboxPath.forUser(BOB, "test.child"), MailboxManager.CreateOption.NONE, bobSession); + appendMessage(MailboxPath.forUser(ALICE, "test"), aliceSession, "alice-parent"); + appendMessage(MailboxPath.forUser(ALICE, "test.child"), aliceSession, "alice-child"); + appendMessage(MailboxPath.forUser(BOB, "test"), bobSession, "bob-parent"); + appendMessage(MailboxPath.forUser(BOB, "test.child"), bobSession, "bob-child"); + + Mono.from(testee.changeUsername(ALICE, BOB)).block(); + + assertThat(mailboxManager.list(mailboxManager.createSystemSession(BOB))) + .containsOnly(MailboxPath.forUser(BOB, "test"), + MailboxPath.forUser(BOB, "test.child")); + assertThat(messageCount(MailboxPath.forUser(BOB, "test"), bobSession)).isEqualTo(2); + assertThat(messageCount(MailboxPath.forUser(BOB, "test.child"), bobSession)).isEqualTo(2); + } + + @Test + void shouldMigrateDeepHierarchyWhenDestinationParentAlreadyExists() throws Exception { + MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE); + MailboxSession bobSession = mailboxManager.createSystemSession(BOB); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test.child"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test.child.grandchild"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(BOB, "test"), MailboxManager.CreateOption.NONE, bobSession); + + Mono.from(testee.changeUsername(ALICE, BOB)).block(); + + assertThat(mailboxManager.list(mailboxManager.createSystemSession(BOB))) + .containsOnly(MailboxPath.forUser(BOB, "test"), + MailboxPath.forUser(BOB, "test.child"), + MailboxPath.forUser(BOB, "test.child.grandchild")); + } + + @Test + void shouldMigrateSubMailboxesWhenOnlyDeepestChildConflicts() throws Exception { + MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE); + MailboxSession bobSession = mailboxManager.createSystemSession(BOB); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test.child"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test.other"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(BOB, "test"), MailboxManager.CreateOption.NONE, bobSession); + mailboxManager.createMailbox(MailboxPath.forUser(BOB, "test.child"), MailboxManager.CreateOption.NONE, bobSession); + + Mono.from(testee.changeUsername(ALICE, BOB)).block(); + + assertThat(mailboxManager.list(mailboxManager.createSystemSession(BOB))) + .containsOnly(MailboxPath.forUser(BOB, "test"), + MailboxPath.forUser(BOB, "test.child"), + MailboxPath.forUser(BOB, "test.other")); + } + + @Test + void shouldRemoveSourceMailboxesWhenDestinationParentAlreadyExists() throws Exception { + MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE); + MailboxSession bobSession = mailboxManager.createSystemSession(BOB); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test.child"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(BOB, "test"), MailboxManager.CreateOption.NONE, bobSession); + + Mono.from(testee.changeUsername(ALICE, BOB)).block(); + + assertThat(mailboxManager.list(mailboxManager.createSystemSession(BOB))) + .filteredOn(path -> ALICE.equals(path.getUser())) + .isEmpty(); + } + + @Test + void shouldTransferSubscriptionOfSubMailboxWhenDestinationParentAlreadyExists() throws Exception { + MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE); + MailboxSession bobSession = mailboxManager.createSystemSession(BOB); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test.child"), MailboxManager.CreateOption.CREATE_SUBSCRIPTION, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(BOB, "test"), MailboxManager.CreateOption.NONE, bobSession); + + Mono.from(testee.changeUsername(ALICE, BOB)).block(); + + assertThat(subscriptionManager.subscriptions(mailboxManager.createSystemSession(BOB))) + .containsOnly(MailboxPath.forUser(BOB, "test.child")); + } + + @Test + void shouldMigrateDelegationAclOfSubMailboxWhenDestinationParentAlreadyExists() throws Exception { + MailboxSession aliceSession = mailboxManager.createSystemSession(ALICE); + MailboxSession bobSession = mailboxManager.createSystemSession(BOB); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(ALICE, "test.child"), MailboxManager.CreateOption.NONE, aliceSession); + mailboxManager.createMailbox(MailboxPath.forUser(BOB, "test"), MailboxManager.CreateOption.NONE, bobSession); + mailboxManager.applyRightsCommand(MailboxPath.forUser(ALICE, "test.child"), + MailboxACL.command().forUser(CEDRIC).rights(MailboxACL.FULL_RIGHTS).asAddition(), + aliceSession); + + Mono.from(testee.changeUsername(ALICE, BOB)).block(); + + MailboxSession cedricSession = mailboxManager.createSystemSession(CEDRIC); + assertThatCode(() -> mailboxManager.getMailbox(MailboxPath.forUser(BOB, "test.child"), cedricSession)) + .doesNotThrowAnyException(); + } + } } \ No newline at end of file --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
