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 2020/12/28 07:43:44 UTC
[james-project] 12/16: JAMES-3469 Mailbox/changes should as well
not return creates+updates when created or deleted within the same API call
This is an automated email from the ASF dual-hosted git repository.
rcordier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit ed215c41fdff420ab3d15ca43213a711b0d0a73c
Author: Benoit Tellier <bt...@linagora.com>
AuthorDate: Mon Dec 21 19:22:28 2020 +0700
JAMES-3469 Mailbox/changes should as well not return creates+updates when created or deleted within the same API call
https://jmap.io/spec-core.html#changes
```
If a record has been created AND updated since the old state, the server SHOULD just return the id in the created list but MAY return it in the updated list as well.
If a record has been updated AND destroyed since the old state, the server SHOULD just return the id in the destroyed list but MAY return it in the updated list as well.
If a record has been created AND destroyed since the old state, the server SHOULD remove the id from the response entirely. However, it MAY include it in just the destroyed list or in both the destroyed and created lists.
```
Of course not returning duplicates enable faster re-synchronisations
---
.../apache/james/jmap/draft/JmapGuiceProbe.java | 5 ++
.../james/jmap/api/change/MailboxChanges.java | 29 +++++---
.../change/MailboxChangeRepositoryContract.java | 24 +++++--
.../contract/MailboxChangesMethodContract.scala | 80 +++++++++++++++++++++-
.../jmap/change/MailboxChangeListenerTest.scala | 4 +-
5 files changed, 121 insertions(+), 21 deletions(-)
diff --git a/server/container/guice/protocols/jmap/src/main/java/org/apache/james/jmap/draft/JmapGuiceProbe.java b/server/container/guice/protocols/jmap/src/main/java/org/apache/james/jmap/draft/JmapGuiceProbe.java
index c6d19fb..9c9affe 100644
--- a/server/container/guice/protocols/jmap/src/main/java/org/apache/james/jmap/draft/JmapGuiceProbe.java
+++ b/server/container/guice/protocols/jmap/src/main/java/org/apache/james/jmap/draft/JmapGuiceProbe.java
@@ -27,6 +27,7 @@ import org.apache.james.core.Username;
import org.apache.james.jmap.JMAPServer;
import org.apache.james.jmap.api.change.MailboxChange;
import org.apache.james.jmap.api.change.MailboxChangeRepository;
+import org.apache.james.jmap.api.change.State;
import org.apache.james.jmap.api.model.AccountId;
import org.apache.james.jmap.api.projections.MessageFastViewProjection;
import org.apache.james.jmap.api.vacation.Vacation;
@@ -94,4 +95,8 @@ public class JmapGuiceProbe implements GuiceProbe {
public void saveMailboxChange(MailboxChange change) {
mailboxChangeRepository.save(change).block();
}
+
+ public State latestState(AccountId accountId) {
+ return mailboxChangeRepository.getLatestState(accountId).block();
+ }
}
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 cbd1deb..bbe718b 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
@@ -32,6 +32,7 @@ import org.apache.commons.lang3.NotImplementedException;
import org.apache.james.mailbox.model.MailboxId;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
public class MailboxChanges {
@@ -95,12 +96,24 @@ public class MailboxChanges {
return this;
}
- Set<MailboxId> createdTemp = new HashSet<>(created);
- Set<MailboxId> updatedTemp = new HashSet<>(updated);
Set<MailboxId> destroyedTemp = new HashSet<>(destroyed);
- createdTemp.addAll(change.getCreated());
- updatedTemp.addAll(change.getUpdated());
- destroyedTemp.addAll(change.getDestroyed());
+
+ Set<MailboxId> createdTemp = Sets.difference(
+ ImmutableSet.<MailboxId>builder()
+ .addAll(created)
+ .addAll(change.getCreated())
+ .build(),
+ ImmutableSet.copyOf(change.getDestroyed()));
+ Set<MailboxId> updatedTemp = Sets.difference(
+ ImmutableSet.<MailboxId>builder()
+ .addAll(updated)
+ .addAll(Sets.difference(ImmutableSet.copyOf(change.getUpdated()),
+ createdTemp))
+ .build(),
+ ImmutableSet.copyOf(change.getDestroyed()));
+ destroyedTemp.addAll(Sets.difference(
+ ImmutableSet.copyOf(change.getDestroyed()),
+ created));
if (createdTemp.size() + updatedTemp.size() + destroyedTemp.size() > limit.getValue()) {
hasMoreChanges = true;
@@ -109,9 +122,9 @@ public class MailboxChanges {
}
state = change.getState();
- this.created.addAll(change.getCreated());
- this.updated.addAll(change.getUpdated());
- this.destroyed.addAll(change.getDestroyed());
+ created = createdTemp;
+ updated = updatedTemp;
+ destroyed = destroyedTemp;
return this;
}
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 5be5163..36a64b5 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
@@ -353,14 +353,24 @@ public interface MailboxChangeRepositoryContract {
State.Factory stateFactory = stateFactory();
State referenceState = stateFactory.generate();
- MailboxChange oldState = MailboxChange.builder().accountId(ACCOUNT_ID).state(referenceState).date(DATE.minusHours(3)).created(ImmutableList.of(TestId.of(1))).build();
- MailboxChange change1 = MailboxChange.builder().accountId(ACCOUNT_ID).state(stateFactory.generate()).date(DATE.minusHours(2)).created(ImmutableList.of(TestId.of(2), TestId.of(3), TestId.of(4), TestId.of(5))).build();
+ MailboxChange oldState = MailboxChange.builder()
+ .accountId(ACCOUNT_ID)
+ .state(referenceState)
+ .date(DATE.minusHours(3))
+ .created(ImmutableList.of(TestId.of(1), TestId.of(9), TestId.of(10)))
+ .build();
+ MailboxChange change1 = MailboxChange.builder()
+ .accountId(ACCOUNT_ID)
+ .state(stateFactory.generate())
+ .date(DATE.minusHours(2))
+ .created(ImmutableList.of(TestId.of(2), TestId.of(3), TestId.of(4), TestId.of(5)))
+ .build();
MailboxChange change2 = MailboxChange.builder()
.accountId(ACCOUNT_ID)
.state(stateFactory.generate())
.date(DATE.minusHours(1))
.created(ImmutableList.of(TestId.of(6), TestId.of(7)))
- .updated(ImmutableList.of(TestId.of(2), TestId.of(3)))
+ .updated(ImmutableList.of(TestId.of(2), TestId.of(3), TestId.of(9)))
.destroyed(ImmutableList.of(TestId.of(4))).build();
MailboxChange change3 = MailboxChange.builder()
.accountId(ACCOUNT_ID)
@@ -368,7 +378,7 @@ public interface MailboxChangeRepositoryContract {
.date(DATE)
.created(ImmutableList.of(TestId.of(8)))
.updated(ImmutableList.of(TestId.of(6), TestId.of(7)))
- .destroyed(ImmutableList.of(TestId.of(5))).build();
+ .destroyed(ImmutableList.of(TestId.of(5), TestId.of(10))).build();
repository.save(oldState).block();
repository.save(change1).block();
@@ -378,9 +388,9 @@ public interface MailboxChangeRepositoryContract {
MailboxChanges mailboxChanges = repository.getSinceState(ACCOUNT_ID, referenceState, Optional.of(Limit.of(20))).block();
SoftAssertions.assertSoftly(softly -> {
- softly.assertThat(mailboxChanges.getCreated()).containsExactlyInAnyOrder(TestId.of(2), TestId.of(3), TestId.of(4), TestId.of(5), TestId.of(6), TestId.of(7), TestId.of(8));
- softly.assertThat(mailboxChanges.getUpdated()).containsExactlyInAnyOrder(TestId.of(2), TestId.of(3), TestId.of(6), TestId.of(7));
- softly.assertThat(mailboxChanges.getDestroyed()).containsExactlyInAnyOrder(TestId.of(4), TestId.of(5));
+ softly.assertThat(mailboxChanges.getCreated()).containsExactlyInAnyOrder(TestId.of(2), TestId.of(3), TestId.of(6), TestId.of(7), TestId.of(8));
+ softly.assertThat(mailboxChanges.getUpdated()).containsExactlyInAnyOrder(TestId.of(9));
+ softly.assertThat(mailboxChanges.getDestroyed()).containsExactlyInAnyOrder(TestId.of(10));
});
}
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/MailboxChangesMethodContract.scala b/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxChangesMethodContract.scala
index 5d9164a..46fd74f 100644
--- a/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxChangesMethodContract.scala
+++ b/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxChangesMethodContract.scala
@@ -1013,6 +1013,80 @@ trait MailboxChangesMethodContract {
provisionSystemMailboxes(server)
+ val path1 = MailboxPath.forUser(BOB, "mailbox1")
+ val mailboxId1: String = mailboxProbe
+ .createMailbox(path1)
+ .serialize
+ val path2 = MailboxPath.forUser(BOB, "mailbox2")
+ val mailboxId2: String = mailboxProbe
+ .createMailbox(path2)
+ .serialize
+ val path3 = MailboxPath.forUser(BOB, "mailbox3")
+ val oldState: State = server.getProbe(classOf[JmapGuiceProbe]).latestState(AccountId.fromUsername(BOB))
+
+ val mailboxId3: String = mailboxProbe
+ .createMailbox(path3)
+ .serialize
+ val message: Message = Message.Builder
+ .of
+ .setSubject("test")
+ .setBody("testmail", StandardCharsets.UTF_8)
+ .build
+ mailboxProbe.appendMessage(BOB.asString(), path1, AppendCommand.from(message))
+
+ server.getProbe(classOf[MailboxProbeImpl])
+ .deleteMailbox(path1.getNamespace, BOB.asString(), path2.getName)
+
+ val request =
+ s"""{
+ | "using": ["urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail"],
+ | "methodCalls": [[
+ | "Mailbox/changes",
+ | {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "sinceState": "${oldState.getValue}"
+ | },
+ | "c1"]]
+ |}""".stripMargin
+
+ 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)
+ .whenIgnoringPaths("methodResponses[0][1].newState")
+ .withOptions(new Options(IGNORING_ARRAY_ORDER))
+ .isEqualTo(
+ s"""{
+ | "sessionState": "${SESSION_STATE.value}",
+ | "methodResponses": [
+ | [ "Mailbox/changes", {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "oldState": "${oldState.getValue}",
+ | "hasMoreChanges": false,
+ | "updatedProperties": [],
+ | "created": ["$mailboxId3"],
+ | "updated": ["$mailboxId1"],
+ | "destroyed": ["$mailboxId2"]
+ | }, "c1"]
+ | ]
+ |}""".stripMargin)
+ }
+
+ @Test
+ def returnedIdsShouldNotReturnDuplicatesAccrossCreatedUpdatedOrDestroyed(server: GuiceJamesServer): Unit = {
+ val mailboxProbe: MailboxProbeImpl = server.getProbe(classOf[MailboxProbeImpl])
+
+ provisionSystemMailboxes(server)
+
val oldState: State = storeReferenceState(server, BOB)
val path1 = MailboxPath.forUser(BOB, "mailbox1")
@@ -1072,9 +1146,9 @@ trait MailboxChangesMethodContract {
| "oldState": "${oldState.getValue}",
| "hasMoreChanges": false,
| "updatedProperties": [],
- | "created": ["$mailboxId1", "$mailboxId2"],
- | "updated": ["$mailboxId1", "$mailboxId2"],
- | "destroyed": ["$mailboxId1"]
+ | "created": ["$mailboxId2"],
+ | "updated": [],
+ | "destroyed": []
| }, "c1"]
| ]
|}""".stripMargin)
diff --git a/server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/change/MailboxChangeListenerTest.scala b/server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/change/MailboxChangeListenerTest.scala
index 32d4be2..7c0c39d 100644
--- a/server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/change/MailboxChangeListenerTest.scala
+++ b/server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/change/MailboxChangeListenerTest.scala
@@ -101,12 +101,10 @@ class MailboxChangeListenerTest {
@Test
def updateMailboxACLShouldStoreUpdatedEvent(): Unit = {
- val state = stateFactory.generate()
- repository.save(MailboxChange.builder().accountId(ACCOUNT_ID).state(state).date(ZonedDateTime.now).created(List[MailboxId](TestId.of(0)).asJava).build).block()
-
val mailboxSession = MailboxSessionUtil.create(BOB)
val path = MailboxPath.inbox(BOB)
val inboxId: MailboxId = mailboxManager.createMailbox(MailboxPath.inbox(BOB), mailboxSession).get
+ val state: State = repository.getLatestState(ACCOUNT_ID).block()
mailboxManager.applyRightsCommand(path, MailboxACL.command().forUser(ALICE).rights(MailboxACL.Right.Read).asAddition(), mailboxSession)
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org