Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reproduce the problems with Jackson serializer #2

Closed
PawelLipski opened this issue Mar 14, 2021 · 2 comments
Closed

Reproduce the problems with Jackson serializer #2

PawelLipski opened this issue Mar 14, 2021 · 2 comments

Comments

@PawelLipski
Copy link
Collaborator

PawelLipski commented Mar 14, 2021

The purpose of this task is mostly for you to realize how poor the Jackson serialization is in Akka, and what Jackson's problems this project is supposed to alleviate.

That's a test suite from my current project that strives to capture all the problems discovered with Jackson serializer so far.

package myproject.archunit

import java.lang.reflect.ParameterizedType
import java.lang.reflect.Type

import scala.jdk.CollectionConverters.asScalaSetConverter

import com.fasterxml.jackson.annotation.JsonTypeInfo
import com.fasterxml.jackson.databind.annotation.JsonDeserialize
import com.fasterxml.jackson.databind.annotation.JsonSerialize
import com.tngtech.archunit.base.DescribedPredicate
import com.tngtech.archunit.core.domain._
import com.tngtech.archunit.lang.ArchCondition
import com.tngtech.archunit.lang.ConditionEvents
import com.tngtech.archunit.lang.SimpleConditionEvent
import com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes
import com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses
import org.scalatest.wordspec.AnyWordSpecLike

import myproject.CborSerializable
import myproject.archunit.BaseArchUnitSpec.importedProductionClasses
import myproject.core.ScalaObjectUtils._
import myproject.core.serialization.EnumEntryDeserializer
import myproject.core.serialization.EnumEntrySerializer

// TODO (GMV3-965): maybe we should extract ArchUnit as separate service with one method for each of those cases
//  and plug in to this test suite to verify whole code base,
//  but additionally add a Spec that for each method will provide some problematic classes
//  and thus we will check the checker service itself
class JacksonSerializationArchUnitSpec extends AnyWordSpecLike with BaseArchUnitSpec {

  implicit class JavaClassJacksonOps(self: JavaClass) {
    def hasExplicitJacksonAnnotations: Boolean = {
      self.hasAnnotation[JsonSerialize] && self.hasAnnotation[JsonDeserialize] ||
      self.hasAnnotation[JsonTypeInfo]
    }
  }

  // The below tests (roughly) check that the classes used as messages/events/state in Akka
  // are always marked as CborSerializable, to ensure that Jackson CBOR and not legacy Java serialization
  // is used for their serialization.
  // For Akka to ensure that condition statically, a major redesign would be necessary -
  // all methods like `ask`, `tell`, `Effect.persist` etc. would need to require an implicit `Codec` (?) parameter.

  // The below tests do NOT ensure that the Jackson serialization of messages/events/state will actually succeed in the runtime.

  "Messages, events and entity state classes" should {
    "implement CborSerializable" in {
      classes
        .should(new ArchCondition[JavaClass]("only use CborSerializable message/event/state types") {
          override def check(clazz: JavaClass, events: ConditionEvents): Unit = {

            clazz.getAllMethods.asScala.foreach { method =>
              def checkType(tpe: Type, category: String, failsWhen: String): Unit = {
                tpe match {
                  case clazz: Class[_] if clazz.getPackageName.startsWith("akka") =>
                  // OK, acceptable

                  case clazz: Class[_] if clazz == classOf[scala.Nothing] =>
                  // OK, acceptable

                  case clazz: Class[_] if !classOf[CborSerializable].isAssignableFrom(clazz) =>
                    val message =
                      s"Type ${clazz.getName} is used as Akka $category (as observed in the return type of method ${method.getFullName}), " +
                      s"but does NOT extend CborSerializable; this will fail in the runtime $failsWhen"
                    events.add(SimpleConditionEvent.violated(clazz, message))

                  case _ =>
                }
              }

              val returnType = method.getRawReturnType
              val genericReturnType = method.reflect.getGenericReturnType

              if (returnType.isEquivalentTo(classOf[akka.persistence.typed.scaladsl.ReplyEffect[_, _]])) {
                genericReturnType match {
                  case parameterizedType: ParameterizedType =>
                    val Array(eventType, stateType) = parameterizedType.getActualTypeArguments
                    checkType(eventType, "event", "when saving to the journal")
                    checkType(stateType, "persistent state", "when doing a snapshot")
                  case _ =>
                }
              } else if (returnType.isEquivalentTo(classOf[akka.projection.eventsourced.EventEnvelope[_]])) {
                genericReturnType match {
                  case parameterizedType: ParameterizedType =>
                    val Array(eventType) = parameterizedType.getActualTypeArguments
                    checkType(eventType, "event", "when saving to the journal")
                  case _ =>
                }
              } else if (returnType.isEquivalentTo(classOf[akka.actor.typed.Behavior[_]])) {
                genericReturnType match {
                  case parameterizedType: ParameterizedType =>
                    val Array(messageType) = parameterizedType.getActualTypeArguments
                    checkType(messageType, "message", "when sending a message outside of the current JVM")
                  case _ =>
                }
              }
            }
          }
        })
        .check(importedProductionClasses)
    }
  }

