JAMES-2255 JMAP GetMessageList maximum limit should be a long
Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/8c26272d Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/8c26272d Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/8c26272d Branch: refs/heads/master Commit: 8c26272df9507941200fda6d51137158481dab52 Parents: aebb2d2 Author: benwa <[email protected]> Authored: Mon Dec 18 18:37:36 2017 +0700 Committer: benwa <[email protected]> Committed: Fri Jan 5 16:12:04 2018 +0700 ---------------------------------------------------------------------- .../james/modules/TestJMAPServerModule.java | 4 +- .../jmap/methods/GetMessageListMethod.java | 8 ++-- .../org/apache/james/jmap/model/Attachment.java | 9 +++-- .../james/jmap/model/FilterCondition.java | 6 ++- .../james/jmap/model/GetMessageListRequest.java | 7 +++- .../org/apache/james/jmap/model/Message.java | 2 +- .../org/apache/james/jmap/model/Number.java | 41 ++++++++++---------- .../apache/james/jmap/model/UploadResponse.java | 2 +- .../james/jmap/model/mailbox/Mailbox.java | 28 ++++++++++--- .../james/jmap/model/FilterConditionTest.java | 6 +-- .../jmap/model/GetMessageListRequestTest.java | 10 ++--- .../org/apache/james/jmap/model/NumberTest.java | 12 +++--- 12 files changed, 80 insertions(+), 55 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/8c26272d/server/container/guice/protocols/jmap/src/test/java/org/apache/james/modules/TestJMAPServerModule.java ---------------------------------------------------------------------- diff --git a/server/container/guice/protocols/jmap/src/test/java/org/apache/james/modules/TestJMAPServerModule.java b/server/container/guice/protocols/jmap/src/test/java/org/apache/james/modules/TestJMAPServerModule.java index f23d3b6..c15f5e4 100644 --- a/server/container/guice/protocols/jmap/src/test/java/org/apache/james/modules/TestJMAPServerModule.java +++ b/server/container/guice/protocols/jmap/src/test/java/org/apache/james/modules/TestJMAPServerModule.java @@ -44,9 +44,9 @@ public class TestJMAPServerModule extends AbstractModule{ "kwIDAQAB\n" + "-----END PUBLIC KEY-----"; - private final int maximumLimit; + private final long maximumLimit; - public TestJMAPServerModule(int maximumLimit) { + public TestJMAPServerModule(long maximumLimit) { this.maximumLimit = maximumLimit; } http://git-wip-us.apache.org/repos/asf/james-project/blob/8c26272d/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/GetMessageListMethod.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/GetMessageListMethod.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/GetMessageListMethod.java index c618126..c849ef0 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/GetMessageListMethod.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/methods/GetMessageListMethod.java @@ -56,20 +56,20 @@ public class GetMessageListMethod implements Method { private static final long DEFAULT_POSITION = 0; public static final String MAXIMUM_LIMIT = "maximumLimit"; - public static final int DEFAULT_MAXIMUM_LIMIT = 256; + public static final long DEFAULT_MAXIMUM_LIMIT = 256; private static final Method.Request.Name METHOD_NAME = Method.Request.name("getMessageList"); private static final Method.Response.Name RESPONSE_NAME = Method.Response.name("messageList"); private final MailboxManager mailboxManager; - private final int maximumLimit; + private final long maximumLimit; private final GetMessagesMethod getMessagesMethod; private final Factory mailboxIdFactory; private final MetricFactory metricFactory; @Inject @VisibleForTesting public GetMessageListMethod(MailboxManager mailboxManager, - @Named(MAXIMUM_LIMIT) int maximumLimit, GetMessagesMethod getMessagesMethod, MailboxId.Factory mailboxIdFactory, + @Named(MAXIMUM_LIMIT) long maximumLimit, GetMessagesMethod getMessagesMethod, MailboxId.Factory mailboxIdFactory, MetricFactory metricFactory) { this.mailboxManager = mailboxManager; @@ -128,7 +128,7 @@ public class GetMessageListMethod implements Method { Long postionValue = messageListRequest.getPosition().map(Number::asLong).orElse(DEFAULT_POSITION); mailboxManager.search(searchQuery, mailboxSession, - postionValue + messageListRequest.getLimit().map(Number::asInt).orElse(maximumLimit)) + postionValue + messageListRequest.getLimit().map(Number::asLong).orElse(maximumLimit)) .stream() .skip(postionValue) .forEach(builder::messageId); http://git-wip-us.apache.org/repos/asf/james-project/blob/8c26272d/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Attachment.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Attachment.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Attachment.java index 37dfa7f..b9b39fb 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Attachment.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Attachment.java @@ -69,7 +69,8 @@ public class Attachment { } public Builder size(long size) { - this.size = Number.fromLong(size); + this.size = Number.DEFAULT_FACTORY.from(size) + .orElseThrow(() -> new IllegalArgumentException(Number.VALIDATION_MESSAGE)); return this; } @@ -90,12 +91,14 @@ public class Attachment { } public Builder width(long width) { - this.width = Number.fromLong(width); + this.width = Number.DEFAULT_FACTORY.from(width) + .orElseThrow(() -> new IllegalArgumentException(Number.VALIDATION_MESSAGE)); return this; } public Builder height(long height) { - this.height = Number.fromLong(height); + this.height = Number.DEFAULT_FACTORY.from(height) + .orElseThrow(() -> new IllegalArgumentException(Number.VALIDATION_MESSAGE)); return this; } http://git-wip-us.apache.org/repos/asf/james-project/blob/8c26272d/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/FilterCondition.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/FilterCondition.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/FilterCondition.java index 02f7657..f98cec0 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/FilterCondition.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/FilterCondition.java @@ -118,12 +118,14 @@ public class FilterCondition implements Filter { } public Builder minSize(long minSize) { - this.minSize = Number.fromLong(minSize); + this.minSize = Number.DEFAULT_FACTORY.from(minSize) + .orElseThrow(() -> new IllegalArgumentException(Number.VALIDATION_MESSAGE)); return this; } public Builder maxSize(long maxSize) { - this.maxSize = Number.fromLong(maxSize); + this.maxSize = Number.DEFAULT_FACTORY.from(maxSize) + .orElseThrow(() -> new IllegalArgumentException(Number.VALIDATION_MESSAGE)); return this; } http://git-wip-us.apache.org/repos/asf/james-project/blob/8c26272d/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/GetMessageListRequest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/GetMessageListRequest.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/GetMessageListRequest.java index b675537..66710ca 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/GetMessageListRequest.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/GetMessageListRequest.java @@ -78,7 +78,9 @@ public class GetMessageListRequest implements JmapRequest { } public Builder position(long position) { - this.position = Optional.of(Number.fromLong(position)); + this.position = Optional.of( + Number.DEFAULT_FACTORY.from(position) + .orElseThrow(() -> new IllegalArgumentException(Number.VALIDATION_MESSAGE))); return this; } @@ -91,7 +93,8 @@ public class GetMessageListRequest implements JmapRequest { } public Builder limit(long limit) { - this.limit = Number.fromLong(limit); + this.limit = Number.DEFAULT_FACTORY.from(limit) + .orElseThrow(() -> new IllegalArgumentException(Number.VALIDATION_MESSAGE)); return this; } http://git-wip-us.apache.org/repos/asf/james-project/blob/8c26272d/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Message.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Message.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Message.java index 9d679b2..66123be 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Message.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Message.java @@ -163,7 +163,7 @@ public class Message { } public Builder size(long size) { - this.size = Number.fromOutboundLong(size); + this.size = Number.BOUND_SANITIZING_FACTORY.from(size); return this; } http://git-wip-us.apache.org/repos/asf/james-project/blob/8c26272d/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Number.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Number.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Number.java index 98a73ce..97c72c0 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Number.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/Number.java @@ -20,34 +20,35 @@ package org.apache.james.jmap.model; import java.util.Objects; +import java.util.Optional; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonValue; import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; import com.google.common.math.LongMath; -import com.google.common.primitives.Ints; public class Number { public static final Logger LOGGER = LoggerFactory.getLogger(Number.class); + public static final String VALIDATION_MESSAGE = "value should be positive and less than 2^53"; - public static Number fromInt(int value) { - Preconditions.checkState(value >= ZERO_VALUE, - "value should be positive and less than 2^31"); - return new Number(value); + public interface Factory<T> { + T from(long value); } - public static Number fromLong(long value) { - Preconditions.checkState(value >= ZERO_VALUE && value <= MAX_VALUE, - "value should be positive and less than 2^53"); - return new Number(value); - } - public static Number fromOutboundLong(long value) { + private static final int ZERO_VALUE = 0; + public static final long MAX_VALUE = LongMath.pow(2, 53); + public static final Number ZERO = new Number(ZERO_VALUE); + + public static final Factory<Optional<Number>> DEFAULT_FACTORY = value -> Optional.of(value) + .filter(Number::isValid) + .map(Number::new); + + public static final Factory<Number> BOUND_SANITIZING_FACTORY = value -> { if (value < ZERO_VALUE) { LOGGER.warn("Received a negative Number"); return new Number(ZERO_VALUE); @@ -57,15 +58,20 @@ public class Number { return new Number(MAX_VALUE); } return new Number(value); + }; + + public static Number fromLong(long value) { + return new Number(value); } - private static final int ZERO_VALUE = 0; - public static final long MAX_VALUE = LongMath.pow(2, 53); - public static final Number ZERO = new Number(ZERO_VALUE); + private static boolean isValid(long value) { + return value >= ZERO_VALUE && value <= MAX_VALUE; + } private final long value; private Number(long value) { + Preconditions.checkArgument(isValid(value), VALIDATION_MESSAGE); this.value = value; } @@ -74,11 +80,6 @@ public class Number { return value; } - @JsonIgnore - public int asInt() { - return Ints.checkedCast(value); - } - @Override public final boolean equals(Object o) { if (o instanceof Number) { http://git-wip-us.apache.org/repos/asf/james-project/blob/8c26272d/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/UploadResponse.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/UploadResponse.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/UploadResponse.java index 269cb15..13c9933 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/UploadResponse.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/UploadResponse.java @@ -61,7 +61,7 @@ public class UploadResponse { } public Builder size(long size) { - this.size = Number.fromOutboundLong(size); + this.size = Number.BOUND_SANITIZING_FACTORY.from(size); return this; } http://git-wip-us.apache.org/repos/asf/james-project/blob/8c26272d/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/mailbox/Mailbox.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/mailbox/Mailbox.java b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/mailbox/Mailbox.java index c910369..9c771f4 100644 --- a/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/mailbox/Mailbox.java +++ b/server/protocols/jmap/src/main/java/org/apache/james/jmap/model/mailbox/Mailbox.java @@ -139,22 +139,22 @@ public class Mailbox { } public Builder totalMessages(long totalMessages) { - this.totalMessages = Optional.of(Number.fromOutboundLong(totalMessages)); + this.totalMessages = Optional.of(Number.BOUND_SANITIZING_FACTORY.from(totalMessages)); return this; } public Builder unreadMessages(long unreadMessages) { - this.unreadMessages = Optional.of(Number.fromOutboundLong(unreadMessages)); + this.unreadMessages = Optional.of(Number.BOUND_SANITIZING_FACTORY.from(unreadMessages)); return this; } public Builder totalThreads(long totalThreads) { - this.totalThreads = Optional.of(Number.fromOutboundLong(totalThreads)); + this.totalThreads = Optional.of(Number.BOUND_SANITIZING_FACTORY.from(totalThreads)); return this; } public Builder unreadThreads(long unreadThreads) { - this.unreadThreads = Optional.of(Number.fromOutboundLong(unreadThreads)); + this.unreadThreads = Optional.of(Number.BOUND_SANITIZING_FACTORY.from(unreadThreads)); return this; } @@ -172,8 +172,24 @@ public class Mailbox { Preconditions.checkState(!Strings.isNullOrEmpty(name), "'name' is mandatory"); Preconditions.checkState(id != null, "'id' is mandatory"); - return new Mailbox(id, name, parentId, role, sortOrder, mustBeOnlyMailbox, mayReadItems, mayAddItems, mayRemoveItems, mayCreateChild, mayRename, mayDelete, - totalMessages.orElse(Number.ZERO), unreadMessages.orElse(Number.ZERO), totalThreads.orElse(Number.ZERO), unreadThreads.orElse(Number.ZERO), sharedWith.orElse(Rights.EMPTY), namespace.orElse(MailboxNamespace.personal())); + return new Mailbox(id, + name, + parentId, + role, + sortOrder, + mustBeOnlyMailbox, + mayReadItems, + mayAddItems, + mayRemoveItems, + mayCreateChild, + mayRename, + mayDelete, + totalMessages.orElse(Number.ZERO), + unreadMessages.orElse(Number.ZERO), + totalThreads.orElse(Number.ZERO), + unreadThreads.orElse(Number.ZERO), + sharedWith.orElse(Rights.EMPTY), + namespace.orElse(MailboxNamespace.personal())); } } http://git-wip-us.apache.org/repos/asf/james-project/blob/8c26272d/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/FilterConditionTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/FilterConditionTest.java b/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/FilterConditionTest.java index d482a42..73bfcf7 100644 --- a/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/FilterConditionTest.java +++ b/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/FilterConditionTest.java @@ -79,8 +79,8 @@ public class FilterConditionTest { public void buildShouldWork() { ZonedDateTime before = ZonedDateTime.parse("2016-07-19T14:30:00Z"); ZonedDateTime after = ZonedDateTime.parse("2016-07-19T14:31:00Z"); - int minSize = 4; - int maxSize = 123; + long minSize = 4; + long maxSize = 123; boolean isFlagged = true; boolean isUnread = true; boolean isAnswered = true; @@ -100,7 +100,7 @@ public class FilterConditionTest { Optional<String> notKeyword = Optional.of("$Flagged"); FilterCondition expectedFilterCondition = new FilterCondition(Optional.of(ImmutableList.of("1")), Optional.of(ImmutableList.of("2")), Optional.of(before), Optional.of(after), - Optional.of(Number.fromInt(minSize)), Optional.of(Number.fromInt(maxSize)), + Optional.of(Number.fromLong(minSize)), Optional.of(Number.fromLong(maxSize)), Optional.of(isFlagged), Optional.of(isUnread), Optional.of(isAnswered), Optional.of(isDraft), Optional.of(isForwarded), Optional.of(hasAttachment), Optional.of(text), Optional.of(from), Optional.of(to), Optional.of(cc), Optional.of(bcc), Optional.of(subject), Optional.of(body), Optional.of(attachments), Optional.of(header), hasKeyword, notKeyword); http://git-wip-us.apache.org/repos/asf/james-project/blob/8c26272d/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/GetMessageListRequestTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/GetMessageListRequestTest.java b/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/GetMessageListRequestTest.java index 3e674e9..f1952e0 100644 --- a/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/GetMessageListRequestTest.java +++ b/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/GetMessageListRequestTest.java @@ -31,14 +31,14 @@ import com.google.common.collect.ImmutableList; public class GetMessageListRequestTest { - @Test(expected=IllegalStateException.class) + @Test(expected=IllegalArgumentException.class) public void builderShouldThrowWhenPositionIsNegative() { - GetMessageListRequest.builder().position(-1L).build(); + GetMessageListRequest.builder().position(-1L); } - @Test(expected=IllegalStateException.class) + @Test(expected=IllegalArgumentException.class) public void builderShouldThrowWhenLimitIsNegative() { - GetMessageListRequest.builder().limit(-1).build(); + GetMessageListRequest.builder().limit(-1); } @Test(expected=NotImplementedException.class) @@ -73,7 +73,7 @@ public class GetMessageListRequestTest { .build(); List<String> sort = ImmutableList.of("date desc"); List<String> fetchMessageProperties = ImmutableList.of("id", "blobId"); - GetMessageListRequest expectedGetMessageListRequest = new GetMessageListRequest(Optional.empty(), Optional.of(filterCondition), sort, Optional.of(true), Optional.of(Number.fromLong(1L)), Optional.empty(), Optional.empty(), Optional.of(Number.fromInt(2)), + GetMessageListRequest expectedGetMessageListRequest = new GetMessageListRequest(Optional.empty(), Optional.of(filterCondition), sort, Optional.of(true), Optional.of(Number.fromLong(1L)), Optional.empty(), Optional.empty(), Optional.of(Number.fromLong(2)), Optional.empty(), Optional.of(true), fetchMessageProperties, Optional.empty()); GetMessageListRequest getMessageListRequest = GetMessageListRequest.builder() http://git-wip-us.apache.org/repos/asf/james-project/blob/8c26272d/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/NumberTest.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/NumberTest.java b/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/NumberTest.java index 59ab9a7..09ab0cf 100644 --- a/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/NumberTest.java +++ b/server/protocols/jmap/src/test/java/org/apache/james/jmap/model/NumberTest.java @@ -26,29 +26,29 @@ import org.junit.Test; public class NumberTest { @Test - public void fromLongShouldReturnMinValueWhenNegativeValueWithLenient() throws Exception { - assertThat(Number.fromOutboundLong(-1)) + public void fromOutboundLongShouldReturnMinValueWhenNegativeValue() throws Exception { + assertThat(Number.BOUND_SANITIZING_FACTORY.from(-1)) .isEqualTo(Number.ZERO); } @Test public void fromOutboundLongShouldSanitizeTooBigNumbers() throws Exception { - assertThat(Number.fromOutboundLong(Number.MAX_VALUE + 1)) - .isEqualTo(Number.MAX_VALUE); + assertThat(Number.BOUND_SANITIZING_FACTORY.from(Number.MAX_VALUE + 1)) + .isEqualTo(Number.fromLong(Number.MAX_VALUE)); } @Test public void fromLongShouldThrowWhenNegativeValue() throws Exception { assertThatThrownBy(() -> Number.fromLong(-1)) - .isInstanceOf(IllegalStateException.class); + .isInstanceOf(IllegalArgumentException.class); } @Test public void fromLongShouldThrowWhenOver2Pow53Value() throws Exception { assertThatThrownBy(() -> Number.fromLong(Number.MAX_VALUE + 1)) - .isInstanceOf(IllegalStateException.class); + .isInstanceOf(IllegalArgumentException.class); } @Test --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
