You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by bt...@apache.org on 2019/12/13 10:01:07 UTC
[james-project] 02/17: MAILBOX-393 MailboxManager::deleteMailbox by
mailboxId
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 <bt...@linagora.com>
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: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org