Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix for issue with Lift-JSON occassionally choosing wrong constructor, resulting in erroneous serialization #3

Open
wants to merge 3 commits into from

2 participants

@wfaler

On decomposing a case class Lift-JSON will pick a constructor to base it's decomposition on with the "primaryConstructorArgs"-function, which previously picked the first available constructor.

However, the JVM does not guarantee the ordering of constructors, so in the case of classes with multiple alternative available constructors, the behavior becomes unpredictable and indeterminable.

I fixed this so that "primaryConstructorArgs" always picks the declared Constructor with the most arguments, as I felt this is a more reasonable assumption on the primary constructor for a case class to be serialized to JSON.

Let me know if this is acceptable, or if you want any changes.

@bethbodekor

love it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
8 framework/lift-base/lift-json/src/main/scala/net/liftweb/json/Meta.scala
@@ -185,7 +185,13 @@ private[json] object Meta {
cachedConstructorArgs.memoize(constructor, argsInfo(_))
}
- def primaryConstructorArgs(c: Class[_]) = constructorArgs(c.getDeclaredConstructors()(0))
+ def primaryConstructorArgs(c: Class[_]) = constructorArgs({var constructor: java.lang.reflect.Constructor[_] = null
+ c.getDeclaredConstructors.foreach(cons =>{
+ if (constructor == null) constructor = cons
+ if(cons.getParameterTypes.size > constructor.getParameterTypes.size) constructor = cons
+ })
+ constructor
+ })
def typeParameters(t: Type, k: Kind): List[Class[_]] = {
def term(i: Int) = t match {
View
15 framework/lift-base/lift-json/src/test/scala/net/liftweb/json/ExtractionBugs.scala
@@ -19,6 +19,7 @@ package json
import org.specs.Specification
import org.specs.runner.{Runner, JUnit}
+import Meta.Reflection._
class ExtractionBugsTest extends Runner(ExtractionBugs) with JUnit
object ExtractionBugs extends Specification {
@@ -34,7 +35,21 @@ object ExtractionBugs extends Specification {
Extraction.decompose(m).extract[PMap] mustEqual m
}
+ // this test will be non-deterministic an may pass some/most of the time even with the old faulty behavior,
+ // as it depends on non-guaranteed JVM compilation- and runtime behavior. It should however never fail with the
+ // new fix in.
+ "Extraction should always choose constructor with the most arguments if more than one constructor exists" in {
+ val args = primaryConstructorArgs(classOf[Author])
+ println("args")
+ args.size mustEqual 4
+ }
+
case class OptionOfInt(opt: Option[Int])
case class PMap(m: Map[String, List[String]])
+
+ // case class with more than one constructor
+ case class Author(val id: Long, val firstName: String, var lastName: String, var email: Option[String]){
+ def this() = this(0,"John","Doe",Some(""))
+ }
}
Something went wrong with that request. Please try again.