diff --git a/.serena/memories/common/file-change-validation-migration-subtleties.md b/.serena/memories/common/file-change-validation-migration-subtleties.md index 014a458a30..d013415730 100644 --- a/.serena/memories/common/file-change-validation-migration-subtleties.md +++ b/.serena/memories/common/file-change-validation-migration-subtleties.md @@ -28,4 +28,5 @@ - Prefer optional attrs/default behavior so old files continue working without migration. If absence cannot preserve old behavior, add a migration. - Migrations are an ordered set mixing legacy version-derived ids and newer named ids. Keep append order stable; `migrate` applies the set difference between available migrations and file migrations. - `migrate-file` synthesizes legacy migration ids from old numeric versions when `:migrations` is absent, migrates legacy features, and records feature flags created through `cfeat/*new*`. -- When a file had no previous `:migrations`, `migrate-file` marks all migrations as migrated in metadata so callers persist the complete migration set, not only transformations that changed data. \ No newline at end of file +- When a file had no previous `:migrations`, `migrate-file` marks all migrations as migrated in metadata so callers persist the complete migration set, not only transformations that changed data. +- Closed schemas (`schema:stroke-attrs` and others) reject a key that was dropped from the declaration, so removing a declared attr needs a migration in the same change, for data already written with it. \ No newline at end of file diff --git a/common/src/app/common/files/migrations.cljc b/common/src/app/common/files/migrations.cljc index 46f24f6add..649f980021 100644 --- a/common/src/app/common/files/migrations.cljc +++ b/common/src/app/common/files/migrations.cljc @@ -2132,6 +2132,25 @@ (update :pages-index d/update-vals repair-container) (d/update-when :components d/update-vals repair-container)))) +(defmethod migrate-data "0030-remove-stroke-per-side-attr" + ;; Individual stroke widths shipped with a `:stroke-per-side` boolean on every + ;; stroke, telling the renderer and the CSS generator whether the four side + ;; widths were meaningful. Comparing the sides answers that on its own, so + ;; the attribute was dropped from the closed stroke schema and the toggle + ;; became ephemeral editor state. Files written while it was still persisted + ;; fail validation with an `:malli.core/extra-key` error; drop the attribute + ;; and keep the side widths, which hold the real design data. + [data _] + (letfn [(repair-shape [shape] + (d/update-when shape :strokes d/update-vals #(dissoc % :stroke-per-side))) + + (repair-container [container] + (d/update-when container :objects d/update-vals repair-shape))] + + (-> data + (update :pages-index d/update-vals repair-container) + (d/update-when :components d/update-vals repair-container)))) + (def available-migrations (into (d/ordered-set) ["legacy-2" @@ -2218,4 +2237,5 @@ "0026-fix-svg-raw-shapes-uuids" "0027-separate-tokens-status" "0028-normalize-constrained-values" - "0029-move-background-blur-out-of-blur"])) + "0029-move-background-blur-out-of-blur" + "0030-remove-stroke-per-side-attr"])) diff --git a/common/test/common_tests/files_migrations_test.cljc b/common/test/common_tests/files_migrations_test.cljc index b87d1b4d4c..7e92deda66 100644 --- a/common/test/common_tests/files_migrations_test.cljc +++ b/common/test/common_tests/files_migrations_test.cljc @@ -317,3 +317,145 @@ "background blur moved before schema validation") (t/is (nil? (get-in file' [:data :pages-index page-id :objects shape-id :blur])) "mis-typed :blur removed"))) + +(t/deftest migration-0030-removes-stroke-per-side-attr + (let [migration-id "0030-remove-stroke-per-side-attr" + page-id (uuid/next) + shape-id (uuid/next) + stroke {:stroke-width 2 + :stroke-width-top 2 + :stroke-width-right 4 + :stroke-width-bottom 3 + :stroke-width-left 6 + :stroke-color "#000000" + :stroke-per-side true} + shape (-> (cts/setup-shape {:id shape-id :type :rect}) + (assoc :strokes [stroke])) + data {:pages-index {page-id {:objects {shape-id shape}}}} + data' (cfm/migrate-data data migration-id) + shape' (get-in data' [:pages-index page-id :objects shape-id]) + stroke' (first (:strokes shape'))] + + (t/is (nil? (:stroke-per-side stroke')) "obsolete :stroke-per-side removed") + (t/is (= (dissoc stroke :stroke-per-side) stroke') + "no other stroke attr changed") + (t/is (= [2 4 3 6] [(:stroke-width-top stroke') + (:stroke-width-right stroke') + (:stroke-width-bottom stroke') + (:stroke-width-left stroke')]) + "per-side widths preserved") + (t/is (= data' (cfm/migrate-data data' migration-id)) + "migration is idempotent"))) + +(t/deftest migration-0030-repairs-strokes-rejected-by-schema + (let [migration-id "0030-remove-stroke-per-side-attr" + file-id (uuid/next) + page-id (uuid/next) + shape-id (uuid/next) + stroke {:stroke-width 1 + :stroke-width-top 1 + :stroke-width-right 1 + :stroke-width-bottom 1 + :stroke-width-left 1 + :stroke-color "#000000" + :stroke-per-side true} + shape (-> (cts/setup-shape {:id shape-id :type :rect}) + (assoc :strokes [stroke])) + data (-> (ctf/make-file-data file-id page-id) + (assoc-in [:pages-index page-id :objects shape-id] shape))] + + (t/is (thrown? #?(:clj Exception :cljs js/Error) + (ctf/check-file-data data)) + "new schema rejects a stroke carrying :stroke-per-side") + + (let [data' (cfm/migrate-data data migration-id)] + (t/is (= data' (ctf/check-file-data data')) + "migrated data passes the schema")))) + +(t/deftest migration-0030-repairs-component-strokes + (let [migration-id "0030-remove-stroke-per-side-attr" + component-id (uuid/next) + shape-id (uuid/next) + stroke {:stroke-width 3 + :stroke-width-top 3 + :stroke-width-right 5 + :stroke-width-bottom 3 + :stroke-width-left 3 + :stroke-color "#000000" + :stroke-per-side true} + shape (-> (cts/setup-shape {:id shape-id :type :rect}) + (assoc :strokes [stroke])) + data {:components {component-id {:objects {shape-id shape}}}} + data' (cfm/migrate-data data migration-id) + stroke' (get-in data' [:components component-id :objects shape-id :strokes 0])] + + (t/is (nil? (:stroke-per-side stroke')) + "obsolete attr removed from a component stroke") + (t/is (= 5 (:stroke-width-right stroke')) + "component per-side width preserved"))) + +(t/deftest migration-0030-keeps-strokes-without-the-obsolete-attr + (let [migration-id "0030-remove-stroke-per-side-attr" + page-id (uuid/next) + shape-id (uuid/next) + stroke {:stroke-width 2 + :stroke-width-top 2 + :stroke-width-right 2 + :stroke-width-bottom 2 + :stroke-width-left 2} + shape (-> (cts/setup-shape {:id shape-id :type :rect}) + (assoc :strokes [stroke])) + data {:pages-index {page-id {:objects {shape-id shape}}}}] + + (t/is (= data (cfm/migrate-data data migration-id)) + "a stroke without the obsolete attr is left untouched"))) + +(t/deftest migration-0030-keeps-distinct-sides-when-the-attr-was-false + (let [migration-id "0030-remove-stroke-per-side-attr" + page-id (uuid/next) + shape-id (uuid/next) + ;; The old toggle set the flag to false without equalizing the sides, + ;; so the per-side values are the saved design intent and must survive. + stroke {:stroke-width 1 + :stroke-width-top 1 + :stroke-width-right 8 + :stroke-width-bottom 1 + :stroke-width-left 1 + :stroke-color "#000000" + :stroke-per-side false} + shape (-> (cts/setup-shape {:id shape-id :type :rect}) + (assoc :strokes [stroke])) + data {:pages-index {page-id {:objects {shape-id shape}}}} + stroke' (get-in (cfm/migrate-data data migration-id) + [:pages-index page-id :objects shape-id :strokes 0])] + + (t/is (nil? (:stroke-per-side stroke')) "obsolete attr removed") + (t/is (= 8 (:stroke-width-right stroke')) + "distinct side width kept instead of being equalized"))) + +(t/deftest migration-0030-runs-through-file-migration + (let [migration-id "0030-remove-stroke-per-side-attr" + shape-id (uuid/next) + stroke {:stroke-width 5 + :stroke-width-top 5 + :stroke-width-right 5 + :stroke-width-bottom 5 + :stroke-width-left 5 + :stroke-color "#000000" + :stroke-per-side true} + file (ctf/make-file {:name "Legacy per-side stroke"}) + page-id (first (get-in file [:data :pages])) + shape (-> (cts/setup-shape {:id shape-id :type :rect}) + (assoc :strokes [stroke])) + file (-> file + (assoc :migrations (disj cfm/available-migrations migration-id)) + (assoc-in [:data :pages-index page-id :objects shape-id] shape)) + file' (cfm/migrate-file file {}) + stroke' (get-in file' [:data :pages-index page-id :objects shape-id :strokes 0])] + + (t/is (cfm/need-migration? file) "new migration detected") + (t/is (not (cfm/need-migration? file')) "new migration recorded") + (t/is (contains? (:migrations file') migration-id) "migration id persisted") + (t/is (nil? (:stroke-per-side stroke')) + "obsolete attr removed before schema validation") + (t/is (= 5 (:stroke-width stroke')) "stroke width preserved")))