Check for forward reference in generated formats

playframework · Jan 20, 2018 · 40554be · 40554be
1 parent 1b04827
commit 40554be
Show file tree

Hide file tree

Showing 2 changed files with 100 additions and 6 deletions.
diff --git a/play-json/shared/src/main/scala/play/api/libs/json/JsMacroImpl.scala b/play-json/shared/src/main/scala/play/api/libs/json/JsMacroImpl.scala
@@ -112,7 +112,8 @@ import scala.reflect.macros.blackbox
     )
 
     val optTpeCtor = typeOf[Option[_]].typeConstructor
-    val forwardName = TermName(c.freshName("forward"))
+    val forwardPrefix = "play_jsmacro"
+    val forwardName = TermName(c.freshName(forwardPrefix))
 
     // MacroOptions
     val options = config.actualType.member(TypeName("Opts")).asType.toTypeIn(config.actualType)
@@ -634,7 +635,7 @@ import scala.reflect.macros.blackbox
 
         case _ => c.abort(
           c.enclosingPosition,
-          s"No apply function found matching unapply parameters"
+          "No apply function found matching unapply parameters"
         )
       }
 
@@ -670,24 +671,36 @@ import scala.reflect.macros.blackbox
           val defaultValue = // not applicable for 'write' only
             defaultValueMap.get(name).filter(_ => methodName != "write")
 
+          val resolvedImpl = {
+            val implTpeName = Option(impl.tpe).fold("null")(_.toString)
+
+            if (implTpeName.startsWith(forwardPrefix) ||
+              (implTpeName.startsWith("play.api.libs.json") &&
+                !implTpeName.contains("MacroSpec"))) {
+              impl // Avoid extra check for builtin formats
+            } else {
+              q"""_root_.java.util.Objects.requireNonNull($impl, "Invalid implicit resolution (forward reference?) for '" + $cn + "': " + ${implTpeName})"""
+            }
+          }
+
           // - If we're an default value, invoke the withDefault version
           // - If we're an option with default value,
           //   invoke the nullableWithDefault version
           (isOption, defaultValue) match {
             case (true, Some(v)) =>
               val c = TermName(s"${methodName}NullableWithDefault")
-              q"$jspathTree.$c($v)($impl)"
+              q"$jspathTree.$c($v)($resolvedImpl)"
 
             case (true, _) =>
               val c = TermName(s"${methodName}Nullable")
-              q"$jspathTree.$c($impl)"
+              q"$jspathTree.$c($resolvedImpl)"
 
             case (false, Some(v)) =>
               val c = TermName(s"${methodName}WithDefault")
-              q"$jspathTree.$c($v)($impl)"
+              q"$jspathTree.$c($v)($resolvedImpl)"
 
             case _ =>
-              q"$jspathTree.${TermName(methodName)}($impl)"
+              q"$jspathTree.${TermName(methodName)}($resolvedImpl)"
           }
       }.reduceLeft[Tree] { (acc, r) =>
         q"$acc.and($r)"
@@ -758,6 +771,7 @@ import scala.reflect.macros.blackbox
             case _ =>
               q"$json.OFormat[${atpe}](instance.reads(_), instance.writes(_))"
           }
+
           val forwardCall =
             q"private val $forwardName = $forward"
 

diff --git a/play-json/shared/src/test/scala/play/api/libs/json/MacroSpec.scala b/play-json/shared/src/test/scala/play/api/libs/json/MacroSpec.scala
@@ -4,6 +4,8 @@
 
 package play.api.libs.json
 
+import scala.util.control.NonFatal
+
 object TestFormats {
   implicit def eitherReads[A: Reads, B: Reads] = Reads[Either[A, B]] { js =>
     implicitly[Reads[A]].reads(js) match {
@@ -142,6 +144,42 @@ class MacroSpec extends WordSpec with MustMatchers
         jsOptional.validate[Family].get mustEqual optional
       }
     }
+
+    "fails due to forward reference to Reads" in {
+      implicit def reads: Reads[Lorem[Simple]] =
+        InvalidForwardResolution.simpleLoremReads
+
+      val jsLorem = Json.obj("age" -> 11, "ipsum" -> Json.obj("bar" -> "foo"))
+
+      try {
+        jsLorem.validate[Lorem[Simple]]
+      } catch {
+        case NonFatal(npe: NullPointerException) => {
+          val expected = "Invalid implicit resolution"
+          npe.getMessage.take(expected.size) mustEqual expected
+        }
+
+        case NonFatal(cause) => throw cause
+      }
+    }
+
+    "fails due to forward reference to Format" in {
+      implicit def format: Format[Lorem[Simple]] =
+        InvalidForwardResolution.simpleLoremFormat
+
+      val jsLorem = Json.obj("age" -> 11, "ipsum" -> Json.obj("bar" -> "foo"))
+
+      try {
+        jsLorem.validate[Lorem[Simple]]
+      } catch {
+        case NonFatal(npe: NullPointerException) => {
+          val expected = "Invalid implicit resolution"
+          npe.getMessage.take(expected.size) mustEqual expected
+        }
+
+        case NonFatal(cause) => throw cause
+      }
+    }
   }
 
   "Writes" should {
@@ -523,6 +561,38 @@ class MacroSpec extends WordSpec with MustMatchers
       jsOptional.validate(Json.reads[Optional]).
         get mustEqual (optional)
     }
+
+    "fails due to forward reference to Writes" in {
+      implicit def writes: Writes[Lorem[Simple]] =
+        InvalidForwardResolution.simpleLoremWrites
+
+      try {
+        Json.toJson(Lorem(age = 11, ipsum = Simple(bar = "foo")))
+      } catch {
+        case NonFatal(npe: NullPointerException) => {
+          val expected = "Invalid implicit resolution"
+          npe.getMessage.take(expected.size) mustEqual expected
+        }
+
+        case NonFatal(cause) => throw cause
+      }
+    }
+
+    "fails due to forward reference to Format" in {
+      implicit def format: Format[Lorem[Simple]] =
+        InvalidForwardResolution.simpleLoremFormat
+
+      try {
+        Json.toJson(Lorem(age = 11, ipsum = Simple(bar = "foo")))
+      } catch {
+        case NonFatal(npe: NullPointerException) => {
+          val expected = "Invalid implicit resolution"
+          npe.getMessage.take(expected.size) mustEqual expected
+        }
+
+        case NonFatal(cause) => throw cause
+      }
+    }
   }
 
   // ---
@@ -544,6 +614,16 @@ class MacroSpec extends WordSpec with MustMatchers
      */
   }
 
+  object InvalidForwardResolution {
+    // Invalids as forward references to `simpleX`
+    val simpleLoremReads = Json.reads[Lorem[Simple]]
+    val simpleLoremWrites = Json.writes[Lorem[Simple]]
+    val simpleLoremFormat = Json.format[Lorem[Simple]]
+
+    implicit val simpleReads: Reads[Simple] = Json.reads
+    implicit val simpleWrites: OWrites[Simple] = Json.writes[Simple]
+  }
+
   object Foo {
     import shapeless.tag.@@