twitter / finatra

Fast, testable, Scala services built on TwitterServer and Finagle
https://twitter.github.io/finatra/
Apache License 2.0
2.27k stars 405 forks source link

Generic Option deserialisation in generic case classes #547

Closed aatasiei closed 4 years ago

aatasiei commented 4 years ago

Jackson deserialisation issue: when using Option[T] as a field in a generic case class, T will always be deserialised as a Map.

Expected behavior

import com.twitter.finatra.jackson.ScalaObjectMapper
val mapper = ScalaObjectMapper()

case class B(value: String)
case class AGeneric[T](b: Option[T])
val aGeneric = mapper.parse[AGeneric[B]]("""{"b":{"value":"string"}}""")
// aGeneric: AGeneric[B] = AGeneric(Some(B(string)))
aGeneric.b.map(_.value)
// res1: Option[String] = Some(string)

Actual behavior

import com.twitter.finatra.jackson.ScalaObjectMapper
val mapper = ScalaObjectMapper()

case class B(value: String)
case class AGeneric[T](b: Option[T])
val aGeneric = mapper.parse[AGeneric[B]]("""{"b":{"value":"string"}}""")
// aGeneric: AGeneric[B] = AGeneric(Some(Map(value -> string)))
aGeneric.b.map(_.value)
// java.lang.ClassCastException: scala.collection.immutable.Map$Map1 cannot be cast to B
//  at scala.Option.map(Option.scala:230)
//  ... 40 elided

The same happens even if you specialise AGeneric when using it as a field in another class:

case class C(a:AGeneric[B])

val c = mapper.parse[C]("""{"a":{"b":{"value":"string"}}}""")
// c: C = C(AGeneric(Some(Map(value -> string))))

c.a.b.map(_.value)
// java.lang.ClassCastException: scala.collection.immutable.Map$Map1 cannot be cast to B
//  at scala.Option.map(Option.scala:230)
//  ... 40 elided

Steps to reproduce the behavior

Actual behavior contains a minimal example.

During my debugging, I've tracked the setting of the scalaType as Option[Object] to this line in CaseClassDeserializer.scala

  private[this] val clazzDescriptor: ClassDescriptor =
    Reflector.describe(clazz).asInstanceOf[ClassDescriptor]

The Reflector.describe call is made against clazz which is set to javaType.getRawClass. Maybe describe could be called with the Reflector.scalaTypeOf(javaType)? (a suggestion based on how I saw the types align, but haven't tested it.)

cacoco commented 4 years ago

Thanks for the issue, we'll take a look!

cacoco commented 4 years ago

@aatasiei we addressed this with a more holistic fix (which also provides a different fix for #548) in a6ba62b6b53ac5470a3b99a36634ec0f35600540.

aatasiei commented 4 years ago

that's great news! thank you very much!