extractOpt[String] seems to be broken in 3.6.x

[klawoj]

json4s 3.6.7

scala version 2.12.10

jdk version 1.8.0_92

In 3.5.x I could do:

val extractedOptionalField = (myJsonObject \ "myOptionalField").extractOpt[String]

Which worked, and produced None when "myOptionalField" is not present in the object. For some reason this does not work in 3.6.x when the field is missing (throws org.json4s.package$MappingException: Did not find value which can be converted into java.lang.String). As a workaround I found that i can use extract[Option[String]] and it works as expected. But I do not understand what is the point of extractOpt if it throws an exception in case the field is missing? What is the reason the behavior changed? For me it seems broken.

[magnolia-k]

@klawoj

I couldn't reproduce with version 3.6.7 and 3.7.0-M2. Can you provide a set of reproducible projects?

scala> import org.json4s._
import org.json4s._

scala> import org.json4s.jackson.JsonMethods._
import org.json4s.jackson.JsonMethods._

scala> implicit val formats = DefaultFormats
formats: org.json4s.DefaultFormats.type = org.json4s.DefaultFormats$@5858bee

scala> val myJsonObject = parse("""{"key" : "value"}""")
myJsonObject: org.json4s.JValue = JObject(List((key,JString(value))))

scala> val extractedOptionalField = (myJsonObject \ "myOptionalField").extractOpt[String]
extractedOptionalField: Option[String] = None

scala> val extractedOptionalField = (myJsonObject \ "key").extractOpt[String]
extractedOptionalField: Option[String] = Some(value)

[klawoj]

@magnolia-k It seems that strictOptionParsing, which we use, makes a difference here, if you use

implicit val formats: Formats = new DefaultFormats { override val strictOptionParsing = true }

Then your code will fail as described

[magnolia-k]

The issue is the code below. When formats.strictOptionParsing is false, the exception is replaced with None.

https://github.com/json4s/json4s/blob/715bc01391c123fa55bcc6391a3def17ad02927e/core/src/main/scala/org/json4s/Extraction.scala#L53-L54

JNothing is returned when the specified key does not exist. But there is no suitable value to convert JNothing.

And the README states that:

Note that when the extraction of an Option[_] fails, the default behavior of extract is to return None. You can make it fail with a [MappingException] by using a custom Formats object:

When strictOptionParsing is true, it seems correct to raise an exception, even for Option [String].

[klawoj]

Ok, I understand that this is the reason why the exception happens, but It does not explain why JNothing.extractOpt[String] should behave differently than JNothing.extract[Option[String]] when strictOptionParsing is true

[magnolia-k]

In the following code, toSome is called and JNothing is converted to None, so None is returned as it is, but originally it seems that the correct behavior is to return MappingException in case of JNothing here.

https://github.com/json4s/json4s/blob/7514b14a66580dbc825b4b49c8ad5aae75d7d715/core/src/main/scala/org/json4s/Extraction.scala#L377

If the code is modified like that, it will behave as described in the document, and the behavior will be the same with extract and extractOpt.

[magnolia-k]

Probably if the code changes like this, the behavior should be the same with extract and extractOpt.

@@ -374,7 +374,12 @@ object Extraction {
         Right(extract(json, scalaType.typeArgs(1)))
       })).getOrElse(fail("Expected value but got " + json))
     } else if (scalaType.isOption) {
-      customOrElse(scalaType, json)(v => (if(formats.strictOptionParsing) v.toSome else v.toOption) flatMap (j => Option(extract(j, scalaType.typeArgs.head))))
+      customOrElse(scalaType, json) { v =>
+        v match {
+          case JNothing if formats.strictOptionParsing => fail("JNothing cannot be converted into " + scalaType.fullName)
+          case _ => v.toSome flatMap (j => Option(extract(j, scalaType.typeArgs.head)))
+        }
+      }
     } else if (scalaType.isMap) {
       customOrElse(scalaType, json)({
         _ match {

[jgulotta]

We just ran into this issue due to the silent change in semantics between 3.5 and 3.6, and this is not the first null/nothing change that has bitten us with json4s. The next most recent one had to do with the change in behavior under the combination of toSome and allowNull = false, where there is no way to get the old behavior back, somewhat related to #664.

The proposed fix above would not work for us because currently we want JNothing to map to None when strictOptionParsing = true, due to the fact that we want extract to fail in buildCtorArg

case e @ MappingException(msg, _) =>
            if (descr.isOptional && !formats.strictOptionParsing) {
              defv(None)
            } else fail("No usable value for " + descr.name + "\n" + msg, e)

The fundamental issue is that allowNull and strictOptionParsing control too many things and their usage has been inconsistent and ill-defined, even changing behavior in patch version releases. Namely, these two flags control both the handling of multiple special values in the input for both optional and non-optional types and also handling the result of deserializing the inner value for options. There are too many combinations for two boolean flags to be flexible enough.

I think it would be ideal for these cases to be separated explicitly. The NullExtractionStrategy in newer branches is a decent start.

These are the potential cases I can see people wanting depending on their domain semantics

Input	Destination Type	Result
JNothing	T	null
JNothing	T	default constructor value
JNothing	T	custom default value
JNothing	T	MappingException
JNull	T	null
JNull	T	default constructor value
JNull	T	custom default value
JNull	T	MappingException
JNothing	Option[T]	null
JNothing	Option[T]	None
JNothing	Option[T]	Some(null)
JNothing	Option[T]	Some(default constructor value)
JNothing	Option[T]	Some(custom default value)
JNothing	Option[T]	Some(delegate to T handler), propagate MappingException
JNothing	Option[T]	Option(delegate to T handler), propagate MappingException
JNothing	Option[T]	Some(delegate to T handler), None on MappingException
JNothing	Option[T]	Option(delegate to T handler), None on MappingException
JNothing	Option[T]	MappingException
JNull	Option[T]	null
JNull	Option[T]	None
JNull	Option[T]	Some(null)
JNull	Option[T]	Some(default constructor value)
JNull	Option[T]	Some(custom default value)
JNull	Option[T]	Some(delegate to T handler), propagate MappingException
JNull	Option[T]	Option(delegate to T handler), propagate MappingException
JNull	Option[T]	Some(delegate to T handler), None on MappingException
JNull	Option[T]	Option(delegate to T handler), None on MappingException
JNull	Option[T]	MappingException

Crucially, people may want to choose different handling for each combination of JNull/JNothing and T/Option[T], potentially having different behavior depending on the type being deserialized. For instance, maybe most things should propagate the mapping exception but some should default to None. This suggests to me that the options would be best placed as part of the type-specific serializer instead of the generic formats.

In such a scenario it would make sense to provide defaults, and I suspect what most people would find intuitive/reasonable there is

Input	Destination Type	Result
JNothing	T	MappingException
JNull	T	MappingException
JNothing	Option[T]	None
JNull	Option[T]	Option(delegate to T handler), propagate MappingException

This aligns with typical usages in Scala where references are assumed to be non-null and T means "required", but allows an explicit null in the JSON to take on special meaning for the underlying type instead of the optional context.

Of course, json4s could choose to only support some of these as a design decision, but that decision should be documented and consistently maintained for a release series.