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 <[email protected]> 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: [email protected] For additional commands, e-mail: [email protected]
