diff --git a/common/src/app/common/files/repair.cljc b/common/src/app/common/files/repair.cljc index a823c152f8..8917dae466 100644 --- a/common/src/app/common/files/repair.cljc +++ b/common/src/app/common/files/repair.cljc @@ -30,14 +30,15 @@ (defn- repair-shapes "The changes that apply `repair-fn` to the shapes `ids` of the page of - `error` (its shape, by default)." + `error` (its shape, by default). A repair is not an override, so it + touches nothing; a repair of the touched groups sets them itself." ([code error file-data repair-fn] (repair-shapes code error file-data [(:id (:shape error))] repair-fn)) ([code {:keys [shape page-id]} file-data ids repair-fn] (log/debug :hint (str "repairing shape " code) :id (:id shape) :name (:name shape) :page-id page-id) (-> (pcb/empty-changes nil page-id) (pcb/with-file-data file-data) - (pcb/update-shapes ids repair-fn)))) + (pcb/update-shapes ids repair-fn {:ignore-touched true})))) (defn- repair-component "The changes that apply `repair-fn` to the component `component-id`." @@ -180,7 +181,10 @@ (repair-shapes code error file-data (fn [shape] (log/debug :hint " -> unset :main-instance") - (dissoc shape :main-instance)))) + (cond-> (dissoc shape :main-instance) + ;; only a head may name a component file + (not (ctk/instance-head? shape)) + (dissoc :component-file))))) (defmethod repair-error :should-not-be-component-root [code error file-data _] @@ -198,7 +202,7 @@ (-> (get libraries (:component-file root-shape)) :data)) component (when component-file (ctkl/get-component component-file (:component-id root-shape) true)) - component-shapes (ctf/get-component-shapes file-data component)] + component-shapes (ctf/get-component-shapes component-file component)] ;; Check if the shape points to the remote main. If so, reassign to the near main. (if-let [near-shape-1 (d/seek #(= (:shape-ref %) (:shape-ref shape)) component-shapes)] @@ -215,7 +219,7 @@ (-> (get libraries (:component-file head-shape)) :data)) component (when component-file (ctkl/get-component component-file (:component-id head-shape) true)) - component-shapes (ctf/get-component-shapes file-data component)] + component-shapes (ctf/get-component-shapes component-file component)] (if-let [near-shape-3 (d/seek #(= (:id %) (:shape-ref random-shape)) component-shapes)] near-shape-3 nil))))))] @@ -580,6 +584,20 @@ (log/error :hint "Unknown error code, don't know how to repair" :code (:code error)) file) +(def ^:private main-instance-codes + "Errors in the pointer from a component to its main instance. Until + they are repaired, the copies of the component find no near main." + #{:invalid-main-instance-id :invalid-main-instance-page :invalid-main-instance}) + +(defn- repairable-now + "The errors to repair in this pass: a copy shape whose main is not found + waits for a later pass while some component's main instance is being + repaired, as the main may be found then." + [errors] + (if (some (comp main-instance-codes :code) errors) + (remove #(= :ref-shape-not-found (:code %)) errors) + errors)) + (defn repair-file [{:keys [data id] :as file} libraries errors] (log/debug :hint "repairing file" :id (str id) :errors (count errors)) @@ -591,7 +609,7 @@ data libraries))) (pcb/empty-changes nil) - errors)] + (repairable-now errors))] redo-changes)) (defn- error-fingerprint diff --git a/common/src/app/common/files/validate.cljc b/common/src/app/common/files/validate.cljc index 938559b9e5..89324fdb32 100644 --- a/common/src/app/common/files/validate.cljc +++ b/common/src/app/common/files/validate.cljc @@ -351,13 +351,10 @@ (defn- check-component-ref "Validate that the referenced shape exists in the near component." [shape file page libraries] - (let [library-exists (library-exists? file libraries shape) - ref-shape (when library-exists - (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)) - shape file page)))) + (when (nil? (find-ref-shape* file page libraries shape)) + (report-error :ref-shape-not-found + (str/ffmt "Referenced shape % not found in near component" (:shape-ref shape)) + shape file page))) (defn- check-component-not-ref "Validate that this shape does not reference other one." @@ -375,7 +372,9 @@ (ctk/instance-head? ref-shape)) (report-error :ref-shape-is-head (str/ffmt "Referenced shape % is a component, so the copy must also be" (:shape-ref shape)) - shape file page)))) + shape file page + :component-file (:component-file ref-shape) + :component-id (:component-id ref-shape))))) (defn- check-ref-is-head "Validate that the referenced shape is a nested copy root." @@ -504,36 +503,42 @@ - :component-root - :shape-ref" [shape file page libraries] - ;; We propagate have to propagate to nested shapes if library is valid or not - (let [library-exists (library-exists? file libraries shape)] - (check-component-not-main-head shape file page libraries) - (check-component-ref shape file page libraries) - (check-ref-is-head shape file page libraries) - (check-no-swap-slot shape file page) - (check-not-swapped shape file page) - (check-valid-touched shape file page) - (check-no-duplicate-ref shape file page) - (run! #(check-shape % file page libraries :context :copy-top :library-exists library-exists) (:shapes shape)))) + (check-component-not-main-head shape file page libraries) + (when (library-exists? file libraries shape) + (check-component-ref shape file page libraries)) + (check-ref-is-head shape file page libraries) + (check-no-swap-slot shape file page) + (check-not-swapped shape file page) + (check-valid-touched shape file page) + (check-no-duplicate-ref shape file page) + (run! #(check-shape % file page libraries :context :copy-top + :local-refs (= (:component-file shape) (:id file))) + (:shapes shape))) (defn- check-shape-copy-root-nested "Root shape of a nested copy instance - :component-id - :component-file - :shape-ref" - [shape file page libraries library-exists] + [shape file page libraries local-refs] (check-component-not-main-head shape file page libraries) (check-component-not-root shape file page) (check-valid-touched shape file page) (check-ref-component-id shape file page libraries) (check-no-swap-slot shape file page) - (check-ref-in-near-main shape file page libraries) - ;; We can have situations where the nested copy and the ancestor copy come from different libraries and some of them have been dettached - ;; so we only validate the shape-ref if the ancestor is from a valid library - (when library-exists - (check-component-ref shape file page libraries) - (check-ref-is-head shape file page libraries)) + ;; a ref into a library may name a shape the library has deleted since + ;; the file last took its updates, which then remove the copy shape + (when local-refs + (check-ref-in-near-main shape file page libraries) + (check-component-ref shape file page libraries)) + (check-ref-is-head shape file page libraries) (check-no-duplicate-ref shape file page) - (run! #(check-shape % file page libraries :context :copy-nested) (:shapes shape))) + ;; the children of a swapped head refer into its own component + (run! #(check-shape % file page libraries :context :copy-nested + :local-refs (if (ctk/swapped? shape) + (= (:component-file shape) (:id file)) + local-refs)) + (:shapes shape))) (defn- check-shape-main-not-root "Not-root shape of a main instance (not any attribute)" @@ -547,16 +552,17 @@ (defn- check-shape-copy-not-root "Not-root shape of a copy instance :shape-ref" - [shape file page libraries] + [shape file page libraries local-refs] (check-component-not-main-not-head shape file page) (check-component-not-root shape file page) - (check-component-ref shape file page libraries) + (when local-refs + (check-component-ref shape file page libraries)) (check-ref-is-not-head shape file page libraries) (check-no-swap-slot shape file page) (check-not-swapped shape file page) (check-valid-touched shape file page) (check-no-duplicate-ref shape file page) - (run! #(check-shape % file page libraries :context :copy-any) (:shapes shape))) + (run! #(check-shape % file page libraries :context :copy-any :local-refs local-refs) (:shapes shape))) (defn- check-shape-not-component "Shape is not in a component or is a fostered children (not any @@ -671,7 +677,7 @@ - :main-any - :copy-any " - [shape-id file page libraries & {:keys [context library-exists] :or {context :not-component library-exists false}}] + [shape-id file page libraries & {:keys [context local-refs] :or {context :not-component local-refs false}}] (let [shape (ctst/get-shape page shape-id)] (when (some? shape) (check-geometry shape file page) @@ -716,14 +722,14 @@ (report-error :nested-copy-not-allowed "Nested copy component only allowed inside other component" shape file page) - (check-shape-copy-root-nested shape file page libraries library-exists))))) + (check-shape-copy-root-nested shape file page libraries local-refs))))) (if (ctk/in-component-copy? shape) (if-not (#{:copy-top :copy-nested :copy-any} context) (report-error :not-head-copy-not-allowed "Non-root copy only allowed inside a copy" shape file page) - (check-shape-copy-not-root shape file page libraries)) + (check-shape-copy-not-root shape file page libraries local-refs)) (cond (#{:main-top :main-nested :main-any} context) diff --git a/common/test/common_tests/files/repair_components_test.cljc b/common/test/common_tests/files/repair_components_test.cljc index 68b9f4c4ee..677ef27558 100644 --- a/common/test/common_tests/files/repair_components_test.cljc +++ b/common/test/common_tests/files/repair_components_test.cljc @@ -16,7 +16,6 @@ [app.common.files.validate :as cfv] [app.common.logic.shapes :as cls] [app.common.test-helpers.components :as thc] - [app.common.test-helpers.composable.core :as tm] [app.common.test-helpers.compositions :as tho] [app.common.test-helpers.files :as thf] [app.common.test-helpers.ids-map :as thi] @@ -171,11 +170,8 @@ (t/is (= #{:invalid-main-instance-id :ref-shape-not-found} found)) (t/is (nil? errors)) (t/is (= (thi/id :main-root) (:main-instance-id (thc/get-component file' :component1)))) - ;; the :ref-shape-not-found repair, run in the same pass, detaches the copy - (tm/known-failure "F60" - [(thi/id :component1) (thi/id :main-root)] - [nil nil] - (copy-link file')))) + ;; the copy keeps its link + (t/is (= [(thi/id :component1) (thi/id :main-root)] (copy-link file'))))) (t/deftest invalid-main-instance-page (let [file (-> (simple) @@ -186,11 +182,8 @@ (t/is (= #{:invalid-main-instance-page :ref-shape-not-found} found)) (t/is (nil? errors)) (t/is (= (thi/id :page1) (:main-instance-page (thc/get-component file' :component1)))) - ;; the :ref-shape-not-found repair, run in the same pass, detaches the copy - (tm/known-failure "F60" - [(thi/id :component1) (thi/id :main-root)] - [nil nil] - (copy-link file')))) + ;; the copy keeps its link + (t/is (= [(thi/id :component1) (thi/id :main-root)] (copy-link file'))))) (t/deftest invalid-main-instance ;; the component names the copy as its main; the repair of @@ -199,11 +192,8 @@ (t/is (= #{:invalid-main-instance :invalid-main-instance-id :ref-shape-not-found} found)) (t/is (nil? errors)) (t/is (= (thi/id :main-root) (:main-instance-id (thc/get-component file' :component1)))) - ;; the :ref-shape-not-found repair, run in the same pass, detaches the copy - (tm/known-failure "F60" - [(thi/id :component1) (thi/id :main-root)] - [nil nil] - (copy-link file')))) + ;; the copy keeps its link + (t/is (= [(thi/id :component1) (thi/id :main-root)] (copy-link file'))))) (t/deftest shape-ref-in-main (let [[found file' errors] (repair (ths/update-shape (simple) :main-root :shape-ref (thi/id :copy-root)))] @@ -220,10 +210,10 @@ (t/is (nil? (:main-instance (ths/get-shape file' :main-child)))))) (t/deftest component-main-file-on-a-child - ;; the repair only removes :main-instance, so the error never goes away - (let [[found _ errors] (repair (ths/update-shape (simple) :main-child :component-file (thi/id :file1)))] + (let [[found file' errors] (repair (ths/update-shape (simple) :main-child :component-file (thi/id :file1)))] (t/is (= #{:component-main} found)) - (tm/known-failure "F47" nil #{:component-main} (codes errors)))) + (t/is (nil? errors)) + (t/is (nil? (:component-file (ths/get-shape file' :main-child)))))) (t/deftest should-not-be-component-root (let [[found file' errors] (repair (ths/update-shape (simple) :main-child :component-root true))] @@ -261,21 +251,83 @@ (t/is (= (thi/id :main2-nested-head) (:shape-ref (ths/get-shape file' :copy2-nested-head)))) (t/is (ctk/instance-head? (ths/get-shape file' :copy2-nested-head))))) +(t/deftest ref-shape-not-found-pointing-at-another-library-copy + ;; a library copy points at another copy of the same component: it is + ;; reassigned to the library main that one references + (let [library (-> (thf/sample-file :library :is-shared true :page-label :lib-page) + (tho/add-simple-component :component1 :lib-main :lib-main-child)) + file (-> (thf/sample-file :file1 :page-label :page1) + (thc/instantiate-component :component1 :copy1 :library library) + (thc/instantiate-component :component1 :copy2 :library library) + (ths/update-shape :copy2 :shape-ref (thi/id :copy1))) + [found file' errors] (repair file {(:id library) library})] + (t/is (= #{:ref-shape-not-found} found)) + (t/is (nil? errors)) + (t/is (= [(thi/id :component1) (thi/id :lib-main)] + ((juxt :component-id :shape-ref) (ths/get-shape file' :copy2)))))) + (t/deftest ref-shape-not-found-in-a-copy-child - ;; the reference of a non-head copy shape is never checked: the check reads - ;; the library from the shape itself, and only heads have one - (let [errors (cfv/validate-file (ths/update-shape (simple) :copy-child :shape-ref (uuid/next)) {})] - (tm/known-failure "F46" #{:ref-shape-not-found} #{} (codes errors)))) + (let [[found file' errors] (repair (ths/update-shape (simple) :copy-child :shape-ref (uuid/next)))] + (t/is (= #{:ref-shape-not-found} found)) + (t/is (nil? errors)) + ;; no matching shape: the child is detached, the copy keeps its link + (t/is (nil? (:shape-ref (ths/get-shape file' :copy-child)))) + (t/is (= [(thi/id :component1) (thi/id :main-root)] (copy-link file'))))) + +(t/deftest ref-shape-not-found-in-a-head-nested-twice + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (tho/add-two-levels-nested-component-with-copy + :component1 :main1-root :main1-child + :component2 :main2-root :nested-head1 + :component3 :main3-root :nested-head2 :nested-subhead2 + :copy-root + :copy2-root-params {:children-labels [:copy-head :copy-subhead :copy-rect]}) + (ths/update-shape :copy-subhead :shape-ref (uuid/next))) + errors (cfv/validate-file file {})] + (t/is (contains? (codes errors) :ref-shape-not-found)))) + +(t/deftest ref-shape-not-found-in-a-head-under-a-group + ;; component2 nests the copy of component1 inside a group + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (tho/add-simple-component :component1 :main1-root :main1-child) + (tho/add-frame :main2-root) + (tho/add-group :main2-group :parent-label :main2-root) + (thc/instantiate-component :component1 :main2-nested-head :parent-label :main2-group) + (thc/make-component :component2 :main2-root) + (thc/instantiate-component :component2 :copy-root + :children-labels [:copy-group :copy-nested-head :copy-child]) + (ths/update-shape :copy-nested-head :shape-ref (uuid/next))) + errors (cfv/validate-file file {})] + (t/is (contains? (codes errors) :ref-shape-not-found)))) + +(t/deftest library-main-child-deleted-before-the-update + ;; a library main deletes its nested copy; the copy in the consuming file + ;; still refers to it until the user takes the library update, which + ;; removes it + (let [library (-> (thf/sample-file :library :is-shared true :page-label :lib-page) + (tho/add-nested-component :component1 :main1-root :main1-child + :component2 :main2-root :main2-nested-head)) + file (-> (thf/sample-file :file1 :page-label :page1) + (thc/instantiate-component :component2 :copy2-root + :library library + :children-labels [:copy2-nested-head])) + page (thf/current-page library) + [_ del] (cls/generate-delete-shapes (pcb/empty-changes nil (:id page)) + (:data library) page (:objects page) + #{(thi/id :main2-nested-head)} {}) + library' (thf/apply-changes library del)] + (t/is (nil? (cfv/validate-file file {(:id library') library'}))))) (t/deftest ref-shape-is-head - ;; the check reports no component, so the repair cannot make the shape a - ;; head again + ;; a nested copy head that lost its component is made a head again (let [file (-> (nested) (ths/update-shape :copy2-nested-head :component-id nil) (ths/update-shape :copy2-nested-head :component-file nil)) - [found _ errors] (repair file)] + [found file' errors] (repair file)] (t/is (= #{:ref-shape-is-head} found)) - (tm/known-failure "F48" nil #{:ref-shape-is-head} (codes errors)))) + (t/is (nil? errors)) + (t/is (= [(thi/id :component1) (thi/id :file1)] + ((juxt :component-id :component-file) (ths/get-shape file' :copy2-nested-head)))))) (t/deftest ref-shape-is-not-head ;; a copy child made a head while the shape it references is not one diff --git a/common/test/common_tests/files/repair_test.cljc b/common/test/common_tests/files/repair_test.cljc index f6f24821ce..268910db34 100644 --- a/common/test/common_tests/files/repair_test.cljc +++ b/common/test/common_tests/files/repair_test.cljc @@ -546,3 +546,20 @@ ;; First iteration used the seed; errors-fn runs only after the repair. (t/is (= 1 @calls))))) +(t/deftest repair-invalid-geometry-of-a-copy-shape + (t/testing "a repair is not an override: the copy shape it resets stays untouched, so sync gives it back its main's geometry" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (tho/add-simple-component :component1 :main-root :main-child) + (thc/instantiate-component :component1 :copy-root + :children-labels [:copy-child]) + (ths/update-shape :copy-child :width nil) + (ths/update-shape :copy-child :touched nil)) + + errors (cfv/validate-file file {}) + changes (cfr/repair-file file {} errors) + file' (thf/apply-changes file {:redo-changes changes} :validate? false) + child' (ths/get-shape file' :copy-child :page-label :page1)] + + (t/is (= [:invalid-geometry] (mapv :code errors))) + (t/is (nil? (cfv/validate-file file' {}))) + (t/is (nil? (:touched child'))))))