  // The below tests (roughly) check that Jackson serialization of classes marked as CborSerializable
  // will actually succeed in the runtime.
  // TODO (GMV3-763): migrate serialization checks from ArchUnit to compile-time verification (Borer-based?)

  // The below tests do NOT ensure that the the classes used as messages/events/state in Akka
  // are always marked as CborSerializable.

  "Classes that implement CborSerializable" should {
    "never contain a non-serializable field" in {
      // These have been semi-manually verified using SerializationTestKit.
      val safeGenericNonCborSerializableTypes: Seq[Class[_]] = Seq(
        classOf[akka.actor.typed.ActorRef[_]],
        classOf[akka.stream.SourceRef[_]],
        classOf[cats.data.NonEmptyList[_]],
        classOf[scala.Option[_]],
        classOf[scala.collection.immutable.List[_]],
        classOf[scala.collection.immutable.Map[_, _]],
        classOf[scala.collection.immutable.Seq[_]],
        classOf[scala.collection.immutable.Set[_]],
        classOf[scala.collection.immutable.SortedSet[_]],
        classOf[scala.collection.Seq[_]])

      val mapGenericTypes: Seq[Class[_]] = Seq(classOf[scala.collection.Map[_, _]])

      val sortedGenericTypes: Seq[Class[_]] = Seq(
        // Explicit upcast required due to a glitch in Scala 2.12
        // (apparently has trouble finding a common supertype for existential types?
        // although that's not a problem in safeGenericNonCborSerializableTypes above... weird)
        classOf[scala.collection.immutable.SortedMap[_, _]]: Class[_],
        classOf[scala.collection.immutable.SortedSet[_]]: Class[_])

      // These have been semi-manually verified using SerializationTestKit.
      val safeNonCborSerializableTypes: Seq[Class[_]] = Seq(
        java.lang.Boolean.TYPE,
        java.lang.Byte.TYPE,
        java.lang.Double.TYPE,
        java.lang.Integer.TYPE,
        java.lang.Long.TYPE,
        classOf[java.lang.String],
        classOf[java.lang.Throwable],
        classOf[java.time.Instant],
        classOf[java.time.OffsetDateTime],
        classOf[scala.concurrent.duration.FiniteDuration],
        classOf[scala.math.BigDecimal])

      classes.that
        .areAssignableTo(classOf[myproject.CborSerializable])
        .should(new ArchCondition[JavaClass]("only contain serializable fields") {

          private def checkParametrizedType(
              parameterizedType: ParameterizedType,
              field: JavaField,
              events: ConditionEvents): Unit = {

            val fieldType = field.getRawType
            val fieldCaption = s"Field ${field.getName} in class ${field.getOwner.getFullName} "

            parameterizedType.getActualTypeArguments.foreach {
              case typeArgumentAsParametrizedType: ParameterizedType =>
                typeArgumentAsParametrizedType.getRawType match {
                  case rawType: Class[_] if classOf[CborSerializable].isAssignableFrom(rawType) =>
                    checkParametrizedType(typeArgumentAsParametrizedType, field, events)
                  case rawType: Class[_] if safeGenericNonCborSerializableTypes.contains(rawType) =>
                    checkParametrizedType(typeArgumentAsParametrizedType, field, events)
                  case rawType =>
                    val message = fieldCaption +
                      s"is of a generic type $parameterizedType, " +
                      s"whose one of type arguments is $rawType (itself a generic type), " +
                      s"which does NOT extend CborSerializable and has NOT been verified by the team to be serializable yet " +
                      s"(tip: mark it as CborSerializable or use Akka's SerializationTestKit#verify(...) " +
                      s"on a case class that has a field of ${rawType.getTypeName} type)"
                    events.add(SimpleConditionEvent.violated(fieldType, message))
                }

              case typeArgumentAsClass: Class[_] if typeArgumentAsClass == classOf[java.lang.Object] =>
              // For some reason, we observed that when type parameter is Boolean in Scala code,
              // then the generic type recorded in classfile is java.lang.Object rather than java.lang.Boolean.
              // java.lang.Object isn't likely to appear for any other type (unless someone uses Any or AnyRef as a type param),
              // so we're giving it a free pass.

              case typeArgumentAsClass: Class[_] if safeNonCborSerializableTypes.contains(typeArgumentAsClass) =>
              // OK, expected

              case typeArgumentAsClass: Class[_] if classOf[CborSerializable].isAssignableFrom(typeArgumentAsClass) =>
              // OK, expected

              case typeArgument =>
                val message = fieldCaption +
                  s"is of a generic type $parameterizedType, " +
                  s"whose one of type arguments is $typeArgument, " +
                  s"which does NOT extend CborSerializable and has NOT been verified by the team to be serializable yet " +
                  s"(tip: mark it as CborSerializable or use Akka's SerializationTestKit#verify(...) " +
                  s"on a case class that has a field of ${typeArgument.getTypeName} type)"
                events.add(SimpleConditionEvent.violated(fieldType, message))
            }
          }

          override def check(clazz: JavaClass, events: ConditionEvents): Unit = {

            clazz.getFields.forEach { field =>
              val fieldType = field.getRawType
              val fieldCaption = s"Field ${field.getName} in class ${field.getOwner.getFullName} "

              if (mapGenericTypes.exists(fieldType.isEquivalentTo)) {
                val fieldGenericType = field.reflect.getGenericType
                val Array(keyType, _) = fieldGenericType.asInstanceOf[ParameterizedType].getActualTypeArguments
                if (keyType != classOf[String]) {
                  val ctor = clazz.getConstructors.asScala.head.reflect
                  val ctorsParamForField = ctor.getParameters.find(_.getName == field.getName).get
                  val annotationsOfCtorsParamForField = ctorsParamForField.getAnnotations.toSeq
                  val jsonSerializeOpt =
                    annotationsOfCtorsParamForField.find(_.annotationType == classOf[JsonSerialize])
                  val jsonDeserializeOpt =
                    annotationsOfCtorsParamForField.find(_.annotationType == classOf[JsonDeserialize])
                  (jsonSerializeOpt, jsonDeserializeOpt) match {
                    // "None" corresponds to com.fasterxml.jackson.databind.JsonSerializer.None, the default class for `keyUsing`
                    // This can't be easily done using `classOf`
                    // since accessing Java's static nested classes is cumbersome from Scala
                    case (Some(jsonSerialize: JsonSerialize), Some(jsonDeserialize: JsonDeserialize))
                        if jsonSerialize.keyUsing.getSimpleName != "None" &&
                        jsonDeserialize.keyUsing.getSimpleName != "None" =>
                    // OK, expected

                    case _ =>
                      val message = fieldCaption +
                        s"is of a map type $fieldGenericType, " +
                        s"whose key type argument is $keyType, " +
                        s"which is NOT annotated with both @JsonSerialize(keyUsing = ...) and @JsonDeserialize(keyUsing = ...)"
                      events.add(SimpleConditionEvent.violated(fieldType, message))
                  }
                }
              }

              if (sortedGenericTypes.exists(fieldType.isEquivalentTo)) {
                val fieldGenericType = field.reflect.getGenericType
                val Array(keyType, _*) = fieldGenericType.asInstanceOf[ParameterizedType].getActualTypeArguments

                def isComparable(clazz: Class[_]): Boolean = classOf[java.lang.Comparable[_]].isAssignableFrom(clazz)

                keyType match {
                  case keyClazz: Class[_] if isComparable(keyClazz) =>
                  // OK, expected

                  case keyClazz: ParameterizedType if isComparable(keyClazz.getRawType.asInstanceOf[Class[_]]) =>
                  // OK, expected

                  case _ =>
                    val message = fieldCaption +
                      s"is of a sorted type $fieldGenericType, " +
                      s"whose key type argument is $keyType, " +
                      s"which does NOT extend java.lang.Comparable; this is bound to fail on deserialization in the runtime"
                    events.add(SimpleConditionEvent.violated(fieldType, message))
                }
              }

              if (safeGenericNonCborSerializableTypes.exists(fieldType.isEquivalentTo)) {
                checkParametrizedType(field.reflect.getGenericType.asInstanceOf[ParameterizedType], field, events)

              } else if (!fieldType.isAssignableTo(classOf[CborSerializable])) {
                if (!safeNonCborSerializableTypes.exists(fieldType.isEquivalentTo)) {

                  val message = fieldCaption +
                    s"is of a non-${classOf[CborSerializable].getSimpleName} type ${fieldType.getFullName}, " +
                    s"which does NOT extend CborSerializable and has NOT been verified by the team to be serializable yet " +
                    s"(tip: mark it as CborSerializable or use Akka's SerializationTestKit#verify(...)" +
                    s" on a case class that has a field of ${fieldType.getFullName} type)"
                  events.add(SimpleConditionEvent.violated(fieldType, message))
                }
              } else if (!fieldType.isConcrete && !fieldType.hasExplicitJacksonAnnotations) {
                val interfaces = clazz.getAllInterfaces.asScala
                if (!interfaces.exists(_.hasExplicitJacksonAnnotations)) {

                  val message = fieldCaption +
                    s"is of a non-concrete type ${fieldType.getFullName}, " +
                    "which is NOT annotated with either (@JsonSerialize + @JsonDeserialize) OR @JsonTypeInfo," +
                    s"and neither is any of its implemented interfaces (${interfaces.map(_.getFullName).mkString(", ")})"
                  events.add(SimpleConditionEvent.violated(fieldType, message))
                }
              }
            }
          }
        })
        .check(importedProductionClasses)
    }

    "not be of unannotated abstract type" in {
      classes.that
        .areAssignableTo(classOf[myproject.CborSerializable])
        .and
        .areNotAssignableFrom(classOf[myproject.CborSerializable])
        .should(new ArchCondition[JavaClass]("not be of unannotated abstract type") {
          override def check(clazz: JavaClass, events: ConditionEvents): Unit = {

            if (!clazz.isConcrete && !clazz.hasExplicitJacksonAnnotations) {
              val interfaces = clazz.getAllInterfaces.asScala
              if (!interfaces.exists(_.hasExplicitJacksonAnnotations)) {

                val message = s"${clazz.getFullName} of a non-concrete type ${clazz.getFullName}, " +
                  "which is NOT annotated with either (@JsonSerialize + @JsonDeserialize) OR @JsonTypeInfo, " +
                  s"and neither is any of its implemented interfaces (${interfaces.map(_.getFullName).mkString(", ")})"
                events.add(SimpleConditionEvent.violated(clazz, message))
              }
            }
          }
        })
        .check(importedProductionClasses)
    }

    "not be Scala objects" in {
      noClasses.that
        .areAssignableTo(classOf[myproject.CborSerializable])
        .and
        // Enums get a free pass since they have a dedicated deserializer.
        .areNotAssignableTo(classOf[enumeratum.EnumEntry])
        .and
        // This one is scheduled for removal in favor of myproject.core.currency.MoneyAmount anyway.
        .areNotAssignableTo(myproject.core.currency.DefaultCurrency.getClass)
        .should(new ArchCondition[JavaClass]("be Scala objects") {
          override def check(clazz: JavaClass, events: ConditionEvents): Unit = {
            if (clazz.reflect().isScalaObject) {
              val message = s"${clazz.getFullName} is a Scala object"
              events.add(SimpleConditionEvent.satisfied(clazz, message))
            }
          }
        })
        .because("Jackson deserializer is NOT aware that Scala objects are supposed to be singletons, " +
        "and creates new instances of the object's class instead, which makes pattern matching on objects completely unreliable; " +
        "use a nullary case class instead of an object")
        .check(importedProductionClasses)
    }
  }

