twitter / finatra

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

Cannot deserialize `Wrapped[_]` types #465

Closed sortega closed 6 years ago

sortega commented 6 years ago

Cannot deserialize case classes with wrapped primitives (those that extend Wrapped[_]

Expected behavior

You should be able to deserialize them.

Actual behavior

You get a nasty error in which a constructor that is being called by reflection gets arguments of the wrong type.

18:09:28.451 [finagle/netty4-2] ERROR com.twitter.finatra.http.internal.exceptions.ThrowableExceptionMapper - Unhandled Exception
java.lang.IllegalArgumentException: argument type mismatch
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
    at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
    at com.twitter.finatra.json.internal.caseclass.jackson.FinatraCaseClassDeserializer.create(FinatraCaseClassDeserializer.scala:220)
    at com.twitter.finatra.json.internal.caseclass.jackson.FinatraCaseClassDeserializer.createAndValidate(FinatraCaseClassDeserializer.scala:213)

Steps to reproduce the behavior

Just run the only test in this minimal example (full repo):

import com.twitter.finatra.http.{Controller, HttpServer}
import com.twitter.finatra.http.routing.HttpRouter
import com.twitter.inject.domain.WrappedValue

final case class Name(value: String) extends AnyVal with WrappedValue[String]

final case class Person(name: Name)

class PersonController extends Controller {
  put("/person") { person: Person =>
    response.ok.json(person)
  }
}

object Launcher extends HttpServer {
  override protected def configureHttp(router: HttpRouter): Unit = {
    router.add[PersonController]
  }
}

Test:

import com.twitter.finagle.http.{Request, Status}
import com.twitter.finatra.http.{EmbeddedHttpServer, HttpServer, HttpTest}
import com.twitter.finatra.http.filters.ExceptionMappingFilter
import com.twitter.finatra.http.routing.HttpRouter
import com.twitter.inject.server.FeatureTest
import org.scalatest.Matchers

class PersonControllerTest extends FeatureTest with HttpTest with Matchers {
  override protected def server: EmbeddedHttpServer =
    new EmbeddedHttpServer(new HttpServer {
      override def configureHttp(router: HttpRouter): Unit = {
        router
          .filter[ExceptionMappingFilter[Request]]
          .add[PersonController]
      }
    })

  test("put") {
    server.httpPut(path = "/person",
                   putBody = """{ "name": "paco" }""",
                   andExpect = Status.Ok)
  }
}
pvcnt commented 6 years ago

FYI, if you do not extend AnyVal (but only WrappedValue) it seems to work just fine. I am not sure why though, the combination of Java reflection + Scala value classes seems to be pretty complex to handle.

sortega commented 6 years ago

In that case, is it still a value class (with no runtime impact)?

pvcnt commented 6 years ago

I suggested this because none of the examples of WrappedValue's included in Finatra extend AnyVal. But good point, although WrappedValue is a universal trait, I am afraid extending it does not automatically create a value class… I cannot find any reliable documentation about that topic though.

cacoco commented 6 years ago

@pvcnt @sortega https://docs.scala-lang.org/overviews/core/value-classes.html. WrappedValue is only a universal trait, it does not create a value class. It is meant to be mixable into a value class and other classes.

cacoco commented 6 years ago

@sortega the solution is as mentioned by @pvcnt you only need to extend the universal trait, not AnyVal.