JAMES-3581 Parsing TypeName should be more lenient while parsing serv…

…er side data (linagora#2343) This could help in the case the operator reverses James to an older version that does not support the recently introduced TypeName. - PushSubscription/get and PushListener would not fail when TypeStateFactory parsing the unknown typename from DB - Deserialize StateChangeEvent would not fail leads to resilient handling of StateChangeEvent Meanwhile, parsing user input, we need to be strict.
vttranlina · Jul 12, 2024 · 869358d · 869358d
1 parent a96615b
commit 869358d
Show file tree

Hide file tree

Showing 9 changed files with 83 additions and 31 deletions.
diff --git a/...n/java/org/apache/james/jmap/cassandra/pushsubscription/CassandraPushSubscriptionDAO.java b/...n/java/org/apache/james/jmap/cassandra/pushsubscription/CassandraPushSubscriptionDAO.java
@@ -177,7 +177,7 @@ private PushSubscriptionExpiredTime toExpires(Row row) {
 
     private Seq<TypeName> toTypes(Row row) {
         return CollectionConverters.asScala(row.getSet(TYPES, String.class).stream()
-                .map(string -> typeStateFactory.parse(string).right().get())
+                .flatMap(string -> OptionConverters.toJava(typeStateFactory.lenientParse(string)).stream())
                 .collect(ImmutableSet.toImmutableSet()))
             .toSeq();
     }

diff --git a/server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/change/TypeStateFactory.scala b/server/data/data-jmap/src/main/scala/org/apache/james/jmap/api/change/TypeStateFactory.scala
@@ -21,15 +21,25 @@ package org.apache.james.jmap.api.change
 
 import jakarta.inject.Inject
 import org.apache.james.jmap.api.model.TypeName
+import org.slf4j.{Logger, LoggerFactory}
 
 import scala.jdk.CollectionConverters._
 
 case class TypeStateFactory @Inject()(setTypeName: java.util.Set[TypeName]) {
+  val logger: Logger = LoggerFactory.getLogger(classOf[TypeStateFactory])
   val all: scala.collection.mutable.Set[TypeName] = setTypeName.asScala
 
-  def parse(string: String): Either[IllegalArgumentException, TypeName] =
+  def strictParse(string: String): Either[IllegalArgumentException, TypeName] =
     all.flatMap(_.parse(string))
       .headOption
       .map(Right(_))
       .getOrElse(Left(new IllegalArgumentException(s"Unknown typeName $string")))
+
+  def lenientParse(string: String): Option[TypeName] =
+    all.flatMap(_.parse(string)).headOption match {
+      case Some(value) => Some(value)
+      case None =>
+        logger.warn("Leniently ignore {} while parsing TypeName", string)
+        None
+    }
 }
diff --git a/...ocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/JmapEventSerializer.scala b/...ocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/change/JmapEventSerializer.scala
@@ -40,7 +40,7 @@ case class StateChangeEventDTOFactory @Inject()(typeStateFactory: TypeStateFacto
     .toDomainObjectConverter(dto => dto.toDomainObject(typeStateFactory))
     .toDTOConverter((event, aType) => toDTO(event))
     .typeName(classOf[StateChangeEvent].getCanonicalName)
-    .withFactory(EventDTOModule.apply);
+    .withFactory(EventDTOModule.apply)
 
   def toDTO(event: StateChangeEvent): StateChangeEventDTO = StateChangeEventDTO(
     getType = classOf[StateChangeEvent].getCanonicalName,
@@ -66,7 +66,7 @@ case class StateChangeEventDTO(@JsonProperty("type") getType: String,
     map = typeStatesFromMap(typeStateFactory))
 
   private def typeStatesFromMap(typeStateFactory: TypeStateFactory): Map[TypeName, State] =
-    getTypeStates.toScala.map(typeStates => typeStates.asScala.flatMap(element => typeStateFactory.parse(element._1).toOption
+    getTypeStates.toScala.map(typeStates => typeStates.asScala.flatMap(element => typeStateFactory.lenientParse(element._1)
       .flatMap(typeName => typeName.parseState(element._2).toOption.map(state => typeName -> state))).toMap)
       .getOrElse(fallbackToOldFormat())
 

diff --git a/...otocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/core/PushSubscriptionSet.scala b/...otocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/core/PushSubscriptionSet.scala
@@ -137,7 +137,7 @@ object TypesUpdate {
     case _ => Left(InvalidUpdateException("types", "Expecting an array of JSON strings as an argument"))
   }
   def parseType(jsValue: JsValue, typeStateFactory: TypeStateFactory): Either[PatchUpdateValidationException, TypeName] = jsValue match {
-    case JsString(aString) => typeStateFactory.parse(aString).left.map(e => InvalidUpdateException("types", e.getMessage))
+    case JsString(aString) => typeStateFactory.strictParse(aString).left.map(e => InvalidUpdateException("types", e.getMessage))
     case _ => Left(InvalidUpdateException("types", "Expecting an array of JSON strings as an argument"))
   }
 }

diff --git a/...er/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/json/PushSerializer.scala b/...er/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/json/PushSerializer.scala
@@ -56,7 +56,7 @@ case class PushSerializer @Inject()(typeStateFactory: TypeStateFactory) {
     case _ => JsError("Expecting a JsObject to represent a webSocket inbound request")
   }
   private implicit val typeNameReads: Reads[TypeName] = {
-    case JsString(string) => typeStateFactory.parse(string)
+    case JsString(string) => typeStateFactory.strictParse(string)
       .fold(throwable => JsError(throwable.getMessage), JsSuccess(_))
     case _ => JsError("Expecting a JsString as typeName")
   }

diff --git a/.../jmap-rfc-8621/src/main/scala/org/apache/james/jmap/json/PushSubscriptionSerializer.scala b/.../jmap-rfc-8621/src/main/scala/org/apache/james/jmap/json/PushSubscriptionSerializer.scala
@@ -56,7 +56,7 @@ class PushSubscriptionSerializer @Inject()(typeStateFactory: TypeStateFactory) {
       ) (PushSubscriptionKeys.apply _)
 
   private implicit val typeNameReads: Reads[TypeName] = {
-    case JsString(serializeValue) => typeStateFactory.parse(serializeValue)
+    case JsString(serializeValue) => typeStateFactory.strictParse(serializeValue)
       .fold(e => JsError(e.getMessage), v => JsSuccess(v))
     case _ => JsError()
   }

diff --git a/...otocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/EventSourceRoutes.scala b/...otocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/EventSourceRoutes.scala
@@ -74,7 +74,7 @@ case class EventSourceOptionsFactory @Inject() (typeStateFactory: TypeStateFacto
       case None => Left(new IllegalArgumentException("types parameter is compulsory"))
       case Some(List("*")) => Right(typeStateFactory.all.toSet)
       case Some(list) => list.flatMap(_.split(','))
-        .map(string => typeStateFactory.parse(string))
+        .map(string => typeStateFactory.strictParse(string))
         .sequence.map(_.toSet)
     }
 

diff --git a/...rfc-8621/src/test/scala/org/apache/james/jmap/change/StateChangeEventSerializerTest.scala b/...rfc-8621/src/test/scala/org/apache/james/jmap/change/StateChangeEventSerializerTest.scala
@@ -101,6 +101,25 @@ class StateChangeEventSerializerTest {
       })
       .verify()
 
+  @Test
+  def deserializeEventShouldLenientlyIgnoreUnknownTypeName(): Unit =
+    assertThat(JsonGenericSerializer
+      .forModules(stateChangeEventDTOFactory.dtoModule)
+      .withoutNestedType()
+      .deserialize("""{
+                     |    "eventId": "6e0dd59d-660e-4d9b-b22f-0354479f47b4",
+                     |    "username": "bob",
+                     |    "typeStates": {
+                     |        "Mailbox": "2c9f1b12-b35a-43e6-9af2-0106fb53a943",
+                     |        "Email": "2d9f1b12-b35a-43e6-9af2-0106fb53a943",
+                     |        "EmailDelivery": "2d9f1b12-0000-1111-3333-0106fb53a943",
+                     |        "VacationResponse": "2d9f1b12-3333-4444-5555-0106fb53a943",
+                     |        "FastForwardTypeName": "2d9f1b12-3333-4444-5555-0106fb53a943"
+                     |    },
+                     |    "type": "org.apache.james.jmap.change.StateChangeEvent"
+                     |}""".stripMargin))
+      .isEqualTo(EVENT)
+
   @Test
   def shouldThrowWhenDeserializeUnknownEvent(): Unit =
     assertThatThrownBy(() =>

diff --git a/...cols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/change/TypeStateFactoryTest.scala b/...cols/jmap-rfc-8621/src/test/scala/org/apache/james/jmap/change/TypeStateFactoryTest.scala
@@ -32,44 +32,67 @@ class TypeStateFactoryTest {
   val factory: TypeStateFactory = TypeStateFactory(ALL.asJava)
 
   @Test
-  def parseEmailTypeNameStringShouldReturnRightEmailTypeName(): Unit =
-    assertThat(factory.parse("Email")).isEqualTo(Right(EmailTypeName))
+  def strictParseEmailTypeNameStringShouldReturnRightEmailTypeName(): Unit =
+    assertThat(factory.strictParse("Email")).isEqualTo(Right(EmailTypeName))
 
   @Test
-  def parseMailboxTypeNameStringShouldReturnRightMailboxTypeName(): Unit =
-    assertThat(factory.parse("Mailbox")).isEqualTo(Right(MailboxTypeName))
+  def strictParseMailboxTypeNameStringShouldReturnRightMailboxTypeName(): Unit =
+    assertThat(factory.strictParse("Mailbox")).isEqualTo(Right(MailboxTypeName))
 
   @Test
-  def parseThreadTypeNameStringShouldReturnRightThreadTypeName(): Unit =
-    assertThat(factory.parse("Thread")).isEqualTo(Right(ThreadTypeName))
+  def strictParseThreadTypeNameStringShouldReturnRightThreadTypeName(): Unit =
+    assertThat(factory.strictParse("Thread")).isEqualTo(Right(ThreadTypeName))
 
   @Test
-  def parseIdentityTypeNameStringShouldReturnRightIdentityTypeName(): Unit =
-    assertThat(factory.parse("Identity")).isEqualTo(Right(IdentityTypeName))
+  def strictParseIdentityTypeNameStringShouldReturnRightIdentityTypeName(): Unit =
+    assertThat(factory.strictParse("Identity")).isEqualTo(Right(IdentityTypeName))
 
   @Test
-  def parseEmailSubmissionTypeNameStringShouldReturnRightEmailSubmissionTypeName(): Unit =
-    assertThat(factory.parse("EmailSubmission")).isEqualTo(Right(EmailSubmissionTypeName))
+  def strictParseEmailSubmissionTypeNameStringShouldReturnRightEmailSubmissionTypeName(): Unit =
+    assertThat(factory.strictParse("EmailSubmission")).isEqualTo(Right(EmailSubmissionTypeName))
 
   @Test
-  def parseEmailDeliveryTypeNameStringShouldReturnRightEmailDeliveryTypeName(): Unit =
-    assertThat(factory.parse("EmailDelivery")).isEqualTo(Right(EmailDeliveryTypeName))
+  def strictParseEmailDeliveryTypeNameStringShouldReturnRightEmailDeliveryTypeName(): Unit =
+    assertThat(factory.strictParse("EmailDelivery")).isEqualTo(Right(EmailDeliveryTypeName))
 
   @Test
-  def parseVacationResponseTypeNameStringShouldReturnRightVacationResponseTypeName(): Unit =
-    assertThat(factory.parse("VacationResponse")).isEqualTo(Right(VacationResponseTypeName))
+  def strictParseVacationResponseTypeNameStringShouldReturnRightVacationResponseTypeName(): Unit =
+    assertThat(factory.strictParse("VacationResponse")).isEqualTo(Right(VacationResponseTypeName))
 
   @Test
-  def parseWrongTypeNameStringShouldReturnLeft(): Unit =
+  def strictParseWrongTypeNameStringShouldReturnLeft(): Unit =
     SoftAssertions.assertSoftly(softly => {
-      softly.assertThat(factory.parse("email").isLeft).isTrue
-      softly.assertThat(factory.parse("mailbox").isLeft).isTrue
-      softly.assertThat(factory.parse("thread").isLeft).isTrue
-      softly.assertThat(factory.parse("identity").isLeft).isTrue
-      softly.assertThat(factory.parse("emailSubmission").isLeft).isTrue
-      softly.assertThat(factory.parse("emailDelivery").isLeft).isTrue
-      softly.assertThat(factory.parse("filter").isLeft).isTrue
-      softly.assertThat(factory.parse("vacationResponse").isLeft).isTrue
+      softly.assertThat(factory.strictParse("email").isLeft).isTrue
+      softly.assertThat(factory.strictParse("mailbox").isLeft).isTrue
+      softly.assertThat(factory.strictParse("thread").isLeft).isTrue
+      softly.assertThat(factory.strictParse("identity").isLeft).isTrue
+      softly.assertThat(factory.strictParse("emailSubmission").isLeft).isTrue
+      softly.assertThat(factory.strictParse("emailDelivery").isLeft).isTrue
+      softly.assertThat(factory.strictParse("filter").isLeft).isTrue
+      softly.assertThat(factory.strictParse("vacationResponse").isLeft).isTrue
     })
 
+  @Test
+  def lenientParseValidTypeNameStringShouldReturnTypeName(): Unit = {
+    assertThat(factory.lenientParse("Email")).isEqualTo(Some(EmailTypeName))
+    assertThat(factory.lenientParse("Mailbox")).isEqualTo(Some(MailboxTypeName))
+    assertThat(factory.lenientParse("Thread")).isEqualTo(Some(ThreadTypeName))
+    assertThat(factory.lenientParse("Identity")).isEqualTo(Some(IdentityTypeName))
+    assertThat(factory.lenientParse("EmailSubmission")).isEqualTo(Some(EmailSubmissionTypeName))
+    assertThat(factory.lenientParse("EmailDelivery")).isEqualTo(Some(EmailDeliveryTypeName))
+    assertThat(factory.lenientParse("VacationResponse")).isEqualTo(Some(VacationResponseTypeName))
+  }
+
+  @Test
+  def lenientParseWrongTypeNameStringShouldReturnEmpty(): Unit =
+    SoftAssertions.assertSoftly(softly => {
+      softly.assertThat(factory.lenientParse("email").isEmpty).isTrue
+      softly.assertThat(factory.lenientParse("mailbox").isEmpty).isTrue
+      softly.assertThat(factory.lenientParse("thread").isEmpty).isTrue
+      softly.assertThat(factory.lenientParse("identity").isEmpty).isTrue
+      softly.assertThat(factory.lenientParse("emailSubmission").isEmpty).isTrue
+      softly.assertThat(factory.lenientParse("emailDelivery").isEmpty).isTrue
+      softly.assertThat(factory.lenientParse("filter").isEmpty).isTrue
+      softly.assertThat(factory.lenientParse("vacationResponse").isEmpty).isTrue
+    })
 }