Przeglądaj źródła

ensure :block/uuid immutability

Tienson Qin 2 tygodni temu
rodzic
commit
1d98f1d214

+ 9 - 22
deps/db-sync/src/logseq/db_sync/checksum.cljs

@@ -154,11 +154,6 @@
          attr
          (normalize-checksum-value db attr (:v datom))]))))
 
-(defn- existing-entity-in-db?
-  [db eid]
-  (and (number? eid)
-       (some? (d/entity db eid))))
-
 (defn recompute-checksum
   [db]
   (let [e2ee? (ldb/get-graph-rtc-e2ee? db)
@@ -206,21 +201,15 @@
             initial-state (if (valid-checksum? checksum)
                             (checksum->state checksum)
                             (checksum->state (recompute-checksum db-before)))
-            ;; UUID mutation on an existing entity can implicitly affect
-            ;; normalized parent/page tuples of referencing entities.
-            ;; Keep incremental logic simple and robust by full recompute.
-            existing-uuid-mutation?
-            (some (fn [{:keys [a e]}]
-                    (and (= :block/uuid a)
-                         (existing-entity-in-db? db-before e)))
-                  tx-data)
             attrs (relevant-attrs after-e2ee?)
-            removed-tuples (keep #(when (false? (:added %))
-                                    (datom->checksum-tuple db-before attrs %))
-                                tx-data)
-            added-tuples (keep #(when (:added %)
-                                  (datom->checksum-tuple db-after attrs %))
-                              tx-data)
+            removed-tuples (->> tx-data
+                                (keep #(when (false? (:added %))
+                                         (datom->checksum-tuple db-before attrs %)))
+                                set)
+            added-tuples (->> tx-data
+                              (keep #(when (:added %)
+                                       (datom->checksum-tuple db-after attrs %)))
+                              set)
             state-after-removals (reduce (fn [checksum-state tuple]
                                            (subtract-digest checksum-state (tuple-digest tuple)))
                                          initial-state
@@ -229,6 +218,4 @@
                                             (add-digest checksum-state (tuple-digest tuple)))
                                           state-after-removals
                                           added-tuples)]
-        (if existing-uuid-mutation?
-          (recompute-checksum db-after)
-          (state->checksum state-after-additions))))))
+        (state->checksum state-after-additions)))))

+ 0 - 18
deps/db-sync/test/logseq/db_sync/checksum_test.cljs

@@ -123,24 +123,6 @@
       (is (not= before-checksum full))
       (is (= full incremental)))))
 
