fix(outliner): stabilize history op entity refs

deps/outliner/src/logseq/outliner/op/construct.cljc

@@ -187,6 +187,21 @@
   [db ids]
   (mapv #(stable-entity-ref db %) ids))
 
+(defn- stable-block-uuid
+  [db x]
+  (let [ref (stable-entity-ref db x)]
+    (cond
+      (uuid? ref)
+      ref
+
+      (and (vector? ref)
+           (= :block/uuid (first ref))
+           (uuid? (second ref)))
+      (second ref)
+
+      :else
+      ref)))
+
 (defn- resolve-target-and-sibling
   [block]
   (if-let [left-sibling (ldb/get-left-sibling block)]
@@ -301,7 +316,36 @@
 
 (defn- maybe-rewrite-delete-block-ids
   [db tx-data ids]
-  (let [ids' (stable-id-coll db ids)
+  (let [deleted-ids-from-tx-data (->> tx-data
+                                      (keep (fn [item]
+                                              (cond
+                                                (and (vector? item)
+                                                     (= :db/retractEntity (first item))
+                                                     (>= (count item) 2))
+                                                (second item)
+
+                                                ;; d/with-style datom maps after retractEntity
+                                                ;; can be used to recover deleted entity ids.
+                                                (and (map? item)
+                                                     (false? (:added item))
+                                                     (= :block/uuid (:a item)))
+                                                (:e item)
+
+                                                ;; datascript Datom records from d/with
+                                                (and (some? (:a item))
+                                                     (false? (:added item))
+                                                     (= :block/uuid (:a item)))
+                                                (:e item)
+
+                                                :else
+                                                nil)))
+                                      distinct
+                                      vec)
+        ids-from-tx-data' (some->> deleted-ids-from-tx-data
+                                   (stable-id-coll db)
+                                   seq
+                                   vec)
+        ids' (stable-id-coll db ids)
         created-uuids (created-block-uuids-from-tx-data tx-data)
         unresolved-created-lookups? (and (seq created-uuids)
                                          (= (count ids') (count created-uuids))
@@ -310,8 +354,15 @@
                                                         (= :block/uuid (first id))
                                                         (nil? (d/entity db id))))
                                                  ids'))]
-    (if unresolved-created-lookups?
+    (cond
+      ;; Prefer the delete ids directly observed in tx-data to avoid carrying stale
+      ;; UI selection ids that weren't actually retracted in this transaction.
+      (seq ids-from-tx-data')
+      ids-from-tx-data'
+
+      unresolved-created-lookups?
       (mapv (fn [block-uuid] [:block/uuid block-uuid]) created-uuids)
+      :else
       ids')))
 
 (defn- moved-block-ids-from-tx-data
@@ -430,54 +481,60 @@
 
     :rename-page
     (let [[page-uuid new-title] args]
-      [:save-block [{:block/uuid page-uuid
+      [:save-block [{:block/uuid (stable-block-uuid db page-uuid)
                      :block/title new-title}
                     {}]])
 
     :delete-page
     (let [[page-uuid opts] args]
-      [:delete-page [page-uuid opts]])
+      [:delete-page [(stable-block-uuid db page-uuid) opts]])
 
     :restore-recycled
     (let [[root-id] args]
-      [:restore-recycled [root-id]])
+      [:restore-recycled [(stable-block-uuid db root-id)]])
 
     :recycle-delete-permanently
     (let [[root-id] args]
-      [:recycle-delete-permanently [(stable-entity-ref db root-id)]])
+      [:recycle-delete-permanently [(stable-block-uuid db root-id)]])
 
     :set-block-property
     (let [[block-eid property-id v] args]
-      [:set-block-property [(stable-entity-ref db block-eid)
-                            property-id
-                            (stable-property-value db property-id v)]])
+      (let [property-id' (stable-entity-ref db property-id)]
+        [:set-block-property [(stable-entity-ref db block-eid)
+                              property-id'
+                              (stable-property-value db property-id' v)]]))
 
     :remove-block-property
     (let [[block-eid property-id] args]
-      [:remove-block-property [(stable-entity-ref db block-eid) property-id]])
+      [:remove-block-property [(stable-entity-ref db block-eid)
+                               (stable-entity-ref db property-id)]])
 
     :batch-set-property
     (let [[block-ids property-id v opts] args]
-      [:batch-set-property [(stable-id-coll db block-ids)
-                            property-id
-                            (stable-property-value db property-id v)
-                            opts]])
+      (let [property-id' (stable-entity-ref db property-id)]
+        [:batch-set-property [(stable-id-coll db block-ids)
+                              property-id'
+                              (stable-property-value db property-id' v)
+                              opts]]))
 
     :batch-remove-property
     (let [[block-ids property-id] args]
-      [:batch-remove-property [(stable-id-coll db block-ids) property-id]])
+      [:batch-remove-property [(stable-id-coll db block-ids)
+                               (stable-entity-ref db property-id)]])
 
     :delete-property-value
     (let [[block-eid property-id property-value] args]
-      [:delete-property-value [(stable-entity-ref db block-eid)
-                               property-id
-                               (stable-property-value db property-id property-value)]])
+      (let [property-id' (stable-entity-ref db property-id)]
+        [:delete-property-value [(stable-entity-ref db block-eid)
+                                 property-id'
+                                 (stable-property-value db property-id' property-value)]]))
 
     :batch-delete-property-value
     (let [[block-eids property-id property-value] args]
-      [:batch-delete-property-value [(stable-id-coll db block-eids)
-                                     property-id
-                                     (stable-property-value db property-id property-value)]])
+      (let [property-id' (stable-entity-ref db property-id)]
+        [:batch-delete-property-value [(stable-id-coll db block-eids)
+                                       property-id'
+                                       (stable-property-value db property-id' property-value)]]))
 
     :create-property-text-block
     (let [[block-id property-id value opts] args]
@@ -503,15 +560,16 @@
 
     :upsert-closed-value
     (let [[property-id opts] args]
-      [:upsert-closed-value [property-id opts]])
+      [:upsert-closed-value [(stable-entity-ref db property-id) opts]])
 
     :add-existing-values-to-closed-values
     (let [[property-id values] args]
-      [:add-existing-values-to-closed-values [property-id values]])
+      [:add-existing-values-to-closed-values [(stable-entity-ref db property-id) values]])
 
     :delete-closed-value
     (let [[property-id value-block-id] args]
-      [:delete-closed-value [property-id (stable-entity-ref db value-block-id)]])
+      [:delete-closed-value [(stable-entity-ref db property-id)
+                             (stable-entity-ref db value-block-id)]])
 
     [op args]))
 
@@ -726,14 +784,14 @@
 
 (defn- selected-block-roots
   [db-before ids]
-  (let [entities (reduce (fn [acc id]
-                           (if-let [ent (d/entity db-before id)]
-                             (if (some #(= (:db/id %) (:db/id ent)) acc)
-                               acc
-                               (conj acc ent))
-                             acc))
+  (let [resolved-entities (mapv #(d/entity db-before %) ids)
+        unresolved-id? (some nil? resolved-entities)
+        entities (reduce (fn [acc ent]
+                           (if (some #(= (:db/id %) (:db/id ent)) acc)
+                             acc
+                             (conj acc ent)))
                          []
-                         ids)
+                         (remove nil? resolved-entities))
         selected-ids (set (map :db/id entities))
         has-selected-ancestor? (fn [ent]
                                  (loop [parent (:block/parent ent)]
@@ -742,9 +800,10 @@
                                        true
                                        (recur (:block/parent parent)))
                                      false)))]
-    (->> entities
-         (remove has-selected-ancestor?)
-         vec)))
+    {:roots (->> entities
+                 (remove has-selected-ancestor?)
+                 vec)
+     :incomplete? (boolean unresolved-id?)}))
 
 (defn- block-restore-target
   [ent]
@@ -783,9 +842,10 @@
 
 (defn- build-inverse-delete-blocks
   [db-before ids]
-  (let [roots (selected-block-roots db-before ids)
+  (let [{:keys [roots incomplete?]} (selected-block-roots db-before ids)
         plans (mapv #(delete-root->restore-plan db-before %) roots)]
-    (when (and (seq roots)
+    (when (and (not incomplete?)
+               (seq roots)
                (every? some? plans))
       (->> plans
            (mapv #(to-insert-op db-before %))
@@ -804,9 +864,10 @@
 
 (defn- build-inverse-move-blocks
   [db-before ids]
-  (let [roots (selected-block-roots db-before ids)
+  (let [{:keys [roots incomplete?]} (selected-block-roots db-before ids)
         restore-ops (mapv #(move-root->restore-op db-before %) roots)]
-    (when (and (seq roots)
+    (when (and (not incomplete?)
+               (seq roots)
                (every? some? restore-ops))
       (seq restore-ops))))
 
@@ -1029,20 +1090,15 @@
     nil
 
     (seq ops)
-    (do
-      (when-not (every? (fn [[op]]
-                          (contains? semantic-outliner-ops op))
-                        ops)
-        (throw (ex-info "Not every op is semantic" {:ops ops})))
-      (->> ops
-           (mapcat (fn [op]
-                     (let [canonicalized-op (canonicalize-semantic-outliner-op db tx-data op)]
-                       (if (and (sequential? canonicalized-op)
-                                (sequential? (first canonicalized-op))
-                                (keyword? (ffirst canonicalized-op)))
-                         canonicalized-op
-                         [canonicalized-op]))))
-           vec))
+    (->> ops
+         (mapcat (fn [op]
+                   (let [canonicalized-op (canonicalize-semantic-outliner-op db tx-data op)]
+                     (if (and (sequential? canonicalized-op)
+                              (sequential? (first canonicalized-op))
+                              (keyword? (ffirst canonicalized-op)))
+                       canonicalized-op
+                       [canonicalized-op]))))
+         vec)
 
     :else
     nil))
@@ -1098,12 +1154,186 @@
       (contains? #{:transact :batch-import-edn} (:outliner-op tx-meta))
       canonical-transact-op)))
 
+(defn- unresolved-numeric-entity-id?
+  [x]
+  (and (integer? x)
+       (not (neg? x))))
+
+(defn- unresolved-numeric-entity-id-coll?
+  [xs]
+  (some unresolved-numeric-entity-id? xs))
+
+(defn- numeric-id-in-ref-value?
+  [v]
+  (cond
+    (unresolved-numeric-entity-id? v)
+    true
+
+    (set? v)
+    (some numeric-id-in-ref-value? v)
+
+    (sequential? v)
+    (some numeric-id-in-ref-value? v)
+
+    :else
+    false))
+
+(defn- numeric-id-in-block-ref-attrs?
+  [db block]
+  (and (map? block)
+       (some (fn [[k v]]
+               (and (ref-attr? db k)
+                    (numeric-id-in-ref-value? v)))
+             block)))
+
+(defn- stale-numeric-id-in-op?
+  [db [op args]]
+  (if (= :transact op)
+    false
+    (case op
+      :save-block
+      (let [[block _opts] args]
+        (numeric-id-in-block-ref-attrs? db block))
+
+      :insert-blocks
+      (let [[blocks target-id _opts] args]
+        (or (some #(numeric-id-in-block-ref-attrs? db %) blocks)
+            (unresolved-numeric-entity-id? target-id)))
+
+      :create-page
+      (let [[_title opts] args]
+        (unresolved-numeric-entity-id? (:uuid opts)))
+
+      :rename-page
+      (let [[page-uuid _new-title] args]
+        (unresolved-numeric-entity-id? page-uuid))
+
+      :delete-page
+      (let [[page-uuid _opts] args]
+        (unresolved-numeric-entity-id? page-uuid))
+
+      :restore-recycled
+      (let [[root-id] args]
+        (unresolved-numeric-entity-id? root-id))
+
+      :apply-template
+      (let [[template-id target-id _opts] args]
+        (or (unresolved-numeric-entity-id? template-id)
+            (unresolved-numeric-entity-id? target-id)))
+
+      :move-blocks
+      (let [[ids target-id _opts] args]
+        (or (unresolved-numeric-entity-id-coll? ids)
+            (unresolved-numeric-entity-id? target-id)))
+
+      :move-blocks-up-down
+      (let [[ids _up?] args]
+        (unresolved-numeric-entity-id-coll? ids))
+
+      :indent-outdent-blocks
+      (let [[ids _indent? _opts] args]
+        (unresolved-numeric-entity-id-coll? ids))
+
+      :delete-blocks
+      (let [[ids _opts] args]
+        (unresolved-numeric-entity-id-coll? ids))
+
+      :set-block-property
+      (let [[block-id property-id _v] args]
+        (or (unresolved-numeric-entity-id? block-id)
+            (unresolved-numeric-entity-id? property-id)))
+
+      :remove-block-property
+      (let [[block-id property-id] args]
+        (or (unresolved-numeric-entity-id? block-id)
+            (unresolved-numeric-entity-id? property-id)))
+
+      :batch-set-property
+      (let [[block-ids property-id _v _opts] args]
+        (or (unresolved-numeric-entity-id-coll? block-ids)
+            (unresolved-numeric-entity-id? property-id)))
+
+      :batch-remove-property
+      (let [[block-ids property-id] args]
+        (or (unresolved-numeric-entity-id-coll? block-ids)
+            (unresolved-numeric-entity-id? property-id)))
+
+      :delete-property-value
+      (let [[block-id property-id _property-value] args]
+        (or (unresolved-numeric-entity-id? block-id)
+            (unresolved-numeric-entity-id? property-id)))
+
+      :batch-delete-property-value
+      (let [[block-ids property-id _property-value] args]
+        (or (unresolved-numeric-entity-id-coll? block-ids)
+            (unresolved-numeric-entity-id? property-id)))
+
+      :create-property-text-block
+      (let [[block-id property-id _value _opts] args]
+        (or (unresolved-numeric-entity-id? block-id)
+            (unresolved-numeric-entity-id? property-id)))
+
+      :class-add-property
+      (let [[class-id property-id] args]
+        (or (unresolved-numeric-entity-id? class-id)
+            (unresolved-numeric-entity-id? property-id)))
+
+      :class-remove-property
+      (let [[class-id property-id] args]
+        (or (unresolved-numeric-entity-id? class-id)
+            (unresolved-numeric-entity-id? property-id)))
+
+      :upsert-property
+      (let [[property-id _schema _opts] args]
+        (unresolved-numeric-entity-id? property-id))
+
+      :upsert-closed-value
+      (let [[property-id _opts] args]
+        (unresolved-numeric-entity-id? property-id))
+
+      :add-existing-values-to-closed-values
+      (let [[property-id _values] args]
+        (unresolved-numeric-entity-id? property-id))
+
+      :delete-closed-value
+      (let [[property-id value-block-id] args]
+        (or (unresolved-numeric-entity-id? property-id)
+            (unresolved-numeric-entity-id? value-block-id)))
+
+      :recycle-delete-permanently
+      (let [[root-id] args]
+        (unresolved-numeric-entity-id? root-id))
+
+      false)))
+
+(defn- stale-numeric-id-in-op-stream?
+  [db ops]
+  (some #(stale-numeric-id-in-op? db %) ops))
+
+(defn- assert-no-stale-numeric-ids!
+  [db ops stage]
+  (when-let [[idx op-entry] (some (fn [[idx op-entry]]
+                                    (when (stale-numeric-id-in-op? db op-entry)
+                                      [idx op-entry]))
+                                  (map-indexed vector ops))]
+    (throw (ex-info "Non-transact outliner ops contain numeric entity ids"
+                    {:stage stage
+                     :index idx
+                     :op op-entry
+                     :ops ops}))))
+
+(defn ^:api assert-no-numeric-entity-ids!
+  [db ops stage]
+  (assert-no-stale-numeric-ids! db (some-> ops seq vec) stage))
+
 (defn derive-history-outliner-ops
   [db-before db-after tx-data tx-meta]
-  (let [forward-outliner-ops (patch-forward-delete-block-op-ids
-                              db-before
-                              (canonicalize-outliner-ops db-after tx-meta tx-data))
-        forward-outliner-ops (some-> forward-outliner-ops seq vec)
+  (let [canonical-forward-outliner-ops (patch-forward-delete-block-op-ids
+                                        db-before
+                                        (canonicalize-outliner-ops db-after tx-meta tx-data))
+        canonical-forward-outliner-ops (some-> canonical-forward-outliner-ops seq vec)
+        _ (assert-no-stale-numeric-ids! db-after canonical-forward-outliner-ops :forward-outliner-ops)
+        forward-outliner-ops canonical-forward-outliner-ops
         forward-outliner-ops (when (seq forward-outliner-ops)
                                (if (and (> (count forward-outliner-ops) 1)
                                         (some (fn [[op]] (= :transact op)) forward-outliner-ops))
@@ -1112,10 +1342,12 @@
         built-inverse-outliner-ops (some-> (build-strict-inverse-outliner-ops db-before db-after tx-data forward-outliner-ops)
                                            seq
                                            vec)
+        _ (assert-no-stale-numeric-ids! db-before built-inverse-outliner-ops :built-inverse-outliner-ops)
         explicit-inverse-outliner-ops (some-> (canonicalize-explicit-outliner-ops db-after tx-data (:db-sync/inverse-outliner-ops tx-meta))
                                               (patch-inverse-delete-block-ops forward-outliner-ops)
                                               seq
                                               vec)
+        _ (assert-no-stale-numeric-ids! db-after explicit-inverse-outliner-ops :explicit-inverse-outliner-ops)
         inverse-outliner-ops (cond
                                (and (= :apply-template (:outliner-op tx-meta))
                                     (:undo? tx-meta)
@@ -1138,6 +1370,7 @@
 
                                :else
                                explicit-inverse-outliner-ops)
-        inverse-outliner-ops (some-> inverse-outliner-ops seq vec)]
+        inverse-outliner-ops (some-> inverse-outliner-ops seq vec)
+        _ (assert-no-stale-numeric-ids! db-before inverse-outliner-ops :inverse-outliner-ops)]
     {:forward-outliner-ops forward-outliner-ops
      :inverse-outliner-ops inverse-outliner-ops}))

deps/outliner/test/logseq/outliner/op_construct_test.cljs

@@ -123,6 +123,40 @@
           (is (not= [:block/uuid child-uuid]
                     (get-in insert-op [1 1]))))))))
 
+(deftest derive-history-outliner-ops-delete-blocks-with-stale-id-throws-test
+  (testing "delete-blocks derive-history throws when semantic ids include numeric db/id"
+    (let [conn (db-test/create-conn-with-blocks
+                {:pages-and-blocks
+                 [{:page {:block/title "page"}
+                   :blocks [{:block/title "parent"
+                             :build/children [{:block/title "child"}]}]}]})
+          child (db-test/find-block-by-content @conn "child")
+          stale-id 99999999
+          tx-meta {:outliner-op :delete-blocks
+                   :outliner-ops [[:delete-blocks [[(:db/id child) stale-id] {}]]]}]
+      (is (thrown? js/Error
+                   (op-construct/derive-history-outliner-ops @conn @conn [] tx-meta))))))
+
+(deftest derive-history-outliner-ops-delete-blocks-prefers-retracted-tx-data-ids-test
+  (testing "delete-blocks derive-history should prefer tx-data retractEntity ids over stale selection ids"
+    (let [conn (db-test/create-conn-with-blocks
+                {:pages-and-blocks
+                 [{:page {:block/title "page"}
+                   :blocks [{:block/title "parent"
+                             :build/children [{:block/title "child"}]}]}]})
+          child (db-test/find-block-by-content @conn "child")
+          child-id (:db/id child)
+          child-uuid (:block/uuid child)
+          stale-id 99999999
+          tx-report (d/with @conn [[:db/retractEntity child-id]] {})
+          tx-meta {:outliner-op :delete-blocks
+                   :outliner-ops [[:delete-blocks [[child-id stale-id] {}]]]}
+          {:keys [forward-outliner-ops]}
+          (op-construct/derive-history-outliner-ops
+           @conn (:db-after tx-report) (:tx-data tx-report) tx-meta)]
+      (is (= [[:delete-blocks [[[:block/uuid child-uuid]] {}]]]
+             forward-outliner-ops)))))
+
 (deftest derive-history-outliner-ops-builds-delete-page-inverse-for-class-property-and-today-page-test
   (testing "delete-page inverse restores hard-retracted class/property/today pages with stable db/ident"
     (let [today (date-time-util/ms->journal-day (js/Date.))