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 f4a708488c9b3c3a3672912ac2d5ce0c24a80a57 Author: Benoit Tellier <btell...@linagora.com> AuthorDate: Mon Dec 5 18:44:11 2022 +0700 JAMES-3754 LIST STATUS should not read mailboxes information twice --- .../org/apache/james/mailbox/MailboxManager.java | 2 ++ .../james/mailbox/model/MailboxMetaData.java | 12 +++++++--- .../james/mailbox/store/StoreMailboxManager.java | 27 ++++++++++++---------- .../apache/james/imap/processor/ListProcessor.java | 23 ++++++++++++++++-- .../james/imap/processor/StatusProcessor.java | 24 +++++++++++-------- .../james/jmap/draft/model/MailboxFactoryTest.java | 9 ++++---- .../webadmin/routes/UserMailboxesRoutesTest.java | 3 ++- 7 files changed, 68 insertions(+), 32 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 1f1552ca12..8fc1cd8241 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 @@ -148,6 +148,8 @@ public interface MailboxManager extends RequestAware, RightManager, MailboxAnnot Publisher<MessageManager> getMailboxReactive(MailboxPath mailboxPath, MailboxSession session); + MessageManager getMailbox(Mailbox mailbox, MailboxSession session) throws MailboxException; + /** * Creates a new mailbox. Any intermediary mailboxes missing from the * hierarchy should be created. diff --git a/mailbox/api/src/main/java/org/apache/james/mailbox/model/MailboxMetaData.java b/mailbox/api/src/main/java/org/apache/james/mailbox/model/MailboxMetaData.java index dd78368bbc..beb5bba8ba 100644 --- a/mailbox/api/src/main/java/org/apache/james/mailbox/model/MailboxMetaData.java +++ b/mailbox/api/src/main/java/org/apache/james/mailbox/model/MailboxMetaData.java @@ -64,6 +64,7 @@ public class MailboxMetaData implements Comparable<MailboxMetaData> { .<MailboxMetaData, Boolean>comparing(metadata -> metadata.getPath().isInbox()).reversed() .thenComparing(metadata -> metadata.getPath().getName()); + private final Mailbox mailbox; private final MailboxPath path; private final char delimiter; private final Children inferiors; @@ -72,9 +73,10 @@ public class MailboxMetaData implements Comparable<MailboxMetaData> { private final MailboxACL resolvedAcls; private final MailboxCounters counters; - public MailboxMetaData(MailboxPath path, MailboxId mailboxId, char delimiter, Children inferiors, Selectability selectability, MailboxACL resolvedAcls, MailboxCounters counters) { - this.path = path; - this.mailboxId = mailboxId; + public MailboxMetaData(Mailbox mailbox, char delimiter, Children inferiors, Selectability selectability, MailboxACL resolvedAcls, MailboxCounters counters) { + this.mailbox = mailbox; + this.mailboxId = mailbox.getMailboxId(); + this.path = mailbox.generateAssociatedPath(); this.delimiter = delimiter; this.inferiors = inferiors; this.selectability = selectability; @@ -127,6 +129,10 @@ public class MailboxMetaData implements Comparable<MailboxMetaData> { return mailboxId; } + public Mailbox getMailbox() { + return mailbox; + } + @Override public String toString() { return MoreObjects.toStringHelper(this) 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 01eec15d01..9577ac410a 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 @@ -284,22 +284,26 @@ public class StoreMailboxManager implements MailboxManager { MailboxMapper mapper = mailboxSessionMapperFactory.getMailboxMapper(session); return mapper.findMailboxByPath(mailboxPath) - .map(Throwing.<Mailbox, MessageManager>function(mailboxRow -> { - if (!assertUserHasAccessTo(mailboxRow, session)) { - LOGGER.info("Mailbox '{}' does not belong to user '{}' but to '{}'", mailboxPath, session.getUser(), mailboxRow.getUser()); - throw new MailboxNotFoundException(mailboxPath); - } - - LOGGER.debug("Loaded mailbox {}", mailboxPath); - - return createMessageManager(mailboxRow, session); - }).sneakyThrow()) + .map(Throwing.<Mailbox, MessageManager>function(mailboxRow -> getMailbox(mailboxRow, session)).sneakyThrow()) .switchIfEmpty(Mono.fromCallable(() -> { LOGGER.debug("Mailbox '{}' not found.", mailboxPath); throw new MailboxNotFoundException(mailboxPath); })); } + @Override + public MessageManager getMailbox(Mailbox mailboxRow, MailboxSession session) throws MailboxException { + MailboxPath mailboxPath = mailboxRow.generateAssociatedPath(); + if (!assertUserHasAccessTo(mailboxRow, session)) { + LOGGER.info("Mailbox '{}' does not belong to user '{}' but to '{}'", mailboxPath, session.getUser(), mailboxRow.getUser()); + throw new MailboxNotFoundException(mailboxPath); + } + + LOGGER.debug("Loaded mailbox {}", mailboxPath); + + return createMessageManager(mailboxRow, session); + } + @Override public MessageManager getMailbox(MailboxId mailboxId, MailboxSession session) throws MailboxException { return block(getMailboxReactive(mailboxId, session)); @@ -837,8 +841,7 @@ public class StoreMailboxManager implements MailboxManager { private MailboxMetaData toMailboxMetadata(MailboxSession session, Map<MailboxPath, Boolean> parentMap, Mailbox mailbox, MailboxCounters counters) throws UnsupportedRightException { return new MailboxMetaData( - mailbox.generateAssociatedPath(), - mailbox.getMailboxId(), + mailbox, getDelimiter(), computeChildren(parentMap, mailbox), Selectability.NONE, diff --git a/protocols/imap/src/main/java/org/apache/james/imap/processor/ListProcessor.java b/protocols/imap/src/main/java/org/apache/james/imap/processor/ListProcessor.java index 8791fbcab8..559529b9ba 100644 --- a/protocols/imap/src/main/java/org/apache/james/imap/processor/ListProcessor.java +++ b/protocols/imap/src/main/java/org/apache/james/imap/processor/ListProcessor.java @@ -45,9 +45,11 @@ import org.apache.james.imap.api.process.MailboxTyper; import org.apache.james.imap.main.PathConverter; import org.apache.james.imap.message.request.ListRequest; import org.apache.james.imap.message.response.ListResponse; +import org.apache.james.imap.message.response.MailboxStatusResponse; import org.apache.james.imap.message.response.MyRightsResponse; import org.apache.james.mailbox.MailboxManager; import org.apache.james.mailbox.MailboxSession; +import org.apache.james.mailbox.MessageManager; import org.apache.james.mailbox.SubscriptionManager; import org.apache.james.mailbox.exception.MailboxException; import org.apache.james.mailbox.model.MailboxACL; @@ -221,10 +223,18 @@ public class ListProcessor<T extends ListRequest> extends AbstractMailboxProcess } }) .doOnNext(metaData -> respondMyRights(request, responder, mailboxSession, metaData)) - .flatMap(metaData -> request.getStatusDataItems().map(statusDataItems -> statusProcessor.sendStatus(metaData.getPath(), statusDataItems, responder, session, mailboxSession)).orElse(Mono.empty())) + .flatMap(metaData -> request.getStatusDataItems().map(statusDataItems -> statusProcessor.sendStatus(retrieveMessageManager(metaData, mailboxSession), statusDataItems, responder, session, mailboxSession)).orElse(Mono.empty())) .then(); } + private MessageManager retrieveMessageManager(MailboxMetaData metaData, MailboxSession mailboxSession) { + try { + return getMailboxManager().getMailbox(metaData.getMailbox(), mailboxSession); + } catch (MailboxException e) { + throw new RuntimeException(e); + } + } + private Mono<Void> processWithSubscribed(ImapSession session, T request, Responder responder, MailboxSession mailboxSession, boolean isRelative, MailboxQuery mailboxQuery) { return Mono.zip(getMailboxManager().search(mailboxQuery, Minimal, mailboxSession).collectList() .map(searchedResultList -> searchedResultList.stream().collect(Collectors.toMap(MailboxMetaData::getPath, Function.identity()))), @@ -233,10 +243,19 @@ public class ListProcessor<T extends ListRequest> extends AbstractMailboxProcess .flatMapIterable(list -> list) .doOnNext(pathAndResponse -> responder.respond(pathAndResponse.getMiddle())) .doOnNext(pathAndResponse -> pathAndResponse.getRight().ifPresent(mailboxMetaData -> respondMyRights(request, responder, mailboxSession, mailboxMetaData))) - .flatMap(pathAndResponse -> request.getStatusDataItems().map(statusDataItems -> statusProcessor.sendStatus(pathAndResponse.getLeft(), statusDataItems, responder, session, mailboxSession)).orElse(Mono.empty())) + .flatMap(pathAndResponse -> sendStatusWhenSubscribed(session, request, responder, mailboxSession, pathAndResponse)) .then(); } + private Mono<MailboxStatusResponse> sendStatusWhenSubscribed(ImapSession session, T request, Responder responder, MailboxSession mailboxSession, + Triple<MailboxPath, ListResponse, Optional<MailboxMetaData>> pathAndResponse) { + return pathAndResponse.getRight() + .map(metaData -> retrieveMessageManager(metaData, mailboxSession)) + .flatMap(messageManager -> request.getStatusDataItems() + .map(statusDataItems -> statusProcessor.sendStatus(messageManager, statusDataItems, responder, session, mailboxSession))) + .orElse(Mono.empty()); + } + private List<Triple<MailboxPath, ListResponse, Optional<MailboxMetaData>>> getListResponseForSelectSubscribed(ImapSession session, Map<MailboxPath, MailboxMetaData> searchedResultMap, List<MailboxPath> allSubscribedSearch, ListRequest listRequest, MailboxSession mailboxSession, boolean relative, MailboxQuery mailboxQuery) { ImmutableList.Builder<Triple<MailboxPath, ListResponse, Optional<MailboxMetaData>>> responseBuilders = ImmutableList.builder(); diff --git a/protocols/imap/src/main/java/org/apache/james/imap/processor/StatusProcessor.java b/protocols/imap/src/main/java/org/apache/james/imap/processor/StatusProcessor.java index 541f84f421..1b513bd1a7 100644 --- a/protocols/imap/src/main/java/org/apache/james/imap/processor/StatusProcessor.java +++ b/protocols/imap/src/main/java/org/apache/james/imap/processor/StatusProcessor.java @@ -107,17 +107,21 @@ public class StatusProcessor extends AbstractMailboxProcessor<StatusRequest> imp .then(); } - Mono<MailboxStatusResponse> sendStatus(MailboxPath mailboxPath, StatusDataItems statusDataItems, Responder responder, ImapSession session, MailboxSession mailboxSession) { + private Mono<MailboxStatusResponse> sendStatus(MailboxPath mailboxPath, StatusDataItems statusDataItems, Responder responder, ImapSession session, MailboxSession mailboxSession) { return Mono.from(getMailboxManager().getMailboxReactive(mailboxPath, mailboxSession)) - .flatMap(mailbox -> retrieveMetadata(mailbox, statusDataItems, mailboxSession) - .flatMap(metaData -> computeStatusResponse(mailbox, statusDataItems, metaData, mailboxSession) - .doOnNext(response -> { - // Enable CONDSTORE as this is a CONDSTORE enabling command - if (response.getHighestModSeq() != null) { - condstoreEnablingCommand(session, responder, metaData, false); - } - responder.respond(response); - }))); + .flatMap(mailbox -> sendStatus(mailbox, statusDataItems, responder, session, mailboxSession)); + } + + Mono<MailboxStatusResponse> sendStatus(MessageManager mailbox, StatusDataItems statusDataItems, Responder responder, ImapSession session, MailboxSession mailboxSession) { + return retrieveMetadata(mailbox, statusDataItems, mailboxSession) + .flatMap(metaData -> computeStatusResponse(mailbox, statusDataItems, metaData, mailboxSession) + .doOnNext(response -> { + // Enable CONDSTORE as this is a CONDSTORE enabling command + if (response.getHighestModSeq() != null) { + condstoreEnablingCommand(session, responder, metaData, false); + } + responder.respond(response); + })); } private Mono<Void> logInitialRequest(MailboxPath mailboxPath) { diff --git a/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/model/MailboxFactoryTest.java b/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/model/MailboxFactoryTest.java index 41f25eeb47..2d7edee171 100644 --- a/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/model/MailboxFactoryTest.java +++ b/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/model/MailboxFactoryTest.java @@ -38,6 +38,7 @@ import org.apache.james.mailbox.model.MailboxCounters; import org.apache.james.mailbox.model.MailboxId; import org.apache.james.mailbox.model.MailboxMetaData; import org.apache.james.mailbox.model.MailboxPath; +import org.apache.james.mailbox.model.UidValidity; import org.apache.james.mailbox.model.search.MailboxQuery; import org.apache.james.mailbox.quota.QuotaManager; import org.apache.james.mailbox.quota.QuotaRootResolver; @@ -179,8 +180,9 @@ public class MailboxFactoryTest { mailboxManager.createMailbox(mailboxPath, mailboxSession); + org.apache.james.mailbox.model.Mailbox mailbox = new org.apache.james.mailbox.model.Mailbox(parentMailboxPath, UidValidity.of(34), parentId); Optional<MailboxId> id = sut.getParentIdFromMailboxPath(mailboxPath, - Optional.of(ImmutableMap.of(parentMailboxPath, new MailboxMetaData(parentMailboxPath, parentId, DELIMITER, + Optional.of(ImmutableMap.of(parentMailboxPath, new MailboxMetaData(mailbox, DELIMITER, MailboxMetaData.Children.CHILDREN_ALLOWED_BUT_UNKNOWN, MailboxMetaData.Selectability.NONE, new MailboxACL(), MailboxCounters.empty(parentId)))), mailboxSession).block(); @@ -205,16 +207,15 @@ public class MailboxFactoryTest { @Test public void buildShouldRelyOnPreloadedMailboxes() throws Exception { MailboxPath inbox = MailboxPath.inbox(user); - Optional<MailboxId> inboxId = mailboxManager.createMailbox(inbox, mailboxSession); Optional<MailboxId> otherId = mailboxManager.createMailbox(inbox.child("child", '.'), mailboxSession); InMemoryId preLoadedId = InMemoryId.of(45); + final org.apache.james.mailbox.model.Mailbox mailbox = new org.apache.james.mailbox.model.Mailbox(inbox, UidValidity.of(34), preLoadedId); Mailbox retrievedMailbox = sut.builder() .id(otherId.get()) .session(mailboxSession) .usingPreloadedMailboxesMetadata(Optional.of(ImmutableMap.of(inbox, new MailboxMetaData( - inbox, - preLoadedId, + mailbox, DELIMITER, MailboxMetaData.Children.NO_INFERIORS, MailboxMetaData.Selectability.NONE, 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 9b157ad7a8..9a818bc828 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 @@ -86,6 +86,7 @@ import org.apache.james.mailbox.model.MailboxMetaData; import org.apache.james.mailbox.model.MailboxPath; import org.apache.james.mailbox.model.MessageResult; import org.apache.james.mailbox.model.ThreadId; +import org.apache.james.mailbox.model.UidValidity; import org.apache.james.mailbox.model.UpdatedFlags; import org.apache.james.mailbox.model.search.MailboxQuery; import org.apache.james.mailbox.opensearch.IndexAttachments; @@ -140,7 +141,7 @@ class UserMailboxesRoutesTest { private static final Logger LOGGER = LoggerFactory.getLogger(UserMailboxesRoutesTest.class); public static MailboxMetaData testMetadata(MailboxPath path, MailboxId mailboxId, char delimiter) { - return new MailboxMetaData(path, mailboxId, delimiter, MailboxMetaData.Children.CHILDREN_ALLOWED_BUT_UNKNOWN, MailboxMetaData.Selectability.NONE, new MailboxACL(), + return new MailboxMetaData(new Mailbox(path, UidValidity.of(45), mailboxId), delimiter, MailboxMetaData.Children.CHILDREN_ALLOWED_BUT_UNKNOWN, MailboxMetaData.Selectability.NONE, new MailboxACL(), MailboxCounters.empty(mailboxId)); } --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org