  "Classes that implement both CborSerializable and EnumEntry" should {
    "be serialized with EnumEntry(De)Serializer" in {
      classes.that.areInterfaces.and
        .areAssignableTo(classOf[enumeratum.EnumEntry])
        .and
        .areAssignableTo(classOf[myproject.CborSerializable])
        .should
        .beAnnotatedWith(new DescribedPredicate[JavaAnnotation[_]](
          s"JsonSerialize(classOf[_ <: ${classOf[EnumEntrySerializer[_]].getSimpleName}])") {
          override def apply(input: JavaAnnotation[_]): Boolean =
            if (input.getRawType.isAssignableTo(classOf[JsonSerialize])) {
              input.getProperties.getOrDefault("using", null) match { // scalastyle:ignore null
                case u: JavaClass if u.isAssignableTo(classOf[EnumEntrySerializer[_]]) => true
                case _                                                                 => false
              }
            } else {
              false
            }
        })
        .andShould
        .beAnnotatedWith(new DescribedPredicate[JavaAnnotation[_]](
          s"JsonDeserialize(classOf[_ <: ${classOf[EnumEntryDeserializer[_]].getSimpleName}])") {
          override def apply(input: JavaAnnotation[_]): Boolean =
            if (input.getRawType.isAssignableTo(classOf[JsonDeserialize])) {
              input.getProperties.getOrDefault("using", null) match { // scalastyle:ignore null
                case u: JavaClass if u.isAssignableTo(classOf[EnumEntryDeserializer[_]]) => true
                case _                                                                   => false
              }
            } else {
              false
            }
        })
        .because("this will fail in the runtime otherwise")
        .check(importedProductionClasses)
    }
  }
}
@PawelLipski
Copy link
Collaborator Author

PawelLipski commented Mar 14, 2021

I suggest toying with akka.actor.testkit.typed.scaladsl.SerializationTestKit to reproduce some of the runtime errors one can encounter with Jackson.

The simplest repro would be sth like:

trait Foo
case class Bar(foo: Foo)

val testKit: ActorTestKit = ActorTestKit()
val system: ActorSystem[Nothing] = testKit.system
val serializationTestKit = new SerializationTestKit(system)
serializationTestKit.verify(Bar(new Foo {})) // will likely fail since Foo is an abstract type without `@JsonSubType` annotation

@PawelLipski
Copy link
Collaborator Author

PawelLipski commented Mar 14, 2021

Some of the necessary/useful dependencies:

"com.typesafe.akka"             %% "akka-serialization-jackson"        % "2.6.10"
"com.typesafe.akka"             %% "akka-actor-testkit-typed"          % "2.6.10"
"com.beachape"                  %% "enumeratum"                        % "1.6.1"
"com.tngtech.archunit"           % "archunit"                          % "0.14.1"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant