This is an automated email from the ASF dual-hosted git repository. btellier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 2363804ea32addf6a1129bcb3a427558964268bb Author: duc91 <[email protected]> AuthorDate: Wed Aug 12 16:42:13 2020 +0700 JAMES-3354: handle parentId not found --- .../DistributedMailboxSetMethodTest.java | 8 +++ .../contract/MailboxSetMethodContract.scala | 64 +++++++++++++++++- .../rfc8621/memory/MemoryMailboxSetMethodTest.java | 9 +++ .../org/apache/james/jmap/mail/MailboxSet.scala | 9 +++ .../james/jmap/method/MailboxSetMethod.scala | 75 ++++++++++++++++------ 5 files changed, 144 insertions(+), 21 deletions(-) diff --git a/server/protocols/jmap-rfc-8621-integration-tests/distributed-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/distributed/DistributedMailboxSetMethodTest.java b/server/protocols/jmap-rfc-8621-integration-tests/distributed-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/distributed/DistributedMailboxSetMethodTest.java index 60e7f40..e2842c1 100644 --- a/server/protocols/jmap-rfc-8621-integration-tests/distributed-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/distributed/DistributedMailboxSetMethodTest.java +++ b/server/protocols/jmap-rfc-8621-integration-tests/distributed-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/distributed/DistributedMailboxSetMethodTest.java @@ -26,12 +26,16 @@ import org.apache.james.DockerElasticSearchExtension; import org.apache.james.JamesServerBuilder; import org.apache.james.JamesServerExtension; import org.apache.james.jmap.rfc8621.contract.MailboxSetMethodContract; +import org.apache.james.mailbox.cassandra.ids.CassandraId; +import org.apache.james.mailbox.model.MailboxId; import org.apache.james.modules.AwsS3BlobStoreExtension; import org.apache.james.modules.RabbitMQExtension; import org.apache.james.modules.TestJMAPServerModule; import org.apache.james.modules.blobstore.BlobStoreConfiguration; import org.junit.jupiter.api.extension.RegisterExtension; +import com.datastax.driver.core.utils.UUIDs; + public class DistributedMailboxSetMethodTest implements MailboxSetMethodContract { @RegisterExtension static JamesServerExtension testExtension = new JamesServerBuilder<CassandraRabbitMQJamesConfiguration>(tmpDir -> @@ -51,4 +55,8 @@ public class DistributedMailboxSetMethodTest implements MailboxSetMethodContract .overrideWith(new TestJMAPServerModule())) .build(); + @Override + public MailboxId randomMailboxId() { + return CassandraId.of(UUIDs.timeBased()); + } } 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/MailboxSetMethodContract.scala b/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala index 7e978bc..8fe321b 100644 --- a/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala +++ b/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala @@ -26,7 +26,8 @@ import net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson import org.apache.http.HttpStatus.SC_OK import org.apache.james.GuiceJamesServer import org.apache.james.jmap.http.UserCredential -import org.apache.james.jmap.rfc8621.contract.Fixture.{ACCEPT_RFC8621_VERSION_HEADER, BOB, BOB_PASSWORD, DOMAIN, authScheme, baseRequestSpecBuilder} +import org.apache.james.jmap.rfc8621.contract.Fixture._ +import org.apache.james.mailbox.model.{MailboxId, MailboxPath} import org.apache.james.modules.MailboxProbeImpl import org.apache.james.utils.DataProbeImpl import org.assertj.core.api.Assertions @@ -46,6 +47,8 @@ trait MailboxSetMethodContract { .build } + def randomMailboxId: MailboxId + @Test def mailboxSetShouldReturnNotCreatedWhenNameIsMissing(): Unit = { val request= @@ -428,11 +431,66 @@ trait MailboxSetMethodContract { .log().ifValidationFails() .statusCode(SC_OK) .contentType(JSON) + + Assertions.assertThatCode(() => server.getProbe(classOf[MailboxProbeImpl]) + .getMailboxId("#private", BOB.asString(), "parentMailbox.childMailbox")).doesNotThrowAnyException() + } + + @Test + def mailboxSetShouldNotCreateMailboxWhenParentIdNotFound(server: GuiceJamesServer): Unit = { + val mailboxId: MailboxId = randomMailboxId + val request= + s""" + |{ + | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail" ], + | "methodCalls": [ + | [ + | "Mailbox/set", + | { + | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6", + | "create": { + | "C42": { + | "name": "childMailbox", + | "parentId":"${mailboxId.serialize}" + | } + | } + | }, + | "c1" + | ] + | ] + |} + |""".stripMargin + + val response = `given` + .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER) + .body(request) + .when + .post + .`then` + .log().ifValidationFails() + .statusCode(SC_OK) + .contentType(JSON) .extract .body .asString - Assertions.assertThatCode(() => server.getProbe(classOf[MailboxProbeImpl]) - .getMailboxId("#private", BOB.asString(), "parentMailbox.childMailbox")).doesNotThrowAnyException() + assertThatJson(response).isEqualTo( + s"""{ + | "sessionState": "75128aab4b1b", + | "methodResponses": [[ + | "Mailbox/set", + | { + | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6", + | "newState": "000001", + | "notCreated": { + | "C42": { + | "type": "invalidArguments", + | "description": "${mailboxId.serialize()} can not be found", + | "properties":{"value":["parentId"]} + | } + | } + | }, + | "c1"]] + |}""".stripMargin) } } diff --git a/server/protocols/jmap-rfc-8621-integration-tests/memory-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/memory/MemoryMailboxSetMethodTest.java b/server/protocols/jmap-rfc-8621-integration-tests/memory-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/memory/MemoryMailboxSetMethodTest.java index 876c7ed..519b119 100644 --- a/server/protocols/jmap-rfc-8621-integration-tests/memory-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/memory/MemoryMailboxSetMethodTest.java +++ b/server/protocols/jmap-rfc-8621-integration-tests/memory-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/memory/MemoryMailboxSetMethodTest.java @@ -21,10 +21,14 @@ package org.apache.james.jmap.rfc8621.memory; import static org.apache.james.MemoryJamesServerMain.IN_MEMORY_SERVER_AGGREGATE_MODULE; +import java.util.concurrent.ThreadLocalRandom; + import org.apache.james.GuiceJamesServer; import org.apache.james.JamesServerBuilder; import org.apache.james.JamesServerExtension; import org.apache.james.jmap.rfc8621.contract.MailboxSetMethodContract; +import org.apache.james.mailbox.inmemory.InMemoryId; +import org.apache.james.mailbox.model.MailboxId; import org.apache.james.modules.TestJMAPServerModule; import org.junit.jupiter.api.extension.RegisterExtension; @@ -35,4 +39,9 @@ public class MemoryMailboxSetMethodTest implements MailboxSetMethodContract { .combineWith(IN_MEMORY_SERVER_AGGREGATE_MODULE) .overrideWith(new TestJMAPServerModule())) .build(); + + @Override + public MailboxId randomMailboxId() { + return InMemoryId.of(ThreadLocalRandom.current().nextInt(100000) + 100); + } } diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxSet.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxSet.scala index 07517dc..bd9983a 100644 --- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxSet.scala +++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxSet.scala @@ -20,6 +20,7 @@ package org.apache.james.jmap.mail import eu.timepit.refined.api.Refined +import eu.timepit.refined.auto._ import eu.timepit.refined.collection.NonEmpty import org.apache.james.jmap.mail.MailboxName.MailboxName import org.apache.james.jmap.mail.MailboxSetRequest.MailboxCreationId @@ -55,6 +56,14 @@ case class MailboxSetResponse(accountId: AccountId, notUpdated: Option[Map[MailboxId, MailboxSetError]], notDestroyed: Option[Map[MailboxId, MailboxSetError]]) +object MailboxSetError { + val invalidArgumentValue: SetErrorType = "invalidArguments" + val serverFailValue: SetErrorType = "serverFail" + + def invalidArgument(description: Option[SetErrorDescription], properties: Option[Properties]) = MailboxSetError(invalidArgumentValue, description, properties) + def serverFail(description: Option[SetErrorDescription], properties: Option[Properties]) = MailboxSetError(serverFailValue, description, properties) +} + case class MailboxSetError(`type`: SetErrorType, description: Option[SetErrorDescription], properties: Option[Properties]) case class MailboxCreationResponse(id: MailboxId, diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MailboxSetMethod.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MailboxSetMethod.scala index ead1c2a..2f2ee59 100644 --- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MailboxSetMethod.scala +++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MailboxSetMethod.scala @@ -23,10 +23,11 @@ import eu.timepit.refined.auto._ import javax.inject.Inject import org.apache.james.jmap.json.Serializer import org.apache.james.jmap.mail.MailboxSetRequest.MailboxCreationId -import org.apache.james.jmap.mail.{IsSubscribed, MailboxCreationRequest, MailboxCreationResponse, MailboxRights, MailboxSetError, MailboxSetRequest, MailboxSetResponse, SetErrorDescription, TotalEmails, TotalThreads, UnreadEmails, UnreadThreads} +import org.apache.james.jmap.mail.{IsSubscribed, MailboxCreationRequest, MailboxCreationResponse, MailboxRights, MailboxSetError, MailboxSetRequest, MailboxSetResponse, Properties, SetErrorDescription, TotalEmails, TotalThreads, UnreadEmails, UnreadThreads} import org.apache.james.jmap.model.CapabilityIdentifier.CapabilityIdentifier import org.apache.james.jmap.model.Invocation.{Arguments, MethodName} import org.apache.james.jmap.model.{Invocation, State} +import org.apache.james.mailbox.exception.MailboxNotFoundException import org.apache.james.mailbox.model.{MailboxId, MailboxPath} import org.apache.james.mailbox.{MailboxManager, MailboxSession} import org.apache.james.metrics.api.MetricFactory @@ -37,6 +38,36 @@ import reactor.core.scheduler.Schedulers import scala.collection.immutable +sealed trait CreationResult { + def mailboxCreationId: MailboxCreationId +} + +case class CreationSuccess(mailboxCreationId: MailboxCreationId, mailboxId: MailboxId) extends CreationResult +case class CreationFailure(mailboxCreationId: MailboxCreationId, exception: Exception) extends CreationResult { + def asMailboxSetError: MailboxSetError = exception match { + case e: MailboxNotFoundException => MailboxSetError.invalidArgument(Some(SetErrorDescription(e.getMessage)), Some(Properties(List("parentId")))) + case _ => MailboxSetError.serverFail(Some(SetErrorDescription(exception.getMessage)), None) + } +} + +case class CreationResults(created: Seq[CreationResult]) { + def retrieveCreated: Map[MailboxCreationId, MailboxId] = created + .flatMap(result => result match { + case success: CreationSuccess => Some((success.mailboxCreationId, success.mailboxId)) + case _ => None + }) + .toSet + .toMap + + def retrieveErrors: Map[MailboxCreationId, MailboxSetError] = created + .flatMap(result => result match { + case failure: CreationFailure => Some((failure.mailboxCreationId, failure.asMailboxSetError)) + case _ => None + }) + .toSet + .toMap +} + class MailboxSetMethod @Inject() (serializer: Serializer, mailboxManager: MailboxManager, metricFactory: MetricFactory) extends Method { @@ -49,8 +80,8 @@ class MailboxSetMethod @Inject() (serializer: Serializer, .flatMap(mailboxSetRequest => { val (unparsableCreateRequests, createRequests) = parseCreateRequests(mailboxSetRequest) for { - created <- createMailboxes(mailboxSession, createRequests) - } yield createResponse(invocation, mailboxSetRequest, unparsableCreateRequests, created) + creationResults <- createMailboxes(mailboxSession, createRequests) + } yield createResponse(invocation, mailboxSetRequest, unparsableCreateRequests, creationResults) })) } @@ -70,34 +101,42 @@ class MailboxSetMethod @Inject() (serializer: Serializer, private def mailboxSetError(errors: collection.Seq[(JsPath, collection.Seq[JsonValidationError])]): MailboxSetError = errors.head match { - case (path, Seq()) => MailboxSetError("invalidArguments", Some(SetErrorDescription(s"'$path' property in mailbox object is not valid")), None) + case (path, Seq()) => MailboxSetError.invalidArgument(Some(SetErrorDescription(s"'$path' property in mailbox object is not valid")), None) case (path, Seq(JsonValidationError(Seq("error.path.missing")))) => MailboxSetError("invalidArguments", Some(SetErrorDescription(s"Missing '$path' property in mailbox object")), None) - case (path, _) => MailboxSetError("invalidArguments", Some(SetErrorDescription(s"Unknown error on property '$path'")), None) + case (path, _) => MailboxSetError.invalidArgument(Some(SetErrorDescription(s"Unknown error on property '$path'")), None) } - private def createMailboxes(mailboxSession: MailboxSession, createRequests: immutable.Iterable[(MailboxCreationId, MailboxCreationRequest)]): SMono[Map[MailboxCreationId, MailboxId]] = { + private def createMailboxes(mailboxSession: MailboxSession, createRequests: immutable.Iterable[(MailboxCreationId, MailboxCreationRequest)]): SMono[CreationResults] = { SFlux.fromIterable(createRequests).flatMap { case (mailboxCreationId: MailboxCreationId, mailboxCreationRequest: MailboxCreationRequest) => { SMono.fromCallable(() => { createMailbox(mailboxSession, mailboxCreationId, mailboxCreationRequest) }).subscribeOn(Schedulers.elastic()) } - }.collectMap(_._1, _._2) + } + .collectSeq() + .map(CreationResults) } - private def createMailbox(mailboxSession: MailboxSession, mailboxCreationId: MailboxCreationId, mailboxCreationRequest: MailboxCreationRequest) = { - val path:MailboxPath = if (mailboxCreationRequest.parentId.isEmpty) { - MailboxPath.forUser(mailboxSession.getUser, mailboxCreationRequest.name) - } else { - val parentId: MailboxId = mailboxCreationRequest.parentId.get - val parentPath: MailboxPath = mailboxManager.getMailbox(parentId, mailboxSession).getMailboxPath - parentPath.child(mailboxCreationRequest.name, mailboxSession.getPathDelimiter) + private def createMailbox(mailboxSession: MailboxSession, mailboxCreationId: MailboxCreationId, mailboxCreationRequest: MailboxCreationRequest): CreationResult = { + try { + val path: MailboxPath = if (mailboxCreationRequest.parentId.isEmpty) { + MailboxPath.forUser(mailboxSession.getUser, mailboxCreationRequest.name) + } else { + val parentId: MailboxId = mailboxCreationRequest.parentId.get + val parentPath: MailboxPath = mailboxManager.getMailbox(parentId, mailboxSession).getMailboxPath + parentPath.child(mailboxCreationRequest.name, mailboxSession.getPathDelimiter) + } + //can safely do a get as the Optional is empty only if the mailbox name is empty which is forbidden by the type constraint on MailboxName + CreationSuccess(mailboxCreationId, mailboxManager.createMailbox(path, mailboxSession).get()) + } catch { + case error: Exception => CreationFailure(mailboxCreationId, error) } - //can safely do a get as the Optional is empty only if the mailbox name is empty which is forbidden by the type constraint on MailboxName - (mailboxCreationId, mailboxManager.createMailbox(path, mailboxSession).get()) } - private def createResponse(invocation: Invocation, mailboxSetRequest: MailboxSetRequest, createErrors: immutable.Iterable[(MailboxCreationId, MailboxSetError)], created: Map[MailboxCreationId, MailboxId]): Invocation = { + private def createResponse(invocation: Invocation, mailboxSetRequest: MailboxSetRequest, unparsableCreateRequests: immutable.Iterable[(MailboxCreationId, MailboxSetError)], creationResults: CreationResults): Invocation = { + val created: Map[MailboxCreationId, MailboxId] = creationResults.retrieveCreated + Invocation(methodName, Arguments(serializer.serialize(MailboxSetResponse( mailboxSetRequest.accountId, oldState = None, @@ -116,7 +155,7 @@ class MailboxSetMethod @Inject() (serializer: Serializer, isSubscribed = IsSubscribed(true) )))).filter(_.nonEmpty), - notCreated = Some(createErrors.toMap).filter(_.nonEmpty), + notCreated = Some(unparsableCreateRequests.toMap ++ creationResults.retrieveErrors).filter(_.nonEmpty), updated = None, notUpdated = None, destroyed = None, --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
