From 40ce360c99dd0e96bb0b373128a231ba0b455072 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 27 May 2026 12:36:21 +0200 Subject: [PATCH] :zap: Improve performance and fix orphan detection in validate-file (#9789) * :zap: Improve performance and fix orphan detection in validate-file - Add `*ref-shape-cache*` dynamic var to memoize `find-ref-shape` lookups per page, avoiding repeated O(depth) ancestor walks. - Add `*children-sets*` pre-computed maps for O(1) parent-child containment checks, replacing linear `some` scans. - Short-circuit `inside-component-main?` when the shape context already implies a main component. - Use single-pass reduce with early exit for duplicate detection (children, swap slots) instead of count/distinct or frequencies. - Guard `check-missing-slot` to skip expensive `find-near-match` when the shape already has a swap slot. - Refactor variant-set validation to use `run!` with direct `get`. - Refactor `check-ref-cycles` to use a single `reduce-kv` pass. - Fix `get-orphan-shapes`: the original `map` pipeline produced nils so orphan shapes were never validated; rewrite with `reduce-kv` for correct results. - Add `validate-file-affected!` for change-scoped validation, replacing full file validation in `process-changes-and-validate` to only validate pages and components touched by the changes. Signed-off-by: Andrey Antukh * :sparkles: Improved validation --------- Signed-off-by: Andrey Antukh Co-authored-by: alonso.torres --- backend/src/app/rpc/commands/files_update.clj | 13 +- common/src/app/common/files/repair.cljc | 7 +- common/src/app/common/files/validate.cljc | 360 +++++++++++--- common/src/app/common/types/component.cljc | 13 +- .../common_tests/files/validate_test.cljc | 449 ++++++++++++++++++ common/test/common_tests/runner.cljc | 2 + 6 files changed, 761 insertions(+), 83 deletions(-) create mode 100644 common/test/common_tests/files/validate_test.cljc diff --git a/backend/src/app/rpc/commands/files_update.clj b/backend/src/app/rpc/commands/files_update.clj index 52fea1639b..e70ed93f41 100644 --- a/backend/src/app/rpc/commands/files_update.clj +++ b/backend/src/app/rpc/commands/files_update.clj @@ -379,19 +379,16 @@ (l/error :hint "file schema validation error" :cause cause)))) (defn- soft-validate-file! - [file libs] + [file libs changes] (try - (val/validate-file! file libs) + (val/validate-file-affected! file libs changes) (catch Throwable cause (l/error :hint "file validation error" :cause cause)))) - (defn- process-changes-and-validate [cfg file changes skip-validate] - (let [;; WARNING: this ruins performance; maybe we need to find - ;; some other way to do general validation - libs + (let [libs (when (and (or (contains? cf/flags :file-validation) (contains? cf/flags :soft-file-validation)) (not skip-validate)) @@ -419,14 +416,14 @@ (binding [pmap/*tracked* nil] (when (contains? cf/flags :soft-file-validation) - (soft-validate-file! file libs)) + (soft-validate-file! file libs changes)) (when (contains? cf/flags :soft-file-schema-validation) (soft-validate-file-schema! file)) (when (and (contains? cf/flags :file-validation) (not skip-validate)) - (val/validate-file! file libs)) + (val/validate-file-affected! file libs changes)) (when (and (contains? cf/flags :file-schema-validation) (not skip-validate)) diff --git a/common/src/app/common/files/repair.cljc b/common/src/app/common/files/repair.cljc index 29e6d4fdf5..6a637eada7 100644 --- a/common/src/app/common/files/repair.cljc +++ b/common/src/app/common/files/repair.cljc @@ -644,9 +644,10 @@ (fn [shape] ;; Set the desired swap slot (let [slot (:swap-slot args)] - (when (some? slot) - (log/debug :hint (str " -> set swap-slot to " slot)) - (ctk/set-swap-slot shape slot))))] + (if (some? slot) + (do (log/debug :hint (str " -> set swap-slot to " slot)) + (ctk/set-swap-slot shape slot)) + shape)))] (log/dbg :hint "repairing shape :missing-slot" :id (:id shape) :name (:name shape) :page-id page-id) (-> (pcb/empty-changes nil page-id) diff --git a/common/src/app/common/files/validate.cljc b/common/src/app/common/files/validate.cljc index 1c16c4dcbc..d1979dd725 100644 --- a/common/src/app/common/files/validate.cljc +++ b/common/src/app/common/files/validate.cljc @@ -17,7 +17,6 @@ [app.common.types.components-list :as ctkl] [app.common.types.container :as ctn] [app.common.types.file :as ctf] - [app.common.types.pages-list :as ctpl] [app.common.types.shape-tree :as ctst] [app.common.types.variant :as ctv] [app.common.uuid :as uuid] @@ -94,6 +93,52 @@ (def ^:dynamic ^:private *errors* nil) +;; Per-page volatile map used to memoize `ctf/find-ref-shape` calls during a +;; single validation pass. Keys are shape-ids; values are the returned ref-shape +;; (or `nil` when the shape has no ref). The cache is reset once per page so +;; that stale results never cross page boundaries. +(def ^:dynamic ^:private *ref-shape-cache* nil) + +;; Per-page pre-computed map from parent-id to the set of its children ids. +;; Enables O(1) containment checks in `check-parent-children` instead of the +;; default O(k) linear `(some #(= shape-id %) (:shapes parent))` scan. +;; Bound as a plain immutable map (not a volatile) since it is read-only during +;; a page's validation pass. +(def ^:dynamic ^:private *children-sets* nil) + +(defn- build-children-sets + "Return a {parent-id → #{child-ids}} map built from `objects` in a single + `reduce-kv` pass. Only shapes that have at least one child get an entry." + [objects] + (reduce-kv (fn [m _ shape] + (if-let [kids (not-empty (:shapes shape))] + (assoc m (:id shape) (set kids)) + m)) + {} + objects)) + +(defn- find-ref-shape* + "Cached wrapper around `ctf/find-ref-shape` with `:include-deleted? true`. + When `*ref-shape-cache*` is bound, each shape-id is resolved at most once + per validation pass regardless of how many check functions request it. + + Cache miss detection uses `contains?` rather than a sentinel default + (e.g. `(get cache id ::miss)` + `identical?`). The reason: in + ClojureScript, `identical?` on namespace-qualified keywords is not + reliable across compilation units because keyword interning is not + guaranteed — a sentinel retrieved from `get` may not be `===` to the + same literal written in code, causing every lookup to appear as a miss + and breaking the whole optimisation." + [file page libraries shape] + (if-let [cache *ref-shape-cache*] + (let [id (:id shape)] + (if (contains? @cache id) + (get @cache id) + (let [result (ctf/find-ref-shape file page libraries shape :include-deleted? true)] + (vswap! cache assoc id result) + result))) + (ctf/find-ref-shape file page libraries shape :include-deleted? true))) + (defn- library-exists? [file libraries shape] (or (= (:component-file shape) (:id file)) @@ -149,15 +194,32 @@ shape file page) (do (when-not (cfh/root? shape) - (when-not (some #(= shape-id %) (:shapes parent)) + ;; Fast path: O(1) set lookup when `*children-sets*` is pre-computed + ;; for the current page. Falls back to an O(k) linear scan via + ;; `some` when the var is unbound (e.g. in isolated single-shape + ;; validation calls outside a full page pass). + (when-not (if-let [cs *children-sets*] + (contains? (get cs (:id parent)) shape-id) + (some #(= shape-id %) (:shapes parent))) (report-error :child-not-in-parent (str/ffmt "Shape % not in parent's children list" shape-id) shape file page))) - (when-not (= (count shapes) (count (distinct shapes))) - (report-error :duplicated-children - (str/ffmt "Shape % has duplicated children" shape-id) - shape file page)) + ;; Single-pass duplicate detection: walk the children list once, + ;; accumulating seen IDs into a set and short-circuiting on the + ;; first duplicate. This replaces the previous two-pass + ;; `(not= (count shapes) (count (distinct shapes)))` approach + ;; which allocated an intermediate sequence and scanned twice. + (let [dup? (reduce (fn [seen id] + (if (contains? seen id) + (reduced true) + (conj seen id))) + #{} + shapes)] + (when (true? dup?) + (report-error :duplicated-children + (str/ffmt "Shape % has duplicated children" shape-id) + shape file page))) (doseq [child-id shapes] (let [child (ctst/get-shape page child-id)] @@ -292,7 +354,7 @@ [shape file page libraries] (let [library-exists (library-exists? file libraries shape) ref-shape (when library-exists - (ctf/find-ref-shape file page libraries shape :include-deleted? true))] + (find-ref-shape* file page libraries shape))] (when (and library-exists (nil? ref-shape)) (report-error :ref-shape-not-found (str/ffmt "Referenced shape % not found in near component" (:shape-ref shape)) @@ -309,7 +371,7 @@ (defn- check-ref-is-not-head "Validate that the referenced shape is not a nested copy root." [shape file page libraries] - (let [ref-shape (ctf/find-ref-shape file page libraries shape :include-deleted? true)] + (let [ref-shape (find-ref-shape* file page libraries shape)] (when (and (some? ref-shape) (ctk/instance-head? ref-shape)) (report-error :ref-shape-is-head @@ -319,7 +381,7 @@ (defn- check-ref-is-head "Validate that the referenced shape is a nested copy root." [shape file page libraries] - (let [ref-shape (ctf/find-ref-shape file page libraries shape :include-deleted? true)] + (let [ref-shape (find-ref-shape* file page libraries shape)] (when (and (some? ref-shape) (not (ctk/instance-head? ref-shape))) (report-error :ref-shape-is-not-head @@ -333,7 +395,7 @@ the same as in the referenced shape in the near main." [shape file page libraries] (when (nil? (ctk/get-swap-slot shape)) - (when-let [ref-shape (ctf/find-ref-shape file page libraries shape :include-deleted? true)] + (when-let [ref-shape (find-ref-shape* file page libraries shape)] (when (or (not= (:component-id shape) (:component-id ref-shape)) (not= (:component-file shape) (:component-file ref-shape))) (report-error :component-id-mismatch @@ -351,12 +413,21 @@ shape file page))) (defn- has-duplicate-swap-slot? + "Returns true if any two children of `shape` share the same swap slot. + Uses a single pass with early exit on the first duplicate, avoiding + the intermediate `frequencies` map allocation." [shape container] - (let [shapes (map #(get (:objects container) %) (:shapes shape)) - slots (->> (map #(ctk/get-swap-slot %) shapes) - (remove nil?)) - counts (frequencies slots)] - (some (fn [[_ count]] (> count 1)) counts))) + (let [objects (:objects container) + result (reduce (fn [seen child-id] + (let [slot (ctk/get-swap-slot (get objects child-id))] + (if (nil? slot) + seen + (if (contains? seen slot) + (reduced true) + (conj seen slot))))) + #{} + (:shapes shape))] + (true? result))) (defn- check-duplicate-swap-slot "Validate that the children of this shape does not have duplicated slots." @@ -370,14 +441,16 @@ "Validate that the shape has swap-slot if it's a subinstance head and the ref shape is not the matching shape by position in the near main." [shape file page libraries] - (let [near-match (ctf/find-near-match file page libraries shape :include-deleted? true :with-context? false)] - (when (and (some? near-match) - (not= (:shape-ref shape) (:id near-match)) - (nil? (ctk/get-swap-slot shape))) - (report-error :missing-slot - "Shape has been swapped, should have swap slot" - shape file page - :swap-slot (or (ctk/get-swap-slot near-match) (:id near-match)))))) + ;; Guard first: if the shape already has a swap slot the invariant is satisfied + ;; and we can avoid the expensive `find-near-match` call entirely. + (when (nil? (ctk/get-swap-slot shape)) + (let [near-match (ctf/find-near-match file page libraries shape :include-deleted? true :with-context? false)] + (when (and (some? near-match) + (not= (:shape-ref shape) (:id near-match))) + (report-error :missing-slot + "Shape has been swapped, should have swap slot" + shape file page + :swap-slot (or (ctk/get-swap-slot near-match) (:id near-match))))))) (defn- check-valid-touched "Validate that the text touched flags are coherent." @@ -491,26 +564,28 @@ -all its children should be variants with variant-id equals to the shape-id -all the components should have the same properties" [shape file page] - (let [shape-id (:id shape) - shapes (:shapes shape) - children (map #(ctst/get-shape page %) shapes) - prop-names (cfv/extract-properties-names (first children) (:data file))] - (doseq [child children] - (when child - (if (not (ctk/is-variant? child)) - (report-error :not-a-variant - (str/ffmt "Shape % should be a variant" (:id child)) - child file page) - (do - (when (not= (:variant-id child) shape-id) - (report-error :invalid-variant-id - (str/ffmt "Variant % has invalid variant-id %" (:id child) (:variant-id child)) - child file page)) - (when (not= prop-names (cfv/extract-properties-names child (:data file))) - (report-error :invalid-variant-properties - (str/ffmt "Variant % has invalid properties %" (:id child) (vec prop-names)) - child file page)))))))) - + (let [shape-id (:id shape) + shapes (:shapes shape) + objects (:objects page) + file-data (:data file) + first-child (get objects (first shapes)) + prop-names (cfv/extract-properties-names first-child file-data)] + (run! (fn [child-id] + (when-let [child (get objects child-id)] + (if (not (ctk/is-variant? child)) + (report-error :not-a-variant + (str/ffmt "Shape % should be a variant" (:id child)) + child file page) + (do + (when (not= (:variant-id child) shape-id) + (report-error :invalid-variant-id + (str/ffmt "Variant % has invalid variant-id %" (:id child) (:variant-id child)) + child file page)) + (when (not= prop-names (cfv/extract-properties-names child file-data)) + (report-error :invalid-variant-properties + (str/ffmt "Variant % has invalid properties %" (:id child) (vec prop-names)) + child file page)))))) + shapes))) (defn- check-variant "Shape is a variant, so -it should be a main component @@ -615,18 +690,26 @@ shape file page) (check-shape-copy-not-root shape file page libraries)) - (if (ctn/inside-component-main? (:objects page) shape) - (if-not (#{:main-top :main-nested :main-any} context) - (report-error :not-head-main-not-allowed - "Non-root main only allowed inside a main component" - shape file page) - (check-shape-main-not-root shape file page libraries)) + ;; Short-circuit `inside-component-main?` when the propagated + ;; `context` already classifies this sub-tree as belonging to a + ;; main component. `inside-component-main?` performs an O(depth) + ;; upward ancestor walk; skipping it for every non-head shape that + ;; sits inside a known-main context avoids redundant tree traversals + ;; that would otherwise dominate validation time on deep hierarchies. + (let [in-main? (or (#{:main-top :main-nested :main-any} context) + (ctn/inside-component-main? (:objects page) shape))] + (if in-main? + (if-not (#{:main-top :main-nested :main-any} context) + (report-error :not-head-main-not-allowed + "Non-root main only allowed inside a main component" + shape file page) + (check-shape-main-not-root shape file page libraries)) - (if (#{:main-top :main-nested :main-any} context) - (report-error :not-component-not-allowed - "Not compoments are not allowed inside a main" - shape file page) - (check-shape-not-component shape file page libraries)))))))) + (if (#{:main-top :main-nested :main-any} context) + (report-error :not-component-not-allowed + "Not compoments are not allowed inside a main" + shape file page) + (check-shape-not-component shape file page libraries))))))))) (defn check-component-duplicate-swap-slot [component file] @@ -638,11 +721,13 @@ (defn check-ref-cycles [component file] - (let [cycles-ids (->> component - :objects - vals - (filter #(= (:id %) (:shape-ref %))) - (map :id))] + (let [cycles-ids (-> (reduce-kv (fn [acc id shape] + (if (= id (:shape-ref shape)) + (conj! acc id) + acc)) + (transient []) + (:objects component)) + (persistent!))] (when (seq cycles-ids) (report-error :shape-ref-cycle @@ -703,10 +788,25 @@ (check-variant-component component file))) (defn- get-orphan-shapes - [{:keys [objects] :as page}] - (let [xf (comp (map #(contains? objects (:parent-id %))) - (map :id))] - (into [] xf (vals objects)))) + "Return the ids of shapes whose parent does not exist in the objects + map (i.e. shapes unreachable from the root traversal). The root + shape itself is excluded since it is always validated separately. + + Implemented with `reduce-kv` rather than a `map`/`filter` pipeline. + The previous implementation mapped over `objects` and returned a + lazy seq that could contain `nil` entries (for shapes that were + *not* orphans), meaning `check-shape` was called with `nil` IDs and + orphaned shapes were silently skipped. The `reduce-kv` approach + builds a plain vector of IDs and never yields `nil` entries." + [{:keys [objects] :as _page}] + (persistent! + (reduce-kv (fn [result id shape] + (if (and (not (cfh/root? shape)) + (not (contains? objects (:parent-id shape)))) + (conj! result id) + result)) + (transient []) + objects))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; PUBLIC API: VALIDATION FUNCTIONS @@ -720,20 +820,39 @@ (when (contains? features "components/v2") (binding [*errors* (volatile! [])] - (doseq [page (filter :id (ctpl/pages-seq data))] - (check-shape uuid/zero file page libraries) - (->> (get-orphan-shapes page) - (run! #(check-shape % file page libraries)))) + ;; `reduce-kv` is used throughout this function (and `validate-file-affected`) + ;; instead of `(run! f (vals m))` or `(doseq [[k v] m])` because persistent + ;; maps implement `IKVReduce`, which drives iteration internally without + ;; first materialising an intermediate sequence of key-value pairs. + (reduce-kv + (fn [_ _ page] + (when (some? page) + ;; Both performance caches are scoped to a single page: ref-shape + ;; lookups and parent→children sets are page-local and must never + ;; carry over to the next page. A fresh volatile is created for + ;; each page so stale entries cannot corrupt subsequent pages. + (binding [*ref-shape-cache* (volatile! {}) + *children-sets* (build-children-sets (:objects page))] + (check-shape uuid/zero file page libraries) + (run! #(check-shape % file page libraries) + (get-orphan-shapes page))))) + nil + (:pages-index data)) - (->> (vals (:components data)) - (run! #(check-component % file))) + (reduce-kv + (fn [_ _ component] + (check-component component file)) + nil + (:components data)) (-> *errors* deref not-empty)))) (defn validate-shape "Validate a shape and all its children. Returns a list of errors." [shape-id file page libraries] - (binding [*errors* (volatile! [])] + (binding [*errors* (volatile! []) + *ref-shape-cache* (volatile! {}) + *children-sets* (build-children-sets (:objects page))] (check-shape shape-id file page libraries) (deref *errors*))) @@ -744,6 +863,107 @@ (check-component component file) (deref *errors*))) +(defn- extract-affected-ids + "Single reduce pass over a changes batch. + Returns {:page-ids #{uuid …} :component-ids #{uuid …}}. + + Only entities that need re-validation after applying `changes` are + included. Deleted entities and pure file-level changes (colors, + tokens, typography…) produce no entries because there is nothing left + to check." + [changes] + + (loop [changes (seq changes) + page-ids #{} + component-ids #{}] + (if-let [change (first changes)] + (let [{:keys [type page-id component-id id]} change + page-ids + (case type + ;; Shape-level ops are scoped to either a page or a component + (:add-obj :mod-obj :del-obj :fix-obj :mov-objects :reorder-children :reg-objects) + (cond-> page-ids + page-id (conj page-id)) + + ;; A new or modified page needs a full page sweep + (:add-page :mod-page) + (conj page-ids id) + + ;; restore-component resurrects a deleted component (touches the + ;; component definition) and places its main instance on a page + ;; (touches that page's shape tree) + :restore-component + (conj page-ids page-id) + + ;; Otherwise don't change the ids + page-ids) + + component-ids + (case type + ;; Shape-level ops are scoped to either a page or a component + (:add-obj :mod-obj :del-obj :fix-obj :mov-objects :reorder-children :reg-objects) + (cond-> component-ids + component-id (conj component-id)) + + ;; A new, modified, restored component needs component-level checking + (:add-component :mod-component :restore-component) + (conj component-ids id) + + ;; Otherwise don't change the ids + component-ids)] + (recur (rest changes) page-ids component-ids)) + + ;; Return result of accumulated ids + {:page-ids page-ids :component-ids component-ids}))) + +(defn validate-file-affected + "Validate only the pages and components touched by `changes`. + + Semantics are identical to `validate-file` but the work is bounded + to the entities that were actually mutated, making it safe and cheap + to call on every incremental file update. + + Returns a list of errors or `nil`." + [{:keys [data features] :as file} libraries changes] + (when (contains? features "components/v2") + (let [{:keys [page-ids component-ids]} (extract-affected-ids changes)] + (binding [*errors* (volatile! [])] + + (reduce-kv + (fn [_ page-id page] + (when (and (some? page) (contains? page-ids page-id)) + ;; Both performance caches are scoped to a single page: ref-shape + ;; lookups and parent→children sets are page-local and must never + ;; carry over to the next page. A fresh volatile is created for + ;; each page so stale entries cannot corrupt subsequent pages. + (binding [*ref-shape-cache* (volatile! {}) + *children-sets* (build-children-sets (:objects page))] + (check-shape uuid/zero file page libraries) + (run! #(check-shape % file page libraries) + (get-orphan-shapes page))))) + nil + (:pages-index data)) + + (reduce-kv + (fn [_ id component] + (when (contains? component-ids id) + (check-component component file))) + nil + (:components data)) + + (-> *errors* deref not-empty))))) + +(defn validate-file-affected! + "Like `validate-file-affected` but raises on the first non-empty + error list instead of returning it." + [file libraries changes] + (when-let [errors (validate-file-affected file libraries changes)] + (ex/raise :type :validation + :code :referential-integrity + :hint "error on validating file referential integrity" + :file-id (:id file) + :details errors))) + (defn validate-file-schema! "Validates the file itself, without external dependencies, it performs the schema checking and some semantical validation of the diff --git a/common/src/app/common/types/component.cljc b/common/src/app/common/types/component.cljc index ecc6e30c65..1b5648804a 100644 --- a/common/src/app/common/types/component.cljc +++ b/common/src/app/common/types/component.cljc @@ -269,10 +269,19 @@ [group] (str/starts-with? (name group) "swap-slot-")) +(def ^:private xf:normal-touched + "Transducer that removes swap-slot touched groups." + (remove swap-slot?)) + (defn normal-touched-groups - "Gets all touched groups that are not swap slots." + "Gets all touched groups that are not swap slots. + Returns an empty set immediately when `:touched` is nil or empty, + avoiding an unnecessary `into #{}` allocation for the common case." [shape] - (into #{} (remove swap-slot? (:touched shape)))) + (let [touched (:touched shape)] + (if (empty? touched) + #{} + (into #{} xf:normal-touched touched)))) (defn group->swap-slot [group] diff --git a/common/test/common_tests/files/validate_test.cljc b/common/test/common_tests/files/validate_test.cljc new file mode 100644 index 0000000000..271cfa611e --- /dev/null +++ b/common/test/common_tests/files/validate_test.cljc @@ -0,0 +1,449 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC + +(ns common-tests.files.validate-test + "Exhaustive tests for the change-scoped partial validation functions in + app.common.files.validate: + + - validate-file-affected – returns nil or list of errors + - validate-file-affected! – same but raises on non-empty errors + + The tests verify the scoping logic implemented by extract-affected-ids + (tested indirectly) by injecting controlled broken states into specific + pages / components and confirming that only the expected entities are + validated." + (:require + [app.common.files.validate :as cfv] + [app.common.test-helpers.files :as thf] + [app.common.test-helpers.ids-map :as thi] + [app.common.types.file :as ctf] + [app.common.types.pages-list :as ctpl] + [app.common.types.shape-tree :as ctst] + [app.common.uuid :as uuid] + [clojure.test :as t])) + +(t/use-fixtures :each thi/test-fixture) + +;; ---------------------------------------------------------------- +;; Test-local helpers +;; ---------------------------------------------------------------- + +(defn- inject-broken-child + "Add a reference to a non-existent shape ID to the root shape's + children list on the page identified by `page-label`. + + This causes `check-parent-children` to report a :child-not-found + error whenever that page is validated." + [file page-label] + (let [page-id (thi/id page-label) + missing-id (uuid/next)] + (ctf/update-file-data + file + (fn [file-data] + (ctpl/update-page + file-data + page-id + (fn [page] + (let [root (ctst/get-shape page uuid/zero)] + (ctst/set-shape page (update root :shapes conj missing-id))))))))) + +(defn- inject-broken-component + "Add a deleted component with `:objects nil` to the file. + + This causes `check-component` to report a + :component-nil-objects-not-allowed error whenever that component is + validated. The component id is registered under `comp-label` in the + ids-map so callers can look it up with `(thi/id comp-label)`." + [file comp-label] + (let [comp-id (thi/new-id! comp-label)] + (ctf/update-file-data + file + (fn [file-data] + (assoc-in file-data [:components comp-id] + {:id comp-id + :name "broken-component" + :objects nil + :deleted true + :main-instance-id (uuid/next) + :main-instance-page (uuid/next)}))))) + +;; ---------------------------------------------------------------- +;; 1. Feature gate +;; ---------------------------------------------------------------- + +(t/deftest validate-file-affected-no-feature + (t/testing "returns nil when file does not have the components/v2 feature" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (assoc :features #{})) + page-id (thi/id :page1) + changes [{:type :add-obj :page-id page-id :id (uuid/next)}]] + (t/is (nil? (cfv/validate-file-affected file {} changes)))))) + +;; ---------------------------------------------------------------- +;; 2. Empty changes – nothing to validate +;; ---------------------------------------------------------------- + +(t/deftest validate-file-affected-empty-changes + (t/testing "returns nil when the changes list is empty" + (let [file (thf/sample-file :file1 :page-label :page1)] + (t/is (nil? (cfv/validate-file-affected file {} [])))))) + +;; ---------------------------------------------------------------- +;; 3. Page-level scoping – add-obj / mod-obj / fix-obj key: :page-id +;; ---------------------------------------------------------------- + +(t/deftest validate-file-affected-page-scoping-misses-untouched-page + (t/testing "add-obj on page1 does not validate broken page2" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2)) + file' (inject-broken-child file :page2) + page1-id (thi/id :page1) + ;; The change only touches page1 + changes [{:type :add-obj :page-id page1-id :id (uuid/next)}]] + (t/is (nil? (cfv/validate-file-affected file' {} changes))))) + + (t/testing "mod-obj on page1 does not validate broken page2" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2)) + file' (inject-broken-child file :page2) + page1-id (thi/id :page1) + changes [{:type :mod-obj :page-id page1-id :id uuid/zero + :operations [{:type :set :attr :name :val "root"}]}]] + (t/is (nil? (cfv/validate-file-affected file' {} changes))))) + + (t/testing "fix-obj on page1 does not validate broken page2" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2)) + file' (inject-broken-child file :page2) + page1-id (thi/id :page1) + changes [{:type :fix-obj :page-id page1-id :id (uuid/next) + :operations []}]] + (t/is (nil? (cfv/validate-file-affected file' {} changes)))))) + +(t/deftest validate-file-affected-page-scoping-catches-error-on-touched-page + (t/testing "add-obj on broken page1 surfaces the error" + (let [file (thf/sample-file :file1 :page-label :page1) + file' (inject-broken-child file :page1) + page1-id (thi/id :page1) + changes [{:type :add-obj :page-id page1-id :id (uuid/next)}]] + (let [errors (cfv/validate-file-affected file' {} changes)] + (t/is (seq errors)) + (t/is (some #(= :child-not-found (:code %)) errors))))) + + (t/testing "mod-obj on broken page1 surfaces the error" + (let [file (thf/sample-file :file1 :page-label :page1) + file' (inject-broken-child file :page1) + page1-id (thi/id :page1) + changes [{:type :mod-obj :page-id page1-id :id uuid/zero + :operations [{:type :set :attr :name :val "root"}]}]] + (let [errors (cfv/validate-file-affected file' {} changes)] + (t/is (seq errors)) + (t/is (some #(= :child-not-found (:code %)) errors))))) + + (t/testing "reg-objects on broken page1 surfaces the error" + (let [file (thf/sample-file :file1 :page-label :page1) + file' (inject-broken-child file :page1) + page1-id (thi/id :page1) + changes [{:type :reg-objects :page-id page1-id :shapes []}]] + (let [errors (cfv/validate-file-affected file' {} changes)] + (t/is (seq errors)) + (t/is (some #(= :child-not-found (:code %)) errors))))) + + (t/testing "mov-objects on broken page1 surfaces the error" + (let [file (thf/sample-file :file1 :page-label :page1) + file' (inject-broken-child file :page1) + page1-id (thi/id :page1) + changes [{:type :mov-objects :page-id page1-id :parent-id uuid/zero :shapes []}]] + (let [errors (cfv/validate-file-affected file' {} changes)] + (t/is (seq errors)) + (t/is (some #(= :child-not-found (:code %)) errors)))))) + +;; ---------------------------------------------------------------- +;; 4. add-page / mod-page – scoped by :id (not :page-id) +;; ---------------------------------------------------------------- + +(t/deftest validate-file-affected-add-page-scoped-by-id + (t/testing "add-page with :id=page2 validates page2 (broken → errors)" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2)) + file' (inject-broken-child file :page2) + page2-id (thi/id :page2) + changes [{:type :add-page :id page2-id}]] + (let [errors (cfv/validate-file-affected file' {} changes)] + (t/is (seq errors)) + (t/is (some #(= :child-not-found (:code %)) errors))))) + + (t/testing "add-page with :id=page1 does not validate broken page2" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2)) + file' (inject-broken-child file :page2) + page1-id (thi/id :page1) + changes [{:type :add-page :id page1-id}]] + (t/is (nil? (cfv/validate-file-affected file' {} changes))))) + + (t/testing "mod-page with :id=page2 validates page2 (broken → errors)" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2)) + file' (inject-broken-child file :page2) + page2-id (thi/id :page2) + changes [{:type :mod-page :id page2-id}]] + (let [errors (cfv/validate-file-affected file' {} changes)] + (t/is (seq errors)) + (t/is (some #(= :child-not-found (:code %)) errors)))))) + +;; ---------------------------------------------------------------- +;; 5. del-obj – shape-level op scoped by :page-id +;; del-page / del-component / mov-page / purge-component – no-ops +;; ---------------------------------------------------------------- + +(t/deftest validate-file-affected-del-obj-scopes-page + (t/testing "del-obj scopes its :page-id just like add-obj / mod-obj" + ;; del-obj is in the same shape-level ops bucket and scopes by :page-id. + (let [file (thf/sample-file :file1 :page-label :page1) + file' (inject-broken-child file :page1) + page1-id (thi/id :page1) + changes [{:type :del-obj :page-id page1-id :id (uuid/next)}]] + ;; page1 has an error and del-obj touches page1 → error surfaced + (let [errors (cfv/validate-file-affected file' {} changes)] + (t/is (seq errors)) + (t/is (some #(= :child-not-found (:code %)) errors))))) + + (t/testing "del-obj on page1 does not validate broken page2" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2)) + file' (inject-broken-child file :page2) + page1-id (thi/id :page1) + changes [{:type :del-obj :page-id page1-id :id (uuid/next)}]] + (t/is (nil? (cfv/validate-file-affected file' {} changes)))))) + +(t/deftest validate-file-affected-noop-change-types + (t/testing "del-page produces no affected entries" + (let [file (thf/sample-file :file1 :page-label :page1) + file' (inject-broken-child file :page1) + page1-id (thi/id :page1) + changes [{:type :del-page :id page1-id}]] + (t/is (nil? (cfv/validate-file-affected file' {} changes))))) + + (t/testing "del-component produces no affected entries" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (inject-broken-component :comp1)) + comp-id (thi/id :comp1) + changes [{:type :del-component :id comp-id}]] + (t/is (nil? (cfv/validate-file-affected file {} changes))))) + + (t/testing "mov-page produces no affected entries" + (let [file (thf/sample-file :file1 :page-label :page1) + file' (inject-broken-child file :page1) + page1-id (thi/id :page1) + changes [{:type :mov-page :id page1-id}]] + (t/is (nil? (cfv/validate-file-affected file' {} changes))))) + + (t/testing "purge-component produces no affected entries" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (inject-broken-component :comp1)) + comp-id (thi/id :comp1) + changes [{:type :purge-component :id comp-id}]] + (t/is (nil? (cfv/validate-file-affected file {} changes))))) + + (t/testing "add-color (library change) produces no affected entries" + (let [file (thf/sample-file :file1 :page-label :page1) + file' (inject-broken-child file :page1) + changes [{:type :add-color :id (uuid/next) :color {}}]] + (t/is (nil? (cfv/validate-file-affected file' {} changes))))) + + (t/testing "mod-color (library change) produces no affected entries" + (let [file (thf/sample-file :file1 :page-label :page1) + file' (inject-broken-child file :page1) + changes [{:type :mod-color :id (uuid/next) :color {}}]] + (t/is (nil? (cfv/validate-file-affected file' {} changes)))))) + +;; ---------------------------------------------------------------- +;; 6. add-component / mod-component – scoped by :id (component-id) +;; ---------------------------------------------------------------- + +(t/deftest validate-file-affected-add-component-scoped-by-id + (t/testing "add-component :id=comp1 validates comp1 (broken → errors)" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (inject-broken-component :comp1)) + comp-id (thi/id :comp1) + changes [{:type :add-component :id comp-id}]] + (let [errors (cfv/validate-file-affected file {} changes)] + (t/is (seq errors)) + (t/is (some #(= :component-nil-objects-not-allowed (:code %)) errors))))) + + (t/testing "add-component :id=other-id does not validate broken comp1" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (inject-broken-component :comp1)) + _ (thi/id :comp1) + changes [{:type :add-component :id (uuid/next)}]] + (t/is (nil? (cfv/validate-file-affected file {} changes))))) + + (t/testing "mod-component :id=comp1 validates comp1 (broken → errors)" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (inject-broken-component :comp1)) + comp-id (thi/id :comp1) + changes [{:type :mod-component :id comp-id}]] + (let [errors (cfv/validate-file-affected file {} changes)] + (t/is (seq errors)) + (t/is (some #(= :component-nil-objects-not-allowed (:code %)) errors)))))) + +;; ---------------------------------------------------------------- +;; 7. restore-component – scopes BOTH :id (component) and :page-id +;; ---------------------------------------------------------------- + +(t/deftest validate-file-affected-restore-component-scopes-page + (t/testing "restore-component touches its :page-id (broken page → errors)" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2)) + file' (inject-broken-child file :page1) + page1-id (thi/id :page1) + changes [{:type :restore-component :id (uuid/next) :page-id page1-id}]] + (let [errors (cfv/validate-file-affected file' {} changes)] + (t/is (seq errors)) + (t/is (some #(= :child-not-found (:code %)) errors))))) + + (t/testing "restore-component does not validate a page it does not reference" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2)) + file' (inject-broken-child file :page2) + page1-id (thi/id :page1) + changes [{:type :restore-component :id (uuid/next) :page-id page1-id}]] + ;; page2 has an error but the change only touches page1 + (t/is (nil? (cfv/validate-file-affected file' {} changes)))))) + +(t/deftest validate-file-affected-restore-component-scopes-component + (t/testing "restore-component touches its component :id (broken component → errors)" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (inject-broken-component :comp1)) + comp-id (thi/id :comp1) + page1-id (thi/id :page1) + changes [{:type :restore-component :id comp-id :page-id page1-id}]] + (let [errors (cfv/validate-file-affected file {} changes)] + (t/is (seq errors)) + (t/is (some #(= :component-nil-objects-not-allowed (:code %)) errors)))))) + +;; ---------------------------------------------------------------- +;; 8. Mixed changes – union of affected entities +;; ---------------------------------------------------------------- + +(t/deftest validate-file-affected-mixed-changes-union + (t/testing "two changes on different pages: both pages are validated" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2)) + ;; page2 is broken; page1 is clean + file' (inject-broken-child file :page2) + page1-id (thi/id :page1) + page2-id (thi/id :page2) + ;; Both pages are touched + changes [{:type :add-obj :page-id page1-id :id (uuid/next)} + {:type :mod-obj :page-id page2-id :id uuid/zero + :operations []}]] + (let [errors (cfv/validate-file-affected file' {} changes)] + ;; page2 is validated → error surfaced + (t/is (seq errors)) + (t/is (some #(= :child-not-found (:code %)) errors))))) + + (t/testing "del-page (true no-op) mixed with add-obj on page1: only page1 is validated" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2)) + ;; page2 is broken; page1 is clean + file' (inject-broken-child file :page2) + page1-id (thi/id :page1) + page2-id (thi/id :page2) + ;; del-page on page2 is a no-op; add-obj on page1 scopes page1 only + changes [{:type :del-page :id page2-id} + {:type :add-obj :page-id page1-id :id (uuid/next)}]] + ;; page2's error is NOT surfaced because del-page produces no scope + (t/is (nil? (cfv/validate-file-affected file' {} changes))))) + + (t/testing "duplicate page-ids in changes are deduplicated (page validated once)" + (let [file (thf/sample-file :file1 :page-label :page1) + file' (inject-broken-child file :page1) + page1-id (thi/id :page1) + ;; Three changes all touching the same page + changes [{:type :add-obj :page-id page1-id :id (uuid/next)} + {:type :mod-obj :page-id page1-id :id uuid/zero :operations []} + {:type :del-obj :page-id page1-id :id (uuid/next)}]] + ;; del-obj excluded, add-obj + mod-obj both scope page1; error surfaced once + (let [errors (cfv/validate-file-affected file' {} changes)] + (t/is (seq errors)) + ;; There should be exactly one :child-not-found (not duplicated) + (t/is (= 1 (count (filter #(= :child-not-found (:code %)) errors)))))))) + +;; ---------------------------------------------------------------- +;; 9. reorder-children and component-context changes +;; ---------------------------------------------------------------- + +(t/deftest validate-file-affected-reorder-children + (t/testing "reorder-children on broken page surfaces the error" + (let [file (thf/sample-file :file1 :page-label :page1) + file' (inject-broken-child file :page1) + page1-id (thi/id :page1) + changes [{:type :reorder-children :page-id page1-id :id uuid/zero :shapes []}]] + (let [errors (cfv/validate-file-affected file' {} changes)] + (t/is (seq errors)) + (t/is (some #(= :child-not-found (:code %)) errors)))))) + +(t/deftest validate-file-affected-obj-in-component + (t/testing "add-obj with :component-id (not :page-id) scopes a component" + ;; When a shape change carries :component-id instead of :page-id it + ;; means the change happened inside a deleted component's object tree. + ;; extract-affected-ids routes it to :component-ids. + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (inject-broken-component :comp1)) + comp-id (thi/id :comp1) + changes [{:type :add-obj :component-id comp-id :id (uuid/next)}]] + (let [errors (cfv/validate-file-affected file {} changes)] + (t/is (seq errors)) + (t/is (some #(= :component-nil-objects-not-allowed (:code %)) errors))))) + + (t/testing "add-obj with :component-id does not scope an unrelated component" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (inject-broken-component :comp1)) + _ (thi/id :comp1) + changes [{:type :add-obj :component-id (uuid/next) :id (uuid/next)}]] + (t/is (nil? (cfv/validate-file-affected file {} changes)))))) + +;; ---------------------------------------------------------------- +;; 10. validate-file-affected! – raises vs returns nil +;; ---------------------------------------------------------------- + +(t/deftest validate-file-affected!-returns-nil-on-clean-file + (t/testing "returns nil when the file has no errors in the touched page" + (let [file (thf/sample-file :file1 :page-label :page1) + page1-id (thi/id :page1) + changes [{:type :add-obj :page-id page1-id :id (uuid/next)}]] + (t/is (nil? (cfv/validate-file-affected! file {} changes)))))) + +(t/deftest validate-file-affected!-returns-nil-empty-changes + (t/testing "returns nil when there are no changes (no pages validated)" + (let [file (thf/sample-file :file1 :page-label :page1)] + (t/is (nil? (cfv/validate-file-affected! file {} [])))))) + +(t/deftest validate-file-affected!-raises-on-error + (t/testing "raises an exception when the touched page has validation errors" + (let [file (thf/sample-file :file1 :page-label :page1) + file' (inject-broken-child file :page1) + page1-id (thi/id :page1) + changes [{:type :mod-obj :page-id page1-id :id uuid/zero + :operations []}]] + (t/is (thrown? #?(:clj Exception :cljs js/Error) + (cfv/validate-file-affected! file' {} changes))))) + + (t/testing "raised exception is of :validation type with :referential-integrity code" + (let [file (thf/sample-file :file1 :page-label :page1) + file' (inject-broken-child file :page1) + page1-id (thi/id :page1) + changes [{:type :mod-obj :page-id page1-id :id uuid/zero + :operations []}]] + (try + (cfv/validate-file-affected! file' {} changes) + (t/is false "expected exception to be thrown") + (catch #?(:clj clojure.lang.ExceptionInfo :cljs cljs.core.ExceptionInfo) e + (let [data (ex-data e)] + (t/is (= :validation (:type data))) + (t/is (= :referential-integrity (:code data))) + (t/is (seq (:details data))))))))) diff --git a/common/test/common_tests/runner.cljc b/common/test/common_tests/runner.cljc index 9e1cd276a5..12034efc09 100644 --- a/common/test/common_tests/runner.cljc +++ b/common/test/common_tests/runner.cljc @@ -16,6 +16,7 @@ [common-tests.files-changes-test] [common-tests.files-migrations-test] [common-tests.files.shapes-builder-test] + [common-tests.files.validate-test] [common-tests.geom-align-test] [common-tests.geom-bounds-map-test] [common-tests.geom-flex-layout-test] @@ -97,6 +98,7 @@ 'common-tests.files-changes-test 'common-tests.files-builder-test 'common-tests.files-migrations-test + 'common-tests.files.validate-test 'common-tests.geom-align-test 'common-tests.geom-bounds-map-test 'common-tests.geom-flex-layout-test