chibenwa commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r620953442
##########
File path:
server/protocols/jmap-rfc-8621-integration-tests/distributed-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/distributed/DistributedAuthenticationTest.java
##########
@@ -51,7 +52,8 @@
.extension(new RabbitMQExtension())
.extension(new AwsS3BlobStoreExtension())
.server(configuration ->
CassandraRabbitMQJamesServerMain.createServer(configuration)
- .overrideWith(new TestJMAPServerModule()))
+ .overrideWith(new TestJMAPServerModule())
+ .overrideWith(new TypeNameModule()))
Review comment:
Why do we need to depend on a module of an unrelated contract (here and
in other places?)
##########
File path:
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/CustomMethodContract.scala
##########
@@ -179,6 +250,80 @@ trait CustomMethodContract {
.build
}
+ @Test
+ def shouldSupportCustomTypeNameWithIntStateFromString(server:
GuiceJamesServer): Unit = {
Review comment:
I cannot see the difference between
shouldSupportCustomTypeNameWithIntStateFromString and
shouldSupportCustomTypeNameWithIntStateFromInt...
##########
File path:
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala
##########
@@ -19,9 +19,6 @@
package org.apache.james.jmap.rfc8621.contract
-import java.nio.charset.StandardCharsets
Review comment:
Please do not change the import order
##########
File path:
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/CustomMethodContract.scala
##########
@@ -154,6 +174,31 @@ class CustomMethodModule extends AbstractModule {
Multibinder.newSetBinder(binder(), classOf[Method])
.addBinding()
.to(classOf[CustomMethod])
+ Multibinder.newSetBinder(binder(), classOf[TypeName])
+ .addBinding()
+ .toInstance(CustomTypeName)
+ Multibinder.newSetBinder(binder(), classOf[GuiceProbe])
+ .addBinding()
+ .to(classOf[JmapEventBusProbe])
+ }
+}
+
+class TypeNameModule extends AbstractModule {
Review comment:
This TypeName module should be part of `RFC8621MethodsModule` ? (At
least the generic parts?)
##########
File path:
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailSetMethodContract.scala
##########
@@ -18,27 +18,19 @@
****************************************************************/
package org.apache.james.jmap.rfc8621.contract
-import java.io.ByteArrayInputStream
Review comment:
Please do not change the import order
##########
File path:
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/WebSocketContract.scala
##########
@@ -18,14 +18,11 @@
****************************************************************/
package org.apache.james.jmap.rfc8621.contract
-import java.net.{ProtocolException, URI}
Review comment:
Please do not change the import order
##########
File path:
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailSubmissionSetMethodContract.scala
##########
@@ -19,9 +19,6 @@
package org.apache.james.jmap.rfc8621.contract
-import java.nio.charset.StandardCharsets
Review comment:
Please do not change the import order
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/StateChange.scala
##########
@@ -80,29 +100,21 @@ case class TypeState(changes: Map[TypeName, State]) {
case class StateChangeEvent(eventId: EventId,
username: Username,
- vacationResponseState: Option[State],
- mailboxState: Option[State],
- emailState: Option[State],
- emailDeliveryState: Option[State]) extends Event {
- def asStateChange: StateChange =
+ map: Map[TypeName, State]) extends Event {
+ def asStateChange: StateChange = {
StateChange(Map(AccountId.from(username).fold(
failure => throw new IllegalArgumentException(failure),
success => success) ->
- TypeState(
- VacationResponseTypeName.asMap(vacationResponseState) ++
- MailboxTypeName.asMap(mailboxState) ++
- EmailDeliveryTypeName.asMap(emailDeliveryState) ++
- EmailTypeName.asMap(emailState))),
+ TypeState(map)),
PushState.fromOption(
- mailboxState.map(state => JavaState.of(state.value)),
- emailState.map(state => JavaState.of(state.value))))
+ map.find(element =>
element._1.asString().equals("Mailbox")).map(element => element._2),
Review comment:
Use :
```
case class StateChangeEvent {
def getState(typeName: TypeName): Option[State] = ???
}
```
Here too...
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/TypeStateFactory.scala
##########
@@ -0,0 +1,15 @@
+package org.apache.james.jmap.change
+
+import javax.inject.Inject
+import scala.jdk.CollectionConverters._
+
+case class TypeStateFactory @Inject()(setTypeName: java.util.Set[TypeName]) {
+ val all: scala.collection.mutable.Set[TypeName] = setTypeName.asScala
+
+ def parse(string: String): Either[IllegalArgumentException, TypeName] =
+ all.flatMap(typeName => typeName.parse(string))
Review comment:
```suggestion
all.flatMap(_.parse(string))
```
##########
File path:
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/VacationResponseGetMethodContract.scala
##########
@@ -19,8 +19,6 @@
package org.apache.james.jmap.rfc8621.contract
-import java.time.ZonedDateTime
Review comment:
Please do not change the import order
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/VacationResponseSetMethod.scala
##########
@@ -88,11 +88,8 @@ class VacationResponseSetMethod
@Inject()(@Named(InjectionKeys.JMAP) eventBus: E
.map(updateResult => createResponse(invocation.invocation, request,
updateResult))
.flatMap(next => {
val event = StateChangeEvent(eventId = EventId.random(),
- mailboxState = None,
- emailState = None,
- emailDeliveryState = None,
username = mailboxSession.getUser,
- vacationResponseState = Some(State(UUID.randomUUID())))
+ map = Map(VacationResponseTypeName -> UuidState(UUID.randomUUID())))
Review comment:
Shouldn't we have `UuidState.generate()` ?
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/JmapEventSerializer.scala
##########
@@ -42,10 +41,10 @@ object StateChangeEventDTO {
getType = classOf[StateChangeEvent].getCanonicalName,
getEventId = event.eventId.getId.toString,
getUsername = event.username.asString(),
- getMailboxState = event.mailboxState.map(_.value).map(_.toString).toJava,
- getEmailState = event.emailState.map(_.value).map(_.toString).toJava,
- getVacationResponseState =
event.vacationResponseState.map(_.value).map(_.toString).toJava,
- getEmailDeliveryState =
event.emailDeliveryState.map(_.value).map(_.toString).toJava)
+ getMailboxState = event.map.find(element =>
element._1.asString().equals("Mailbox")).map(element =>
element._2.serialize).toJava,
Review comment:
How about adding the method:
```
case class StateChangeEvent {
def getState(typeName: TypeName): Option[State] = ???
}
```
##########
File path:
server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/change/TypeStateFactoryTest.scala
##########
@@ -0,0 +1,81 @@
+package org.apache.james.jmap.change
+
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.jupiter.api.Test
+
+import scala.jdk.CollectionConverters._
+
+class TypeStateFactoryTest {
+ val ALL: Set[TypeName] = Set(EmailTypeName, MailboxTypeName, ThreadTypeName,
IdentityTypeName, EmailSubmissionTypeName, EmailDeliveryTypeName)
+ val factory: TypeStateFactory = TypeStateFactory(ALL.asJava)
+
+ @Test
+ def parseEmailTypeNameStringShouldReturnRightEmailTypeName(): Unit = {
+ assertThat(factory.parse("Email")).isEqualTo(Right(EmailTypeName))
+ }
+
+ @Test
+ def parseMailboxTypeNameStringShouldReturnRightMailboxTypeName(): Unit = {
+ assertThat(factory.parse("Mailbox")).isEqualTo(Right(MailboxTypeName))
+ }
+
+ @Test
+ def parseThreadTypeNameStringShouldReturnRightThreadTypeName(): Unit = {
+ assertThat(factory.parse("Thread")).isEqualTo(Right(ThreadTypeName))
+ }
+
+ @Test
+ def parseIdentityTypeNameStringShouldReturnRightIdentityTypeName(): Unit = {
+ assertThat(factory.parse("Identity")).isEqualTo(Right(IdentityTypeName))
+ }
+
+ @Test
+ def
parseEmailSubmissionTypeNameStringShouldReturnRightEmailSubmissionTypeName():
Unit = {
+
assertThat(factory.parse("EmailSubmission")).isEqualTo(Right(EmailSubmissionTypeName))
+ }
+
+ @Test
+ def
parseEmailDeliveryTypeNameStringShouldReturnRightEmailDeliveryTypeName(): Unit
= {
+
assertThat(factory.parse("EmailDelivery")).isEqualTo(Right(EmailDeliveryTypeName))
+ }
+
+ @Test
+ def parseWrongTypeNameStringShouldReturnLeftIllegalArgumentException(): Unit
= {
+ assertThat(factory.parse("email").fold(
Review comment:
You can do asserts `.isInstanceOf(classOf[Left])` then you can drop all
the calls to .fold
Also please consider using SoftAssertions....
##########
File path:
server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/change/StateChangeListenerTest.scala
##########
@@ -31,25 +30,23 @@ import reactor.core.scala.publisher.SMono
import reactor.core.scheduler.Schedulers
class StateChangeListenerTest {
- private val mailboxState =
State.fromStringUnchecked("2f9f1b12-b35a-43e6-9af2-0106fb53a943")
- private val emailState =
State.fromStringUnchecked("2d9f1b12-b35a-43e6-9af2-0106fb53a943")
+ private val mailboxState =
UuidState.fromStringUnchecked("2f9f1b12-b35a-43e6-9af2-0106fb53a943")
+ private val emailState =
UuidState.fromStringUnchecked("2d9f1b12-b35a-43e6-9af2-0106fb53a943")
private val eventId = EventId.of("6e0dd59d-660e-4d9b-b22f-0354479f47b4")
@Test
def reactiveEventShouldSendAnOutboundMessage(): Unit = {
val sink: Sinks.Many[OutboundMessage] =
Sinks.many().unicast().onBackpressureBuffer()
val event = StateChangeEvent(eventId = eventId,
username = Username.of("bob"),
- mailboxState = Some(mailboxState),
- emailState = Some(emailState),
- vacationResponseState = None,
- emailDeliveryState = None)
+ map = (Map(MailboxTypeName -> mailboxState) ++
+ Map(EmailTypeName -> emailState)).toMap)
Review comment:
Single call to Map
##########
File path:
server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/change/StateChangeListenerTest.scala
##########
@@ -61,16 +58,14 @@ class StateChangeListenerTest {
val sink: Sinks.Many[OutboundMessage] =
Sinks.many().unicast().onBackpressureBuffer()
val event = StateChangeEvent(eventId = eventId,
username = Username.of("bob"),
- mailboxState = Some(mailboxState),
- emailState = Some(emailState),
- vacationResponseState = None,
- emailDeliveryState = None)
+ map = (Map(MailboxTypeName -> mailboxState) ++
+ Map(EmailTypeName -> emailState)).toMap)
Review comment:
Single call to Map
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/WebSocketRoutes.scala
##########
@@ -50,6 +45,11 @@ import reactor.core.scheduler.Schedulers
import reactor.netty.http.server.{HttpServerRequest, HttpServerResponse}
import reactor.netty.http.websocket.{WebsocketInbound, WebsocketOutbound}
+import java.nio.charset.StandardCharsets
Review comment:
Order import order
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/TypeStateFactory.scala
##########
@@ -0,0 +1,15 @@
+package org.apache.james.jmap.change
+
+import javax.inject.Inject
+import scala.jdk.CollectionConverters._
+
+case class TypeStateFactory @Inject()(setTypeName: java.util.Set[TypeName]) {
+ val all: scala.collection.mutable.Set[TypeName] = setTypeName.asScala
+
+ def parse(string: String): Either[IllegalArgumentException, TypeName] =
+ all.flatMap(typeName => typeName.parse(string))
+ .headOption
+ .map(typeName => Right(typeName))
Review comment:
```suggestion
.map(Right(_))
```
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/EventSourceRoutes.scala
##########
@@ -49,12 +44,16 @@ import reactor.core.scala.publisher.{SFlux, SMono}
import reactor.core.scheduler.Schedulers
import reactor.netty.http.server.{HttpServerRequest, HttpServerResponse}
+import java.nio.charset.StandardCharsets
Review comment:
Order import
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/json/EmailGetSerializer.scala
##########
@@ -22,9 +22,9 @@ package org.apache.james.jmap.json
import eu.timepit.refined
import org.apache.james.jmap.api.model.Preview
import org.apache.james.jmap.core.Id.IdConstraint
-import org.apache.james.jmap.core.{Properties, State}
+import org.apache.james.jmap.core.{Properties, UuidState}
import org.apache.james.jmap.mail.Email.Size
-import org.apache.james.jmap.mail.{AddressesHeaderValue, BlobId, Charset,
DateHeaderValue, Disposition, EmailAddress, EmailAddressGroup, EmailBody,
EmailBodyMetadata, EmailBodyPart, EmailBodyValue, EmailChangesRequest,
EmailChangesResponse, EmailFastView, EmailFullView, EmailGetRequest,
EmailGetResponse, EmailHeader, EmailHeaderName, EmailHeaderValue,
EmailHeaderView, EmailHeaders, EmailIds, EmailMetadata, EmailMetadataView,
EmailNotFound, EmailView, EmailerName, FetchAllBodyValues, FetchHTMLBodyValues,
FetchTextBodyValues, GroupName, GroupedAddressesHeaderValue, HasAttachment,
HeaderMessageId, HeaderURL, IsEncodingProblem, IsTruncated, Keyword, Keywords,
Language, Languages, Location, MailboxIds, MessageIdsHeaderValue, Name, PartId,
RawHeaderValue, Subject, TextHeaderValue, ThreadId, Type, URLsHeaderValue,
UnparsedEmailId}
+import org.apache.james.jmap.mail._
Review comment:
Do not use a WILDCARD....
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/json/EmailSetSerializer.scala
##########
@@ -24,15 +24,14 @@ import eu.timepit.refined
import eu.timepit.refined.collection.NonEmpty
import eu.timepit.refined.refineV
import eu.timepit.refined.types.string.NonEmptyString
-import javax.inject.Inject
import org.apache.james.jmap.core.Id.IdConstraint
-import org.apache.james.jmap.core.{Id, SetError, State, UTCDate}
+import org.apache.james.jmap.core.{Id, SetError, UTCDate, UuidState}
import org.apache.james.jmap.mail.KeywordsFactory.STRICT_KEYWORDS_FACTORY
-import org.apache.james.jmap.mail.{AddressesHeaderValue, AsAddresses, AsDate,
AsGroupedAddresses, AsMessageIds, AsRaw, AsText, AsURLs, Attachment, BlobId,
Charset, ClientBody, ClientCid, ClientEmailBodyValue, ClientPartId,
DateHeaderValue, DestroyIds, Disposition, EmailAddress, EmailAddressGroup,
EmailCreationId, EmailCreationRequest, EmailCreationResponse, EmailHeader,
EmailHeaderName, EmailHeaderValue, EmailSetRequest, EmailSetResponse,
EmailSetUpdate, EmailerName, GroupName, GroupedAddressesHeaderValue,
HeaderMessageId, HeaderURL, IsEncodingProblem, IsTruncated, Keyword, Keywords,
Language, Languages, Location, MailboxIds, MessageIdsHeaderValue, Name,
ParseOption, RawHeaderValue, SpecificHeaderRequest, Subject, TextHeaderValue,
Type, URLsHeaderValue, UnparsedMessageId}
-import org.apache.james.jmap.mail.{AddressesHeaderValue, AsAddresses, AsDate,
AsGroupedAddresses, AsMessageIds, AsRaw, AsText, AsURLs, Attachment, BlobId,
Charset, ClientBody, ClientCid, ClientEmailBodyValue, ClientPartId,
DateHeaderValue, DestroyIds, Disposition, EmailAddress, EmailAddressGroup,
EmailCreationRequest, EmailCreationResponse, EmailHeader, EmailHeaderName,
EmailHeaderValue, EmailImport, EmailImportRequest, EmailImportResponse,
EmailSetRequest, EmailSetResponse, EmailSetUpdate, EmailerName, GroupName,
GroupedAddressesHeaderValue, HeaderMessageId, HeaderURL, IsEncodingProblem,
IsTruncated, Keyword, Keywords, Language, Languages, Location, MailboxIds,
MessageIdsHeaderValue, Name, ParseOption, RawHeaderValue,
SpecificHeaderRequest, Subject, TextHeaderValue, Type, URLsHeaderValue}
+import org.apache.james.jmap.mail._
Review comment:
Do not use a WILDCARD....
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/json/PushSerializer.scala
##########
@@ -0,0 +1,120 @@
+/****************************************************************
+ * 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.json
+
+import org.apache.james.jmap.change.{TypeName, TypeState, TypeStateFactory}
+import org.apache.james.jmap.core.{AccountId, OutboundMessage, PingMessage,
PushState, RequestId, State, StateChange, WebSocketError,
WebSocketInboundMessage, WebSocketPushDisable, WebSocketPushEnable,
WebSocketRequest, WebSocketResponse}
+import play.api.libs.json.{Format, JsError, JsNull, JsObject, JsResult,
JsString, JsSuccess, JsValue, Json, OWrites, Reads, Writes}
+
+import javax.inject.Inject
+import scala.util.Try
+
+case class PushSerializer @Inject()(typeStateFactory: TypeStateFactory) {
+ private implicit val stateWrites: Writes[State] = new Writes[State] {
+ def writes(state: State) = JsString(state.serialize)
+ }
Review comment:
```suggestion
private implicit val stateWrites: Writes[State] = state =>
JsString(state.serialize)
```
##########
File path:
server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/change/StateChangeEventSerializerTest.scala
##########
@@ -34,10 +34,10 @@ object StateChangeEventSerializerTest {
val USERNAME: Username = Username.of("bob")
val EVENT: StateChangeEvent = StateChangeEvent(eventId = EVENT_ID,
username = USERNAME,
- mailboxState = Some(State.INSTANCE),
- emailState =
Some(State.fromStringUnchecked("2d9f1b12-b35a-43e6-9af2-0106fb53a943")),
- vacationResponseState =
Some(State.fromStringUnchecked("2d9f1b12-3333-4444-5555-0106fb53a943")),
- emailDeliveryState =
Some(State.fromStringUnchecked("2d9f1b12-0000-1111-3333-0106fb53a943")))
+ map = (Map(MailboxTypeName -> UuidState.INSTANCE) ++
Review comment:
Please do a single call to `Map(...)`
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/EventSourceRoutes.scala
##########
@@ -69,11 +68,11 @@ object EventSourceOptions {
private def retrieveTypes(request: HttpServerRequest):
Either[IllegalArgumentException, Set[TypeName]] =
queryParam(request, "types") match {
case None => Left(new IllegalArgumentException("types parameter is
compulsory"))
- case Some(List("*")) => Right(TypeName.ALL)
+ case Some(List("*")) => Right(typeStateFactory.all.toSet)
case Some(list) => list.flatMap(_.split(","))
.map(string =>
- TypeName.parse(string)
- .left.map(errorMessage => new
IllegalArgumentException(errorMessage)))
+ typeStateFactory.parse(string)
+ .left.map(throwable => throwable))
Review comment:
You can remove `.left.map(throwable => throwable)` (it does nothing)
##########
File path:
server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/change/StateChangeEventSerializerTest.scala
##########
@@ -50,10 +50,8 @@ object StateChangeEventSerializerTest {
|}""".stripMargin
val EVENT_NO_DELIVERY: StateChangeEvent = StateChangeEvent(eventId =
EVENT_ID,
username = USERNAME,
- mailboxState = Some(State.INSTANCE),
- emailState =
Some(State.fromStringUnchecked("2d9f1b12-b35a-43e6-9af2-0106fb53a943")),
- emailDeliveryState = None,
- vacationResponseState = None)
+ map = (Map(MailboxTypeName -> UuidState.INSTANCE) ++
Review comment:
Single call to `Map` ?
##########
File path:
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/core/WebSocketTransport.scala
##########
@@ -19,13 +19,12 @@
package org.apache.james.jmap.core
-import java.nio.charset.StandardCharsets
Review comment:
Please revert these import changes...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
