From b46bcc920743b4731545d05cddd2f4e4233025d1 Mon Sep 17 00:00:00 2001 From: Eva Marco Date: Tue, 29 Sep 2026 14:26:28 +0200 Subject: [PATCH] :wrench: Validate variant containers after property changes apply-changes-local replays the pending redo changes on a local copy of the data, so that later builder calls can read up-to-date values. It replayed :validate-shapes too, so the validator ran on that copy, which has no real file id and no libraries. Any chain of two generators that apply local changes then failed with false errors, such as :component-not-found. Skip :validate-shapes when applying changes locally. Add the variant container to the shapes that generate-update-property-name and generate-remove-property validate. Validating only the main instances misses errors that only the container check finds, such as siblings with different property names. Register variant-properties-test in the runner so that it runs on JS, where validation takes place. Add tests that corrupt a main instance or the container and expect a validation error. Remove two debug prn calls from the validator. AI-assisted-by: claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- .../src/app/common/files/changes_builder.cljc | 2 + common/src/app/common/files/validate.cljc | 2 - .../app/common/logic/variant_properties.cljc | 6 +- .../logic/variant_properties_test.cljc | 103 +++++++++++++++++- common/test/common_tests/runner.cljc | 2 + 5 files changed, 109 insertions(+), 6 deletions(-) diff --git a/common/src/app/common/files/changes_builder.cljc b/common/src/app/common/files/changes_builder.cljc index dbaf45ae69..95a80ec344 100644 --- a/common/src/app/common/files/changes_builder.cljc +++ b/common/src/app/common/files/changes_builder.cljc @@ -197,6 +197,8 @@ redo-changes (:redo-changes changes) new-changes (if (< index (count redo-changes)) (->> (subvec (:redo-changes changes) index) + ;; Validation only makes sense on the real file data + (remove #(= :validate-shapes (:type %))) (map #(-> % (assoc :page-id uuid/zero) (dissoc :component-id)))) diff --git a/common/src/app/common/files/validate.cljc b/common/src/app/common/files/validate.cljc index 9e25d86021..f4b702d229 100644 --- a/common/src/app/common/files/validate.cljc +++ b/common/src/app/common/files/validate.cljc @@ -270,8 +270,6 @@ "Shape expected to be main instance" shape file page)) (when-not (= (:component-file shape) (:id file)) - (prn "shape" (:id shape) (:name shape) (:component-file shape)) - (prn "file" (:id file) (:name file)) (report-error :component-main-external "Main instance should refer to a component in the same file" shape file page)) diff --git a/common/src/app/common/logic/variant_properties.cljc b/common/src/app/common/logic/variant_properties.cljc index 673e73cfe1..ad48a06140 100644 --- a/common/src/app/common/logic/variant_properties.cljc +++ b/common/src/app/common/logic/variant_properties.cljc @@ -41,8 +41,8 @@ {:apply-changes-local-library? true})) changes related-components) - - ids-to-validate (map :main-instance-id related-components) + + ids-to-validate (conj (mapv :main-instance-id related-components) variant-id) changes (pcb/validate-shapes changes (pcb/get-page-id changes) @@ -74,7 +74,7 @@ changes related-components) - ids-to-validate (map :main-instance-id related-components) + ids-to-validate (conj (mapv :main-instance-id related-components) variant-id) changes (pcb/validate-shapes changes (pcb/get-page-id changes) diff --git a/common/test/common_tests/logic/variant_properties_test.cljc b/common/test/common_tests/logic/variant_properties_test.cljc index f73214236a..404e459d51 100644 --- a/common/test/common_tests/logic/variant_properties_test.cljc +++ b/common/test/common_tests/logic/variant_properties_test.cljc @@ -15,10 +15,29 @@ [app.common.test-helpers.shapes :as ths] [app.common.test-helpers.variants :as thv] [app.common.uuid :as uuid] - [clojure.test :as t])) + [clojure.test :as t] + [cuerdas.core :as str])) (t/use-fixtures :each thi/test-fixture) +#?(:cljs + (defn- validation-error + "Apply the changes and return the ex-data of the :validate-shapes + error they raise, or nil if they raise none." + [file changes] + (try + (thf/apply-changes file changes :validate? false) + nil + (catch :default e + (let [data (ex-data e)] + (when (= :referential-integrity (:code data)) + data)))))) + +#?(:cljs + (defn- error-codes + [error] + (into #{} (map :code) (:details error)))) + ;; ============================================================================= ;; generate-update-property-name tests ;; ============================================================================= @@ -655,3 +674,85 @@ (t/is (= "Property 1" (-> c03' :variant-properties first :name))) (t/is (= "Frame1" (-> c03' :variant-properties first :value))) (t/is (= "Frame1" (-> m03' :variant-name))))) + +;; ============================================================================= +;; validate-shapes failure tests (only run on CLJS, where validation runs) +;; ============================================================================= + +#?(:cljs + (t/deftest test-update-property-name-validates-main + (let [file (-> (thf/sample-file :file1) + (thv/add-variant-two-properties :v01 :c01 :m01 :c02 :m02) + (ths/update-shape :m01 :variant-id (uuid/next))) + v-id (-> (ths/get-shape file :v01) :id) + page (thf/current-page file) + + changes (-> (pcb/empty-changes nil) + (pcb/with-page-id (:id page)) + (pcb/with-library-data (:data file)) + (pcb/with-objects (:objects page)) + (clvp/generate-update-property-name v-id 0 "NewName")) + + error (validation-error file changes)] + + (t/is (str/includes? (:hint error) "generate-update-property-name")) + (t/is (contains? (error-codes error) :variant-component-bad-id))))) + +#?(:cljs + (t/deftest test-update-property-name-validates-container + (let [file (-> (thf/sample-file :file1) + (thv/add-variant-two-properties :v01 :c01 :m01 :c02 :m02) + (thc/update-component :c02 {:variant-properties [{:name "Other" :value "p1v2"} + {:name "Property 2" :value "p2v2"}]})) + v-id (-> (ths/get-shape file :v01) :id) + page (thf/current-page file) + + changes (-> (pcb/empty-changes nil) + (pcb/with-page-id (:id page)) + (pcb/with-library-data (:data file)) + (pcb/with-objects (:objects page)) + (clvp/generate-update-property-name v-id 1 "NewName")) + + error (validation-error file changes)] + + (t/is (str/includes? (:hint error) "generate-update-property-name")) + (t/is (contains? (error-codes error) :invalid-variant-properties))))) + +#?(:cljs + (t/deftest test-remove-property-validates-main + (let [file (-> (thf/sample-file :file1) + (thv/add-variant-two-properties :v01 :c01 :m01 :c02 :m02) + (ths/update-shape :m01 :variant-id (uuid/next))) + v-id (-> (ths/get-shape file :v01) :id) + page (thf/current-page file) + + changes (-> (pcb/empty-changes nil) + (pcb/with-page-id (:id page)) + (pcb/with-library-data (:data file)) + (pcb/with-objects (:objects page)) + (clvp/generate-remove-property v-id 1)) + + error (validation-error file changes)] + + (t/is (str/includes? (:hint error) "generate-remove-property")) + (t/is (contains? (error-codes error) :variant-component-bad-id))))) + +#?(:cljs + (t/deftest test-remove-property-validates-container + (let [file (-> (thf/sample-file :file1) + (thv/add-variant-two-properties :v01 :c01 :m01 :c02 :m02) + (thc/update-component :c02 {:variant-properties [{:name "Other" :value "p1v2"} + {:name "Property 2" :value "p2v2"}]})) + v-id (-> (ths/get-shape file :v01) :id) + page (thf/current-page file) + + changes (-> (pcb/empty-changes nil) + (pcb/with-page-id (:id page)) + (pcb/with-library-data (:data file)) + (pcb/with-objects (:objects page)) + (clvp/generate-remove-property v-id 1)) + + error (validation-error file changes)] + + (t/is (str/includes? (:hint error) "generate-remove-property")) + (t/is (contains? (error-codes error) :invalid-variant-properties))))) diff --git a/common/test/common_tests/runner.cljc b/common/test/common_tests/runner.cljc index 35db81954b..f9b30b2415 100644 --- a/common/test/common_tests/runner.cljc +++ b/common/test/common_tests/runner.cljc @@ -63,6 +63,7 @@ [common-tests.logic.swap-and-reset-test] [common-tests.logic.swap-as-override-test] [common-tests.logic.token-test] + [common-tests.logic.variant-properties-test] [common-tests.logic.variants-switch-test] [common-tests.math-test] [common-tests.media-test] @@ -146,6 +147,7 @@ 'common-tests.logic.swap-and-reset-test 'common-tests.logic.swap-as-override-test 'common-tests.logic.token-test + 'common-tests.logic.variant-properties-test 'common-tests.logic.variants-switch-test 'common-tests.math-test 'common-tests.types.variant-test