JAMES-2257 Centralize OldKeyword knowledge We just need to explicitly handle OldKeywords in two points: during updates and during message creations. We should ensure that a single class in each process encapsulate this knowledge and don't leak it.
Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/71a6b1b1 Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/71a6b1b1 Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/71a6b1b1 Branch: refs/heads/master Commit: 71a6b1b1ab70c71773cfe9df65402d832961460e Parents: 4357889 Author: benwa <btell...@linagora.com> Authored: Tue Dec 12 09:07:10 2017 +0700 Committer: benwa <btell...@linagora.com> Committed: Fri Dec 15 14:03:32 2017 +0700 ---------------------------------------------------------------------- .../james/jmap/methods/MessageAppender.java | 16 ++------- .../methods/SetMessagesUpdateProcessor.java | 24 ++------------ .../james/jmap/model/CreationMessage.java | 35 ++++++++++---------- .../james/jmap/model/UpdateMessagePatch.java | 16 +++------ .../apache/james/jmap/model/OldKeywordTest.java | 13 +++++++- .../jmap/model/UpdateMessagePatchTest.java | 33 ++++++++++++++++++ 6 files changed, 72 insertions(+), 65 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/71a6b1b1/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageAppender.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageAppender.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageAppender.java index dd3e7e8..c74f59d 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageAppender.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageAppender.java @@ -30,9 +30,7 @@ import javax.mail.util.SharedByteArrayInputStream; import org.apache.james.jmap.methods.ValueWithId.CreationMessageEntry; import org.apache.james.jmap.model.Attachment; import org.apache.james.jmap.model.CreationMessage; -import org.apache.james.jmap.model.Keywords; import org.apache.james.jmap.model.MessageFactory; -import org.apache.james.jmap.model.OldKeyword; import org.apache.james.mailbox.AttachmentManager; import org.apache.james.mailbox.MailboxManager; import org.apache.james.mailbox.MailboxSession; @@ -89,7 +87,7 @@ public class MessageAppender { return MessageFactory.MetaDataWithContent.builder() .uid(message.getUid()) - .keywords(keywordsOrDefault(createdEntry.getValue())) + .keywords(createdEntry.getValue().getKeywords()) .internalDate(internalDate.toInstant()) .sharedContent(content) .size(messageContent.length) @@ -106,17 +104,7 @@ public class MessageAppender { } private Flags getFlags(CreationMessage message) { - return message.getOldKeyword() - .map(OldKeyword::asKeywords) - .map(Optional::of) - .orElse(message.getKeywords()) - .orElse(Keywords.DEFAULT_VALUE) - .asFlags(); - } - - private Keywords keywordsOrDefault(CreationMessage message) { - return message.getKeywords() - .orElse(Keywords.DEFAULT_VALUE); + return message.getKeywords().asFlags(); } private ImmutableList<MessageAttachment> getMessageAttachments(MailboxSession session, ImmutableList<Attachment> attachments) throws MailboxException { http://git-wip-us.apache.org/repos/asf/james-project/blob/71a6b1b1/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesUpdateProcessor.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesUpdateProcessor.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesUpdateProcessor.java index 3fd703b..02ae2d2 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesUpdateProcessor.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesUpdateProcessor.java @@ -40,7 +40,6 @@ import org.apache.james.core.MailAddress; import org.apache.james.jmap.exceptions.DraftMessageMailboxUpdateException; import org.apache.james.jmap.exceptions.InvalidOutboxMoveException; import org.apache.james.jmap.model.MessageProperties; -import org.apache.james.jmap.model.OldKeyword; import org.apache.james.jmap.model.SetError; import org.apache.james.jmap.model.SetMessagesRequest; import org.apache.james.jmap.model.SetMessagesResponse; @@ -216,13 +215,6 @@ public class SetMessagesUpdateProcessor implements SetMessagesProcessor { .orElse(previousMailboxes); } - private List<Flags> flagsFromOldKeyword(List<MessageResult> messagesToBeUpdated, OldKeyword oldKeyword) { - return messagesToBeUpdated.stream() - .map(MessageResult::getFlags) - .map(oldKeyword::applyToState) - .collect(Guavate.toImmutableList()); - } - private List<MailboxId> mailboxIdFor(Role role, MailboxSession session) throws MailboxException { return systemMailboxesProvider.getMailboxByRole(role, session) .map(MessageManager::getId) @@ -260,7 +252,9 @@ public class SetMessagesUpdateProcessor implements SetMessagesProcessor { private Stream<MailboxException> updateFlags(MessageId messageId, UpdateMessagePatch updateMessagePatch, MailboxSession mailboxSession, MessageResult messageResult) { try { if (!updateMessagePatch.isFlagsIdentity()) { - messageIdManager.setFlags(newState(messageResult, updateMessagePatch), FlagsUpdateMode.REPLACE, messageId, ImmutableList.of(messageResult.getMailboxId()), mailboxSession); + messageIdManager.setFlags( + updateMessagePatch.applyToState(messageResult.getFlags()), + FlagsUpdateMode.REPLACE, messageId, ImmutableList.of(messageResult.getMailboxId()), mailboxSession); } return Stream.of(); } catch (MailboxException e) { @@ -268,18 +262,6 @@ public class SetMessagesUpdateProcessor implements SetMessagesProcessor { } } - private Flags newState(MessageResult messageResult, UpdateMessagePatch updateMessagePatch) { - return updateMessagePatch.getOldKeyword() - .map(oldKeyword -> firstFlagsFromOldKeyword(ImmutableList.of(messageResult), oldKeyword)) - .orElse(updateMessagePatch.applyToState(messageResult.getFlags())); - } - - private Flags firstFlagsFromOldKeyword(List<MessageResult> messagesToBeUpdated, OldKeyword oldKeyword) { - return flagsFromOldKeyword(messagesToBeUpdated, oldKeyword).stream() - .findFirst() - .orElse(null); - } - private void setInMailboxes(MessageId messageId, UpdateMessagePatch updateMessagePatch, MailboxSession mailboxSession) throws MailboxException { Optional<List<String>> serializedMailboxIds = updateMessagePatch.getMailboxIds(); if (serializedMailboxIds.isPresent()) { http://git-wip-us.apache.org/repos/asf/james-project/blob/71a6b1b1/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/CreationMessage.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/CreationMessage.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/CreationMessage.java index 6c7f170..fb45422 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/CreationMessage.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/CreationMessage.java @@ -34,6 +34,7 @@ import javax.mail.internet.InternetAddress; import org.apache.james.jmap.methods.ValidationResult; import org.apache.james.jmap.model.MessageProperties.MessageProperty; import org.apache.james.mailbox.MessageManager; +import org.apache.james.util.OptionalUtils; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder; @@ -220,10 +221,9 @@ public class CreationMessage { Optional<Keywords> maybeKeywords = creationKeywords(); Optional<OldKeyword> oldKeywords = oldKeywordBuilder.computeOldKeyword(); - Preconditions.checkArgument(!(maybeKeywords.isPresent() && oldKeywords.isPresent()), "Does not support keyword and is* at the same time"); return new CreationMessage(mailboxIds, Optional.ofNullable(inReplyToMessageId), headers.build(), from, to.build(), cc.build(), bcc.build(), replyTo.build(), subject, date, Optional.ofNullable(textBody), Optional.ofNullable(htmlBody), - attachments, attachedMessages, maybeKeywords, oldKeywords); + attachments, attachedMessages, computeKeywords(maybeKeywords, oldKeywords)); } private Optional<Keywords> creationKeywords() { @@ -232,6 +232,14 @@ public class CreationMessage { .fromMap(map)); } + public Keywords computeKeywords(Optional<Keywords> keywords, Optional<OldKeyword> oldKeywords) { + Preconditions.checkArgument(!(keywords.isPresent() && oldKeywords.isPresent()), "Does not support keyword and is* at the same time"); + return OptionalUtils + .or(keywords, + oldKeywords.map(OldKeyword::asKeywords)) + .orElse(Keywords.DEFAULT_VALUE); + } + } private final ImmutableList<String> mailboxIds; @@ -248,13 +256,12 @@ public class CreationMessage { private final Optional<String> htmlBody; private final ImmutableList<Attachment> attachments; private final ImmutableMap<BlobId, SubMessage> attachedMessages; - private final Optional<Keywords> keywords; - private final Optional<OldKeyword> oldKeyword; + private final Keywords keywords; @VisibleForTesting CreationMessage(ImmutableList<String> mailboxIds, Optional<String> inReplyToMessageId, ImmutableMap<String, String> headers, Optional<DraftEmailer> from, ImmutableList<DraftEmailer> to, ImmutableList<DraftEmailer> cc, ImmutableList<DraftEmailer> bcc, ImmutableList<DraftEmailer> replyTo, String subject, ZonedDateTime date, Optional<String> textBody, Optional<String> htmlBody, ImmutableList<Attachment> attachments, - ImmutableMap<BlobId, SubMessage> attachedMessages, Optional<Keywords> keywords, Optional<OldKeyword> oldKeyword) { + ImmutableMap<BlobId, SubMessage> attachedMessages, Keywords keywords) { this.mailboxIds = mailboxIds; this.inReplyToMessageId = inReplyToMessageId; this.headers = headers; @@ -270,7 +277,10 @@ public class CreationMessage { this.attachments = attachments; this.attachedMessages = attachedMessages; this.keywords = keywords; - this.oldKeyword = oldKeyword; + } + + public Keywords getKeywords() { + return keywords; } public ImmutableList<String> getMailboxIds() { @@ -334,18 +344,7 @@ public class CreationMessage { } public boolean isDraft() { - return oldKeyword - .map(OldKeyword::isDraft) - .orElse(keywords.map(keywords -> keywords.contains(Keyword.DRAFT))) - .orElse(false); - } - - public Optional<Keywords> getKeywords() { - return keywords; - } - - public Optional<OldKeyword> getOldKeyword() { - return oldKeyword; + return keywords.contains(Keyword.DRAFT); } public List<ValidationResult> validate() { http://git-wip-us.apache.org/repos/asf/james-project/blob/71a6b1b1/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/UpdateMessagePatch.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/UpdateMessagePatch.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/UpdateMessagePatch.java index 830181b..34878c6 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/UpdateMessagePatch.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/UpdateMessagePatch.java @@ -135,14 +135,6 @@ public class UpdateMessagePatch { return mailboxIds; } - public Optional<Keywords> getKeywords() { - return keywords; - } - - public Optional<OldKeyword> getOldKeyword() { - return oldKeywords; - } - public boolean isFlagsIdentity() { return !oldKeywords.isPresent() && !keywords.isPresent(); } @@ -156,8 +148,10 @@ public class UpdateMessagePatch { } public Flags applyToState(Flags currentFlags) { - return keywords - .map(keyword -> keyword.asFlagsWithRecentAndDeletedFrom(currentFlags)) - .orElse(currentFlags); + return oldKeywords + .map(oldKeyword -> oldKeyword.applyToState(currentFlags)) + .orElse(keywords + .map(keyword -> keyword.asFlagsWithRecentAndDeletedFrom(currentFlags)) + .orElse(currentFlags)); } } http://git-wip-us.apache.org/repos/asf/james-project/blob/71a6b1b1/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/OldKeywordTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/OldKeywordTest.java b/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/OldKeywordTest.java index bcc079f..46bdce4 100644 --- a/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/OldKeywordTest.java +++ b/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/OldKeywordTest.java @@ -259,7 +259,7 @@ public class OldKeywordTest { } @Test - public void applyStateShouldDeletedFlag() { + public void applyStateShouldPreserveDeletedFlag() { Optional<OldKeyword> testee = OldKeyword.builder() .isUnread(Optional.of(false)) .isFlagged(Optional.empty()) @@ -271,4 +271,15 @@ public class OldKeywordTest { assertThat(testee.get().applyToState(new Flags(Flag.DELETED))) .isEqualTo(new FlagsBuilder().add(Flag.DELETED, Flag.SEEN).build()); } + + @Test + public void applyStateShouldPreserveCustomFlag() { + Optional<OldKeyword> testee = OldKeyword.builder() + .isUnread(Optional.of(false)) + .computeOldKeyword(); + + String customFlag = "custom"; + assertThat(testee.get().applyToState(new Flags(customFlag))) + .isEqualTo(new FlagsBuilder().add(Flag.SEEN).add(customFlag).build()); + } } \ No newline at end of file http://git-wip-us.apache.org/repos/asf/james-project/blob/71a6b1b1/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/UpdateMessagePatchTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/UpdateMessagePatchTest.java b/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/UpdateMessagePatchTest.java index 5b8c4a1..faf7f53 100644 --- a/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/UpdateMessagePatchTest.java +++ b/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/UpdateMessagePatchTest.java @@ -47,6 +47,39 @@ public class UpdateMessagePatchTest { } @Test + public void applyToStateShouldNotResetSystemFlagsWhenUsingOldKeywords() { + UpdateMessagePatch testee = UpdateMessagePatch.builder() + .isAnswered(true) + .build(); + + Flags isSeen = new Flags(Flags.Flag.SEEN); + assertThat(testee.applyToState(isSeen).getSystemFlags()) + .containsExactly(Flags.Flag.ANSWERED, Flags.Flag.SEEN); + } + + @Test + public void applyToStateShouldNotModifySpecifiedOldKeywordsWhenAlreadySet() { + UpdateMessagePatch testee = UpdateMessagePatch.builder() + .isAnswered(true) + .build(); + + Flags isSeen = new Flags(Flags.Flag.ANSWERED); + assertThat(testee.applyToState(isSeen).getSystemFlags()) + .containsExactly(Flags.Flag.ANSWERED); + } + + @Test + public void applyToStateShouldResetSpecifiedOldKeywords() { + UpdateMessagePatch testee = UpdateMessagePatch.builder() + .isAnswered(false) + .build(); + + Flags isSeen = new Flags(Flags.Flag.ANSWERED); + assertThat(testee.applyToState(isSeen).getSystemFlags()) + .containsExactly(); + } + + @Test public void applyStateShouldReturnNewFlagsWhenKeywords() { ImmutableMap<String, Boolean> keywords = ImmutableMap.of( "$Answered", true, --------------------------------------------------------------------- To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org
