play-json icon indicating copy to clipboard operation
play-json copied to clipboard

NullPointerException trying to pickle Coursier case class

Open lihaoyi opened this issue 8 years ago • 8 comments

$ amm
Loading...
Welcome to the Ammonite Repl 1.0.3
(Scala 2.12.4 Java 1.8.0_152)
If you like Ammonite, please support our development at www.patreon.com/lihaoyi

@ import $ivy.`com.typesafe.play::play-json:2.6.6`
import $ivy.$

@ import play.api.libs.json._
import play.api.libs.json._

@ {
    implicit val modFormat: Format[coursier.Module] = Json.format
    implicit val depFormat: Format[coursier.Dependency] = Json.format
    implicit val attrFormat: Format[coursier.Attributes] = Json.format
  }
modFormat: Format[coursier.package.Module] = play.api.libs.json.OFormat$$anon$1@295d8852
depFormat: Format[coursier.package.Dependency] = play.api.libs.json.OFormat$$anon$1@142409b4
attrFormat: Format[coursier.package.Attributes] = play.api.libs.json.OFormat$$anon$1@7445fdb2

@ Json.toJson(coursier.Dependency(coursier.Module("org.scala-lang", "scala-reflect"), "2.12.4"))
java.lang.NullPointerException
  play.api.libs.json.PathWrites.$anonfun$at$3(JsConstraints.scala:175)
  play.api.libs.json.OWrites$$anon$3.writes(Writes.scala:112)
  play.api.libs.json.OFormat$$anon$2.writes(Format.scala:46)
  play.api.libs.json.OWrites$MergedOWrites$.mergeIn(Writes.scala:76)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writeFields(Writes.scala:68)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writeFields(Writes.scala:64)
  play.api.libs.json.OWrites$OWritesFromFields.writes(Writes.scala:96)
  play.api.libs.json.OWrites$OWritesFromFields.writes$(Writes.scala:94)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writes(Writes.scala:64)
  play.api.libs.json.OFormat$$anon$2.writes(Format.scala:46)
  play.api.libs.json.OWrites$MergedOWrites$.mergeIn(Writes.scala:76)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writeFields(Writes.scala:67)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writeFields(Writes.scala:64)
  play.api.libs.json.OWrites$OWritesFromFields.writes(Writes.scala:96)
  play.api.libs.json.OWrites$OWritesFromFields.writes$(Writes.scala:94)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writes(Writes.scala:64)
  play.api.libs.json.OFormat$$anon$2.writes(Format.scala:46)
  play.api.libs.json.OWrites$MergedOWrites$.mergeIn(Writes.scala:76)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writeFields(Writes.scala:67)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writeFields(Writes.scala:64)
  play.api.libs.json.OWrites$OWritesFromFields.writes(Writes.scala:96)
  play.api.libs.json.OWrites$OWritesFromFields.writes$(Writes.scala:94)
  play.api.libs.json.OWrites$MergedOWrites$$anon$1.writes(Writes.scala:64)
  play.api.libs.json.OFormat$$anon$2.writes(Format.scala:46)
  play.api.libs.json.OFormat$$anon$5.$anonfun$inmap$2(Format.scala:29)
  play.api.libs.json.OFormat$$anon$1.writes(Format.scala:39)
  ammonite.$sess.cmd7$.$anonfun$depFormat$4(cmd7.sc:2)
  play.api.libs.json.OFormat$$anon$1.writes(Format.scala:39)
  play.api.libs.json.OFormat$$anon$1.writes(Format.scala:35)
  play.api.libs.json.Json$.toJson(Json.scala:189)
  ammonite.$sess.cmd8$.<init>(cmd8.sc:1)
  ammonite.$sess.cmd8$.<clinit>(cmd8.sc)

lihaoyi avatar Nov 01 '17 04:11 lihaoyi

FWIW this seems to work fine

  implicit val depFormat: Format[coursier.Dependency] =  new Format[coursier.Dependency] {
    def writes(o: Dependency) = {
      Json.obj(
        "module" -> Json.toJson(o.module),
        "version" -> Json.toJson(o.version),
        "configuration" -> Json.toJson(o.configuration),
        "exclusions" -> Json.toJson(o.exclusions),
        "attributes" -> Json.toJson(o.attributes),
        "optional" -> Json.toJson(o.optional),
        "transitive" -> Json.toJson(o.transitive)
      )
    }

    def reads(json: JsValue) = json match{
      case x: JsObject =>
        JsSuccess(coursier.Dependency(
          Json.fromJson[coursier.Module](x.value("module")).get,
          Json.fromJson[String](x.value("version")).get,
          Json.fromJson[String](x.value("configuration")).get,
          Json.fromJson[coursier.Attributes](x.value("attributes")).get,
          Json.fromJson[Set[(String, String)]](x.value("exclusions")).get,
          Json.fromJson[Boolean](x.value("optional")).get,
          Json.fromJson[Boolean](x.value("transitive")).get
        ))

      case _ => JsError("Dep must be an object")
    }
  }

