From 74f51ca0cf9d99cccf1cea8ee8b3fb84ef7496d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Tejero=20Cantero?= Date: Tue, 15 Sep 2026 15:13:33 +0200 Subject: [PATCH] :bug: Refuse a reference the projection never defined, and re-measure honestly Review found two defects in the bulk build. A reference may name a tempid the transaction never defines, because ref-resolver answers with whatever ctf/find-ref-shape names and that shape can sit in a container the builder skipped. The flattener resolved such a target to nil, which puts a nil-valued reference in the index. d/db-with refuses the whole transaction there, with :transact/syntax and "Tempids used only as value", so the bulk path refuses it too and names the defect: :projection/dangling-reference. A unit test pins both refusals, and pins that the same transaction works on both paths once the target is defined. The flattener is now tx->datoms, taking a transaction, so the case is testable without a document that reaches it. The timings in the docstring were the prototype's, taken on a JVM that had served the session. Re-taken thirty seconds after boot, with the first pass reported apart: a build is 800 ms cold and 471 to 513 ms warm against 2,357 ms and 2,147 to 2,181 ms, so 4.2 times rather than 5.3. The walk alone is 314 to 325 ms and steady from the first pass, which puts the flatten and the load together at about 185 ms of a warm build. Entity ids are an artefact of the interning order, and the docstring now says so: deleting one leaf shape shifts every later id. Two builds of one document are comparable by id and two builds of two documents are not, which is a constraint on whatever compares them. backend suite unchanged at 161 pre-existing failures, 826 tests and 5499 assertions against 824 and 5491 before. --- backend/src/app/graph/overlay.clj | 69 +++++++++++++------ .../test/backend_tests/graph_overlay_test.clj | 25 +++++++ 2 files changed, 74 insertions(+), 20 deletions(-) diff --git a/backend/src/app/graph/overlay.clj b/backend/src/app/graph/overlay.clj index f088c29eb5..6645e6fbfb 100644 --- a/backend/src/app/graph/overlay.clj +++ b/backend/src/app/graph/overlay.clj @@ -487,25 +487,45 @@ (when (= :db.type/ref (:db/valueType decl)) attr))) schema)) -(defn build-datoms - "The projection of `build-tx`, flattened into datoms whose entity ids are - already assigned, which is what `datascript.core/init-db` takes. +(defn- intern-tempid + [eids tempid] + (if (contains? eids tempid) + eids + (assoc eids tempid (inc (count eids))))) + +(defn tx->datoms + "Flatten a `build-tx` transaction into datoms whose entity ids are already + assigned, which is what `datascript.core/init-db` takes. Interning the tempid strings in first-appearance order is the whole of the id assignment, and it is sound only because every entity is emitted once and takes its identity from the document rather than from an upsert. A build transform that read the index instead would break that property and force the build back onto the transact path, which is what the rule - in this project's entry set exists to prevent." - [data file] - (let [tx (build-tx data file) - eids (reduce (fn [acc entity] - (let [tempid (:db/id entity)] - (if (contains? acc tempid) - acc - (assoc acc tempid (inc (count acc)))))) - {} - tx)] + in this project's entry set exists to prevent. + + A reference naming a tempid the transaction never defines is refused + rather than resolved, which is what `d/db-with` does with the same + transaction (`:error :transact/syntax`, \"Tempids used only as value\"). + The case is reachable, because `ref-resolver` answers with the tempid of + whatever shape `ctf/find-ref-shape` names and that shape may sit in a + container the builder skipped, and it is a projection defect rather than a + document one. Resolving it to `nil` would put a nil-valued reference in + the index and allocating an entity for it would index an edge to nothing, + and neither would say so. + + The entity ids are an artefact of this assignment and carry no meaning. + They are comparable between two builds of one document and not between + builds of two documents, because inserting one entity shifts every later + first appearance." + [tx] + (let [eids (reduce (fn [eids entity] (intern-tempid eids (:db/id entity))) {} tx) + ref! (fn [tempid] + (or (get eids tempid) + (throw (ex-info (str "reference to a tempid the transaction never defines: " + tempid) + {:error :projection/dangling-reference + :tempid tempid}))))] (persistent! (reduce (fn [acc entity] (let [eid (get eids (:db/id entity))] @@ -516,7 +536,7 @@ (let [ref? (contains? ref-attrs attr) emit (fn [acc value] (conj! acc (d/datom eid attr - (if ref? (get eids value) value))))] + (if ref? (ref! value) value))))] (if (contains? card-many-attrs attr) (reduce emit acc value) (emit acc value))))) @@ -525,16 +545,25 @@ (transient []) tx)))) +(defn build-datoms + "The projection of `build-tx`, flattened by `tx->datoms`." + [data file] + (tx->datoms (build-tx data file))) + (defn build "Project file `data` into a fresh overlay. Pure: returns a datascript database value. - The index is built bottom-up from datoms rather than transacted, which - on a 21,218-shape file is 410 ms against 2,150 ms for the same 217,598 - datoms, measured back to back in one JVM. The two paths produce the same - graph: entity ids differ, and the datom set is identical once the ids are - read back through their tempids, which `backend-tests.graph-overlay-test` - asserts." + The index is built bottom-up from datoms rather than transacted, which on + a 21,218-shape file is 500 ms warm and 800 ms on the first call, against + 2,150 ms for the same 217,598 datoms, measured on a fresh JVM with the + passes reported separately. The walk in `build-tx` is 315 ms of that, so + the remaining cost is the document rather than the index. + + The two paths produce the same graph. Entity ids differ and carry no + meaning, and the datom set is identical once every id is read back + through the tempid that minted it, which + `backend-tests.graph-overlay-test` asserts." ([data] (build data nil)) ([data file] (d/init-db (build-datoms data file) schema))) diff --git a/backend/test/backend_tests/graph_overlay_test.clj b/backend/test/backend_tests/graph_overlay_test.clj index 13b17c772d..4e8d5def7e 100644 --- a/backend/test/backend_tests/graph_overlay_test.clj +++ b/backend/test/backend_tests/graph_overlay_test.clj @@ -918,3 +918,28 @@ (canonical bulk (by-tempid (into {} (map (fn [[t e]] [t e])) bulk-ids))))) (t/is (= (queries/stats (:db-after transacted)) (queries/stats bulk))) (t/is (= (queries/edge-counts (:db-after transacted)) (queries/edge-counts bulk))))) + +(t/deftest a-reference-to-an-undefined-tempid-is-refused-on-both-paths + ;; `ref-resolver` answers with the tempid of whatever shape + ;; `ctf/find-ref-shape` names, and that shape can sit in a container the + ;; builder skipped, so a reference may name a tempid the transaction never + ;; defines. `d/db-with` refuses such a transaction outright, so the bulk + ;; path must refuse it too: resolving the target to nil would index a + ;; nil-valued reference, and allocating an entity for it would index an + ;; edge to nothing, and neither would say so. This is a unit test of the + ;; flattener rather than of a document, because no fixture reaches the + ;; case and the risk lives here. + (let [tx [{:db/id "doc" :document/id (uuid/next)} + {:db/id "c/1" :container/id (uuid/next) :container/document "doc"} + {:db/id "s/1" :shape/id (uuid/next) :shape/container "c/1" + :shape/refers-to "s/absent"}]] + (t/is (thrown? clojure.lang.ExceptionInfo (overlay/tx->datoms tx))) + (t/is (= :projection/dangling-reference + (:error (ex-data (try (overlay/tx->datoms tx) (catch Throwable e e))))) + "the refusal names the defect rather than the symptom") + (t/is (thrown? clojure.lang.ExceptionInfo (d/with (d/empty-db overlay/schema) tx)) + "as the transact path always did") + (let [whole (conj tx {:db/id "s/absent" :shape/id (uuid/next) :shape/container "c/1"})] + (t/is (= (count (d/datoms (:db-after (d/with (d/empty-db overlay/schema) whole)) :eavt)) + (count (d/datoms (d/init-db (overlay/tx->datoms whole) overlay/schema) :eavt))) + "and with the target defined, both paths carry the same datoms"))))