Incorrect JSON Null reading for value types

amnaredo commented 3 years ago

Reading basic data values types from JSON Null values should trigger errors since values types in Scala are not nullable.

Currently, reading a JSON Null value produces the default value for given value type which is typically some form of zero. This is also inconsistent with JSON specification which defines null as a separate data type unrelated to number.

Environment

uPickle: 1.4.0
Scala: 3.0.0
JRE: 17

Reproduce

import ujson.Null
import upickle.default.read

object Main extends App:

  // Produces default value for Int
  def readValue = read[Int](Null)
  println(s"\nread[Int](Null):\n$readValue")

  // Interestingly, using the same expression in String interpolation produces null instead
  val interpolated = s"\nInterpolated read[Int](Null):\n${read[Int](Null)}"
  println(interpolated)

Result

read[Int](Null):
0

Interpolated read[Int](Null):
null

Expected

read[Int](Null):
<unexpected data type error>

Interpolated read[Int](Null):
<unexpected data type error>

Fix

Add the following method override to each value type Reader (e.g. IntReader):

override def visitNull(index: Int) = throw new Abort(expectedMsg + " got null")

Workaround

Add the equivalent implicit overrides for each value type Reader to your custom configuration:

  // IntReader override
  implicit override val IntReader: Reader[Int] =
    new SimpleReader[Int] {

      override def expectedMsg = "expected number"
      override def visitInt32(d: Int, index: Int) = d
      override def visitInt64(d: Long, index: Int) = d.toInt
      override def visitUInt64(d: Long, index: Int) = d.toInt
      override def visitFloat64(d: Double, index: Int) = d.toInt

      override def visitFloat64StringParts(s: CharSequence, decIndex: Int, expIndex: Int, index: Int) =
        Util.parseIntegralNum(s, decIndex, expIndex, index).toInt
      override def visitNull(index: Int) = throw new Abort(expectedMsg + " got null")
    }

   // Similar for other values types
   // ...

ID: 355 Original Author: martin-ockajak

amnaredo commented 3 years ago

The proposed change makes sense to me. We took this a step further in https://github.com/rallyhealth/weePickle/pull/77 by making SimpleVisitor throw on visitNull so that even reference types are null-hostile unless overridden.

And yet, there are upickle tests that explicitly assert the current behavior: https://github.com/com-lihaoyi/upickle/blob/28a860bd490e0d541e8b30a70dd1bbb80fa7fdb2/upickle/test/src/upickle/PrimitiveTests.scala#L67

For context, this behavior comes from scala:

Welcome to the Ammonite Repl 2.2.0 (Scala 2.13.3 Java 15.0.1)
@ null.asInstanceOf[Int]
res0: Int = 0

JS coerces to the same values if needed.

Welcome to Node.js v14.15.0.
Type ".help" for more information.
> null + 1
1
> null || true
true

Regardless, this type coercion seems slightly https://www.destroyallsoftware.com/talks/wat in a language like scala.

@lihaoyi, how would you like to handle? Original Author: htmldoug

amnaredo commented 3 years ago

Could we make this easily configurable on the custom configuration with a flag? I'd be inclined to leave the default behavior unchanged, but it should be easy to do a single override def throwOnUnexpectedNulls = true to get the new behavior (whether just for primitive types, for primitives + macroRWs, or for all reference types as well).

It might take some plumbing to get that boolean to all the relevant places, but I think the current behavior is both long lived and also-kinda-correct enough that we should make the stricter behavior opt-in Original Author: lihaoyi

amnaredo commented 3 years ago

Here's a start to mimic weepickle's all-or-nothing null handling: https://github.com/com-lihaoyi/upickle/commit/c301c6e6e8cd1429f67f23743d5dd184ff0abd27. I'm stopping short of submitting a PR since:

It introduces complexity.
It doesn't support targeting only primitives, i.e. this issue.
I don't actually need it.

Going to leave for someone else to pick up the torch if they want it. Original Author: htmldoug

amnaredo commented 3 years ago

Could we make this easily configurable on the custom configuration with a flag? I'd be inclined to leave the default behavior unchanged, but it should be easy to do a single override def throwOnUnexpectedNulls = true to get the new behavior (whether just for primitive types, for primitives + macroRWs, or for all reference types as well).

Perhaps this calls for an additional default strict uPickle Api instance implementing the above mechanism and thus granting straightforward access to the stricter behavior, i.e.:

import upickle.strict._

This would follow well-known enable strict mode pattern and would be easy to document.

Original Author: martin-ockajak

amnaredo / test