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.

Sign up for GitHub

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

Jump to bottom

Using a value class as a path parameter causes java.lang.IllegalArgumentException: argument type mismatch #585

Open
TimMoore opened this issue Mar 9, 2017 · 1 comment · May be fixed by #1724
Open

Using a value class as a path parameter causes java.lang.IllegalArgumentException: argument type mismatch #585

TimMoore opened this issue Mar 9, 2017 · 1 comment · May be fixed by #1724
Labels
topic:documentation topic:service-api type:defect

Comments

@TimMoore
Copy link
Contributor

TimMoore commented Mar 9, 2017

If you use a value class (a class that extends AnyVal) as a path parameter in a Lagom service call, and define a PathParamSerializer for it, it will fail at runtime with an IllegalArgumentException.

This simplified example illustrates why:

scala> final case class UserId(value: String) extends AnyVal
defined class UserId

scala> class UserService {
     |   def user(userId: UserId) = println(userId.getClass)
     | }
defined class UserService

scala> val svc = new UserService
svc: UserService = UserService@348b7268

scala> val tmoore = UserId("tmoore")
tmoore: UserId = UserId(tmoore)

scala> tmoore.getClass
res0: Class[_ <: UserId] = class UserId

scala> svc.user(tmoore)
class $line3.$read$$iw$$iw$UserId

scala> println(tmoore.getClass)
class $line3.$read$$iw$$iw$UserId

I create instances of the service and a UserId and try to inspect the types. Everything looks like it matches, but everything is not as it seems...

scala> var m = svc.getClass.getMethods.find(_.getName == "user").get
m: java.lang.reflect.Method = public void UserService.user(java.lang.String)

This is a reflective lookup of the user method, and it's similar to what Lagom actually does at runtime. We can see that the type of the parameter is java.lang.String, which is what is intended when you use a value class: it should avoid allocating a wrapper.

So why do the calls to tmoore.getClass and svc.user(tmoore) print that the class is the UserId wrapper? There are certain situations in which the Scala compiler inserts code to explicitly allocate a wrapper. Calling getClass is one of them. You can see what's happening if you disassemble the method:

scala> :javap UserService#user
  public void user(java.lang.String);
    descriptor: (Ljava/lang/String;)V
    flags: ACC_PUBLIC
    Code:
      stack=4, locals=2, args_size=2
         0: getstatic     #13                 // Field scala/Predef$.MODULE$:Lscala/Predef$;
         3: new           #15                 // class UserId
         6: dup
         7: aload_1
         8: invokespecial #18                 // Method UserId."<init>":(Ljava/lang/String;)V
        11: invokevirtual #22                 // Method java/lang/Object.getClass:()Ljava/lang/Class;
        14: invokevirtual #26                 // Method scala/Predef$.println:(Ljava/lang/Object;)V
        17: return
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0      18     0  this   LUserService;
            0      18     1 userId   Ljava/lang/String;
      LineNumberTable:
        line 14: 0

Assigning to an array is another case that triggers wrapping:

scala> var a = Array(tmoore)
a: Array[UserId] = Array(UserId(tmoore))

scala> println(a.getClass)
class [L$line3.$read$$iw$$iw$UserId;

Note that this is actually an Array[UserId], not an Array[String], and that the type of an array is not erased at runtime in the way that generic classes are. So the UserId wrapper must be created before the array assignment.

This is what causes it to blow up at runtime:

scala> m.invoke(svc, a)
java.lang.IllegalArgumentException: argument type mismatch
  at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
  at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
  at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
  at java.lang.reflect.Method.invoke(Method.java:498)
  ... 42 elided

The service method is expecting a String but gets a wrapped UserId.

Workaround

The best workaround at this time is to avoid the use of value classes in path parameters. We should add a warning to the documentation about this.

I think we could also add better runtime validation of service call parameter types. That wouldn't actually fix the problem, but it would make it more obvious why it's happening.
@jroper
Copy link
Member

jroper commented Mar 10, 2017

The thing that's doing the wrapping is the PathParamSerializer:

scala> final case class UserId(value: String) extends AnyVal
defined class UserId

scala> val pathParamSerializer: PathParamSerializer[UserId] = PathParamSerializer.required("id")(UserId.apply)(_.value)
pathParamSerializer: com.lightbend.lagom.scaladsl.api.deser.PathParamSerializer[UserId] = PathParamSerializer(id)

scala> pathParamSerializer.asInstanceOf[PathParamSerializer[_]]
res0: com.lightbend.lagom.scaladsl.api.deser.PathParamSerializer[_] = PathParamSerializer(id)

scala> res0.deserialize(List("foo"))
res1: Any = UserId(foo)

I see two ways that we could solve this. One is to detect discrepancies in the type signature, and check whether unwrapping a value class would solve the problem. Another is to let the compiler do the heavy lifting, and generate the code that invokes the service call using a macro, rather than using reflection. The second approach is possibly the simpler one.

@golemiso golemiso linked a pull request Feb 4, 2019 that will close this issue
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic:documentation topic:service-api type:defect
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add AnyVal support for path parameters golemiso/lagom
3 participants
@TimMoore @jroper @dwijnand