From aed3651eb7946a61a15ce7e208c04097274d4938 Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Mon, 19 Oct 2026 14:23:29 +0200 Subject: [PATCH] :zap: Check the schema of only the pages a save touches update-file checked the schema of the whole file on every save, which loads and decodes every page even when the changes touch one. On files with several pages that was most of the time of a save. validate-file-schema-affected! checks the file data with the pages index reduced to the pages the changes modify; the data outside pages is still checked whole, and a change type it does not know makes it check the whole file. update-file uses it for the strict and the soft schema check. A save of 10 commits on a file of 10 pages of 2k shapes goes from ~1.1 s to ~240 ms; a single page file costs the same. --- ...-change-validation-migration-subtleties.md | 1 + backend/src/app/rpc/commands/files_update.clj | 11 +-- .../files_update_validation_timing_test.clj | 10 +-- .../backend_tests/rpc_file_update_test.clj | 8 +-- common/src/app/common/files/validate.cljc | 52 ++++++++++++++ .../common_tests/files/validate_test.cljc | 69 +++++++++++++++++++ 6 files changed, 137 insertions(+), 14 deletions(-) diff --git a/.serena/memories/common/file-change-validation-migration-subtleties.md b/.serena/memories/common/file-change-validation-migration-subtleties.md index d013415730..d551bf14ed 100644 --- a/.serena/memories/common/file-change-validation-migration-subtleties.md +++ b/.serena/memories/common/file-change-validation-migration-subtleties.md @@ -20,6 +20,7 @@ - Full referential/semantic validation currently runs only when file features contain `"components/v2"`. - Validation starts at root plus orphan shapes, then validates component records. `validate-file!` raises `:validation :referential-integrity` with collected details. +- `update-file` validates per save with the change-scoped variants: `validate-file-affected!` (referential, whole touched pages/components) and `validate-file-schema-affected!` (file schema with `:pages-index` reduced to the touched pages; non-page data checked whole). The schema variant maps change types to pages itself (`touched-page-ids`): a new change type that writes a page must be added there, or to `file-level-change-types` if it touches no page; an unknown type makes it check the whole file. The whole-file schema check loads and decodes every page, ~75–80% of a save on multi-page files. - `repair-file` does not mutate data directly; it reduces validation errors into redo changes using `changes-builder`. Callers must apply or persist those changes. - `:missing-slot` fires only for a REAL swap: a copy sub-head whose `shape-ref` is no longer a child of the near main parent. A pure positional mismatch (ref still a sibling elsewhere) is a reorder — valid, realigned by the async component sync; do not "repair" it by assigning swap slots (a slot freezes the child out of normal sync). `fix-missing-swap-slots` (migration 0019) follows the same membership rule. diff --git a/backend/src/app/rpc/commands/files_update.clj b/backend/src/app/rpc/commands/files_update.clj index 0c1ba4d0b1..637c4af3c6 100644 --- a/backend/src/app/rpc/commands/files_update.clj +++ b/backend/src/app/rpc/commands/files_update.clj @@ -444,9 +444,9 @@ (apply update-fn cfg file args))) (defn- soft-validate-file-schema! - [file] + [file changes] (try - (val/validate-file-schema! file) + (val/validate-file-schema-affected! file changes) (catch Throwable cause (l/error :hint "file schema validation error" :cause cause)))) @@ -479,7 +479,8 @@ (record-validation-timing ~cfg ~step-key ~step-label (inst-ms (t#))))))) (defn- validate-file - "Validates the entities affected by `changes` and the file schema." + "Validates the entities `changes` affect and the schema of the data they + modify." [cfg file libs changes skip-validate] (binding [pmap/*tracked* nil] (when (contains? cf/flags :soft-file-validation) @@ -488,7 +489,7 @@ (when (contains? cf/flags :soft-file-schema-validation) (with-validation-timing cfg :soft-validate-schema-ms "soft-validate-schema" - (soft-validate-file-schema! file))) + (soft-validate-file-schema! file changes))) (when (and (contains? cf/flags :file-validation) (not skip-validate)) @@ -498,7 +499,7 @@ (when (and (contains? cf/flags :file-schema-validation) (not skip-validate)) (with-validation-timing cfg :validate-schema-ms "validate-schema" - (val/validate-file-schema! file))))) + (val/validate-file-schema-affected! file changes))))) (def ^:private debug-fault-prefix "Start of the plugin data key that asks for a debug save fault." diff --git a/backend/test/backend_tests/files_update_validation_timing_test.clj b/backend/test/backend_tests/files_update_validation_timing_test.clj index 7dacdc3b45..a2477698c7 100644 --- a/backend/test/backend_tests/files_update_validation_timing_test.clj +++ b/backend/test/backend_tests/files_update_validation_timing_test.clj @@ -66,7 +66,7 @@ pmap/*tracked* (pmap/create-tracked)] (with-redefs [bfc/get-resolved-file-libraries (fn [_ _] {}) val/validate-file-affected! (fn [_ _ _] nil) - val/validate-file-schema! (fn [_] nil)] + val/validate-file-schema-affected! (fn [_ _] nil)] (let [file (@#'files-update/process-changes-and-validate cfg (test-file) [] nil nil) timings @(::files-update/validation-timings cfg)] (t/is (map? file)) @@ -92,8 +92,8 @@ (throw (ex-info "must not resolve libs" {}))) val/validate-file-affected! (fn [_ _ _] (throw (ex-info "must not validate" {}))) - val/validate-file-schema! (fn [_] - (throw (ex-info "must not validate schema" {})))] + val/validate-file-schema-affected! (fn [_ _] + (throw (ex-info "must not validate schema" {})))] ;; soft validations still run: they only check flags, not skip-validate (let [file (@#'files-update/process-changes-and-validate cfg (test-file) [] nil true) timings @(::files-update/validation-timings cfg)] @@ -115,7 +115,7 @@ (with-redefs [bfc/get-resolved-file-libraries (fn [_ _] {}) val/validate-file-affected! (fn [_ _ _] (throw (ex-info "bad file" {:code :referential-integrity}))) - val/validate-file-schema! (fn [_] nil)] + val/validate-file-schema-affected! (fn [_ _] nil)] (let [timings-atom (::files-update/validation-timings cfg)] (t/is (thrown? clojure.lang.ExceptionInfo (@#'files-update/process-changes-and-validate cfg (test-file) [] nil nil))) @@ -145,7 +145,7 @@ (when (some #(= "bad" (:name %)) changes) (throw (ex-info "bad file" {:type :validation :code :referential-integrity})))) - val/validate-file-schema! (fn [_] nil)] + val/validate-file-schema-affected! (fn [_ _] nil)] (let [file (@#'files-update/process-changes-and-validate cfg (test-file) (into [] (mapcat :changes) commits) commits nil) timings @(::files-update/validation-timings cfg)] diff --git a/backend/test/backend_tests/rpc_file_update_test.clj b/backend/test/backend_tests/rpc_file_update_test.clj index 74a6adb89b..d219fb0e0e 100644 --- a/backend/test/backend_tests/rpc_file_update_test.clj +++ b/backend/test/backend_tests/rpc_file_update_test.clj @@ -178,15 +178,15 @@ :id shape-a :operations [{:type :set :attr :name :val "schema-breaker"}]}) commit3 (commit (add-rect page-id shape-c)) - validate val/validate-file-schema! + validate val/validate-file-schema-affected! ;; Changes and shapes are schema checked while applied, so no real ;; change reaches the file schema check invalid: one is simulated - out (with-redefs [val/validate-file-schema! - (fn [file] + out (with-redefs [val/validate-file-schema-affected! + (fn [file changes] (when (= "schema-breaker" (get-in file [:data :pages-index page-id :objects shape-a :name])) (ex/raise :type :validation :code :schema-validation :hint "schema")) - (validate file))] + (validate file changes))] (update-file ctx {:changes-with-metadata [commit1 commit2 commit3]})) objects (-> (get-file-data ctx) :pages-index (get page-id) :objects)] diff --git a/common/src/app/common/files/validate.cljc b/common/src/app/common/files/validate.cljc index 6654e28508..78f300b738 100644 --- a/common/src/app/common/files/validate.cljc +++ b/common/src/app/common/files/validate.cljc @@ -1030,6 +1030,58 @@ [file] (update file :data ctf/check-file-data)) +(def ^:private file-level-change-types + "Change types that leave every page as it was." + #{:del-page :mov-page + :add-color :mod-color :del-color + :add-media :mod-media :del-media + :add-component :mod-component :del-component :restore-component :purge-component + :add-typography :mod-typography :del-typography + :set-tokens-lib :set-token :set-token-set :set-token-theme :set-tokens-status + :rename-token-set-group :move-token-set :move-token-set-group + :set-base-font-size :set-tokens-source}) + +(defn- touched-page-ids + "Ids of the pages `changes` modify, or nil if a change has a type not + known here." + [changes] + (reduce (fn [page-ids {:keys [type page-id id page object-type object-id]}] + (case type + (:add-obj :mod-obj :del-obj :fix-obj :mov-objects :reorder-children :reg-objects + :set-guide :set-flow :set-default-grid :set-comment-thread-position) + (cond-> page-ids (some? page-id) (conj page-id)) + + :add-page + (conj page-ids (or id (:id page))) + + :mod-page + (conj page-ids id) + + :set-plugin-data + (case object-type + :page (conj page-ids object-id) + :shape (cond-> page-ids (some? page-id) (conj page-id)) + page-ids) + + (if (contains? file-level-change-types type) + page-ids + (reduced nil)))) + #{} + changes)) + +(defn validate-file-schema-affected! + "Like `validate-file-schema!` but checks, of the pages, only those that + `changes` modify; the rest of the file data is checked whole. If a change + has a type not known here, checks the whole file. Returns `file`." + [file changes] + (if-let [page-ids (touched-page-ids changes)] + (do + (-> (:data file) + (update :pages-index select-keys page-ids) + (ctf/check-file-data)) + file) + (validate-file-schema! file))) + (defn validate-file! "Validate full referential integrity and semantic coherence on file data. diff --git a/common/test/common_tests/files/validate_test.cljc b/common/test/common_tests/files/validate_test.cljc index 48fd3e3f07..d36743ee24 100644 --- a/common/test/common_tests/files/validate_test.cljc +++ b/common/test/common_tests/files/validate_test.cljc @@ -10,6 +10,7 @@ - validate-file-affected – returns nil or list of errors - validate-file-affected! – same but raises on non-empty errors + - validate-file-schema-affected! – schema check of the touched pages The tests verify the scoping logic implemented by extract-affected-ids (tested indirectly) by injecting controlled broken states into specific @@ -50,6 +51,15 @@ (let [root (ctst/get-shape page uuid/zero)] (ctst/set-shape page (update root :shapes conj missing-id))))))))) +(defn- inject-invalid-page + "Give the page identified by `page-label` a name that is not a string, + so the page fails the file schema check." + [file page-label] + (ctf/update-file-data + file + (fn [file-data] + (ctpl/update-page file-data (thi/id page-label) #(assoc % :name 42))))) + (defn- inject-broken-component "Add a deleted component with `:objects nil` to the file. @@ -447,3 +457,62 @@ (t/is (= :validation (:type data))) (t/is (= :referential-integrity (:code data))) (t/is (seq (:details data))))))))) + +;; ---------------------------------------------------------------- +;; 11. validate-file-schema-affected! – schema of the touched pages +;; ---------------------------------------------------------------- + +(defn- schema-error? + [file changes] + (try + (cfv/validate-file-schema-affected! file changes) + false + (catch #?(:clj clojure.lang.ExceptionInfo :cljs cljs.core.ExceptionInfo) _ + true))) + +(t/deftest validate-file-schema-affected!-returns-the-file + (let [file (thf/sample-file :file1 :page-label :page1) + changes [{:type :mod-obj :page-id (thi/id :page1) :id uuid/zero :operations []}]] + (t/is (= file (cfv/validate-file-schema-affected! file changes))))) + +(t/deftest validate-file-schema-affected!-checks-touched-pages + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2) + (inject-invalid-page :page2))] + + (t/testing "a shape change on the invalid page raises" + (t/is (schema-error? file [{:type :mod-obj :page-id (thi/id :page2) :id uuid/zero :operations []}]))) + + (t/testing "mod-page on the invalid page raises" + (t/is (schema-error? file [{:type :mod-page :id (thi/id :page2) :name "page2"}]))) + + (t/testing "page plugin data on the invalid page raises" + (t/is (schema-error? file [{:type :set-plugin-data :object-type :page :object-id (thi/id :page2) + :namespace "ns" :key "k" :value "v"}]))) + + (t/testing "a guide on the invalid page raises" + (t/is (schema-error? file [{:type :set-guide :page-id (thi/id :page2) :id (uuid/next) :params nil}]))))) + +(t/deftest validate-file-schema-affected!-skips-untouched-pages + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2) + (inject-invalid-page :page2))] + + (t/testing "a shape change on another page does not check the invalid page" + (t/is (not (schema-error? file [{:type :mod-obj :page-id (thi/id :page1) :id uuid/zero :operations []}])))) + + (t/testing "a file level change does not check the invalid page" + (t/is (not (schema-error? file [{:type :del-color :id (uuid/next)}])))))) + +(t/deftest validate-file-schema-affected!-checks-the-rest-of-the-file + (let [color-id (uuid/next) + file (-> (thf/sample-file :file1 :page-label :page1) + (ctf/update-file-data #(assoc-in % [:colors color-id] {:id color-id :name 42})))] + (t/is (schema-error? file [{:type :mod-obj :page-id (thi/id :page1) :id uuid/zero :operations []}])))) + +(t/deftest validate-file-schema-affected!-checks-everything-for-unknown-changes + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (thf/add-sample-page :page2) + (inject-invalid-page :page2))] + (t/is (schema-error? file [{:type :mod-obj :page-id (thi/id :page1) :id uuid/zero :operations []} + {:type :some-future-change}]))))