You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by rc...@apache.org on 2021/04/27 02:53:44 UTC
[james-project] 03/06: JAMES-3557 */changes: Fail explicitly when
too much entries on a single change
This is an automated email from the ASF dual-hosted git repository.
rcordier pushed a commit to branch 3.6.x
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 50bd76cd3151f7e7bc0135384fdb484ca88e51a4
Author: Benoit Tellier <bt...@linagora.com>
AuthorDate: Fri Apr 9 12:00:46 2021 +0700
JAMES-3557 */changes: Fail explicitly when too much entries on a single change
---
.../change/CanNotCalculateChangesException.java | 26 +++++
.../apache/james/jmap/api/change/EmailChanges.java | 4 +
.../james/jmap/api/change/MailboxChanges.java | 4 +
.../api/change/EmailChangeRepositoryContract.java | 27 +++--
.../change/MailboxChangeRepositoryContract.java | 11 ++-
.../contract/EmailChangesMethodContract.scala | 110 +++++++++++++++++++++
.../james/jmap/method/EmailChangesMethod.scala | 8 +-
.../james/jmap/method/MailboxChangesMethod.scala | 8 +-
8 files changed, 173 insertions(+), 25 deletions(-)
diff --git a/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/CanNotCalculateChangesException.java b/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/CanNotCalculateChangesException.java
new file mode 100644
index 0000000..91bc2d0
--- /dev/null
+++ b/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/CanNotCalculateChangesException.java
@@ -0,0 +1,26 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.jmap.api.change;
+
+public class CanNotCalculateChangesException extends RuntimeException {
+ public CanNotCalculateChangesException(String message) {
+ super(message);
+ }
+}
diff --git a/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/EmailChanges.java b/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/EmailChanges.java
index 7cb78ab..807a058 100644
--- a/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/EmailChanges.java
+++ b/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/EmailChanges.java
@@ -129,6 +129,10 @@ public class EmailChanges {
}
public EmailChanges build() {
+ if (hasMoreChanges && created.isEmpty() && updated.isEmpty() && destroyed.isEmpty()) {
+ throw new CanNotCalculateChangesException(String.format("Current change collector limit %d is exceeded by a single change, hence we cannot calculate changes.", limit.getValue()));
+ }
+
return new EmailChanges(state, hasMoreChanges, created, updated, destroyed);
}
}
diff --git a/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/MailboxChanges.java b/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/MailboxChanges.java
index 29522df..32a2160 100644
--- a/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/MailboxChanges.java
+++ b/server/data/data-jmap/src/main/java/org/apache/james/jmap/api/change/MailboxChanges.java
@@ -133,6 +133,10 @@ public class MailboxChanges {
}
public MailboxChanges build() {
+ if (hasMoreChanges && created.isEmpty() && updated.isEmpty() && destroyed.isEmpty()) {
+ throw new CanNotCalculateChangesException(String.format("Current change collector limit %d is exceeded by a single change, hence we cannot calculate changes.", limit.getValue()));
+ }
+
return new MailboxChanges(state, hasMoreChanges, isCountChangeOnly, created, updated, destroyed);
}
diff --git a/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/change/EmailChangeRepositoryContract.java b/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/change/EmailChangeRepositoryContract.java
index d72b4a4..d2a6641 100644
--- a/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/change/EmailChangeRepositoryContract.java
+++ b/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/change/EmailChangeRepositoryContract.java
@@ -445,7 +445,7 @@ public interface EmailChangeRepositoryContract {
}
@Test
- default void getChangesShouldReturnEmptyWhenNumberOfChangesExceedMaxChanges() {
+ default void getChangesShoulThrowWhenNumberOfChangesExceedMaxChanges() {
EmailChangeRepository repository = emailChangeRepository();
State state = generateNewState();
@@ -470,18 +470,12 @@ public interface EmailChangeRepositoryContract {
.isDelegated(false)
.created(messageId2, messageId3)
.build();
- EmailChange change2 = EmailChange.builder()
- .accountId(ACCOUNT_ID)
- .state(generateNewState())
- .date(DATE)
- .isDelegated(false)
- .created(messageId4, messageId5)
- .build();
repository.save(oldState).block();
repository.save(change1).block();
- assertThat(repository.getSinceState(ACCOUNT_ID, state, Optional.of(Limit.of(1))).block().getAllChanges())
- .isEmpty();
+ assertThatThrownBy(() -> repository.getSinceState(ACCOUNT_ID, state, Optional.of(Limit.of(1))).block().getAllChanges())
+ .isInstanceOf(CanNotCalculateChangesException.class)
+ .hasMessage("Current change collector limit 1 is exceeded by a single change, hence we cannot calculate changes.");
}
@Test
@@ -552,13 +546,13 @@ public interface EmailChangeRepositoryContract {
.state(generateNewState())
.date(DATE)
.isDelegated(false)
- .updated(messageId2, messageId3)
+ .updated(messageId2, messageId1)
.build();
repository.save(oldState).block();
repository.save(change1).block();
repository.save(change2).block();
- assertThat(repository.getSinceState(ACCOUNT_ID, state, Optional.of(Limit.of(1))).block().hasMoreChanges())
+ assertThat(repository.getSinceState(ACCOUNT_ID, state, Optional.of(Limit.of(2))).block().hasMoreChanges())
.isTrue();
}
@@ -1083,7 +1077,7 @@ public interface EmailChangeRepositoryContract {
}
@Test
- default void getSinceStateWithDelegationShouldReturnEmptyWhenNumberOfChangesExceedMaxChanges() {
+ default void getSinceStateWithDelegationShouldThrowWhenNumberOfChangesExceedMaxChanges() {
EmailChangeRepository repository = emailChangeRepository();
State state = generateNewState();
@@ -1112,8 +1106,9 @@ public interface EmailChangeRepositoryContract {
repository.save(oldState).block();
repository.save(change1).block();
- assertThat(repository.getSinceStateWithDelegation(ACCOUNT_ID, state, Optional.of(Limit.of(1))).block().getAllChanges())
- .isEmpty();
+ assertThatThrownBy(() -> repository.getSinceStateWithDelegation(ACCOUNT_ID, state, Optional.of(Limit.of(1))).block().getAllChanges())
+ .isInstanceOf(CanNotCalculateChangesException.class)
+ .hasMessage("Current change collector limit 1 is exceeded by a single change, hence we cannot calculate changes.");
}
@Test
@@ -1182,7 +1177,7 @@ public interface EmailChangeRepositoryContract {
repository.save(change1).block();
repository.save(change2).block();
- assertThat(repository.getSinceStateWithDelegation(ACCOUNT_ID, state, Optional.of(Limit.of(1))).block().hasMoreChanges())
+ assertThat(repository.getSinceStateWithDelegation(ACCOUNT_ID, state, Optional.of(Limit.of(2))).block().hasMoreChanges())
.isTrue();
}
diff --git a/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/change/MailboxChangeRepositoryContract.java b/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/change/MailboxChangeRepositoryContract.java
index fc895a8..cd6ad4a 100644
--- a/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/change/MailboxChangeRepositoryContract.java
+++ b/server/data/data-jmap/src/test/java/org/apache/james/jmap/api/change/MailboxChangeRepositoryContract.java
@@ -376,7 +376,7 @@ public interface MailboxChangeRepositoryContract {
}
@Test
- default void getChangesShouldReturnEmptyWhenNumberOfChangesExceedMaxChanges() {
+ default void getChangesShouldReturnThrowWhenNumberOfChangesExceedMaxChanges() {
MailboxChangeRepository repository = mailboxChangeRepository();
State.Factory stateFactory = stateFactory();
State referenceState = stateFactory.generate();
@@ -389,8 +389,9 @@ public interface MailboxChangeRepositoryContract {
repository.save(oldState).block();
repository.save(change1).block();
- assertThat(repository.getSinceState(ACCOUNT_ID, referenceState, Optional.of(Limit.of(1))).block().getAllChanges())
- .isEmpty();
+ assertThatThrownBy(() -> repository.getSinceState(ACCOUNT_ID, referenceState, Optional.of(Limit.of(1))).block().getAllChanges())
+ .isInstanceOf(CanNotCalculateChangesException.class)
+ .hasMessage("Current change collector limit 1 is exceeded by a single change, hence we cannot calculate changes.");
}
@Test
@@ -424,12 +425,12 @@ public interface MailboxChangeRepositoryContract {
MailboxId id3 = generateNewMailboxId();
MailboxChange oldState = MailboxChange.builder().accountId(ACCOUNT_ID).state(referenceState).date(DATE.minusHours(2)).isCountChange(false).created(ImmutableList.of(id1)).build();
MailboxChange change1 = MailboxChange.builder().accountId(ACCOUNT_ID).state(stateFactory.generate()).date(DATE.minusHours(1)).isCountChange(false).created(ImmutableList.of(id2, id3)).build();
- MailboxChange change2 = MailboxChange.builder().accountId(ACCOUNT_ID).state(stateFactory.generate()).date(DATE).isCountChange(false).updated(ImmutableList.of(id2, id3)).build();
+ MailboxChange change2 = MailboxChange.builder().accountId(ACCOUNT_ID).state(stateFactory.generate()).date(DATE).isCountChange(false).updated(ImmutableList.of(id2, id1)).build();
repository.save(oldState).block();
repository.save(change1).block();
repository.save(change2).block();
- assertThat(repository.getSinceState(ACCOUNT_ID, referenceState, Optional.of(Limit.of(1))).block().hasMoreChanges())
+ assertThat(repository.getSinceState(ACCOUNT_ID, referenceState, Optional.of(Limit.of(2))).block().hasMoreChanges())
.isTrue();
}
diff --git a/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailChangesMethodContract.scala b/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailChangesMethodContract.scala
index 80f7306..59222f7 100644
--- a/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailChangesMethodContract.scala
+++ b/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailChangesMethodContract.scala
@@ -198,6 +198,116 @@ trait EmailChangesMethodContract {
}
@Test
+ def shouldFailWithCannotCalculateChangesWhenSingleChangeIsTooLarge(server: GuiceJamesServer): Unit = {
+ val mailboxProbe: MailboxProbeImpl = server.getProbe(classOf[MailboxProbeImpl])
+ val path: MailboxPath = MailboxPath.forUser(BOB, "mailbox1")
+
+ mailboxProbe.createMailbox(path)
+
+ val message: Message = Message.Builder
+ .of
+ .setSubject("test")
+ .setBody("testmail", StandardCharsets.UTF_8)
+ .build
+ val messageId1: MessageId = mailboxProbe.appendMessage(BOB.asString(), path, AppendCommand.from(message)).getMessageId
+ val state1: State = waitForNextState(server, AccountId.fromUsername(BOB), State.INITIAL)
+ val messageId2: MessageId = mailboxProbe.appendMessage(BOB.asString(), path, AppendCommand.from(message)).getMessageId
+ val state2: State = waitForNextState(server, AccountId.fromUsername(BOB), State.INITIAL)
+ val messageId3: MessageId = mailboxProbe.appendMessage(BOB.asString(), path, AppendCommand.from(message)).getMessageId
+ val state3: State = waitForNextState(server, AccountId.fromUsername(BOB), State.INITIAL)
+ val messageId4: MessageId = mailboxProbe.appendMessage(BOB.asString(), path, AppendCommand.from(message)).getMessageId
+ val state4: State = waitForNextState(server, AccountId.fromUsername(BOB), State.INITIAL)
+ val messageId5: MessageId = mailboxProbe.appendMessage(BOB.asString(), path, AppendCommand.from(message)).getMessageId
+ val state5: State = waitForNextState(server, AccountId.fromUsername(BOB), State.INITIAL)
+ val messageId6: MessageId = mailboxProbe.appendMessage(BOB.asString(), path, AppendCommand.from(message)).getMessageId
+ val state6: State = waitForNextState(server, AccountId.fromUsername(BOB), State.INITIAL)
+
+ val updateEmail =
+ s"""{
+ | "using": [
+ | "urn:ietf:params:jmap:core",
+ | "urn:ietf:params:jmap:mail"],
+ | "methodCalls": [
+ | ["Email/set",
+ | {
+ | "accountId": "$ACCOUNT_ID",
+ | "update": {
+ | "${messageId1.serialize}":{
+ | "keywords/$$flagged": true
+ | },
+ | "${messageId2.serialize}":{
+ | "keywords/$$flagged": true
+ | },
+ | "${messageId3.serialize}":{
+ | "keywords/$$flagged": true
+ | },
+ | "${messageId4.serialize}":{
+ | "keywords/$$flagged": true
+ | },
+ | "${messageId5.serialize}":{
+ | "keywords/$$flagged": true
+ | },
+ | "${messageId6.serialize}":{
+ | "keywords/$$flagged": true
+ | }
+ | }
+ | },
+ | "c1"]]
+ |}""".stripMargin
+
+ `with`()
+ .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
+ .body(updateEmail)
+ .post
+ .`then`
+ .statusCode(SC_OK)
+ .contentType(JSON)
+ .extract
+ .body
+ .asString
+
+ val request =
+ s"""{
+ | "using": ["urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail"],
+ | "methodCalls": [[
+ | "Email/changes",
+ | {
+ | "accountId": "$ACCOUNT_ID",
+ | "sinceState": "${state6.getValue}"
+ | },
+ | "c1"]]
+ |}""".stripMargin
+
+ awaitAtMostTenSeconds.untilAsserted { () =>
+ val response = `given`
+ .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
+ .body(request)
+ .when
+ .post
+ .`then`
+ .statusCode(SC_OK)
+ .contentType(JSON)
+ .extract
+ .body
+ .asString
+
+ assertThatJson(response)
+ .inPath("methodResponses[0]")
+ .isEqualTo(
+ s"""[
+ | "error",
+ | {
+ | "type": "cannotCalculateChanges",
+ | "description": "Current change collector limit 5 is exceeded by a single change, hence we cannot calculate changes."
+ | },
+ | "c1"
+ |]""".stripMargin)
+ }
+ }
+
+
+
+ @Test
def shouldReturnUpdatedWhenMessageMove(server: GuiceJamesServer): Unit = {
val mailboxProbe: MailboxProbeImpl = server.getProbe(classOf[MailboxProbeImpl])
val path: MailboxPath = MailboxPath.forUser(BOB, "mailbox1")
diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/EmailChangesMethod.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/EmailChangesMethod.scala
index d33a3c6..d83525d 100644
--- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/EmailChangesMethod.scala
+++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/EmailChangesMethod.scala
@@ -21,11 +21,11 @@ package org.apache.james.jmap.method
import eu.timepit.refined.auto._
import javax.inject.Inject
-import org.apache.james.jmap.api.change.{EmailChangeRepository, EmailChanges, State => JavaState}
+import org.apache.james.jmap.api.change.{CanNotCalculateChangesException, EmailChangeRepository, EmailChanges, State => JavaState}
import org.apache.james.jmap.api.model.{AccountId => JavaAccountId}
import org.apache.james.jmap.core.CapabilityIdentifier.{CapabilityIdentifier, JAMES_SHARES, JMAP_MAIL}
import org.apache.james.jmap.core.Invocation.{Arguments, MethodName}
-import org.apache.james.jmap.core.{Invocation, State}
+import org.apache.james.jmap.core.{ErrorCode, Invocation, State}
import org.apache.james.jmap.json.{EmailGetSerializer, ResponseSerializer}
import org.apache.james.jmap.mail.{EmailChangesRequest, EmailChangesResponse, HasMoreChanges}
import org.apache.james.jmap.routes.SessionSupplier
@@ -66,6 +66,10 @@ class EmailChangesMethod @Inject()(val metricFactory: MetricFactory,
arguments = Arguments(EmailGetSerializer.serializeChanges(response)),
methodCallId = invocation.invocation.methodCallId),
processingContext = invocation.processingContext))
+ .onErrorResume {
+ case e: CanNotCalculateChangesException => SMono.just(InvocationWithContext(Invocation.error(ErrorCode.CannotCalculateChanges, e.getMessage, invocation.invocation.methodCallId), invocation.processingContext))
+ case e => SMono.error(e)
+ }
override def getRequest(mailboxSession: MailboxSession, invocation: Invocation): Either[IllegalArgumentException, EmailChangesRequest] =
EmailGetSerializer.deserializeEmailChangesRequest(invocation.arguments.value) match {
diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MailboxChangesMethod.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MailboxChangesMethod.scala
index d96361b..9f91b78 100644
--- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MailboxChangesMethod.scala
+++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MailboxChangesMethod.scala
@@ -21,11 +21,11 @@ package org.apache.james.jmap.method
import eu.timepit.refined.auto._
import javax.inject.Inject
-import org.apache.james.jmap.api.change.{MailboxChangeRepository, MailboxChanges, State => JavaState}
+import org.apache.james.jmap.api.change.{CanNotCalculateChangesException, MailboxChangeRepository, MailboxChanges, State => JavaState}
import org.apache.james.jmap.api.model.{AccountId => JavaAccountId}
import org.apache.james.jmap.core.CapabilityIdentifier.{CapabilityIdentifier, JMAP_MAIL}
import org.apache.james.jmap.core.Invocation.{Arguments, MethodName}
-import org.apache.james.jmap.core.{CapabilityIdentifier, Invocation, Properties, State}
+import org.apache.james.jmap.core.{CapabilityIdentifier, ErrorCode, Invocation, Properties, State}
import org.apache.james.jmap.json.{MailboxSerializer, ResponseSerializer}
import org.apache.james.jmap.mail.{HasMoreChanges, MailboxChangesRequest, MailboxChangesResponse}
import org.apache.james.jmap.method.MailboxChangesMethod.updatedProperties
@@ -73,6 +73,10 @@ class MailboxChangesMethod @Inject()(mailboxSerializer: MailboxSerializer,
arguments = Arguments(mailboxSerializer.serializeChanges(response)),
methodCallId = invocation.invocation.methodCallId),
processingContext = invocation.processingContext))
+ .onErrorResume {
+ case e: CanNotCalculateChangesException => SMono.just(InvocationWithContext(Invocation.error(ErrorCode.CannotCalculateChanges, e.getMessage, invocation.invocation.methodCallId), invocation.processingContext))
+ case e => SMono.error(e)
+ }
override def getRequest(mailboxSession: MailboxSession, invocation: Invocation): Either[IllegalArgumentException, MailboxChangesRequest] =
mailboxSerializer.deserializeMailboxChangesRequest(invocation.arguments.value) match {
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org