You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2021/04/16 10:22:50 UTC
[GitHub] [james-project] quantranhong1999 opened a new pull request #391: JAMES-XXXX Modularize TypeState
quantranhong1999 opened a new pull request #391:
URL: https://github.com/apache/james-project/pull/391
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #391: JAMES-3581 Modularize TypeState
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-840252461
Giving a second thought about it I would really appreciate to have backward compatibility on the DTO deserialization with some sort of fallback.
Let me have a look at it this morning...
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael edited a comment on pull request #391: JAMES-3581 Modularize TypeState
Posted by GitBox <gi...@apache.org>.
Arsnael edited a comment on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-838215410
```
java.lang.NoClassDefFoundError: org/apache/james/utils/FailingPropertiesProvider
at org.apache.james.DefaultCassandraJamesServerTest.lambda$static$0(DefaultCassandraJamesServerTest.java:39)
Caused by: java.lang.ClassNotFoundException: org.apache.james.utils.FailingPropertiesProvider
at org.apache.james.DefaultCassandraJamesServerTest.lambda$static$0(DefaultCassandraJamesServerTest.java:39)
```
> Can someone restart the CI? I ran the failed test on local and it's green.
If you want but I find that stacktrace rather suspicious...
EDIT: I'm trying to double check, but first thing I see is that it's not rebased on latest master. The CI does it before building the project, and I would advice you to do the same when the CI reports an error like this that you can't reproduce locally. Sometimes it's the answer (but not always)
EDIT2: Could not reproduce locally either... Let's wait and see the result of this new CI build...
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-836567903
I dropped the default method commit, rebased to squash commits and solve conflicts.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r629211220
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/StateChange.scala
##########
@@ -31,7 +31,7 @@ trait TypeName {
def asString(): String
def parse(string: String): Option[TypeName]
- def parseState(string: String): Either[IllegalArgumentException, State]
+ def parseState(string: String): Either[IllegalArgumentException, State] = UuidState.parse(string)
Review comment:
My bad, I suggested that
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r629092067
##########
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 +229,88 @@ trait CustomMethodContract {
.build
}
+ @Test
+ def pushShouldSupportCustomTypeNameAndIntStateWithDataTypesAreMyTypeName(server: GuiceJamesServer): Unit = {
Review comment:
```suggestion
def pushShouldSupportCustomTypeNameAndIntStateWhenDataTypesAreMyTypeName(server: GuiceJamesServer): Unit = {
```
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/StateChange.scala
##########
@@ -44,10 +41,7 @@ case object MailboxTypeName extends TypeName {
case _ => None
}
- override def parseState(string: String): Either[IllegalArgumentException, UuidState] = Try(UUID.fromString(string))
- .toEither
- .map(UuidState(_))
- .left.map(new IllegalArgumentException(_))
+ override def parseState(string: String): Either[IllegalArgumentException, UuidState] = UuidState.parse(string)
Review comment:
You override this method with the same syntax in every of your subclasses... Why not defining this only once as a default method instead in your `TypeName` trait?
##########
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 +229,88 @@ trait CustomMethodContract {
.build
}
+ @Test
+ def pushShouldSupportCustomTypeNameAndIntStateWithDataTypesAreMyTypeName(server: GuiceJamesServer): Unit = {
+ val accountId: AccountId = AccountId.fromUsername(BOB)
+ val intState = IntState.fromString("1")
+ val stateChangeEvent: StateChangeEvent = StateChangeEvent(eventId = CustomMethodContract.eventId, username = BOB, map = Map(CustomTypeName -> intState))
+ Thread.sleep(100)
+
+ val response: Either[String, String] =
+ authenticatedRequest(server)
+ .response(asWebSocket[Identity, String] {
+ ws =>
+ ws.send(WebSocketFrame.text(
+ """{
+ | "@type": "WebSocketPushEnable",
+ | "dataTypes": ["MyTypeName"]
+ |}""".stripMargin))
+
+ Thread.sleep(100)
+
+ server.getProbe(classOf[JmapEventBusProbe])
+ .emitStateChange(stateChangeEvent, accountId)
+
+ ws.receive()
+ .map { case t: Text => t.payload }
+ })
+ .send(backend)
+ .body
+
+ Thread.sleep(100)
+
+ assertThatJson(response.toOption.get)
+ .isEqualTo("""{
+ | "@type": "StateChange",
+ | "changed": {
+ | "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6": {
+ | "MyTypeName": "1"
+ | }
+ | }
+ |}""".stripMargin)
+ }
+
+ @Test
+ def pushShouldSupportCustomTypeNameAndIntStateWithDataTypesAreNull(server: GuiceJamesServer): Unit = {
Review comment:
```suggestion
def pushShouldSupportCustomTypeNameAndIntStateWhenDataTypesAreNull(server: GuiceJamesServer): Unit = {
```
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-827305708
Still need to write more tests I think. Beside that, should I change PushState hashing from all kind of states instead of 2 states as now?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r614765575
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/TypeNameParserFactory.scala
##########
@@ -0,0 +1,13 @@
+package org.apache.james.jmap.change
+
+import javax.inject.Inject
+
+class TypeNameParserFactory @Inject() (setTypeName: Set[TypeName]) {
Review comment:
Injects needs to be performed on java.util.Set not to confuse Guice. You need then to convert it to a scala set.
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/StateChange.scala
##########
@@ -40,33 +40,69 @@ object TypeName {
}
}
-sealed trait TypeName {
+trait TypeName {
def asMap(maybeState: Option[State]): Map[TypeName, State] =
maybeState.map(state => Map[TypeName, State](this -> state))
.getOrElse(Map())
def asString(): String
+ def parse(string: String): TypeName
Review comment:
I guess it can fail so the return type could be
```
Either[IllegalArgumentException, TypeName]
```
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/TypeNameParserFactory.scala
##########
@@ -0,0 +1,13 @@
+package org.apache.james.jmap.change
+
+import javax.inject.Inject
+
+class TypeNameParserFactory @Inject() (setTypeName: Set[TypeName]) {
+ val ALL: Set[TypeName] = setTypeName
+
+ def parse(string: String): Either[String, TypeName] = {
Review comment:
Either[IllegalArgumentException, TypeName] ?
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/TypeNameParserFactory.scala
##########
@@ -0,0 +1,13 @@
+package org.apache.james.jmap.change
+
+import javax.inject.Inject
+
+class TypeNameParserFactory @Inject() (setTypeName: Set[TypeName]) {
+ val ALL: Set[TypeName] = setTypeName
Review comment:
val all
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/TypeNameParserFactory.scala
##########
@@ -0,0 +1,13 @@
+package org.apache.james.jmap.change
+
+import javax.inject.Inject
+
+class TypeNameParserFactory @Inject() (setTypeName: Set[TypeName]) {
Review comment:
Can we have unit tests for this class?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-837714286
Can you rebase, squash the fixups, reword the commits to use `JAMES-3581` as a ticket prefix?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r629109068
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/StateChange.scala
##########
@@ -44,10 +41,7 @@ case object MailboxTypeName extends TypeName {
case _ => None
}
- override def parseState(string: String): Either[IllegalArgumentException, UuidState] = Try(UUID.fromString(string))
- .toEither
- .map(UuidState(_))
- .left.map(new IllegalArgumentException(_))
+ override def parseState(string: String): Either[IllegalArgumentException, UuidState] = UuidState.parse(string)
Review comment:
That is why I am doing this PR: to make TypeState(CustomTypeName -> CustomState) customization is able.
Every state now should not tied to Uuid State, cause we have some custom State like FilterState(based on Integer).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r627267238
##########
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.getState(MailboxTypeName).map(state => state.serialize).toJava,
+ getEmailState = event.getState(EmailTypeName).map(state => state.serialize).toJava,
+ getVacationResponseState = event.getState(VacationResponseTypeName).map(state => state.serialize).toJava,
+ getEmailDeliveryState = event.getState(EmailDeliveryTypeName).map(state => state.serialize).toJava)
}
case class StateChangeEventDTO(@JsonProperty("type") getType: String,
Review comment:
I don't really get your point here.
You means Map<String, String> likes ("mailboxState" -> "abcbd", "emailState" -> "aaaa") and keep the old JSON format in the tests:
```
{
"eventId":"6e0dd59d-660e-4d9b-b22f-0354479f47b4",
"username":"bob",
"mailboxState":"2c9f1b12-b35a-43e6-9af2-0106fb53a943",
"emailState":"2d9f1b12-b35a-43e6-9af2-0106fb53a943",
"emailDeliveryState":"2d9f1b12-0000-1111-3333-0106fb53a943",
"vacationResponseState":"2d9f1b12-3333-4444-5555-0106fb53a943",
"type":"org.apache.james.jmap.change.StateChangeEvent"
}
```
Or you means I should use TypeStateFactory to parse something likes:
```
{
"eventId":"6e0dd59d-660e-4d9b-b22f-0354479f47b4",
"username":"bob",
"MailboxTypeName":"2c9f1b12-b35a-43e6-9af2-0106fb53a943",
"EmailTypeName":"2d9f1b12-b35a-43e6-9af2-0106fb53a943",
"EmailDeliveryTypeName":"2d9f1b12-0000-1111-3333-0106fb53a943",
"VacationResponseTypeName":"2d9f1b12-3333-4444-5555-0106fb53a943",
"type":"org.apache.james.jmap.change.StateChangeEvent"
}
```
and change JSON format also?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-826487924
> Read it. You might wanna be careful when squashing your fixups.
>
> Like this one [626c002](https://github.com/apache/james-project/commit/626c002413bae26e279c57dc9e71341ec8dfe46b), I would suspect it's definitely mixing 2 or 3 commits together
Yes I know it's quite mess up. But now I have to handle the compile before I can clean up commit history:
```
Cannot resolve overloaded method 'collect': 86 Identity.scala
```
Which is not related to my work, still figuring it out.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r629056902
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/StateChange.scala
##########
@@ -22,51 +22,110 @@ package org.apache.james.jmap.change
import org.apache.james.core.Username
import org.apache.james.events.Event
import org.apache.james.events.Event.EventId
-import org.apache.james.jmap.api.change.{State => JavaState}
-import org.apache.james.jmap.core.{AccountId, PushState, State, StateChange}
-
-object TypeName {
- val ALL: Set[TypeName] = Set(EmailTypeName, MailboxTypeName, ThreadTypeName, IdentityTypeName, EmailSubmissionTypeName, EmailDeliveryTypeName)
-
- def parse(string: String): Either[String, TypeName] = string match {
- case MailboxTypeName.asString => Right(MailboxTypeName)
- case EmailTypeName.asString => Right(EmailTypeName)
- case ThreadTypeName.asString => Right(ThreadTypeName)
- case IdentityTypeName.asString => Right(IdentityTypeName)
- case EmailSubmissionTypeName.asString => Right(EmailSubmissionTypeName)
- case EmailDeliveryTypeName.asString => Right(EmailDeliveryTypeName)
- case VacationResponseTypeName.asString => Right(VacationResponseTypeName)
- case _ => Left(s"Unknown typeName $string")
- }
-}
+import org.apache.james.jmap.core.{AccountId, PushState, State, StateChange, UuidState}
-sealed trait TypeName {
+import java.util.UUID
+import scala.util.Try
+
+trait TypeName {
def asMap(maybeState: Option[State]): Map[TypeName, State] =
maybeState.map(state => Map[TypeName, State](this -> state))
.getOrElse(Map())
def asString(): String
+ def parse(string: String): Option[TypeName]
+ def parseState(string: String): Either[IllegalArgumentException, State]
}
case object MailboxTypeName extends TypeName {
override val asString: String = "Mailbox"
+
+ override def parse(string: String): Option[TypeName] = string match {
+ case MailboxTypeName.asString => Some(MailboxTypeName)
+ case _ => None
+ }
+
+ override def parseState(string: String): Either[IllegalArgumentException, UuidState] = Try(UUID.fromString(string))
+ .toEither
+ .map(UuidState(_))
+ .left.map(new IllegalArgumentException(_))
}
case object EmailTypeName extends TypeName {
override val asString: String = "Email"
+
+ override def parse(string: String): Option[TypeName] = string match {
+ case EmailTypeName.asString => Some(EmailTypeName)
+ case _ => None
+ }
+
+ override def parseState(string: String): Either[IllegalArgumentException, UuidState] = Try(UUID.fromString(string))
+ .toEither
+ .map(UuidState(_))
+ .left.map(new IllegalArgumentException(_))
Review comment:
```
Try(UUID.fromString(string))
.toEither
.map(UuidState(_))
.left.map(new IllegalArgumentException(_))
```
should be factorized in a `UuidState.parse` method...
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r628310734
##########
File path: server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/change/StateChangeEventSerializerTest.scala
##########
@@ -40,33 +41,54 @@ object StateChangeEventSerializerTest {
EmailDeliveryTypeName -> UuidState.fromStringUnchecked("2d9f1b12-0000-1111-3333-0106fb53a943")))
val EVENT_JSON: String =
"""{
- | "eventId":"6e0dd59d-660e-4d9b-b22f-0354479f47b4",
- | "username":"bob",
- | "mailboxState":"2c9f1b12-b35a-43e6-9af2-0106fb53a943",
- | "emailState":"2d9f1b12-b35a-43e6-9af2-0106fb53a943",
- | "emailDeliveryState":"2d9f1b12-0000-1111-3333-0106fb53a943",
- | "vacationResponseState":"2d9f1b12-3333-4444-5555-0106fb53a943",
- | "type":"org.apache.james.jmap.change.StateChangeEvent"
+ | "eventId": "6e0dd59d-660e-4d9b-b22f-0354479f47b4",
+ | "username": "bob",
+ | "typeStateMap": {
Review comment:
"typeStates"
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r628316121
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/JmapEventSerializer.scala
##########
@@ -19,55 +19,45 @@
package org.apache.james.jmap.change
-import java.util.Optional
-
import com.fasterxml.jackson.annotation.JsonProperty
import org.apache.james.core.Username
import org.apache.james.events.Event.EventId
import org.apache.james.events.{Event, EventSerializer}
import org.apache.james.jmap.core.UuidState
import org.apache.james.json.JsonGenericSerializer
-import scala.jdk.OptionConverters._
+import javax.inject.Inject
+import scala.jdk.CollectionConverters._
-object StateChangeEventDTO {
+case class StateChangeEventDTOFactory @Inject()(typeStateFactory: TypeStateFactory) {
val dtoModule: EventDTOModule[StateChangeEvent, StateChangeEventDTO] = EventDTOModule.forEvent(classOf[StateChangeEvent])
.convertToDTO(classOf[StateChangeEventDTO])
- .toDomainObjectConverter(dto => dto.toDomainObject)
- .toDTOConverter((event, aType) => StateChangeEventDTO.toDTO(event))
+ .toDomainObjectConverter(dto => dto.toDomainObject(typeStateFactory))
+ .toDTOConverter((event, aType) => toDTO(event))
.typeName(classOf[StateChangeEvent].getCanonicalName)
.withFactory(EventDTOModule.apply);
def toDTO(event: StateChangeEvent): StateChangeEventDTO = StateChangeEventDTO(
getType = classOf[StateChangeEvent].getCanonicalName,
getEventId = event.eventId.getId.toString,
- getUsername = event.username.asString(),
- getMailboxState = event.getState(MailboxTypeName).map(_.serialize).toJava,
- getEmailState = event.getState(EmailTypeName).map(_.serialize).toJava,
- getVacationResponseState = event.getState(VacationResponseTypeName).map(_.serialize).toJava,
- getEmailDeliveryState = event.getState(EmailDeliveryTypeName).map(_.serialize).toJava)
+ getUsername = event.username.asString,
+ getTypeStateMap = event.map.map(element => element._1.asString() -> element._2.serialize).asJava)
}
case class StateChangeEventDTO(@JsonProperty("type") getType: String,
@JsonProperty("eventId") getEventId: String,
@JsonProperty("username") getUsername: String,
- @JsonProperty("vacationResponseState") getVacationResponseState: Optional[String],
- @JsonProperty("mailboxState") getMailboxState: Optional[String],
- @JsonProperty("emailState") getEmailState: Optional[String],
- @JsonProperty("emailDeliveryState") getEmailDeliveryState: Optional[String]) extends EventDTO {
- def toDomainObject: StateChangeEvent = StateChangeEvent(
+ @JsonProperty("typeStateMap") getTypeStateMap: java.util.Map[String, String]) extends EventDTO {
+ def toDomainObject(typeStateFactory: TypeStateFactory): StateChangeEvent = StateChangeEvent(
eventId = EventId.of(getEventId),
username = Username.of(getUsername),
- map = List(VacationResponseTypeName -> getVacationResponseState,
- MailboxTypeName -> getMailboxState,
- EmailTypeName -> getEmailState,
- EmailDeliveryTypeName -> getEmailDeliveryState)
- .flatMap(element => element._2.toScala.map(stateString => element._1 -> UuidState.fromStringUnchecked(stateString))).toMap)
+ map = getTypeStateMap.asScala.filter(element => typeStateFactory.parse(element._1).isRight)
+ .map(element => typeStateFactory.parse(element._1).toOption.get -> UuidState.fromStringUnchecked(element._2)).toMap)
Review comment:
Why do you parse `element._1` two time?
I would be you I would definitly flatMap on the option...
```
getTypeStateMap.asScala.flatMap(element => typeStateFactory.parse(element._1).toOption
.map(typeState => typeState -> UuidState.fromStringUnchecked(element._2)).toMap
```
Also:
- 'get' calls are always suspicious. Here it was needed because you did not leverage type state
- The distributed test will not be passing. Because you enforce the use of an UuidState. If I remember well it is the responsibility of the TypeState factory to parse state. That way the state could have virtually any format, any subtype <3
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on pull request #391: JAMES-3581 Modularize TypeState
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-838270746
> Have you been rebasing on master?
I rebased on master yesterday, today yet. Let's see the result of CI and I will rebase again if needed.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r629169882
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/StateChange.scala
##########
@@ -44,10 +41,7 @@ case object MailboxTypeName extends TypeName {
case _ => None
}
- override def parseState(string: String): Either[IllegalArgumentException, UuidState] = Try(UUID.fromString(string))
- .toEither
- .map(UuidState(_))
- .left.map(new IllegalArgumentException(_))
+ override def parseState(string: String): Either[IllegalArgumentException, UuidState] = UuidState.parse(string)
Review comment:
Ok i think i get your point now. It's a nice improved suggestion for me :)
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on pull request #391: JAMES-3581 Modularize TypeState
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-838178665
Can someone restart the CI? I ran the failed test on local and it's green.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r623186096
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/distributed-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/distributed/DistributedMDNSendMethodTest.java
##########
@@ -56,8 +54,7 @@
.extension(new CassandraExtension())
.extension(new RabbitMQExtension())
.extension(new AwsS3BlobStoreExtension())
- .server(configuration -> CassandraRabbitMQJamesServerMain.createServer(configuration)
- .overrideWith(new TestJMAPServerModule()))
+ .server(configuration -> CassandraRabbitMQJamesServerMain.createServer(configuration))
Review comment:
You lost TestJMAPServerModule
##########
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 +231,80 @@ trait CustomMethodContract {
.build
}
+ @Test
+ def pushShouldSupportCustomTypeNameAndIntStateWithDataTypesAreMyTypeName(server: GuiceJamesServer): Unit = {
+ val accountId: AccountId = AccountId.fromUsername(BOB)
+ val intState = IntState.fromString("1")
+ val stateChangeEvent: StateChangeEvent = StateChangeEvent(eventId = CustomMethodContract.eventId, username = BOB, map = Map(CustomTypeName -> intState))
+ Thread.sleep(100)
+
+ val response: Either[String, List[String]] =
+ authenticatedRequest(server)
+ .response(asWebSocket[Identity, List[String]] {
+ ws =>
+ ws.send(WebSocketFrame.text(
+ """{
+ | "@type": "WebSocketPushEnable",
+ | "dataTypes": ["MyTypeName"]
+ |}""".stripMargin))
+
+ Thread.sleep(100)
+
+ server.getProbe(classOf[JmapEventBusProbe])
+ .emitStateChange(stateChangeEvent, accountId)
+
+ List(ws.receive()
+ .map { case t: Text =>
+ t.payload
+ })
+ })
+ .send(backend)
+ .body
+
+ Thread.sleep(100)
+ val stateChange: String = s"""{"@type":"StateChange","changed":{"$ACCOUNT_ID":{"MyTypeName":"${intState.serialize}"}}}"""
+
+ assertThat(response.toOption.get.asJava)
+ .containsOnly(stateChange)
+ }
+
+ @Test
+ def pushShouldSupportCustomTypeNameAndIntStateWithDataTypesAreNull(server: GuiceJamesServer): Unit = {
+ val accountId: AccountId = AccountId.fromUsername(BOB)
+ val intState = IntState(1)
+ val stateChangeEvent: StateChangeEvent = StateChangeEvent(eventId = CustomMethodContract.eventId, username = BOB, map = Map(CustomTypeName -> intState))
+ Thread.sleep(100)
+
+ val response: Either[String, List[String]] =
+ authenticatedRequest(server)
+ .response(asWebSocket[Identity, List[String]] {
+ ws =>
+ ws.send(WebSocketFrame.text(
+ """{
+ | "@type": "WebSocketPushEnable",
+ | "dataTypes": null
+ |}""".stripMargin))
+
+ Thread.sleep(100)
+
+ server.getProbe(classOf[JmapEventBusProbe])
+ .emitStateChange(stateChangeEvent, accountId)
+
+ List(ws.receive()
Review comment:
Idem you can remove this list...
##########
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 +231,80 @@ trait CustomMethodContract {
.build
}
+ @Test
+ def pushShouldSupportCustomTypeNameAndIntStateWithDataTypesAreMyTypeName(server: GuiceJamesServer): Unit = {
+ val accountId: AccountId = AccountId.fromUsername(BOB)
+ val intState = IntState.fromString("1")
+ val stateChangeEvent: StateChangeEvent = StateChangeEvent(eventId = CustomMethodContract.eventId, username = BOB, map = Map(CustomTypeName -> intState))
+ Thread.sleep(100)
+
+ val response: Either[String, List[String]] =
Review comment:
1. Put `Either[String, String]` here...
##########
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
Review comment:
Missing license
##########
File path: server/protocols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/change/TypeStateFactoryTest.scala
##########
@@ -0,0 +1,49 @@
+package org.apache.james.jmap.change
Review comment:
License
##########
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.getState(MailboxTypeName).map(state => state.serialize).toJava,
+ getEmailState = event.getState(EmailTypeName).map(state => state.serialize).toJava,
Review comment:
```suggestion
getEmailState = event.getState(EmailTypeName).map(_.serialize).toJava,
```
##########
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.getState(MailboxTypeName).map(state => state.serialize).toJava,
+ getEmailState = event.getState(EmailTypeName).map(state => state.serialize).toJava,
+ getVacationResponseState = event.getState(VacationResponseTypeName).map(state => state.serialize).toJava,
+ getEmailDeliveryState = event.getState(EmailDeliveryTypeName).map(state => state.serialize).toJava)
}
case class StateChangeEventDTO(@JsonProperty("type") getType: String,
Review comment:
The DTO should rely on a Map<String, String> so that it carries over thegeneric typeStates no?
(Did you run the distributed version of your 'custom typeName' test?)
##########
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.getState(MailboxTypeName).map(state => state.serialize).toJava,
+ getEmailState = event.getState(EmailTypeName).map(state => state.serialize).toJava,
+ getVacationResponseState = event.getState(VacationResponseTypeName).map(state => state.serialize).toJava,
+ getEmailDeliveryState = event.getState(EmailDeliveryTypeName).map(state => state.serialize).toJava)
Review comment:
```suggestion
getEmailDeliveryState = event.getState(EmailDeliveryTypeName).map(_.serialize).toJava)
```
##########
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 +231,80 @@ trait CustomMethodContract {
.build
}
+ @Test
+ def pushShouldSupportCustomTypeNameAndIntStateWithDataTypesAreMyTypeName(server: GuiceJamesServer): Unit = {
+ val accountId: AccountId = AccountId.fromUsername(BOB)
+ val intState = IntState.fromString("1")
+ val stateChangeEvent: StateChangeEvent = StateChangeEvent(eventId = CustomMethodContract.eventId, username = BOB, map = Map(CustomTypeName -> intState))
+ Thread.sleep(100)
+
+ val response: Either[String, List[String]] =
+ authenticatedRequest(server)
+ .response(asWebSocket[Identity, List[String]] {
+ ws =>
+ ws.send(WebSocketFrame.text(
+ """{
+ | "@type": "WebSocketPushEnable",
+ | "dataTypes": ["MyTypeName"]
+ |}""".stripMargin))
+
+ Thread.sleep(100)
+
+ server.getProbe(classOf[JmapEventBusProbe])
+ .emitStateChange(stateChangeEvent, accountId)
+
+ List(ws.receive()
Review comment:
And 2. you can remove the `List` here
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/JmapEventSerializer.scala
##########
@@ -58,10 +57,13 @@ case class StateChangeEventDTO(@JsonProperty("type") getType: String,
def toDomainObject: StateChangeEvent = StateChangeEvent(
eventId = EventId.of(getEventId),
username = Username.of(getUsername),
- vacationResponseState = getVacationResponseState.toScala.map(State.fromStringUnchecked),
- mailboxState = getMailboxState.toScala.map(State.fromStringUnchecked),
- emailState = getEmailState.toScala.map(State.fromStringUnchecked),
- emailDeliveryState = getEmailDeliveryState.toScala.map(State.fromStringUnchecked))
+ map = (toMap(getVacationResponseState, VacationResponseTypeName) ++
+ toMap(getMailboxState, MailboxTypeName) ++
+ toMap(getEmailState, EmailTypeName) ++
+ toMap(getEmailDeliveryState, EmailDeliveryTypeName)))
Review comment:
```suggestion
map = List(
VacationResponseTypeName -> getVacationResponseState, // state can be optional...
MailboxResponseTypeName -> getMailboxResponseState, // state can be optional...
// etc... )
.flatMap {
case (typeName, None) => None
case (typeName, Some(state)) => Some(typeName -> state)
}.toMap
```
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/core/Session.scala
##########
@@ -31,7 +27,11 @@ import org.apache.james.core.Username
import org.apache.james.jmap.api.change.{EmailChanges, MailboxChanges, State => JavaState}
import org.apache.james.jmap.core.CapabilityIdentifier.CapabilityIdentifier
import org.apache.james.jmap.core.Id.Id
-import org.apache.james.jmap.core.State.INSTANCE
+import org.apache.james.jmap.core.UuidState.INSTANCE
+
+import java.net.URL
+import java.nio.charset.StandardCharsets
+import java.util.UUID
Review comment:
Oups wrong import reorder
##########
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.getState(MailboxTypeName).map(state => state.serialize).toJava,
+ getEmailState = event.getState(EmailTypeName).map(state => state.serialize).toJava,
+ getVacationResponseState = event.getState(VacationResponseTypeName).map(state => state.serialize).toJava,
Review comment:
```suggestion
getVacationResponseState = event.getState(VacationResponseTypeName).map(_.serialize).toJava,
```
##########
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.getState(MailboxTypeName).map(state => state.serialize).toJava,
Review comment:
```suggestion
getMailboxState = event.getState(MailboxTypeName).map(_.serialize).toJava,
```
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/json/ResponseSerializer.scala
##########
@@ -35,9 +31,10 @@ import org.apache.james.jmap.core.{Account, Invocation, Session, _}
import play.api.libs.functional.syntax._
import play.api.libs.json._
+import java.io.InputStream
+import java.net.URL
Review comment:
Oups wrong import reorder
##########
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 +231,80 @@ trait CustomMethodContract {
.build
}
+ @Test
+ def pushShouldSupportCustomTypeNameAndIntStateWithDataTypesAreMyTypeName(server: GuiceJamesServer): Unit = {
+ val accountId: AccountId = AccountId.fromUsername(BOB)
+ val intState = IntState.fromString("1")
+ val stateChangeEvent: StateChangeEvent = StateChangeEvent(eventId = CustomMethodContract.eventId, username = BOB, map = Map(CustomTypeName -> intState))
+ Thread.sleep(100)
+
+ val response: Either[String, List[String]] =
+ authenticatedRequest(server)
+ .response(asWebSocket[Identity, List[String]] {
+ ws =>
+ ws.send(WebSocketFrame.text(
+ """{
+ | "@type": "WebSocketPushEnable",
+ | "dataTypes": ["MyTypeName"]
+ |}""".stripMargin))
+
+ Thread.sleep(100)
+
+ server.getProbe(classOf[JmapEventBusProbe])
+ .emitStateChange(stateChangeEvent, accountId)
+
+ List(ws.receive()
+ .map { case t: Text =>
+ t.payload
+ })
+ })
+ .send(backend)
+ .body
+
+ Thread.sleep(100)
+ val stateChange: String = s"""{"@type":"StateChange","changed":{"$ACCOUNT_ID":{"MyTypeName":"${intState.serialize}"}}}"""
+
+ assertThat(response.toOption.get.asJava)
Review comment:
3... that would make this assertion easier...
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/memory-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/memory/MemoryMDNSendMethodTest.java
##########
@@ -21,23 +21,21 @@
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.MDNSendMethodContract;
import org.apache.james.mailbox.inmemory.InMemoryMessageId;
import org.apache.james.mailbox.model.MessageId;
-import org.apache.james.modules.TestJMAPServerModule;
import org.junit.jupiter.api.extension.RegisterExtension;
-import java.util.concurrent.ThreadLocalRandom;
-
public class MemoryMDNSendMethodTest implements MDNSendMethodContract {
@RegisterExtension
static JamesServerExtension testExtension = new JamesServerBuilder<>(JamesServerBuilder.defaultConfigurationProvider())
.server(configuration -> GuiceJamesServer.forConfiguration(configuration)
- .combineWith(IN_MEMORY_SERVER_AGGREGATE_MODULE)
- .overrideWith(new TestJMAPServerModule()))
+ .combineWith(IN_MEMORY_SERVER_AGGREGATE_MODULE))
Review comment:
You lost TestJMAPServerModule
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-826487924
> Read it. You might wanna be careful when squashing your fixups.
>
> Like this one [626c002](https://github.com/apache/james-project/commit/626c002413bae26e279c57dc9e71341ec8dfe46b), I would suspect it's definitely mixing 2 or 3 commits together
Yes I know it's quite mess up. But now I have to handle the compile before I can clean up commit history:
```
Cannot resolve overloaded method 'collect': 86 Identity.scala
```
Which is not related to my work, still figuring it out.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael edited a comment on pull request #391: JAMES-3581 Modularize TypeState
Posted by GitBox <gi...@apache.org>.
Arsnael edited a comment on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-838215410
```
java.lang.NoClassDefFoundError: org/apache/james/utils/FailingPropertiesProvider
at org.apache.james.DefaultCassandraJamesServerTest.lambda$static$0(DefaultCassandraJamesServerTest.java:39)
Caused by: java.lang.ClassNotFoundException: org.apache.james.utils.FailingPropertiesProvider
at org.apache.james.DefaultCassandraJamesServerTest.lambda$static$0(DefaultCassandraJamesServerTest.java:39)
```
> Can someone restart the CI? I ran the failed test on local and it's green.
If you want but I find that stacktrace rather suspicious...
EDIT: I'm trying to double check, but first thing I see is that it's not rebased on latest master. The CI does it before building the project, and I would advice you to do the same when the CI reports an error like this that you can't reproduce locally. Sometimes it's the answer (but not always)
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
Arsnael commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-837708056
No JIRA ticket number btw?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael merged pull request #391: JAMES-3581 Modularize TypeState
Posted by GitBox <gi...@apache.org>.
Arsnael merged pull request #391:
URL: https://github.com/apache/james-project/pull/391
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-837713655
See https://issues.apache.org/jira/browse/JAMES-3581 for a ticket :-)
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on pull request #391: JAMES-3581 Modularize TypeState
Posted by GitBox <gi...@apache.org>.
Arsnael commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-838215410
```
java.lang.NoClassDefFoundError: org/apache/james/utils/FailingPropertiesProvider
at org.apache.james.DefaultCassandraJamesServerTest.lambda$static$0(DefaultCassandraJamesServerTest.java:39)
Caused by: java.lang.ClassNotFoundException: org.apache.james.utils.FailingPropertiesProvider
at org.apache.james.DefaultCassandraJamesServerTest.lambda$static$0(DefaultCassandraJamesServerTest.java:39)
```
> Can someone restart the CI? I ran the failed test on local and it's green.
If you want but I find that stacktrace rather suspicious...
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #391: JAMES-3581 Modularize TypeState
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-838262168
Have you been rebasing on master?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r629109068
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/StateChange.scala
##########
@@ -44,10 +41,7 @@ case object MailboxTypeName extends TypeName {
case _ => None
}
- override def parseState(string: String): Either[IllegalArgumentException, UuidState] = Try(UUID.fromString(string))
- .toEither
- .map(UuidState(_))
- .left.map(new IllegalArgumentException(_))
+ override def parseState(string: String): Either[IllegalArgumentException, UuidState] = UuidState.parse(string)
Review comment:
That is why I am doing this PR: to make TypeState(CustomTypeName -> CustomState) customization is able.
Every state now should not tied to Uuid State, cause we have some custom State like FilterState(which is parsed from Integer).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r629203941
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/StateChange.scala
##########
@@ -31,7 +31,7 @@ trait TypeName {
def asString(): String
def parse(string: String): Option[TypeName]
- def parseState(string: String): Either[IllegalArgumentException, State]
+ def parseState(string: String): Either[IllegalArgumentException, State] = UuidState.parse(string)
Review comment:
No I think this change is a regression.
Less line of code is not always good: It is perfectly fine to have all factories use 1 line to specify how they deserialise stuff explicitly...
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r629139324
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/StateChange.scala
##########
@@ -44,10 +41,7 @@ case object MailboxTypeName extends TypeName {
case _ => None
}
- override def parseState(string: String): Either[IllegalArgumentException, UuidState] = Try(UUID.fromString(string))
- .toEither
- .map(UuidState(_))
- .left.map(new IllegalArgumentException(_))
+ override def parseState(string: String): Either[IllegalArgumentException, UuidState] = UuidState.parse(string)
Review comment:
Well if you need to customize it you can override it then, I mean it's not because you have a default method that you can't override it if you need to?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-825577707
There are so many changes, hope commits log is not too mess.
Also I look like need to solve conflict and write more tests.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
Arsnael commented on pull request #391:
URL: https://github.com/apache/james-project/pull/391#issuecomment-836162056
Do you think you could squash your commits, except for the 3 last fixups answering last comments of @chibenwa , to ease the review for others? Thanks :)
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #391: JAMES-XXXX Modularize TypeState
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #391:
URL: https://github.com/apache/james-project/pull/391#discussion_r615556747
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/TypeNameFactory.scala
##########
@@ -0,0 +1,16 @@
+package org.apache.james.jmap.change
+
+import javax.inject.Inject
+import scala.jdk.CollectionConverters._
+
+case class TypeNameFactory @Inject()(setTypeName: java.util.Set[TypeName]) {
+ val all: scala.collection.mutable.Set[TypeName] = setTypeName.asScala
+
+ def parse(string: String): Either[IllegalArgumentException, TypeName] = {
+ all.foreach(typeName => typeName.parse(string).fold(
+ e => e.printStackTrace(),
Review comment:
I think we can safely discard the error here (`{}`)
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/TypeNameFactory.scala
##########
@@ -0,0 +1,16 @@
+package org.apache.james.jmap.change
+
+import javax.inject.Inject
+import scala.jdk.CollectionConverters._
+
+case class TypeNameFactory @Inject()(setTypeName: java.util.Set[TypeName]) {
+ val all: scala.collection.mutable.Set[TypeName] = setTypeName.asScala
+
+ def parse(string: String): Either[IllegalArgumentException, TypeName] = {
+ all.foreach(typeName => typeName.parse(string).fold(
+ e => e.printStackTrace(),
+ validTypeName => return Right(validTypeName)))
Review comment:
avoid return in scala
##########
File path: server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/StateChange.scala
##########
@@ -46,62 +46,70 @@ trait TypeName {
.getOrElse(Map())
def asString(): String
- def parse(string: String): TypeName
+ def parse(string: String): Either[IllegalArgumentException, TypeName]
Review comment:
Or... `Option[TypeName]` as we go through all value, generating an error is not meaningful...
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org