From 8c13a3199322fe3efe41a4c10376e3574957f9a8 Mon Sep 17 00:00:00 2001 From: cchantep Date: Sun, 5 Nov 2017 21:31:14 +0100 Subject: [PATCH] Check for forward reference in generated formats --- .../play/api/libs/json/JsMacroImpl.scala | 26 ++++-- .../scala/play/api/libs/json/MacroSpec.scala | 80 +++++++++++++++++++ 2 files changed, 100 insertions(+), 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 index 01eb42afd..f16c47252 100644 --- 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 index 79788bb0e..c19dbbd76 100644 --- 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.@@