lihaoyi avatar Nov 01 '17 04:11 lihaoyi

Hi, thanks for reporting.

Please note that the following codes are working:

{
  implicit val modFormat: Format[coursier.Module] = Json.format
  implicit val attrFormat: Format[coursier.Attributes] = Json.format
  implicit val depFormat: Format[coursier.Dependency] = Json.format

  Json.toJson(coursier.Dependency(coursier.Module("org.scala-lang", "scala-reflect"), "2.12.4"))
}

With instance of Format[Attributes] declared before the one for Dependency, which makes sense as the Dependency type depends on the Attributes one.

{
  implicit val modFormat: Format[coursier.Module] = Json.format
  implicit val depFormat: Format[coursier.Dependency] = Json.format
  implicit lazy val attrFormat: Format[coursier.Attributes] = Json.format

  Json.toJson(coursier.Dependency(coursier.Module("org.scala-lang", "scala-reflect"), "2.12.4"))
}

With a lazy declaration.

From there I would say that the issue, not specific to Play JSON, is a forward declaration of implicit instances.

Best regards.

cchantep avatar Nov 01 '17 14:11 cchantep

We had a similar issue (pasting stack because it points to a different line)

[error]    java.lang.NullPointerException: null (JsConstraints.scala:70)
[error] play.api.libs.json.PathReads.$anonfun$nullableWithDefault$5(JsConstraints.scala:70)
[error] play.api.libs.json.JsSuccess.fold(JsResult.scala:16)
[error] play.api.libs.json.PathReads.$anonfun$nullableWithDefault$3(JsConstraints.scala:68)
[error] play.api.libs.json.PathReads.$anonfun$nullableWithDefault$1(JsConstraints.scala:66)
[error] play.api.libs.json.Reads$$anon$8.reads(Reads.scala:132)
[error] play.api.libs.json.Reads$$anon$3$$anon$4.reads(Reads.scala:110)
[error] play.api.libs.json.Reads.$anonfun$map$1(Reads.scala:43)
[error] play.api.libs.json.Reads$$anon$8.reads(Reads.scala:132)

The case class Foo it was trying to read contains an Option[Bar] value with default None. The Json.reads[Foo] macro that depends on an implicit Reads[Bar] that was indeed declared before the implicit Reads[Bar]. As explained above moving the implicit Reads[Bar] in front of this one fixed the Nullpointer.

Would it be possible to detect this case in the macro and throw a compile-time exception?

francisdb avatar Nov 02 '17 08:11 francisdb

@francisdb not sure it's possible. Btw the same kind of NPE is raise for val using forward declaration (not specific to macro).

cchantep avatar Nov 02 '17 08:11 cchantep

A simple null check on the reads after implicit resolution could throw an exception indicating that this forward declaration might be the cause? Anything is better than a NullpointerException deep in the play-json code.

francisdb avatar Nov 02 '17 09:11 francisdb

Unfortunately, it doesn't seem possible for a macro to check the actual position of an resolved implicit instance.

On the other side, as such forward reference raises a compilation warning as bellow, it can be handled (or blocked by making the warning fatal).

...: Reference to uninitialized value ...

cchantep avatar Nov 02 '17 21:11 cchantep

@cchantep Thanks, it seems you are right that the initialization order is the problem.

Perhaps one mitigation would be to change the examples in the readme to use lazy val instead of val. That seems to resolve the initialization order problems for me, and shouldn't result in a noticeable performance impact

Also, while it may be unavoidable to throw a NPE, I wonder if it is possible to tweak the macro to throw an NPE as early as possible? Ideally I'd like the NPE to be thrown when defining the vals, rather than when using them. Perhaps sprinkling a few Object.requireNotNulls in the macro-generated code would be enough to surface these errors at the implicit declaration site, which would make them far easier to recognize and debug.

lihaoyi avatar Nov 05 '17 06:11 lihaoyi

It still exists in version 2.9.0. It is really annoying, since it is a runtime error + there is absolutely no sign where the problem is or what type is missing the required Writes...

b-gyula avatar Apr 14 '21 10:04 b-gyula