diff --git a/.serena/memories/common/file-change-validation-migration-subtleties.md b/.serena/memories/common/file-change-validation-migration-subtleties.md index 7333faadc3..fff0b03a23 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. - `repair-file-until-valid` stops on: no errors; unchanged `:data` after apply (backend page/`PointerMap` `=` is pointer identity); stable error fingerprint `(frequencies (map #(dissoc % :shape) errors))` vs previous pass (check fingerprint before `repair-file`/`process-changes`); or `max-iterations`. Each `repair-error` builds from the original `file-data`, so N `:set`-based errors on one shape take N passes. - `: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 41604e4014..8cac0979d6 100644 --- a/backend/src/app/rpc/commands/files_update.clj +++ b/backend/src/app/rpc/commands/files_update.clj @@ -514,9 +514,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)))) @@ -549,7 +549,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) @@ -558,7 +559,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)) @@ -568,7 +569,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}]))))