diff --git a/.serena/memories/common/file-change-validation-migration-subtleties.md b/.serena/memories/common/file-change-validation-migration-subtleties.md index d013415730..7333faadc3 100644 --- a/.serena/memories/common/file-change-validation-migration-subtleties.md +++ b/.serena/memories/common/file-change-validation-migration-subtleties.md @@ -21,6 +21,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. - `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. ## Migrations diff --git a/backend/src/app/main.clj b/backend/src/app/main.clj index d0b457cc8c..bdb5802053 100644 --- a/backend/src/app/main.clj +++ b/backend/src/app/main.clj @@ -75,6 +75,12 @@ ::mdef/buckets mtx/default-request-histogram-buckets ::mdef/type :histogram} + :update-file-repair + {::mdef/name "penpot_rpc_update_file_repair_total" + ::mdef/help "update-file auto-repair attempts by outcome (repaired or unrepaired)." + ::mdef/labels ["outcome"] + ::mdef/type :counter} + :rpc-main-timing {::mdef/name "penpot_rpc_main_timing" ::mdef/help "RPC command method call timing for main" diff --git a/backend/src/app/rpc/commands/files_update.clj b/backend/src/app/rpc/commands/files_update.clj index 85a0a58b1f..5c47dbf61d 100644 --- a/backend/src/app/rpc/commands/files_update.clj +++ b/backend/src/app/rpc/commands/files_update.clj @@ -12,6 +12,7 @@ [app.common.features :as cfeat] [app.common.files.changes :as cpc] [app.common.files.migrations :as fmg] + [app.common.files.repair :as cfr] [app.common.files.validate :as val] [app.common.logging :as l] [app.common.schema :as sm] @@ -41,6 +42,7 @@ (declare ^:private get-lagged-changes) (declare ^:private send-notifications!) +(declare ^:private send-repair-notifications) (declare ^:private update-file) (declare ^:private update-file*) (declare ^:private process-changes-and-validate) @@ -124,6 +126,27 @@ (contains? token-change-types type) (contains? file-change-types type))) +;; Not a real editor session: file-change messages with this id are +;; delivered to every connected session, including the saver. +(def repair-session-id + #uuid "a11c0de5-0000-4000-8000-000000000001") + +(defn- referential-integrity-error? + [cause] + (= :referential-integrity (:code (ex-data cause)))) + +(defn- record-repair-outcome + [cfg outcome] + (when-let [metrics (::mtx/metrics cfg)] + (when (mtx/metrics? metrics) + (mtx/run! metrics {:id :update-file-repair + :labels [(name outcome)] + :inc 1})))) + +(defn- error-codes + [errors] + (into [] (keep :code) errors)) + ;; If features are specified from params and the final feature ;; set is different than the persisted one, update it on the ;; database. @@ -225,7 +248,9 @@ :soft-validate-affected-ms (get timings :soft-validate-affected-ms) :soft-validate-schema-ms (get timings :soft-validate-schema-ms) :validate-affected-ms (get timings :validate-affected-ms) - :validate-schema-ms (get timings :validate-schema-ms))))) + :validate-schema-ms (get timings :validate-schema-ms) + :repair-ms (get timings :repair-ms) + :reprocess-changes-ms (get timings :reprocess-changes-ms))))) (catch Throwable cause ;; NOTE: `with-defer` hooks only run on the success path ;; (`handle-response`), so timings collected before a hard @@ -241,7 +266,9 @@ :soft-validate-affected-ms (get timings :soft-validate-affected-ms) :soft-validate-schema-ms (get timings :soft-validate-schema-ms) :validate-affected-ms (get timings :validate-affected-ms) - :validate-schema-ms (get timings :validate-schema-ms))) + :validate-schema-ms (get timings :validate-schema-ms) + :repair-ms (get timings :repair-ms) + :reprocess-changes-ms (get timings :reprocess-changes-ms))) (throw cause)))))) (defn- update-file* @@ -275,27 +302,65 @@ changes skip-validate)) deleted-at - (ct/plus timestamp (ct/duration {:hours 1}))] + (ct/plus timestamp (ct/duration {:hours 1})) - (when-let [file (::snapshot file)] - (let [deleted-at (ct/plus timestamp (ldel/get-deletion-delay team)) - label (str "internal/snapshot/" revn)] + repair + (::repair file) - (fsnap/create! cfg file - {:label label + snapshot-deleted-at + (ct/plus timestamp (ldel/get-deletion-delay team)) + + change-created-at + (cond-> timestamp + repair + (ct/plus (ct/duration {:millis 1})))] + + (if repair + (let [snapshot-file (or (::snapshot file) + (fdata/realize cfg (:original repair)))] + (fsnap/create! cfg snapshot-file + {:label (str "internal/repair/" revn) :created-by "system" - :deleted-at deleted-at + :deleted-at snapshot-deleted-at + :profile-id profile-id + :session-id repair-session-id})) + (when-let [file (::snapshot file)] + (fsnap/create! cfg file + {:label (str "internal/snapshot/" revn) + :created-by "system" + :deleted-at snapshot-deleted-at :profile-id profile-id :session-id session-id}))) + (when repair + (db/insert! conn :file-change + {:id (uuid/next) + :session-id repair-session-id + :profile-id profile-id + :created-at timestamp + :updated-at timestamp + :deleted-at deleted-at + :file-id (:id file) + :revn (:revn repair) + :version (:version file) + :features (into-array (:features file)) + :changes (blob/encode (:changes repair))} + {::db/return-keys false}) + (l/wrn :hint "file auto-repaired" + :file-id (str (:id file)) + :error-codes (error-codes (:errors repair)) + :error-count (count (:errors repair)) + :change-count (count (:changes repair))) + (record-repair-outcome cfg :repaired)) + ;; Insert change (xlog) with deleted_at in a future data for ;; make them automatically eleggible for GC once they expires (db/insert! conn :file-change {:id (uuid/next) :session-id session-id :profile-id profile-id - :created-at timestamp - :updated-at timestamp + :created-at change-created-at + :updated-at change-created-at :deleted-at deleted-at :file-id (:id file) :revn (:revn file) @@ -304,12 +369,14 @@ :changes (blob/encode changes)} {::db/return-keys false}) - (persist-file! cfg file) + (persist-file! cfg (dissoc file ::repair)) (when (contains? cf/flags :redis-cache) (invalidate-caches! cfg file)) ;; Send asynchronous notifications + (when repair + (send-repair-notifications cfg params file repair)) (send-notifications! cfg params file) (with-meta {:revn revn :lagged (get-lagged-changes conn params)} @@ -434,9 +501,105 @@ (l/error :hint "file validation error" :cause cause)))) +(defn- apply-changes + [cfg file changes] + (let [state (atom {}) + file (binding [cpc/*state* state] + (-> file + (update :data cpc/process-changes changes) + (update :data d/without-nils)))] + (if-let [media-refs (-> @state :media-refs not-empty)] + (bfc/update-media-references! cfg file media-refs) + file))) + +(defn- fail-repair + [cfg file errors cause] + (l/wrn :hint "file auto-repair did not recover the file" + :file-id (str (:id file)) + :error-codes (error-codes errors) + :error-count (count errors)) + (record-repair-outcome cfg :unrepaired) + (throw cause)) + +(defn- repair-preexisting-damage + "Repair damage that was already in `original` on the pages/components + the client's `changes` touch, then re-apply those changes. Throws + `cause` when the client's changes are at fault or the repair cannot + fix the file." + [cfg original libs changes tracked tracked-0 cause] + (let [libs (or libs {})] + (try + (let [pre-errors + (binding [pmap/*tracked* nil] + (val/validate-file-affected original libs changes))] + + (when (empty? pre-errors) + (throw cause)) + + (when tracked + (reset! tracked tracked-0)) + + (let [trepair (ct/tpoint) + + result + (try + (binding [pmap/*tracked* tracked] + (cfr/repair-file-until-valid + original libs + (fn [file applied] + (binding [pmap/*tracked* nil] + (val/validate-file-affected file libs (into applied changes)))) + :initial-errors pre-errors)) + (finally + (record-validation-timing cfg :repair-ms "repair" (inst-ms (trepair))))) + + repair-changes (:changes result)] + + (when (seq (:errors result)) + (fail-repair cfg original (:errors result) cause)) + + (let [treprocess (ct/tpoint) + repaired (try + (binding [pmap/*tracked* tracked] + (apply-changes cfg (:file result) changes)) + (finally + (record-validation-timing cfg :reprocess-changes-ms "reprocess-changes" + (inst-ms (treprocess))))) + scope (into repair-changes changes)] + + (binding [pmap/*tracked* nil] + (val/validate-file-affected! repaired libs scope) + + (when (contains? cf/flags :file-schema-validation) + (let [t (ct/tpoint)] + (try + (val/validate-file-schema! repaired) + (finally + (record-validation-timing cfg :validate-schema-ms "validate-schema" (inst-ms (t)))))))) + + (-> repaired + (assoc :revn (+ (:revn original) 2)) + (assoc ::repair {:changes repair-changes + :errors pre-errors + :revn (inc (:revn original)) + :original (dissoc original ::snapshot)}))))) + (catch Throwable inner + (if (identical? inner cause) + (throw cause) + (do + (l/wrn :hint "file auto-repair failed" + :file-id (str (:id original)) + :cause inner) + (record-repair-outcome cfg :unrepaired) + (throw cause))))))) + (defn- process-changes-and-validate [cfg file changes skip-validate] - (let [should-resolve-libs? + (let [original file + tracked pmap/*tracked* + tracked-0 (some-> tracked deref) + + should-resolve-libs? (and (or (contains? cf/flags :file-validation) (contains? cf/flags :soft-file-validation)) (not skip-validate)) @@ -452,33 +615,20 @@ (when should-resolve-libs? (record-validation-timing cfg :resolve-libs-ms "resolve-libs" (inst-ms (tlibs)))))) - ;; The main purpose of this atom is provide a contextual state - ;; for the changes subsystem where optionally some hints can - ;; be provided for the changes processing. Right now we are - ;; using it for notify about the existence of media refs when - ;; a new shape is added. - state - (atom {}) - tproc (ct/tpoint) file (try - (let [file - (binding [cpc/*state* state] - (-> (files/check-version! file) - (update :revn inc) - (update :data cpc/process-changes changes) - (update :data d/without-nils))) - - file - (if-let [media-refs (-> @state :media-refs not-empty)] - (bfc/update-media-references! cfg file media-refs) - file)] - file) + (apply-changes cfg + (-> (files/check-version! file) + (update :revn inc)) + changes) (finally - (record-validation-timing cfg :process-changes-ms "process-changes" (inst-ms (tproc)))))] + (record-validation-timing cfg :process-changes-ms "process-changes" (inst-ms (tproc))))) + + pending-repair + (atom nil)] (binding [pmap/*tracked* nil] (when (contains? cf/flags :soft-file-validation) @@ -500,18 +650,25 @@ (let [t (ct/tpoint)] (try (val/validate-file-affected! file libs changes) + (catch Throwable cause + (reset! pending-repair cause) + (when-not (referential-integrity-error? cause) + (throw cause))) (finally (record-validation-timing cfg :validate-affected-ms "validate-affected" (inst-ms (t))))))) (when (and (contains? cf/flags :file-schema-validation) - (not skip-validate)) + (not skip-validate) + (nil? @pending-repair)) (let [t (ct/tpoint)] (try (val/validate-file-schema! file) (finally (record-validation-timing cfg :validate-schema-ms "validate-schema" (inst-ms (t)))))))) - file)) + (if-let [cause @pending-repair] + (repair-preexisting-damage cfg original libs changes tracked tracked-0 cause) + file))) (defn- take-snapshot? "Defines the rule when file `data` snapshot should be saved." @@ -541,6 +698,34 @@ (mapv (fn [row] (update row :changes blob/decode))))) +(defn- send-repair-notifications + [cfg {:keys [team session-id] :as params} file repair] + (let [changes (:changes repair) + lchanges (filter library-change? changes) + msgbus (::mbus/msgbus cfg)] + + (mbus/pub! msgbus + :topic (:id file) + :message {:type :file-change + :profile-id (:profile-id params) + :file-id (:id file) + :session-id repair-session-id + :revn (:revn repair) + :vern (:vern file) + :changes changes + :repaired true}) + + (when (and (:is-shared file) (seq lchanges)) + (mbus/pub! msgbus + :topic (:id team) + :message {:type :library-change + :profile-id (:profile-id params) + :file-id (:id file) + :session-id session-id + :revn (:revn repair) + :modified-at (ct/now) + :changes lchanges})))) + (defn- send-notifications! [cfg {:keys [team changes session-id] :as params} file] (let [lchanges (filter library-change? changes) diff --git a/backend/src/app/srepl/procs/file_repair.clj b/backend/src/app/srepl/procs/file_repair.clj index f863f250cf..c671f2d736 100644 --- a/backend/src/app/srepl/procs/file_repair.clj +++ b/backend/src/app/srepl/procs/file_repair.clj @@ -6,7 +6,6 @@ (ns app.srepl.procs.file-repair (:require - [app.common.files.changes :as cfc] [app.common.files.repair :as cfr] [app.common.files.validate :as cfv] [app.common.logging :as l])) @@ -17,44 +16,28 @@ (defn repair-file "Internal helper for validate and repair the file. The operation is - applied multiple times untile file is fixed or max iteration counter + applied multiple times until file is fixed or max iteration counter is reached (default 10). - This function should not be used directly, it is used throught the + This function should not be used directly, it is used through the app.srepl.main/repair-file! helper. In practical terms this function is private and implementation detail." [file libs & {:keys [max-iterations] :or {max-iterations 10}}] - (let [validate-and-repair - (fn [file libs iteration] - (when-let [errors (not-empty (cfv/validate-file file libs))] - (l/trc :hint "repairing file" - :file-id (str (:id file)) - :iteration iteration - :errors (count errors)) - (let [changes (cfr/repair-file file libs errors)] - (-> file - (update :revn inc) - (update :data cfc/process-changes changes))))) + (let [orig-revn (:revn file) + result (cfr/repair-file-until-valid + file libs + (fn [file _] + (cfv/validate-file file libs)) + :max-iterations max-iterations) + file' (update (:file result) :revn (fnil + 0) (:iterations result))] - process-file - (fn [file libs] - (loop [file file - iteration 0] - (if (< iteration max-iterations) - (if-let [file (validate-and-repair file libs iteration)] - (recur file (inc iteration)) - file) - (do - (l/wrn :hint "max retry num reached on repairing file" - :file-id (str (:id file)) - :iteration iteration) - file)))) + (when (>= (:iterations result) max-iterations) + (l/wrn :hint "max retry num reached on repairing file" + :file-id (str (:id file)) + :iteration (:iterations result))) - file' - (process-file file libs)] - - (when (not= (:revn file) (:revn file')) + (when (not= orig-revn (:revn file')) (l/trc :hint "file repaired" :file-id (str (:id file)))) file')) diff --git a/backend/test/backend_tests/rpc_file_update_repair_test.clj b/backend/test/backend_tests/rpc_file_update_repair_test.clj new file mode 100644 index 0000000000..e681d559ff --- /dev/null +++ b/backend/test/backend_tests/rpc_file_update_repair_test.clj @@ -0,0 +1,433 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns backend-tests.rpc-file-update-repair-test + (:require + [app.binfile.common :as bfc] + [app.common.features :as cfeat] + [app.common.files.repair :as cfr] + [app.common.files.validate :as cfv] + [app.common.test-helpers.compositions :as tho] + [app.common.test-helpers.files :as thf] + [app.common.test-helpers.ids-map :as thi] + [app.common.test-helpers.shapes :as ths] + [app.common.types.shape :as cts] + [app.common.uuid :as uuid] + [app.config :as cf] + [app.db :as db] + [app.features.file-snapshots :as fsnap] + [app.metrics :as mtx] + [app.msgbus :as mbus] + [app.rpc :as-alias rpc] + [app.rpc.commands.files-update :as files-update] + [app.srepl.procs.file-repair :as file-repair] + [app.util.pointer-map :as pmap] + [backend-tests.helpers :as th] + [clojure.test :as t]) + (:import + io.prometheus.client.CollectorRegistry)) + +(t/use-fixtures :once th/state-init) +(t/use-fixtures :each th/database-reset) + +(defn- with-repair-flags + [f] + (with-redefs [cf/flags (conj cf/flags :file-validation)] + (f))) + +(defn- with-full-repair-flags + [f] + (with-redefs [cf/flags (conj cf/flags :file-validation :file-schema-validation)] + (f))) + +(defn- repair-count + [outcome] + (or (.getSampleValue ^CollectorRegistry + (mtx/get-registry (::mtx/metrics th/*system*)) + "penpot_rpc_update_file_repair_total" + (into-array String ["outcome"]) + (into-array String [(name outcome)])) + 0.0)) + +(defn- add-rect + [page-id shape-id] + {:type :add-obj + :page-id page-id + :id shape-id + :parent-id uuid/zero + :frame-id uuid/zero + :components-v2 true + :obj (cts/setup-shape + {:id shape-id + :name "rect" + :frame-id uuid/zero + :parent-id uuid/zero + :type :rect})}) + +(defn- set-attr + [page-id shape-id attr val] + {:type :mod-obj + :id shape-id + :page-id page-id + :operations [{:type :set :attr attr :val val}]}) + +(defn- update-file + [profile-id file-id revn changes & {:keys [skip-validate session-id] + :or {session-id (uuid/random)}}] + (th/command! (cond-> {::th/type :update-file + ::rpc/profile-id profile-id + :id file-id + :session-id session-id + :revn revn + :vern 0 + :features cfeat/supported-features + :changes changes} + skip-validate (assoc :skip-validate true)))) + +(defn- stored-file + [file-id] + (bfc/get-file th/*system* file-id :realize? true)) + +(defn- change-rows + [file-id] + (->> (th/db-query :file-change {:file-id file-id} {::db/remove-deleted false}) + (filter (fn [row] + (and (some? (:changes row)) + (nil? (:label row))))) + (sort-by :created-at) + (vec))) + +(defn- label-rows + [file-id] + (->> (th/db-query :file-change {:file-id file-id} {::db/remove-deleted false}) + (filter :label) + (vec))) + +(defn- pointer-map-in? + [x] + (cond + (pmap/pointer-map? x) true + (map? x) (boolean (some pointer-map-in? (vals x))) + (sequential? x) (boolean (some pointer-map-in? x)) + :else false)) + +(defn- setup-file + [& {:keys [is-shared] :or {is-shared false}}] + (let [profile (th/create-profile* 1 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared is-shared}) + page-id (first (get-in file [:data :pages])) + shape-a (uuid/next) + shape-b (uuid/next) + out (update-file (:id profile) (:id file) 0 + [(add-rect page-id shape-a) + (add-rect page-id shape-b)])] + (t/is (th/success? out)) + {:profile profile + :file-id (:id file) + :page-id page-id + :shape-a shape-a + :shape-b shape-b + :revn (:revn (stored-file (:id file)))})) + +(defn- break-frame + [{:keys [profile file-id page-id shape-a revn]}] + (let [out (update-file (:id profile) file-id revn + [(set-attr page-id shape-a :frame-id (uuid/next))] + :skip-validate true)] + (t/is (th/success? out)) + (stored-file file-id))) + +(t/deftest repairs-preexisting-frame-not-found + (with-repair-flags + (fn [] + (let [{:keys [profile file-id page-id shape-a shape-b] :as ctx} (setup-file) + broken (break-frame ctx) + before (repair-count :repaired) + out (update-file (:id profile) file-id (:revn broken) + [(set-attr page-id shape-b :name "renamed")]) + stored (stored-file file-id) + shape (get-in stored [:data :pages-index page-id :objects shape-a]) + frame (get-in stored [:data :pages-index page-id :objects (:frame-id shape)])] + (t/is (th/success? out)) + (t/is (nil? (cfv/validate-file stored {}))) + (t/is (some? frame)) + (t/is (= "renamed" + (get-in stored [:data :pages-index page-id :objects shape-b :name]))) + (t/is (= 1.0 (- (repair-count :repaired) before))))))) + +(t/deftest repairs-preexisting-frame-not-found-with-schema-validation + (with-full-repair-flags + (fn [] + (let [{:keys [profile file-id page-id shape-a shape-b] :as ctx} (setup-file) + broken (break-frame ctx) + n (:revn broken) + before-repaired (repair-count :repaired) + before-unrepaired (repair-count :unrepaired) + out (update-file (:id profile) file-id n + [(set-attr page-id shape-b :name "renamed")]) + stored (stored-file file-id) + shape (get-in stored [:data :pages-index page-id :objects shape-a]) + frame (get-in stored [:data :pages-index page-id :objects (:frame-id shape)]) + labels (label-rows file-id) + repair (filter #(= (str "internal/repair/" n) (:label %)) labels) + snap (filter #(= (str "internal/snapshot/" n) (:label %)) labels)] + (t/is (th/success? out)) + (t/is (nil? (cfv/validate-file stored {}))) + (t/is (some? frame)) + (t/is (= "renamed" + (get-in stored [:data :pages-index page-id :objects shape-b :name]))) + (t/is (= (+ n 2) (:revn stored))) + (t/is (= 1 (count repair))) + (t/is (zero? (count snap))) + (t/is (= 1.0 (- (repair-count :repaired) before-repaired))) + (t/is (= 0.0 (- (repair-count :unrepaired) before-unrepaired))))))) + +(t/deftest rejects-client-caused-break + (with-repair-flags + (fn [] + (let [{:keys [profile file-id page-id shape-a revn]} (setup-file) + before (stored-file file-id) + out (update-file (:id profile) file-id revn + [(set-attr page-id shape-a :frame-id (uuid/next))]) + after (stored-file file-id)] + (t/is (not (th/success? out))) + (t/is (= :referential-integrity (:code (ex-data (:error out))))) + (t/is (= (:revn before) (:revn after))))))) + +(t/deftest skip-validate-does-not-repair + (with-repair-flags + (fn [] + (let [{:keys [profile file-id page-id shape-a shape-b] :as ctx} (setup-file) + broken (break-frame ctx) + out (update-file (:id profile) file-id (:revn broken) + [(set-attr page-id shape-b :name "renamed")] + :skip-validate true) + stored (stored-file file-id) + shape (get-in stored [:data :pages-index page-id :objects shape-a])] + (t/is (th/success? out)) + (t/is (nil? (get-in stored [:data :pages-index page-id :objects (:frame-id shape)]))))))) + +(t/deftest damage-on-untouched-page-is-left + (with-repair-flags + (fn [] + (let [{:keys [profile file-id page-id shape-a revn]} (setup-file) + page-2 (uuid/next) + shape-c (uuid/next) + _ (t/is (th/success? + (update-file (:id profile) file-id revn + [{:type :add-page :id page-2 :name "page-2"} + (add-rect page-2 shape-c)]))) + file (stored-file file-id) + _ (t/is (th/success? + (update-file (:id profile) file-id (:revn file) + [(set-attr page-2 shape-c :frame-id (uuid/next))] + :skip-validate true))) + file (stored-file file-id) + out (update-file (:id profile) file-id (:revn file) + [(set-attr page-id shape-a :name "on-page-1")]) + stored (stored-file file-id) + shape (get-in stored [:data :pages-index page-2 :objects shape-c])] + (t/is (th/success? out)) + (t/is (nil? (get-in stored [:data :pages-index page-2 :objects (:frame-id shape)]))))))) + +(t/deftest growing-scope-repairs-component-touched-by-repair + (thi/reset-idmap!) + (with-repair-flags + (fn [] + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (assoc :revn 0) + (tho/add-simple-component :component1 :root1 :child1) + (ths/add-sample-shape :other) + (update-in [:data :components (thi/id :component1)] + assoc :main-instance-id (uuid/next) + :objects {(uuid/next) {:id (uuid/next)}})) + page-id (thi/id :page1) + changes [(set-attr page-id (thi/id :other) :name "renamed")] + cfg {::files-update/validation-timings (atom {})} + result (with-redefs [bfc/get-resolved-file-libraries (fn [_ _] {})] + (@#'files-update/process-changes-and-validate cfg file changes nil)) + stored result] + (t/is (some? (::files-update/repair stored))) + (t/is (nil? (cfv/validate-file stored {}))) + (t/is (nil? (get-in stored [:data :components (thi/id :component1) :objects]))))))) + +(t/deftest repair-records-timings-with-schema-validation + (thi/reset-idmap!) + (with-full-repair-flags + (fn [] + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (assoc :revn 0) + (ths/add-sample-shape :shape1) + (ths/update-shape :shape1 :frame-id (uuid/next))) + changes [(set-attr (thi/id :page1) (thi/id :shape1) :name "renamed")] + timings (atom {}) + cfg {::files-update/validation-timings timings} + result (with-redefs [bfc/get-resolved-file-libraries (fn [_ _] {})] + (@#'files-update/process-changes-and-validate cfg file changes nil))] + (t/is (some? (::files-update/repair result))) + (t/is (number? (:repair-ms @timings))) + (t/is (number? (:reprocess-changes-ms @timings))) + (t/is (number? (:validate-schema-ms @timings))))))) + +(t/deftest unrepairable-rejects-and-calls-repair-once + (with-repair-flags + (fn [] + (let [{:keys [profile file-id page-id shape-b] :as ctx} (setup-file) + broken (break-frame ctx) + calls (atom 0) + before (repair-count :unrepaired) + out (with-redefs [cfr/repair-file (fn [_ _ _] + (swap! calls inc) + [])] + (update-file (:id profile) file-id (:revn broken) + [(set-attr page-id shape-b :name "renamed")]))] + (t/is (not (th/success? out))) + (t/is (= :referential-integrity (:code (ex-data (:error out))))) + (t/is (= 1 @calls)) + (t/is (= 1.0 (- (repair-count :unrepaired) before))))))) + +(t/deftest repair-writes-change-rows-and-reuses-snapshot + (with-repair-flags + (fn [] + (let [{:keys [profile file-id page-id shape-b] :as ctx} (setup-file) + broken (break-frame ctx) + before (count (change-rows file-id)) + session (uuid/next) + calls (atom []) + n (:revn broken) + out (with-redefs [mbus/pub! (fn [_cfg & {:keys [topic message]}] + (swap! calls conj {:topic topic :message message}))] + (update-file (:id profile) file-id n + [(set-attr page-id shape-b :name "renamed")] + :session-id session)) + rows (change-rows file-id) + new (subvec rows before) + labels (label-rows file-id) + repair (filter #(= (str "internal/repair/" n) (:label %)) labels) + snap (filter #(= (str "internal/snapshot/" n) (:label %)) labels) + msgs (filter #(= :file-change (:type (:message %))) @calls) + stored (stored-file file-id)] + (t/is (th/success? out)) + (t/is (= "renamed" + (get-in stored [:data :pages-index page-id :objects shape-b :name]))) + (t/is (= 2 (count new))) + (t/is (= (inc n) (:revn (first new)))) + (t/is (= files-update/repair-session-id (:session-id (first new)))) + (t/is (some? (:deleted-at (first new)))) + (t/is (= (:created-at (first new)) (:updated-at (first new)))) + (t/is (= (+ n 2) (:revn (second new)))) + (t/is (= session (:session-id (second new)))) + (t/is (some? (:deleted-at (second new)))) + (t/is (= (:created-at (second new)) (:updated-at (second new)))) + (t/is (not (pos? (compare (:created-at (first new)) + (:created-at (second new)))))) + (t/is (= 1 (count repair))) + (t/is (zero? (count snap))) + (t/is (>= (count msgs) 2)) + (t/is (true? (get-in (first msgs) [:message :repaired]))) + (t/is (= files-update/repair-session-id + (get-in (first msgs) [:message :session-id]))) + (t/is (= session (get-in (second msgs) [:message :session-id]))) + (t/is (not (true? (get-in (second msgs) [:message :repaired])))) + (t/is (= (+ n 2) (get-in (second msgs) [:message :revn]))))))) + +(t/deftest repair-library-change-keeps-saver-session + (with-repair-flags + (fn [] + (let [{:keys [profile file-id page-id shape-b] :as ctx} (setup-file :is-shared true) + broken (break-frame ctx) + session (uuid/next) + calls (atom []) + out (with-redefs [mbus/pub! (fn [_cfg & {:keys [topic message]}] + (swap! calls conj {:topic topic :message message}))] + (update-file (:id profile) file-id (:revn broken) + [(set-attr page-id shape-b :name "renamed")] + :session-id session)) + lib-msgs (filter #(= :library-change (:type (:message %))) @calls)] + (t/is (th/success? out)) + (t/is (seq lib-msgs)) + (doseq [msg lib-msgs] + (t/is (= session (get-in msg [:message :session-id]))) + (t/is (not= files-update/repair-session-id + (get-in msg [:message :session-id])))))))) + +(t/deftest repair-snapshot-without-auto-snapshot + (with-redefs [cf/flags (-> cf/flags + (conj :file-validation) + (disj :auto-file-snapshot))] + (let [{:keys [profile file-id page-id shape-b] :as ctx} (setup-file) + broken (break-frame ctx) + n (:revn broken) + out (update-file (:id profile) file-id n + [(set-attr page-id shape-b :name "renamed")]) + labels (label-rows file-id) + repair (filter #(= (str "internal/repair/" n) (:label %)) labels) + snaps (filter #(re-find #"^internal/snapshot/" (str (:label %))) labels) + row (first repair) + snap (fsnap/get-snapshot th/*system* file-id (:id row)) + sfile {:id file-id + :features (:features (stored-file file-id)) + :data (:data snap)}] + (t/is (th/success? out)) + (t/is (= 1 (count repair))) + (t/is (zero? (count snaps))) + (t/is (false? (pointer-map-in? (:data snap)))) + (t/is (some? (cfv/validate-file sfile {})))))) + +(t/deftest srepl-repair-grows-revn-by-iterations + (thi/reset-idmap!) + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (assoc :revn 3) + (ths/add-sample-shape :shape1) + (ths/update-shape :shape1 :frame-id (uuid/next))) + loop (cfr/repair-file-until-valid + file {} + (fn [file _] (cfv/validate-file file {}))) + wrapped (file-repair/repair-file file {})] + (t/is (pos? (:iterations loop))) + (t/is (= (+ (:revn file) (:iterations loop)) (:revn wrapped))) + (t/is (nil? (cfv/validate-file wrapped {}))))) + +(t/deftest healthy-save-writes-one-change-row + (with-repair-flags + (fn [] + (let [{:keys [profile file-id page-id shape-b revn]} (setup-file) + before (count (change-rows file-id)) + out (update-file (:id profile) file-id revn + [(set-attr page-id shape-b :name "ok")]) + labels (label-rows file-id)] + (t/is (th/success? out)) + (t/is (= 1 (- (count (change-rows file-id)) before))) + (t/is (empty? (filter #(re-find #"^internal/repair/" (str (:label %))) labels))))))) + +(t/deftest repairs-several-missing-children-on-root + (with-full-repair-flags + (fn [] + (let [{:keys [profile file-id page-id shape-a shape-b revn]} (setup-file) + r1 (uuid/next) + r2 (uuid/next) + r3 (uuid/next) + _ (t/is (th/success? + (update-file (:id profile) file-id revn + [(set-attr page-id uuid/zero :shapes + [shape-a shape-b r1 r2 r3])] + :skip-validate true))) + broken (stored-file file-id) + errors (cfv/validate-file broken {}) + out (update-file (:id profile) file-id (:revn broken) + [(set-attr page-id shape-b :name "renamed")]) + stored (stored-file file-id) + root (get-in stored [:data :pages-index page-id :objects uuid/zero])] + (t/is (= [:child-not-found :child-not-found :child-not-found] + (mapv :code errors))) + (t/is (= #{uuid/zero} (into #{} (map :shape-id) errors))) + (t/is (th/success? out)) + (t/is (= [shape-a shape-b] (:shapes root))) + (t/is (nil? (cfv/validate-file stored {}))) + (t/is (= "renamed" + (get-in stored [:data :pages-index page-id :objects shape-b :name]))))))) diff --git a/common/src/app/common/files/repair.cljc b/common/src/app/common/files/repair.cljc index 4756f8a758..f0a52b788c 100644 --- a/common/src/app/common/files/repair.cljc +++ b/common/src/app/common/files/repair.cljc @@ -8,6 +8,7 @@ (:require [app.common.data :as d] [app.common.data.macros :as dm] + [app.common.files.changes :as cfc] [app.common.files.changes-builder :as pcb] [app.common.files.helpers :as cfh] [app.common.files.tokens :as cfo] @@ -22,8 +23,6 @@ [app.common.types.variant :as ctv] [app.common.uuid :as uuid])) -(log/set-level! :debug) - (defmulti repair-error (fn [code _error _file-data _libraries] code)) @@ -867,3 +866,58 @@ (pcb/empty-changes nil) errors)] redo-changes)) + +(defn- error-fingerprint + [errors] + (frequencies (map #(dissoc % :shape) errors))) + +(defn repair-file-until-valid + "Repeatedly validate and repair `file` until `errors-fn` returns nil, + a repair pass leaves `:data` unchanged, or the error fingerprint + (errors without `:shape`) is unchanged from the previous pass. + + `errors-fn` is `(fn [file applied-changes] errors-or-nil)`. Never + changes `:revn`. + + Returns `{:file file' :changes [...] :iterations n :errors remaining}`." + [file libraries errors-fn & {:keys [max-iterations initial-errors] + :or {max-iterations 10}}] + (loop [file file + applied [] + iteration 0 + prev-fp nil + seed-errors initial-errors] + (if (>= iteration max-iterations) + {:file file + :changes applied + :iterations iteration + :errors (not-empty (errors-fn file applied))} + (let [errors (if (and (zero? iteration) (some? seed-errors)) + seed-errors + (errors-fn file applied))] + (if (empty? errors) + {:file file + :changes applied + :iterations iteration + :errors nil} + (let [fp (error-fingerprint errors)] + (cond + (= fp prev-fp) + {:file file + :changes applied + :iterations (inc iteration) + :errors (not-empty errors)} + + :else + (let [batch (repair-file file libraries errors) + data' (cfc/process-changes (:data file) batch)] + (if (= data' (:data file)) + {:file file + :changes applied + :iterations (inc iteration) + :errors (not-empty errors)} + (recur (assoc file :data data') + (into applied batch) + (inc iteration) + fp + nil)))))))))) diff --git a/common/test/common_tests/files/repair_test.cljc b/common/test/common_tests/files/repair_test.cljc index 27f6964de2..f6f24821ce 100644 --- a/common/test/common_tests/files/repair_test.cljc +++ b/common/test/common_tests/files/repair_test.cljc @@ -378,3 +378,171 @@ (t/is (= uuid/zero (:parent-id shape1'))) (t/is (= uuid/zero (:parent-id shape2'))) (t/is (= #{(thi/id :shape1) (thi/id :shape2)} (set (:shapes root'))))))) + +(defn- broken-frame-file + [] + (-> (thf/sample-file :file1 :page-label :page1) + (assoc :revn 3) + (ths/add-sample-shape :shape1) + (ths/update-shape :shape1 :frame-id (uuid/next)))) + +(t/deftest repair-file-until-valid-fixes-frame-not-found + (t/testing "repairs a :frame-not-found shape and leaves :revn untouched" + (let [file (broken-frame-file) + errors (cfv/validate-file file {}) + result (cfr/repair-file-until-valid + file {} + (fn [file _] (cfv/validate-file file {}))) + file' (:file result) + errors' (cfv/validate-file file' {}) + shape' (ths/get-shape file' :shape1 :page-label :page1)] + + (t/is (= #{:frame-not-found} (into #{} (map :code) errors))) + (t/is (pos? (count (:changes result)))) + (t/is (nil? (:errors result))) + (t/is (nil? errors')) + (t/is (= 3 (:revn file'))) + (t/is (some? (get-in file' [:data :pages-index (thi/id :page1) :objects (:frame-id shape')])))))) + +(t/deftest repair-file-until-valid-healthy-file + (t/testing "a valid file is returned unchanged" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (assoc :revn 2) + (ths/add-sample-shape :shape1)) + result (cfr/repair-file-until-valid + file {} + (fn [file _] (cfv/validate-file file {})))] + (t/is (identical? file (:file result))) + (t/is (= [] (:changes result))) + (t/is (zero? (:iterations result))) + (t/is (nil? (:errors result))) + (t/is (= 2 (:revn (:file result))))))) + +(t/deftest repair-file-until-valid-passes-applied-changes + (t/testing "errors-fn receives the changes applied so far" + (let [calls (atom []) + file (broken-frame-file) + result (cfr/repair-file-until-valid + file {} + (fn [file applied] + (swap! calls conj applied) + (cfv/validate-file file {})))] + (t/is (nil? (:errors result))) + (t/is (>= (count @calls) 2)) + (t/is (= [] (first @calls))) + (t/is (pos? (count (last @calls))))))) + +(t/deftest repair-file-until-valid-stops-on-noop-repair + (t/testing "an empty repair batch stops after one iteration" + (let [file (broken-frame-file) + errors (cfv/validate-file file {})] + (with-redefs [cfr/repair-file (fn [_ _ _] [])] + (let [result (cfr/repair-file-until-valid + file {} + (fn [file _] (cfv/validate-file file {})))] + (t/is (= 1 (:iterations result))) + (t/is (= [] (:changes result))) + (t/is (= (mapv :code errors) (mapv :code (:errors result))))))))) + +(t/deftest repair-file-until-valid-repairs-several-errors-on-one-shape + (t/testing "three missing children of one frame are all removed although + every pass reports the same [code shape-id]" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (ths/add-sample-shape :frame1 :type :frame) + (ths/update-shape :frame1 :shapes [(uuid/next) (uuid/next) (uuid/next)])) + errors (cfv/validate-file file {}) + result (cfr/repair-file-until-valid + file {} + (fn [file _] (cfv/validate-file file {}))) + frame1' (ths/get-shape (:file result) :frame1 :page-label :page1)] + (t/is (= [:child-not-found :child-not-found :child-not-found] (mapv :code errors))) + (t/is (= #{(thi/id :frame1)} (into #{} (map :shape-id) errors))) + (t/is (nil? (:errors result))) + (t/is (nil? (cfv/validate-file (:file result) {}))) + (t/is (= [] (:shapes frame1')))))) + +(t/deftest repair-file-until-valid-stops-when-repair-changes-nothing + (t/testing "a non-empty repair that leaves data equal stops after one iteration" + (let [file (broken-frame-file) + shape-id (thi/id :shape1) + page-id (thi/id :page1) + cur-name (:name (ths/get-shape file :shape1 :page-label :page1))] + (with-redefs [cfr/repair-file + (fn [_ _ _] + [{:type :mod-obj + :id shape-id + :page-id page-id + :operations [{:type :set :attr :name :val cur-name}]}])] + (let [result (cfr/repair-file-until-valid + file {} + (fn [_ _] [{:code :frame-not-found :shape-id shape-id}]) + :max-iterations 10)] + (t/is (= 1 (:iterations result))) + (t/is (= [] (:changes result))) + (t/is (= [:frame-not-found] (mapv :code (:errors result))))))))) + +(t/deftest repair-file-until-valid-stops-when-error-fingerprint-stable + (t/testing "a mutating repair with an unchanged error report stops after two iterations" + (let [file (broken-frame-file) + shape-id (thi/id :shape1) + page-id (thi/id :page1) + pass (atom 0) + sticky {:code :frame-not-found + :shape-id shape-id + :hint "stuck" + :args {:x 1}}] + (with-redefs [cfr/repair-file + (fn [_ _ _] + [{:type :mod-obj + :id shape-id + :page-id page-id + :operations [{:type :set + :attr :name + :val (str "pass-" (swap! pass inc))}]}])] + (let [result (cfr/repair-file-until-valid + file {} + (fn [_ _] [sticky]) + :max-iterations 10)] + (t/is (= 2 (:iterations result))) + (t/is (= 1 (count (:changes result)))) + (t/is (= [:frame-not-found] (mapv :code (:errors result))))))))) + +(t/deftest repair-file-until-valid-stops-at-max-iterations + (t/testing "a repair that never empties errors stops at max-iterations" + (let [file (broken-frame-file) + pass (atom 0) + shape-id (thi/id :shape1) + page-id (thi/id :page1)] + (with-redefs [cfr/repair-file + (fn [_ _ _] + [{:type :mod-obj + :id shape-id + :page-id page-id + :operations [{:type :set + :attr :name + :val (str "pass-" (swap! pass inc))}]}])] + (let [result (cfr/repair-file-until-valid + file {} + (fn [_ _] + [{:code :frame-not-found + :shape-id (uuid/next)}]) + :max-iterations 3)] + (t/is (= 3 (:iterations result))) + (t/is (= 3 (count (:changes result)))) + (t/is (some? (:errors result)))))))) + +(t/deftest repair-file-until-valid-uses-initial-errors + (t/testing "errors-fn is not called on the first iteration when :initial-errors is given" + (let [calls (atom 0) + file (broken-frame-file) + seed (cfv/validate-file file {}) + result (cfr/repair-file-until-valid + file {} + (fn [file applied] + (swap! calls inc) + (cfv/validate-file file {})) + :initial-errors seed)] + (t/is (nil? (:errors result))) + ;; First iteration used the seed; errors-fn runs only after the repair. + (t/is (= 1 @calls))))) + diff --git a/frontend/src/app/main/data/workspace/notifications.cljs b/frontend/src/app/main/data/workspace/notifications.cljs index b3eead45ef..c733337e2a 100644 --- a/frontend/src/app/main/data/workspace/notifications.cljs +++ b/frontend/src/app/main/data/workspace/notifications.cljs @@ -16,6 +16,7 @@ [app.main.data.common :as dc] [app.main.data.helpers :as dsh] [app.main.data.modal :as modal] + [app.main.data.notifications :as ntf] [app.main.data.plugins :as dpl] [app.main.data.websocket :as dws] [app.main.data.workspace :as-alias dw] @@ -26,6 +27,7 @@ [app.main.data.workspace.texts :as dwt] [app.main.router :as rt] [app.util.globals :refer [global]] + [app.util.i18n :refer [tr]] [app.util.mouse :as mse] [app.util.object :as obj] [app.util.rxops :as rxs] @@ -245,7 +247,8 @@ [:session-id ::sm/uuid] [:revn :int] [:vern :int] - [:changes cpc/schema:changes]]) + [:changes cpc/schema:changes] + [:repaired {:optional true} :boolean]]) (def ^:private check-file-change-params! (sm/check-fn schema:handle-file-change)) @@ -267,13 +270,17 @@ ;; and update the persistence internal state with the updated ;; file-revn - (rx/of (dch/commit {:file-id file-id - :file-revn revn - :file-vern vern - :save-undo? false - :source :remote - :redo-changes (vec changes) - :undo-changes []}))))) + (rx/concat + (rx/of (dch/commit {:file-id file-id + :file-revn revn + :file-vern vern + :save-undo? false + :source :remote + :redo-changes (vec changes) + :undo-changes []})) + (if (:repaired msg) + (rx/of (ntf/info (tr "workspace.notifications.file-repaired"))) + (rx/empty)))))) (defn handle-file-deleted [{:keys [file-id] :as msg}] diff --git a/frontend/test/frontend_tests/data/workspace_notifications_test.cljs b/frontend/test/frontend_tests/data/workspace_notifications_test.cljs new file mode 100644 index 0000000000..95872a0034 --- /dev/null +++ b/frontend/test/frontend_tests/data/workspace_notifications_test.cljs @@ -0,0 +1,50 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns frontend-tests.data.workspace-notifications-test + (:require + [app.common.data :as d] + [app.common.uuid :as uuid] + [app.main.data.changes :as dch] + [app.main.data.notifications :as ntf] + [app.main.data.workspace.notifications :as dwntf] + [app.util.i18n :as i18n] + [beicon.v2.core :as rx] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.async :as async] + [potok.v2.core :as ptk])) + +(defn- file-change-msg + [extra] + (merge {:type :file-change + :profile-id (uuid/next) + :file-id (uuid/next) + :session-id (uuid/next) + :revn 1 + :vern 0 + :changes []} + extra)) + +(defn- ^:async watch-file-change + [msg] + (let [events (atom [])] + (await (async/observe (ptk/watch (dwntf/handle-file-change msg) {} (rx/empty)) + :on-next #(swap! events conj %))) + @events)) + +(t/deftest ^:async repaired-file-change-applies-and-notifies + (let [events (await (watch-file-change (file-change-msg {:repaired true}))) + types (mapv ptk/type events) + show (d/seek #(= ::ntf/show (ptk/type %)) events) + state (ptk/update show {})] + (t/is (= [::dch/commit ::ntf/show] types)) + (t/is (= :info (get-in state [:notification :level]))) + (t/is (= (i18n/tr "workspace.notifications.file-repaired") + (get-in state [:notification :content]))))) + +(t/deftest ^:async ordinary-file-change-does-not-notify + (let [events (await (watch-file-change (file-change-msg {})))] + (t/is (= [::dch/commit] (mapv ptk/type events))))) diff --git a/frontend/translations/en.po b/frontend/translations/en.po index c1699b9564..2f4810c536 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -6977,6 +6977,12 @@ msgstr "UPDATES" msgid "workspace.notification-pill.detail" msgstr "Details" +#: src/app/main/data/workspace/notifications.cljs:282 +msgid "workspace.notifications.file-repaired" +msgstr "" +"We fixed some issues in this file. You can find a copy of the original version in " +"the version history." + #: src/app/main/ui/workspace/sidebar/options/menus/interactions.cljs:784 msgid "workspace.options.add-interaction" msgstr "Click the + button to add interactions." diff --git a/frontend/translations/es.po b/frontend/translations/es.po index b579adfd77..137798f51f 100644 --- a/frontend/translations/es.po +++ b/frontend/translations/es.po @@ -7035,6 +7035,12 @@ msgstr "ACTUALIZACIONES" msgid "workspace.notification-pill.detail" msgstr "Detalles" +#: src/app/main/data/workspace/notifications.cljs:282 +msgid "workspace.notifications.file-repaired" +msgstr "" +"Hemos corregido algunos problemas de este archivo. Hay una copia de la " +"versión original en el historial de versiones." + #: src/app/main/ui/workspace/sidebar/options/menus/interactions.cljs:784 msgid "workspace.options.add-interaction" msgstr "Pulsa el botón + para añadir interacciones."