playframework / play-json

The Play JSON library
Apache License 2.0
361 stars 134 forks source link

Monoid[JsObject] does not satisfy associative law #6

Open gmethvin opened 7 years ago

gmethvin commented 7 years ago

(Moved from playframework/playframework#4651)

Welcome to Scala version 2.11.6 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_45).
Type in expressions to have them evaluated.
Type :help for more information.

scala> import play.api.libs.json._
import play.api.libs.json._

scala> import play.api.libs.json.Reads.JsObjectMonoid
import play.api.libs.json.Reads.JsObjectMonoid

scala> val a = Json.obj("key" -> Json.obj("x" -> 1))
a: play.api.libs.json.JsObject = {"key":{"x":1}}

scala> val b = Json.obj("key" -> 2)
b: play.api.libs.json.JsObject = {"key":2}

scala> val c = Json.obj("key" -> Json.obj("y" -> 3))
c: play.api.libs.json.JsObject = {"key":{"y":3}}

scala> JsObjectMonoid.append(JsObjectMonoid.append(a, b), c)
res0: play.api.libs.json.JsObject = {"key":{"y":3}}

scala> JsObjectMonoid.append(a, JsObjectMonoid.append(b, c))
res1: play.api.libs.json.JsObject = {"key":{"x":1,"y":3}}
libraryDependencies += "com.typesafe.play" %% "play-json" % "2.4.0"

scalaVersion := "2.11.6"
jdrphillips commented 7 years ago

What is the solution to this? You can't have a Monoid that isn't associative, it makes it unpredictable and dangerous.

The only JsObject monoid that makes sense is replacing the keys if a duplicate is found on a summand, not deepMerge.

I would be in favour of either removing the monoid instance entirely or implementing the above, but it is a major behaviour change either way.

I think the current solution of just keeping it is the worst of the three.

jdrphillips commented 7 years ago

A further solution is the same as lift's:

    def ++(other: JValue) = {
      def append(value1: JValue, value2: JValue): JValue = (value1, value2) match {
        case (JNothing, x) => x
        case (x, JNothing) => x
        case (JArray(xs), JArray(ys)) => JArray(xs ::: ys)
        case (JArray(xs), v: JValue) => JArray(xs ::: List(v))
        case (v: JValue, JArray(xs)) => JArray(v :: xs)
        case (x, y) => JArray(x :: y :: Nil)
      }
      append(this, other)
    }

where everything becomes an array on addition. Which is not obvious behaviour but is at least associative