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 feb5c5fd390fde56d0162f580b7c7f059cfd0335 Author: Benoit Tellier <[email protected]> AuthorDate: Fri Dec 6 11:37:13 2019 +0700 MAILBOX-393 MailboxManager::deleteMailbox by mailboxId Currently we can only delete a mailbox by it's mailboxPath. MailboxPath is mutable as a mailbox can be renamed. Mutable identifiers are bad, and thus race conditions can arise upon mailbox deletion {code:java} Given a mailboxId I read the corresponding mailboxPath and a concurrent session renames the mailbox (switch name) after my read When I delete the mailbox using the previously read path Then I bloew up the wrong mailbox (!!!) {code} --- .../org/apache/james/mailbox/MailboxManager.java | 8 +++ .../apache/james/mailbox/MailboxManagerTest.java | 72 ++++++++++++++++++++ .../james/mailbox/store/StoreMailboxManager.java | 78 +++++++++++++--------- .../webadmin/routes/UserMailboxesRoutesTest.java | 12 ++-- 4 files changed, 134 insertions(+), 36 deletions(-) diff --git a/mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java b/mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java index 8ab0941..1287dcd 100644 --- a/mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java +++ b/mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java @@ -26,6 +26,7 @@ import java.util.Optional; import org.apache.james.mailbox.exception.MailboxException; import org.apache.james.mailbox.exception.MailboxExistsException; import org.apache.james.mailbox.exception.MailboxNotFoundException; +import org.apache.james.mailbox.model.Mailbox; import org.apache.james.mailbox.model.MailboxId; import org.apache.james.mailbox.model.MailboxMetaData; import org.apache.james.mailbox.model.MailboxPath; @@ -153,6 +154,13 @@ public interface MailboxManager extends RequestAware, RightManager, MailboxAnnot void deleteMailbox(MailboxPath mailboxPath, MailboxSession session) throws MailboxException; /** + * Delete the mailbox with the given id + * + * @return the Mailbox when deleted + */ + Mailbox deleteMailbox(MailboxId mailboxId, MailboxSession session) throws MailboxException; + + /** * Renames a mailbox. * * @param from diff --git a/mailbox/api/src/test/java/org/apache/james/mailbox/MailboxManagerTest.java b/mailbox/api/src/test/java/org/apache/james/mailbox/MailboxManagerTest.java index 79a9fb0..201b648 100644 --- a/mailbox/api/src/test/java/org/apache/james/mailbox/MailboxManagerTest.java +++ b/mailbox/api/src/test/java/org/apache/james/mailbox/MailboxManagerTest.java @@ -643,6 +643,24 @@ public abstract class MailboxManagerTest<T extends MailboxManager> { } @Test + void deleteMailboxByIdShouldFireMailboxDeletionEvent() throws Exception { + assumeTrue(mailboxManager.hasCapability(MailboxCapabilities.Quota)); + retrieveEventBus(mailboxManager).register(listener, new MailboxIdRegistrationKey(inboxId)); + + mailboxManager.deleteMailbox(inboxId, session); + + assertThat(listener.getEvents()) + .filteredOn(event -> event instanceof MailboxListener.MailboxDeletion) + .hasSize(1) + .extracting(event -> (MailboxListener.MailboxDeletion) event) + .element(0) + .satisfies(event -> assertThat(event.getMailboxId()).isEqualTo(inboxId)) + .satisfies(event -> assertThat(event.getQuotaRoot()).isEqualTo(quotaRoot)) + .satisfies(event -> assertThat(event.getDeletedMessageCount()).isEqualTo(QuotaCountUsage.count(0))) + .satisfies(event -> assertThat(event.getTotalDeletedSize()).isEqualTo(QuotaSizeUsage.size(0))); + } + + @Test void createMailboxShouldFireMailboxAddedEvent() throws Exception { retrieveEventBus(mailboxManager).register(listener); @@ -1494,6 +1512,23 @@ public abstract class MailboxManagerTest<T extends MailboxManager> { assertThat(mailboxManager.mailboxExists(inboxSubMailbox, session)).isTrue(); } + + @Test + void user1ShouldBeAbleToDeleteInboxById() throws Exception { + session = mailboxManager.createSystemSession(USER_1); + mailboxManager.startProcessingRequest(session); + + MailboxPath inbox = MailboxPath.inbox(session); + MailboxId inboxId = mailboxManager.createMailbox(inbox, session).get(); + MailboxPath inboxSubMailbox = new MailboxPath(inbox, "INBOX.Test"); + mailboxManager.createMailbox(inboxSubMailbox, session); + + mailboxManager.deleteMailbox(inboxId, session); + + assertThat(mailboxManager.mailboxExists(inbox, session)).isFalse(); + assertThat(mailboxManager.mailboxExists(inboxSubMailbox, session)).isTrue(); + } + @Test void user1ShouldBeAbleToDeleteSubmailbox() throws Exception { session = mailboxManager.createSystemSession(USER_1); @@ -1511,6 +1546,22 @@ public abstract class MailboxManagerTest<T extends MailboxManager> { } @Test + void user1ShouldBeAbleToDeleteSubmailboxByid() throws Exception { + session = mailboxManager.createSystemSession(USER_1); + mailboxManager.startProcessingRequest(session); + + MailboxPath inbox = MailboxPath.inbox(session); + mailboxManager.createMailbox(inbox, session); + MailboxPath inboxSubMailbox = new MailboxPath(inbox, "INBOX.Test"); + MailboxId inboxSubMailboxId = mailboxManager.createMailbox(inboxSubMailbox, session).get(); + + mailboxManager.deleteMailbox(inboxSubMailboxId, session); + + assertThat(mailboxManager.mailboxExists(inbox, session)).isTrue(); + assertThat(mailboxManager.mailboxExists(inboxSubMailbox, session)).isFalse(); + } + + @Test void listShouldReturnMailboxes() throws Exception { session = mailboxManager.createSystemSession(Username.of("manager")); mailboxManager.startProcessingRequest(session); @@ -1670,6 +1721,27 @@ public abstract class MailboxManagerTest<T extends MailboxManager> { } @Test + void deleteMailboxByIdShouldCallAllPreDeletionHooks() throws Exception { + ComposedMessageId composeId = inboxManager.appendMessage(AppendCommand.builder() + .withFlags(new Flags(Flags.Flag.DELETED)) + .build(message), session); + mailboxManager.deleteMailbox(inboxId, session); + + ArgumentCaptor<PreDeletionHook.DeleteOperation> preDeleteCaptor1 = ArgumentCaptor.forClass(PreDeletionHook.DeleteOperation.class); + ArgumentCaptor<PreDeletionHook.DeleteOperation> preDeleteCaptor2 = ArgumentCaptor.forClass(PreDeletionHook.DeleteOperation.class); + verify(preDeletionHook1, times(1)).notifyDelete(preDeleteCaptor1.capture()); + verify(preDeletionHook2, times(1)).notifyDelete(preDeleteCaptor2.capture()); + + assertThat(preDeleteCaptor1.getValue().getDeletionMetadataList()) + .hasSize(1) + .hasSameElementsAs(preDeleteCaptor2.getValue().getDeletionMetadataList()) + .allSatisfy(deleteMetadata -> SoftAssertions.assertSoftly(softy -> { + softy.assertThat(deleteMetadata.getMailboxId()).isEqualTo(inboxId); + softy.assertThat(deleteMetadata.getMessageMetaData().getMessageId()).isEqualTo(composeId.getMessageId()); + })); + } + + @Test void expungeShouldCallAllPreDeletionHooksOnEachMessageDeletionCall() throws Exception { ComposedMessageId composeId1 = inboxManager.appendMessage(AppendCommand.builder() .withFlags(new Flags(Flags.Flag.DELETED)) diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java index cb473c3..a355be1 100644 --- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java +++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java @@ -412,45 +412,63 @@ public class StoreMailboxManager implements MailboxManager { LOGGER.info("deleteMailbox {}", mailboxPath); assertIsOwner(session, mailboxPath); MailboxMapper mailboxMapper = mailboxSessionMapperFactory.getMailboxMapper(session); - MessageMapper messageMapper = mailboxSessionMapperFactory.getMessageMapper(session); mailboxMapper.execute(() -> { Mailbox mailbox = mailboxMapper.findMailboxByPath(mailboxPath); if (mailbox == null) { throw new MailboxNotFoundException(mailboxPath); } + return doDeleteMailbox(mailboxMapper, mailbox, session); + }); + } + + @Override + public Mailbox deleteMailbox(MailboxId mailboxId, MailboxSession session) throws MailboxException { + LOGGER.info("deleteMailbox {}", mailboxId); + MailboxMapper mailboxMapper = mailboxSessionMapperFactory.getMailboxMapper(session); - QuotaRoot quotaRoot = quotaRootResolver.getQuotaRoot(mailboxPath); - long messageCount = messageMapper.countMessagesInMailbox(mailbox); - - List<MetadataWithMailboxId> metadata = Iterators.toStream(messageMapper.findInMailbox(mailbox, MessageRange.all(), MessageMapper.FetchType.Metadata, UNLIMITED)) - .map(message -> MetadataWithMailboxId.from(message.metaData(), message.getMailboxId())) - .collect(Guavate.toImmutableList()); - - long totalSize = metadata.stream() - .map(MetadataWithMailboxId::getMessageMetaData) - .mapToLong(MessageMetaData::getSize) - .sum(); - - preDeletionHooks.runHooks(PreDeletionHook.DeleteOperation.from(metadata)).block(); - - // We need to create a copy of the mailbox as maybe we can not refer to the real - // mailbox once we remove it - Mailbox m = new Mailbox(mailbox); - mailboxMapper.delete(mailbox); - eventBus.dispatch(EventFactory.mailboxDeleted() - .randomEventId() - .mailboxSession(session) - .mailbox(mailbox) - .quotaRoot(quotaRoot) - .quotaCount(QuotaCountUsage.count(messageCount)) - .quotaSize(QuotaSizeUsage.size(totalSize)) - .build(), - new MailboxIdRegistrationKey(mailbox.getMailboxId())) - .block(); - return m; + return mailboxMapper.execute(() -> { + Mailbox mailbox = mailboxMapper.findMailboxById(mailboxId); + if (mailbox == null) { + throw new MailboxNotFoundException(mailboxId); + } + assertIsOwner(session, mailbox.generateAssociatedPath()); + return doDeleteMailbox(mailboxMapper, mailbox, session); }); + } + + private Mailbox doDeleteMailbox(MailboxMapper mailboxMapper, Mailbox mailbox, MailboxSession session) throws MailboxException { + MessageMapper messageMapper = mailboxSessionMapperFactory.getMessageMapper(session); + QuotaRoot quotaRoot = quotaRootResolver.getQuotaRoot(mailbox.generateAssociatedPath()); + long messageCount = messageMapper.countMessagesInMailbox(mailbox); + + List<MetadataWithMailboxId> metadata = Iterators.toStream(messageMapper.findInMailbox(mailbox, MessageRange.all(), MessageMapper.FetchType.Metadata, UNLIMITED)) + .map(message -> MetadataWithMailboxId.from(message.metaData(), message.getMailboxId())) + .collect(Guavate.toImmutableList()); + + long totalSize = metadata.stream() + .map(MetadataWithMailboxId::getMessageMetaData) + .mapToLong(MessageMetaData::getSize) + .sum(); + + preDeletionHooks.runHooks(PreDeletionHook.DeleteOperation.from(metadata)).block(); + + // We need to create a copy of the mailbox as maybe we can not refer to the real + // mailbox once we remove it + Mailbox m = new Mailbox(mailbox); + mailboxMapper.delete(mailbox); + eventBus.dispatch(EventFactory.mailboxDeleted() + .randomEventId() + .mailboxSession(session) + .mailbox(mailbox) + .quotaRoot(quotaRoot) + .quotaCount(QuotaCountUsage.count(messageCount)) + .quotaSize(QuotaSizeUsage.size(totalSize)) + .build(), + new MailboxIdRegistrationKey(mailbox.getMailboxId())) + .block(); + return m; } @Override diff --git a/server/protocols/webadmin/webadmin-mailbox/src/test/java/org/apache/james/webadmin/routes/UserMailboxesRoutesTest.java b/server/protocols/webadmin/webadmin-mailbox/src/test/java/org/apache/james/webadmin/routes/UserMailboxesRoutesTest.java index 4e636ca..dd0d7bf 100644 --- a/server/protocols/webadmin/webadmin-mailbox/src/test/java/org/apache/james/webadmin/routes/UserMailboxesRoutesTest.java +++ b/server/protocols/webadmin/webadmin-mailbox/src/test/java/org/apache/james/webadmin/routes/UserMailboxesRoutesTest.java @@ -783,7 +783,7 @@ class UserMailboxesRoutesTest { ImmutableList.of( MailboxMetaData.unselectableMailbox( MailboxPath.forUser(USERNAME, MAILBOX_NAME), mailboxId, '.'))); - doThrow(new RuntimeException()).when(mailboxManager).deleteMailbox(any(), any()); + doThrow(new RuntimeException()).when(mailboxManager).deleteMailbox(any(MailboxPath.class), any()); when() .delete(MAILBOX_NAME) @@ -808,7 +808,7 @@ class UserMailboxesRoutesTest { .thenReturn( ImmutableList.of( MailboxMetaData.unselectableMailbox(MailboxPath.forUser(USERNAME, MAILBOX_NAME), mailboxId, '.'))); - doThrow(new MailboxException()).when(mailboxManager).deleteMailbox(any(), any()); + doThrow(new MailboxException()).when(mailboxManager).deleteMailbox(any(MailboxPath.class), any()); when() .delete(MAILBOX_NAME) @@ -828,7 +828,7 @@ class UserMailboxesRoutesTest { @Test void deleteShouldReturnOkOnMailboxDoesNotExists() throws Exception { - doThrow(new MailboxNotFoundException(MAILBOX_NAME)).when(mailboxManager).deleteMailbox(any(), any()); + doThrow(new MailboxNotFoundException(MAILBOX_NAME)).when(mailboxManager).deleteMailbox(any(MailboxPath.class), any()); when() .delete(MAILBOX_NAME) @@ -864,7 +864,7 @@ class UserMailboxesRoutesTest { .thenReturn( ImmutableList.of( MailboxMetaData.unselectableMailbox(MailboxPath.forUser(USERNAME, "any"), mailboxId, '.'))); - doThrow(new RuntimeException()).when(mailboxManager).deleteMailbox(any(), any()); + doThrow(new RuntimeException()).when(mailboxManager).deleteMailbox(any(MailboxPath.class), any()); when() .delete() @@ -878,7 +878,7 @@ class UserMailboxesRoutesTest { when(mailboxManager.search(any(MailboxQuery.class), any())) .thenReturn( ImmutableList.of(MailboxMetaData.unselectableMailbox(MailboxPath.forUser(USERNAME, "any"), mailboxId, '.'))); - doThrow(new MailboxNotFoundException("any")).when(mailboxManager).deleteMailbox(any(), any()); + doThrow(new MailboxNotFoundException("any")).when(mailboxManager).deleteMailbox(any(MailboxPath.class), any()); when() .delete() @@ -892,7 +892,7 @@ class UserMailboxesRoutesTest { when(mailboxManager.search(any(MailboxQuery.class), any())) .thenReturn( ImmutableList.of(MailboxMetaData.unselectableMailbox(MailboxPath.forUser(USERNAME, "any"), mailboxId, '.'))); - doThrow(new MailboxException()).when(mailboxManager).deleteMailbox(any(), any()); + doThrow(new MailboxException()).when(mailboxManager).deleteMailbox(any(MailboxPath.class), any()); when() .delete() --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