-(deftest incremental-checksum-matches-recompute-when-referenced-uuid-is-retracted-test
-  (testing "incremental checksum updates dependents when a referenced entity loses its block UUID"
-    (let [db-before (sample-db)
-          parent-uuid (:block/uuid (d/entity db-before 3))
-          tx-report (d/with db-before [[:db/retract 3 :block/uuid parent-uuid]])
-          full (checksum/recompute-checksum (:db-after tx-report))
-          incremental (checksum/update-checksum (checksum/recompute-checksum db-before) tx-report)]
-      (is (= full incremental)))))
-
-(deftest incremental-checksum-matches-recompute-when-block-uuid-changes-test
-  (testing "incremental checksum matches full recompute when an existing block UUID changes"
-    (let [db-before (sample-db)
-          new-parent-uuid (random-uuid)
-          tx-report (d/with db-before [[:db/add 3 :block/uuid new-parent-uuid]])
-          full (checksum/recompute-checksum (:db-after tx-report))
-          incremental (checksum/update-checksum (checksum/recompute-checksum db-before) tx-report)]
-      (is (= full incremental)))))
-
 (deftest incremental-checksum-matches-recompute-when-block-is-readded-test
   (testing "incremental checksum remains equal to recompute when a block is deleted and re-added with the same UUID"
     (let [db0 (sample-db)

+ 54 - 25
deps/db/src/logseq/db/frontend/validate.cljs

@@ -21,10 +21,35 @@
   [closed-schema?]
   (if closed-schema? closed-db-schema-explainer db-schema-explainer))
 
+(defn- block-uuid-immutability-errors
+  [{:keys [db-before db-after tx-data tx-meta]}]
+  (let [uuid-touched-existing-eids
+        (->> tx-data
+             (keep (fn [{:keys [e a]}]
+                     (when (and (= :block/uuid a)
+                                (number? e)
+                                (some? (:block/uuid (d/entity db-before e))))
+                       e)))
+             distinct)]
+    (->> uuid-touched-existing-eids
+         (keep (fn [eid]
+                 (let [before-uuid (:block/uuid (d/entity db-before eid))
+                       after-ent (d/entity db-after eid)
+                       after-uuid (:block/uuid after-ent)
+                       deleted? (nil? after-ent)]
+                   (when (and (not deleted?)
+                              (not= before-uuid after-uuid))
+                     {:entity-map {:db/id eid
+                                   :block/uuid before-uuid
+                                   :block/uuid-after after-uuid}
+                      :errors {:block/uuid ["immutable for existing entities; use :db/retractEntity to delete entities"]}
+                      :tx-meta tx-meta}))))
+         vec)))
+
 (defn validate-tx-report
   "Validates the datascript tx-report for entities that have changed. Returns
   boolean indicating if db is valid"
-  [{:keys [db-after tx-data tx-meta]} {:keys [closed-schema?]}]
+  [{:keys [db-before db-after tx-data tx-meta] :as tx-report} {:keys [closed-schema?]}]
   (binding [db-malli-schema/*skip-strict-url-validate?* true]
     (let [changed-ids (->> tx-data (keep :e) distinct)
           datoms (d/datoms db-after :eavt)
@@ -37,30 +62,34 @@
           validator (get-schema-validator closed-schema?)]
       (binding [db-malli-schema/*db-for-validate-fns* db-after]
         (let [invalid-ent-maps (remove
-                              ;; remove :db/id as it adds needless declarations to schema
-                                #(validator [(dissoc % :db/id)])
-                                ent-maps)]
-          (if (seq invalid-ent-maps)
-            (do
-              (prn "Invalid datascript entities detected amongst changed entity ids:" changed-ids :tx-meta tx-meta)
-              (let [explainer (get-schema-explainer closed-schema?)
-                    errors (doall
-                            (map
-                             (fn [m]
-                               (let [m' (update m :block/properties (fn [properties]
-                                                                      (map (fn [[p v]]
-                                                                             [(:db/ident p) v])
-                                                                           properties)))
-                                     data {:entity-map m'
-                                           :errors (me/humanize (explainer [(dissoc m :db/id)]))}]
-                                 (try
-                                   (pprint/pprint data)
-                                   (catch :default _e
-                                     (prn data)))
-                                 data))
-                             invalid-ent-maps))]
-
-                [false errors]))
+                                ;; remove :db/id as it adds needless declarations to schema
+                                 #(validator [(dissoc % :db/id)])
+                                 ent-maps)
+              schema-errors
+              (when (seq invalid-ent-maps)
+                (prn "Invalid datascript entities detected amongst changed entity ids:" changed-ids :tx-meta tx-meta)
+                (let [explainer (get-schema-explainer closed-schema?)]
+                  (doall
+                   (map
+                    (fn [m]
+                      (let [m' (update m :block/properties (fn [properties]
+                                                             (map (fn [[p v]]
+                                                                    [(:db/ident p) v])
+                                                                  properties)))
+                            data {:entity-map m'
+                                  :errors (me/humanize (explainer [(dissoc m :db/id)]))}]
+                        (try
+                          (pprint/pprint data)
+                          (catch :default _e
+                            (prn data)))
+                        data))
+                    invalid-ent-maps))))
+              uuid-errors (block-uuid-immutability-errors tx-report)
+              errors (->> (concat schema-errors uuid-errors)
+                          (remove nil?)
+                          vec)]
+          (if (seq errors)
+            [false errors]
             [true nil]))))))
 
 (defn- group-errors-by-entity

+ 28 - 0
deps/db/test/logseq/db_test.cljs

@@ -112,6 +112,34 @@
                                     :block/tags :logseq.class/Property}])
          (ldb/transact! temp-conn [[:db/retract :logseq.class/Task :block/tags :logseq.class/Property]]))))))
 
+(deftest block-uuid-is-immutable-for-existing-entity-test
+  (let [conn (db-test/create-conn-with-blocks
+              {:pages-and-blocks
+               [{:page {:block/title "page1"}}]})
+        page (db-test/find-page-by-title @conn "page1")
+        db-id (:db/id page)
+        old-uuid (:block/uuid page)
+        new-uuid (random-uuid)]
+    (testing "cannot replace :block/uuid on existing entity"
+      (is (thrown? js/Error
+                   (db-test/silence-stderr
+                    (ldb/transact! conn [[:db/add db-id :block/uuid new-uuid]]))))
+      (is (= old-uuid (:block/uuid (d/entity @conn db-id)))))
+    (testing "cannot retract :block/uuid on existing entity"
+      (is (thrown? js/Error
+                   (db-test/silence-stderr
+                    (ldb/transact! conn [[:db/retract db-id :block/uuid old-uuid]]))))
+      (is (= old-uuid (:block/uuid (d/entity @conn db-id)))))))
+
+(deftest block-uuid-immutability-allows-retract-entity-test
+  (let [conn (db-test/create-conn-with-blocks
+              {:pages-and-blocks
+               [{:page {:block/title "page1"}}]})
+        page (db-test/find-page-by-title @conn "page1")
+        db-id (:db/id page)]
+    (ldb/transact! conn [[:db/retractEntity db-id]])
+    (is (nil? (d/entity @conn db-id)))))
+
 (deftest get-bidirectional-properties
   (testing "disabled by default"
     (let [conn (db-test/create-conn-with-blocks