Skip to content

Commit

Permalink
JAMES-2161 Fix Keywords.Factory construction
Browse files Browse the repository at this point in the history 
 - Building the factory was too complex and required a deep understanding of its logic

We propose in this pull request a simple (lenient/strict) model for the caller. Note that this model
fill all caller needs.

 - The factory was mutable leading to some potential subtle bugs lurking in.
  • Loading branch information
@chibenwa
chibenwa committed Nov 27, 2018
1 parent 34aad5b commit e9f4865
Show file tree
Hide file tree
Showing 16 changed files with 112 additions and 113 deletions.
2 changes: 1 addition & 1 deletion ...st/java/org/apache/james/jmap/methods/integration/cucumber/SetMessagesMethodStepdefs.java
View file
Expand Up @@ -292,7 +292,7 @@ private String toKeywordsString(List<String> keywords) {

@When("^message \"([^\"]*)\" has flags (.*) in mailbox \"([^\"]*)\" of user \"([^\"]*)\"$")
public void setMessageFlagsInSpecifiedMailbox(String message, List<String> flags, String mailbox, String mailboxOwner) throws Exception {
Flags newFlags = Keywords.factory().fromList(flags).asFlags();
Flags newFlags = Keywords.lenientFactory().fromList(flags).asFlags();
String username = userStepdefs.getConnectedUser();
MessageId messageId = messageIdStepdefs.getMessageId(message);
MailboxId mailboxId = mainStepdefs.getMailboxId(mailboxOwner, mailbox);
Expand Down
3 changes: 1 addition & 2 deletions server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/GetMessagesMethod.java
View file
Expand Up @@ -79,8 +79,7 @@ public class GetMessagesMethod implements Method {
this.messageFactory = messageFactory;
this.messageIdManager = messageIdManager;
this.metricFactory = metricFactory;
this.keywordsFactory = Keywords.factory()
.filterImapNonExposedKeywords();
this.keywordsFactory = Keywords.lenientFactory();
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/MessageAppender.java
View file
Expand Up @@ -123,7 +123,7 @@ public MessageFactory.MetaDataWithContent appendMessageInMailbox(org.apache.jame

return MessageFactory.MetaDataWithContent.builder()
.uid(appendedMessage.getUid())
.keywords(Keywords.factory().fromFlags(flags))
.keywords(Keywords.lenientFactory().fromFlags(flags))
.internalDate(internalDate.toInstant())
.sharedContent(content)
.size(messageContent.length)
Expand Down
2 changes: 1 addition & 1 deletion ...rotocols/jmap/src/main/java/org/apache/james/jmap/methods/SetMessagesUpdateProcessor.java
View file
Expand Up @@ -209,7 +209,7 @@ private void assertValidUpdate(List<MessageResult> messagesToBeUpdated, UpdateMe

boolean isDraft = messagesToBeUpdated.stream()
.map(MessageResult::getFlags)
.map(Keywords.factory().filterImapNonExposedKeywords()::fromFlags)
.map(Keywords.lenientFactory()::fromFlags)
.reduce(new KeywordsCombiner())
.orElse(Keywords.DEFAULT_VALUE)
.contains(Keyword.DRAFT);
Expand Down
3 changes: 1 addition & 2 deletions server/protocols/jmap/src/main/java/org/apache/james/jmap/model/CreationMessage.java
View file
Expand Up @@ -227,8 +227,7 @@ public CreationMessage build() {
}

private Optional<Keywords> creationKeywords() {
return keywords.map(map -> Keywords.factory()
.throwOnImapNonExposedKeywords()
return keywords.map(map -> Keywords.strictFactory()
.fromMap(map));
}

Expand Down
52 changes: 29 additions & 23 deletions server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Keywords.java
View file
Expand Up @@ -22,7 +22,6 @@
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.function.Predicate;
import java.util.stream.Stream;
Expand All @@ -43,13 +42,9 @@

public class Keywords {

public static final Keywords DEFAULT_VALUE = factory().fromSet(ImmutableSet.of());
public static final Keywords DEFAULT_VALUE = new Keywords(ImmutableSet.of());
private static final Logger LOGGER = LoggerFactory.getLogger(Keywords.class);

public interface KeywordsValidator {
void validate(Set<Keyword> keywords);
}

private FlagsBuilder combiner(FlagsBuilder firstBuilder, FlagsBuilder secondBuilder) {
return firstBuilder.add(secondBuilder.build());
}
Expand All @@ -59,31 +54,36 @@ private FlagsBuilder accumulator(FlagsBuilder accumulator, Keyword keyword) {
}

public static class KeywordsFactory {
private Optional<KeywordsValidator> validator;
private Optional<Predicate<Keyword>> filter;
@FunctionalInterface
interface KeywordsValidator {
KeywordsValidator THROW_ON_IMAP_NON_EXPOSED_KEYWORDS = keywords -> Preconditions.checkArgument(
keywords.stream().allMatch(Keyword::isExposedImapKeyword),
"Does not allow to update 'Deleted' or 'Recent' flag");

KeywordsValidator IGNORE_NON_EXPOSED_IMAP_KEYWORDS = keywords -> { };

private KeywordsFactory() {
validator = Optional.empty();
filter = Optional.empty();
void validate(Set<Keyword> keywords);
}

public KeywordsFactory throwOnImapNonExposedKeywords() {
validator = Optional.of(keywords -> Preconditions.checkArgument(
keywords.stream().allMatch(Keyword::isExposedImapKeyword), "Does not allow to update 'Deleted' or 'Recent' flag"));
return this;
@FunctionalInterface
interface KeywordFilter extends Predicate<Keyword> {
KeywordFilter FILTER_IMAP_NON_EXPOSED_KEYWORDS = Keyword::isExposedImapKeyword;
KeywordFilter KEEP_ALL = keyword -> true;
}

public KeywordsFactory filterImapNonExposedKeywords() {
filter = Optional.of(Keyword::isExposedImapKeyword);
return this;
private final KeywordsValidator validator;
private final Predicate<Keyword> filter;

public KeywordsFactory(KeywordsValidator validator, Predicate<Keyword> filter) {
this.validator = validator;
this.filter = filter;
}

public Keywords fromSet(Set<Keyword> setKeywords) {
validator.orElse(keywords -> { })
.validate(setKeywords);
validator.validate(setKeywords);

return new Keywords(setKeywords.stream()
.filter(filter.orElse(keyword -> true))
.filter(filter)
.collect(Guavate.toImmutableSet()));
}

Expand Down Expand Up @@ -130,8 +130,14 @@ private Stream<Keyword> asKeyword(String flagName) {
}
}

public static KeywordsFactory factory() {
return new KeywordsFactory();
public static KeywordsFactory strictFactory() {
return new KeywordsFactory(KeywordsFactory.KeywordsValidator.THROW_ON_IMAP_NON_EXPOSED_KEYWORDS,
KeywordsFactory.KeywordFilter.KEEP_ALL);
}

public static KeywordsFactory lenientFactory() {
return new KeywordsFactory(KeywordsFactory.KeywordsValidator.IGNORE_NON_EXPOSED_IMAP_KEYWORDS,
KeywordsFactory.KeywordFilter.FILTER_IMAP_NON_EXPOSED_KEYWORDS);
}

private final ImmutableSet<Keyword> keywords;
Expand Down
2 changes: 1 addition & 1 deletion server/protocols/jmap/src/main/java/org/apache/james/jmap/model/OldKeyword.java
View file
Expand Up @@ -173,7 +173,7 @@ public Flags applyToState(Flags currentFlags) {
}

public Keywords asKeywords() {
return Keywords.factory()
return Keywords.strictFactory()
.fromSet(StreamUtils
.flatten(
OptionalUtils.toStream(isAnswered.filter(b -> b).map(b -> Keyword.ANSWERED)),
Expand Down
3 changes: 1 addition & 2 deletions server/protocols/jmap/src/main/java/org/apache/james/jmap/model/UpdateMessagePatch.java
View file
Expand Up @@ -108,8 +108,7 @@ public UpdateMessagePatch build() {
}

private Optional<Keywords> creationKeywords() {
return keywords.map(map -> Keywords.factory()
.throwOnImapNonExposedKeywords()
return keywords.map(map -> Keywords.strictFactory()
.fromMap(map));
}

Expand Down
3 changes: 1 addition & 2 deletions server/protocols/jmap/src/main/java/org/apache/james/jmap/utils/KeywordsCombiner.java
View file
Expand Up @@ -37,8 +37,7 @@ public class KeywordsCombiner implements BinaryOperator<Keywords> {
private Keywords.KeywordsFactory keywordsFactory;

public KeywordsCombiner() {
this.keywordsFactory = Keywords.factory()
.filterImapNonExposedKeywords();
this.keywordsFactory = Keywords.lenientFactory();
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion server/protocols/jmap/src/test/java/org/apache/james/jmap/json/ParsingWritingObjects.java
View file
Expand Up @@ -77,7 +77,7 @@ interface Common {
.threadId(Common.THREAD_ID)
.mailboxIds(Common.MAILBOX_IDS)
.inReplyToMessageId(Common.IN_REPLY_TO_MESSAGE_ID)
.keywords(Keywords.factory().fromSet(Common.KEYWORDS))
.keywords(Keywords.strictFactory().fromSet(Common.KEYWORDS))
.headers(Common.HEADERS)
.from(Common.FROM)
.to(Common.TO)
Expand Down
33 changes: 15 additions & 18 deletions server/protocols/jmap/src/test/java/org/apache/james/jmap/model/KeywordsTest.java
View file
Expand Up @@ -51,13 +51,13 @@ public void shouldRespectBeanContract() {
public void fromMapShouldThrowWhenWrongKeywordValue() {
expectedException.expect(IllegalArgumentException.class);

Keywords.factory()
Keywords.lenientFactory()
.fromMap(ImmutableMap.of(ANY_KEYWORD, false));
}

@Test
public void fromMapShouldReturnKeywordsFromMapStringAndBoolean() {
Keywords keywords = Keywords.factory()
Keywords keywords = Keywords.lenientFactory()
.fromMap(ImmutableMap.of(ANY_KEYWORD, Keyword.FLAG_VALUE));

assertThat(keywords.getKeywords())
Expand All @@ -66,7 +66,7 @@ public void fromMapShouldReturnKeywordsFromMapStringAndBoolean() {

@Test
public void fromFlagsShouldReturnKeywordsFromAllFlag() {
Keywords keywords = Keywords.factory()
Keywords keywords = Keywords.lenientFactory()
.fromFlags(new Flags(Flags.Flag.ANSWERED));

assertThat(keywords.getKeywords())
Expand All @@ -75,7 +75,7 @@ public void fromFlagsShouldReturnKeywordsFromAllFlag() {

@Test
public void fromSetShouldReturnKeywordsFromSetOfKeywords() {
Keywords keywords = Keywords.factory()
Keywords keywords = Keywords.lenientFactory()
.fromSet(ImmutableSet.of(Keyword.ANSWERED));

assertThat(keywords.getKeywords())
Expand All @@ -84,7 +84,7 @@ public void fromSetShouldReturnKeywordsFromSetOfKeywords() {

@Test
public void asFlagsShouldBuildFlagsFromKeywords() {
assertThat(Keywords.factory()
assertThat(Keywords.lenientFactory()
.fromSet(ImmutableSet.of(Keyword.ANSWERED))
.asFlags())
.isEqualTo(new Flags(Flags.Flag.ANSWERED));
Expand All @@ -100,7 +100,7 @@ public void asFlagsWithRecentAndDeletedFromShouldBuildFlagsFromKeywordsAndRecent
.add(Flag.ANSWERED, Flag.RECENT)
.build();

assertThat(Keywords.factory()
assertThat(Keywords.lenientFactory()
.fromSet(ImmutableSet.of(Keyword.ANSWERED))
.asFlagsWithRecentAndDeletedFrom(originFlags))
.isEqualTo(expectedFlags);
Expand All @@ -116,15 +116,15 @@ public void asFlagsWithRecentAndDeletedFromShouldBuildFlagsFromKeywordsWithDelet
.add(Flag.ANSWERED, Flag.RECENT, Flag.DELETED)
.build();

assertThat(Keywords.factory()
assertThat(Keywords.lenientFactory()
.fromSet(ImmutableSet.of(Keyword.ANSWERED))
.asFlagsWithRecentAndDeletedFrom(originFlags))
.isEqualTo(expectedFlags);
}

@Test
public void asMapShouldReturnEmptyWhenEmptyMapOfStringAndBoolean() {
assertThat(Keywords.factory()
assertThat(Keywords.lenientFactory()
.fromSet(ImmutableSet.of())
.asMap())
.isEmpty();
Expand All @@ -133,7 +133,7 @@ public void asMapShouldReturnEmptyWhenEmptyMapOfStringAndBoolean() {
@Test
public void asMapShouldReturnMapOfStringAndBoolean() {
Map<String, Boolean> expectedMap = ImmutableMap.of("$Answered", Keyword.FLAG_VALUE);
assertThat(Keywords.factory()
assertThat(Keywords.lenientFactory()
.fromSet(ImmutableSet.of(Keyword.ANSWERED))
.asMap())
.isEqualTo(expectedMap);
Expand All @@ -143,15 +143,13 @@ public void asMapShouldReturnMapOfStringAndBoolean() {
public void throwWhenUnsupportedKeywordShouldThrowWhenHaveUnsupportedKeywords() {
expectedException.expect(IllegalArgumentException.class);

Keywords.factory()
.throwOnImapNonExposedKeywords()
Keywords.strictFactory()
.fromSet(ImmutableSet.of(Keyword.DRAFT, Keyword.DELETED));
}

@Test
public void throwWhenUnsupportedKeywordShouldNotThrowWhenHaveDraft() {
Keywords keywords = Keywords.factory()
.throwOnImapNonExposedKeywords()
Keywords keywords = Keywords.strictFactory()
.fromSet(ImmutableSet.of(Keyword.ANSWERED, Keyword.DRAFT));

assertThat(keywords.getKeywords())
Expand All @@ -160,8 +158,7 @@ public void throwWhenUnsupportedKeywordShouldNotThrowWhenHaveDraft() {

@Test
public void filterUnsupportedShouldFilter() {
Keywords keywords = Keywords.factory()
.filterImapNonExposedKeywords()
Keywords keywords = Keywords.lenientFactory()
.fromSet(ImmutableSet.of(Keyword.ANSWERED, Keyword.DELETED, Keyword.RECENT, Keyword.DRAFT));

assertThat(keywords.getKeywords())
Expand All @@ -170,23 +167,23 @@ public void filterUnsupportedShouldFilter() {

@Test
public void containsShouldReturnTrueWhenKeywordsContainKeyword() {
Keywords keywords = Keywords.factory()
Keywords keywords = Keywords.lenientFactory()
.fromSet(ImmutableSet.of(Keyword.SEEN));

assertThat(keywords.contains(Keyword.SEEN)).isTrue();
}

@Test
public void containsShouldReturnFalseWhenKeywordsDoNotContainKeyword() {
Keywords keywords = Keywords.factory()
Keywords keywords = Keywords.lenientFactory()
.fromSet(ImmutableSet.of());

assertThat(keywords.contains(Keyword.SEEN)).isFalse();
}

@Test
public void fromListShouldReturnKeywordsFromListOfStrings() {
Keywords keywords = Keywords.factory()
Keywords keywords = Keywords.lenientFactory()
.fromList(ImmutableList.of("$Answered", "$Flagged"));

assertThat(keywords.getKeywords())
Expand Down

0 comments on commit e9f4865

Please sign in to comment.