From d3e15ce87334ee677c6a00c745e91e64198049ef Mon Sep 17 00:00:00 2001 From: cchantep Date: Sun, 5 Nov 2017 21:31:14 +0100 Subject: [PATCH 1/2] Check for forward reference in generated formats --- .../play/api/libs/json/JsMacroImpl.scala | 35 +++++--- .../scala/play/api/libs/json/MacroSpec.scala | 82 ++++++++++++++++++- 2 files changed, 105 insertions(+), 12 deletions(-) diff --git a/play-json/shared/src/main/scala-2/play/api/libs/json/JsMacroImpl.scala b/play-json/shared/src/main/scala-2/play/api/libs/json/JsMacroImpl.scala index 8ceb4a49e..5f5ae0ef7 100644 --- a/play-json/shared/src/main/scala-2/play/api/libs/json/JsMacroImpl.scala +++ b/play-json/shared/src/main/scala-2/play/api/libs/json/JsMacroImpl.scala @@ -170,8 +170,9 @@ class JsMacroImpl(val c: blackbox.Context) { selfRef: Boolean ) - val optTpeCtor = typeOf[Option[_]].typeConstructor - val forwardName = TermName(c.freshName("forward")) + val optTpeCtor = typeOf[Option[_]].typeConstructor + val forwardPrefix = "play_jsmacro" + val forwardName = TermName(c.freshName(forwardPrefix)) // MacroOptions val options = config.actualType.member(TypeName("Opts")).asType.toTypeIn(config.actualType) @@ -690,11 +691,10 @@ class JsMacroImpl(val c: blackbox.Context) { val (applyFunction, tparams, params, defaultValues) = utility.applyFunction match { case Some(info) => info - case _ => - c.abort( - c.enclosingPosition, - s"No apply function found matching unapply parameters" - ) + case _ => c.abort( + c.enclosingPosition, + "No apply function found matching unapply parameters" + ) } // --- @@ -731,24 +731,36 @@ class JsMacroImpl(val c: blackbox.Context) { 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 WithDefault version (isOption, defaultValue) match { case (true, Some(v)) => val c = TermName(s"${methodName}HandlerWithDefault") - q"$config.optionHandlers.$c($jspathTree, $v)($impl)" + q"$config.optionHandlers.$c($jspathTree, $v)($resolveImpl)" case (true, _) => val c = TermName(s"${methodName}Handler") - q"$config.optionHandlers.$c($jspathTree)($impl)" + q"$config.optionHandlers.$c($jspathTree)($resolveImpl)" case (false, Some(v)) => val c = TermName(s"${methodName}WithDefault") - q"$jspathTree.$c($v)($impl)" + q"$jspathTree.$c($v)($resolveImpl)" case _ => - q"$jspathTree.${TermName(methodName)}($impl)" + q"$jspathTree.${TermName(methodName)}($resolveImpl)" } } .reduceLeft[Tree] { (acc, r) => @@ -820,6 +832,7 @@ class JsMacroImpl(val c: blackbox.Context) { 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 08a32c920..abd9560e3 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 + import org.scalatest.matchers.must.Matchers import Matchers._ import org.scalatest.wordspec.AnyWordSpec @@ -141,11 +143,47 @@ class MacroSpec extends AnyWordSpec with Matchers with org.scalatestplus.scalach jsOptional.validate[Family].mustEqual(JsSuccess(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 { "be generated for simple case class" in { - Json.writes[Simple].writes(Simple("lorem")).mustEqual(Json.obj("bar" -> "lorem")) + Json.writes[Simple].writes(Simple("lorem")) mustEqual Json.obj("bar" -> "lorem") } "as Format for a generic case class" in { @@ -483,6 +521,38 @@ class MacroSpec extends AnyWordSpec with Matchers with org.scalatestplus.scalach formatter.writes(Obj).mustEqual(jsObj) formatter.reads(jsObj).mustEqual(JsSuccess(Obj)) } + + "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 + } + } } } @@ -512,6 +582,16 @@ object MacroSpec { */ } + 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 { // https://github.com/lampepfl/dotty/issues/11054 Type aliasing breaks constValue // import shapeless.tag.@@ From 02570d04c3807d0a5548455555bcdc3dadb4a29c Mon Sep 17 00:00:00 2001 From: cchantep Date: Wed, 9 May 2018 15:44:50 +0200 Subject: [PATCH 2/2] WIP --- .../play/api/libs/json/JsMacroImpl.scala | 45 ++++++------ .../scala/play/api/libs/json/Writes.scala | 20 ++++-- .../libs/json/JsonExtensionScala2Spec.scala | 4 +- .../scala/play/api/libs/json/MacroSpec.scala | 70 ++++--------------- 4 files changed, 55 insertions(+), 84 deletions(-) diff --git a/play-json/shared/src/main/scala-2/play/api/libs/json/JsMacroImpl.scala b/play-json/shared/src/main/scala-2/play/api/libs/json/JsMacroImpl.scala index 5f5ae0ef7..6b8aa93dc 100644 --- a/play-json/shared/src/main/scala-2/play/api/libs/json/JsMacroImpl.scala +++ b/play-json/shared/src/main/scala-2/play/api/libs/json/JsMacroImpl.scala @@ -170,9 +170,9 @@ class JsMacroImpl(val c: blackbox.Context) { selfRef: Boolean ) - val optTpeCtor = typeOf[Option[_]].typeConstructor + val optTpeCtor = typeOf[Option[_]].typeConstructor val forwardPrefix = "play_jsmacro" - val forwardName = TermName(c.freshName(forwardPrefix)) + val forwardName = TermName(c.freshName(forwardPrefix)) // MacroOptions val options = config.actualType.member(TypeName("Opts")).asType.toTypeIn(config.actualType) @@ -691,10 +691,11 @@ class JsMacroImpl(val c: blackbox.Context) { val (applyFunction, tparams, params, defaultValues) = utility.applyFunction match { case Some(info) => info - case _ => c.abort( - c.enclosingPosition, - "No apply function found matching unapply parameters" - ) + case _ => + c.abort( + c.enclosingPosition, + "No apply function found matching unapply parameters" + ) } // --- @@ -731,17 +732,18 @@ class JsMacroImpl(val c: blackbox.Context) { val defaultValue = // not applicable for 'write' only defaultValueMap.get(name).filter(_ => methodName != "write") -val resolvedImpl = { -val implTpeName = Option(impl.tpe).fold("null")(_.toString) + 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 (implTpeName.startsWith(forwardPrefix) || + (implTpeName.startsWith("play.api.libs.json") && + !(implTpeName.startsWith("play.api.libs.json.Functional") || + implTpeName.contains("MacroSpec")))) { + impl // Avoid extra check for builtin formats + } else { + q"""_root_.java.util.Objects.requireNonNull($impl, "Implicit value for '" + $cn + "' was null (forward reference?): " + ${implTpeName})""" + } + } // - If we're an default value, invoke the withDefault version // - If we're an option with default value, @@ -749,18 +751,19 @@ q"""_root_.java.util.Objects.requireNonNull($impl, "Invalid implicit resolution (isOption, defaultValue) match { case (true, Some(v)) => val c = TermName(s"${methodName}HandlerWithDefault") - q"$config.optionHandlers.$c($jspathTree, $v)($resolveImpl)" + q"$config.optionHandlers.$c($jspathTree, $v)($resolvedImpl)" - case (true, _) => + case (true, _) => { val c = TermName(s"${methodName}Handler") - q"$config.optionHandlers.$c($jspathTree)($resolveImpl)" + q"$config.optionHandlers.$c($jspathTree)($impl)" + } case (false, Some(v)) => val c = TermName(s"${methodName}WithDefault") - q"$jspathTree.$c($v)($resolveImpl)" + q"$jspathTree.$c($v)($resolvedImpl)" case _ => - q"$jspathTree.${TermName(methodName)}($resolveImpl)" + q"$jspathTree.${TermName(methodName)}($resolvedImpl)" } } .reduceLeft[Tree] { (acc, r) => diff --git a/play-json/shared/src/main/scala/play/api/libs/json/Writes.scala b/play-json/shared/src/main/scala/play/api/libs/json/Writes.scala index 362892212..70be3dc74 100644 --- a/play-json/shared/src/main/scala/play/api/libs/json/Writes.scala +++ b/play-json/shared/src/main/scala/play/api/libs/json/Writes.scala @@ -144,9 +144,7 @@ object OWrites extends PathWrites with ConstraintWrites { /** * Returns an instance which uses `f` as [[OWrites.writes]] function. */ - def apply[A](f: A => JsObject): OWrites[A] = new OWrites[A] { - def writes(a: A): JsObject = f(a) - } + def apply[A](f: A => JsObject): OWrites[A] = new FunctionalOWrites[A](f) /** * Transforms the resulting [[JsObject]] using the given function, @@ -159,6 +157,12 @@ object OWrites extends PathWrites with ConstraintWrites { OWrites[A] { a => f(a, w.writes(a)) } + + // --- + + private[json] final class FunctionalOWrites[A](w: A => JsObject) extends OWrites[A] { + def writes(a: A): JsObject = w(a) + } } /** @@ -177,9 +181,7 @@ object Writes extends PathWrites with ConstraintWrites with DefaultWrites with G /** * Returns an instance which uses `f` as [[Writes.writes]] function. */ - def apply[A](f: A => JsValue): Writes[A] = new Writes[A] { - def writes(a: A): JsValue = f(a) - } + def apply[A](f: A => JsValue): Writes[A] = new FunctionalWrites[A](f) /** * Transforms the resulting [[JsValue]] using the given function, @@ -194,6 +196,12 @@ object Writes extends PathWrites with ConstraintWrites with DefaultWrites with G Writes[A] { a => f(a, w.writes(a)) } + + // --- + + private[json] final class FunctionalWrites[A](w: A => JsValue) extends Writes[A] { + def writes(a: A): JsValue = w(a) + } } /** diff --git a/play-json/shared/src/test/scala-2/play/api/libs/json/JsonExtensionScala2Spec.scala b/play-json/shared/src/test/scala-2/play/api/libs/json/JsonExtensionScala2Spec.scala index 70ecf20f8..5deee2335 100644 --- a/play-json/shared/src/test/scala-2/play/api/libs/json/JsonExtensionScala2Spec.scala +++ b/play-json/shared/src/test/scala-2/play/api/libs/json/JsonExtensionScala2Spec.scala @@ -127,7 +127,9 @@ class JsonExtensionScala2Spec extends AnyWordSpec with Matchers { implicit val jsonConfiguration: JsonConfiguration.Aux[Json.WithDefaultValues] = JsonConfiguration[Json.WithDefaultValues](optionHandlers = OptionHandlers.WritesNull) val formatter = Json.format[OptionalWithDefault] - formatter.writes(OptionalWithDefault()).mustEqual(Json.obj("props" -> JsNull)) + + formatter.writes(OptionalWithDefault()).mustEqual(Json.obj("props" -> JsNull)) + formatter.writes(OptionalWithDefault(Some("foo"))).mustEqual(Json.obj("props" -> "foo")) formatter.reads(Json.obj()).mustEqual(JsSuccess(OptionalWithDefault())) 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 abd9560e3..b9325ff04 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 @@ -151,39 +151,29 @@ class MacroSpec extends AnyWordSpec with Matchers with org.scalatestplus.scalach 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 + try { + jsLorem.validate[Lorem[Simple]] + } catch { + case e: Throwable if e.getCause != null => + // scalatest ExceptionInInitializerError + throw e.getCause } - - 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 + val expected = "Implicit value for 'ipsum'" + npe.getMessage.take(expected.size).mustEqual(expected) } - case NonFatal(cause) => throw cause + case cause: Throwable => + cause.printStackTrace() + throw cause } } } "Writes" should { "be generated for simple case class" in { - Json.writes[Simple].writes(Simple("lorem")) mustEqual Json.obj("bar" -> "lorem") + Json.writes[Simple].writes(Simple("lorem")).mustEqual(Json.obj("bar" -> "lorem")) } "as Format for a generic case class" in { @@ -521,38 +511,6 @@ class MacroSpec extends AnyWordSpec with Matchers with org.scalatestplus.scalach formatter.writes(Obj).mustEqual(jsObj) formatter.reads(jsObj).mustEqual(JsSuccess(Obj)) } - - "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 - } - } } } @@ -584,11 +542,11 @@ object MacroSpec { object InvalidForwardResolution { // Invalids as forward references to `simpleX` - val simpleLoremReads = Json.reads[Lorem[Simple]] + 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 simpleReads: Reads[Simple] = Json.reads implicit val simpleWrites: OWrites[Simple] = Json.writes[Simple] }