[BREAKING] Get smarter about diffing records

Before we treated them as maps, which meant maps and records with the same
key/values were considered equal. We now only compare records if the two
compared values are both records, and are of the same type. Otherwise it's a
Mismatch.

Author: plexus

CHANGELOG.md

@@ -1,11 +1,12 @@
 # Unreleased
 
-## Added
-
-## Fixed
-
 ## Changed
 
+- [BREAKING] Get smarter about diffing records, instead of simply diffing them
+  as maps. We now only recurse into records if the two compared values are both
+  records of the same type.
+- Bump dependencies: fipp, rrb-vector
+
 # 2.11.216 (2024-02-17 / e77c3bf)
 
 ## Added
@@ -14,7 +15,7 @@
 
 ## Fixed
 
-Varying key order in maps should produce a consistent diff (#47)
+- Varying key order in maps should produce a consistent diff (#47)
 
 ## Changed
 
@@ -180,4 +181,4 @@ Varying key order in maps should produce a consistent diff (#47)
 
 ## Added
 
-- Extracted from Kaocha, and added a top-level namespace.
+- Extracted from Kaocha, and added a top-level namespace.

bb.edn

@@ -1,7 +1,7 @@
 {:deps
  {lambdaisland/deep-diff2 {:local/root "."}
   lambdaisland/open-source {:git/url "https://github.com/lambdaisland/open-source"
-                            :git/sha "7ce125cbd14888590742da7ab3b6be9bba46fc7a"}}
+                            :git/sha "4cb6231e7df947ee8f5434e7ad48ffd8e273bcf5"}}
  :tasks
  {test:bb {:doc "Run babashka tests with custom runner"
            :extra-paths ["src" "test"]

bin/kaocha

@@ -2,4 +2,4 @@
 
 [[ -d "node_modules/ws" ]] || npm install ws
 
-exec clojure -A:dev:test -m kaocha.runner "$@"
+exec clojure -A:dev:test -M -m kaocha.runner "$@"

deps.edn

@@ -1,6 +1,6 @@
 {:paths ["resources" "src"]
- :deps  {fipp/fipp                   {:mvn/version "0.6.26"}
-         org.clojure/core.rrb-vector {:mvn/version "0.1.2"}
+ :deps  {fipp/fipp                   {:mvn/version "0.6.27"}
+         org.clojure/core.rrb-vector {:mvn/version "0.2.0"}
          lambdaisland/clj-diff       {:mvn/version "1.4.78"}
          mvxcvi/arrangement          {:mvn/version "2.1.0"}}
 

package.json

@@ -4,6 +4,6 @@
   "dependencies": {
     "react": "^16.13.1",
     "react-dom": "^16.13.1",
-    "ws": "^8.13.0"
+    "ws": "^8.18.0"
   }
 }

src/lambdaisland/deep_diff2/diff_impl.cljc

@@ -114,24 +114,36 @@
     (diff-meta exp act)))
 
 (defn diff-map [exp act]
-  (with-meta
-    (let [exp-ks (set (keys exp))
-          act-ks (set (keys act))]
-      (reduce
-       (fn [m k]
-         (case [(contains? exp-ks k) (contains? act-ks k)]
-           [true false]
-           (assoc m (->Deletion k) (get exp k))
-           [false true]
-           (assoc m (->Insertion k) (get act k))
-           [true true]
-           (assoc m k (diff (get exp k) (get act k)))
-          ; `[false false]` will never occur because `k` necessarily
-          ; originated from at least one of the two sets
-           ))
-       {}
-       (set/union exp-ks act-ks)))
-    (diff-meta exp act)))
+  (if (not= (record? exp) (record? act))
+    ;; If one of them is a record, and the other one a plain map, that's a
+    ;; mismatch. The case where both of them are records, but of different
+    ;; types, is handled in [[diff]]
+    (->Mismatch exp act)
+    (with-meta
+      (let [exp-ks (set (keys exp))
+            act-ks (set (keys act))]
+        (reduce
+         (fn [m k]
+           (case [(contains? exp-ks k) (contains? act-ks k)]
+             [true false]
+             ;; The `dissoc` is only relevant for records, which at this point
+             ;; we are certain are of the same type. If the key is present in
+             ;; one and not in the other, we know it's an optional key (not part
+             ;; of the record base), and we can safely `dissoc` it while
+             ;; retaining the record type.
+             (assoc (dissoc m k) (->Deletion k) (get exp k))
+             [false true]
+             (assoc m (->Insertion k) (get act k))
+             [true true]
+             (assoc m k (diff (get exp k) (get act k)))
+             ;; `[false false]` will never occur because `k` necessarily
+             ;; originated from at least one of the two sets
+             ))
+         ;; In case of a record, we want to preserve the type, and you can't
+         ;; call `empty` on records, so we start from `exp` and assoc/dissoc.
+         (if (record? exp) exp {})
+         (set/union exp-ks act-ks)))
+      (diff-meta exp act))))
 
 (defn diff-meta [exp act]
   (when (or (meta exp) (meta act))
@@ -160,19 +172,26 @@
 
 (defn diff [exp act]
   (cond
+    (= exp act)
+    exp
+
     (nil? exp)
     (diff-atom exp act)
 
+    (record? exp)
+    (if (= (type exp) (type act))
+      (diff-map exp act)
+      ;; Either act is not a record, or it's a record of a different type, so
+      ;; that's a mismatch
+      (->Mismatch exp act))
+
     (and (diffable? exp)
          (= (data/equality-partition exp) (data/equality-partition act)))
     (diff-similar exp act)
 
     (array? exp)
     (diff-seq exp act)
 
-    (record? exp)
-    (diff-map exp act)
-
     :else
     (diff-atom exp act)))
 

test/lambdaisland/deep_diff2/diff_test.cljc

@@ -1,12 +1,14 @@
 (ns lambdaisland.deep-diff2.diff-test
-  (:require [clojure.test :refer [deftest testing is are]]
-            [clojure.test.check :as tc]
-            [clojure.test.check.clojure-test :refer [defspec]]
-            [clojure.test.check.generators :as gen]
-            [clojure.test.check.properties :as prop]
-            [lambdaisland.deep-diff2.diff-impl :as diff]))
+  (:require
+   [clojure.test :refer [deftest testing is are]]
+   [clojure.test.check :as tc]
+   [clojure.test.check.clojure-test :refer [defspec]]
+   [clojure.test.check.generators :as gen]
+   [clojure.test.check.properties :as prop]
+   [lambdaisland.deep-diff2.diff-impl :as diff]))
 
 (defrecord ARecord [])
+(defrecord BRecord [])
 
 (deftest diff-test
   (testing "diffing atoms"
@@ -111,12 +113,20 @@
                         (array-map :age 40 :name "Alyssa P Hacker")))))
 
     (testing "records"
-      (is (= {:a (diff/->Mismatch 1 2)}
+      (is (= (map->ARecord {:a (diff/->Mismatch 1 2)})
              (diff/diff (map->ARecord {:a 1}) (map->ARecord {:a 2}))))
-      (is (= {(diff/->Insertion :a) 1}
+      (is (= (map->ARecord {(diff/->Insertion :a) 1})
              (diff/diff (map->ARecord {}) (map->ARecord {:a 1}))))
-      (is (= {(diff/->Deletion :a) 1}
-             (diff/diff (map->ARecord {:a 1}) (map->ARecord {}))))))
+      (is (= (map->ARecord {(diff/->Deletion :a) 1})
+             (diff/diff (map->ARecord {:a 1})
+                        (map->ARecord {}))))
+      (is (= (diff/->Mismatch (map->ARecord {:a 1}) (map->BRecord {:a 1}))
+             (diff/diff (map->ARecord {:a 1})
+                        (map->BRecord {:a 1}))))
+      (is (= (diff/->Mismatch {:a 1} (map->ARecord {:a 1}))
+             (diff/diff {:a 1} (map->ARecord {:a 1}))))
+      (is (= (diff/->Mismatch (map->ARecord {:a 1}) {:a 1})
+             (diff/diff (map->ARecord {:a 1}) {:a 1})))))
 
   (is (= [{:x (diff/->Mismatch 1 2)}]
          (diff/diff [{:x 1}] [{:x 2}])))