From 24efa867e7e75d2d5798f81a4d8746937669a9b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Moya?= Date: Tue, 26 Sep 2023 14:54:10 +0200 Subject: [PATCH] :tada: Do file validation on each file change --- backend/scripts/repl | 3 +- backend/scripts/start-dev | 3 +- backend/src/app/rpc/commands/files_update.clj | 58 +++++++++++++------ backend/test/backend_tests/helpers.clj | 3 +- frontend/src/debug.cljs | 3 +- 5 files changed, 47 insertions(+), 23 deletions(-) diff --git a/backend/scripts/repl b/backend/scripts/repl index 7ceffb38b..4004b392f 100755 --- a/backend/scripts/repl +++ b/backend/scripts/repl @@ -24,7 +24,8 @@ export PENPOT_FLAGS="\ enable-rpc-rlimit \ enable-soft-rpc-rlimit \ enable-webhooks \ - enable-access-tokens"; + enable-access-tokens \ + disable-file-validation"; # export PENPOT_DATABASE_URI="postgresql://172.17.0.1:5432/penpot" # export PENPOT_DATABASE_USERNAME="penpot" diff --git a/backend/scripts/start-dev b/backend/scripts/start-dev index d9ba4b1ce..33f4dcac8 100755 --- a/backend/scripts/start-dev +++ b/backend/scripts/start-dev @@ -15,7 +15,8 @@ export PENPOT_FLAGS="\ enable-fdata-storage-objets-map \ disable-secure-session-cookies \ enable-smtp \ - enable-access-tokens"; + enable-access-tokens \ + disable-file-validation"; set -ex diff --git a/backend/src/app/rpc/commands/files_update.clj b/backend/src/app/rpc/commands/files_update.clj index 843e2aafc..0884ffcfe 100644 --- a/backend/src/app/rpc/commands/files_update.clj +++ b/backend/src/app/rpc/commands/files_update.clj @@ -6,9 +6,11 @@ (ns app.rpc.commands.files-update (:require + [app.common.data :as d] [app.common.exceptions :as ex] [app.common.files.features :as ffeat] [app.common.files.migrations :as pmg] + [app.common.files.validate :as val] [app.common.logging :as l] [app.common.pages :as cp] [app.common.pages.changes :as cpc] @@ -55,7 +57,8 @@ ::sm/set-of-strings] [:changes {:optional true} ::changes] [:changes-with-metadata {:optional true} - [:vector ::change-with-metadata]]]) + [:vector ::change-with-metadata]] + [:skip-validate {:optional true} :boolean]]) (sm/def! ::update-file-result [:vector {:title "UpdateFileResults"} @@ -156,7 +159,7 @@ (l/trace :hint "update-file" :time (dt/format-duration elapsed)))))))) (defn update-file - [{:keys [::db/conn ::mtx/metrics] :as cfg} {:keys [profile-id id changes changes-with-metadata] :as params}] + [{:keys [::db/conn ::mtx/metrics] :as cfg} {:keys [profile-id id changes changes-with-metadata skip-validate] :as params}] (let [file (get-file conn id) features (->> (concat (:features file) (:features params)) @@ -176,12 +179,24 @@ (wrap-with-objects-map-context)) file (assoc file :features features) + + ;; TODO: this ruins performance. + ;; We must find some other way to do general validation. + libraries (when (and (cf/flags :file-validation) + (not skip-validate)) + (-> (->> (files/get-file-libraries conn (:id file)) + (map #(get-file conn (:id %))) + (map #(update % :data blob/decode)) + (d/index-by :id)) + (assoc (:id file) file))) + changes (if changes-with-metadata (->> changes-with-metadata (mapcat :changes) vec) (vec changes)) params (-> params (assoc :file file) + (assoc :libraries libraries) (assoc :changes changes) (assoc ::created-at (dt/now)))] @@ -210,12 +225,12 @@ :team-id (:team-id file)})))))) (defn- update-file* - [{:keys [::db/conn] :as cfg} {:keys [profile-id file changes session-id ::created-at] :as params}] + [{:keys [::db/conn] :as cfg} {:keys [profile-id file libraries changes session-id ::created-at skip-validate] :as params}] (let [;; Process the file data in the CLIMIT context; scheduling it ;; to be executed on a separated executor for avoid to do the ;; CPU intensive operation on vthread. file (-> (climit/configure cfg :update-file) - (climit/submit! (partial update-file-data file changes)))] + (climit/submit! (partial update-file-data file libraries changes skip-validate)))] (db/insert! conn :file-change {:id (uuid/next) @@ -249,23 +264,28 @@ (get-lagged-changes conn params)))) (defn- update-file-data - [file changes] - (-> file - (update :revn inc) - (update :data (fn [data] - (cond-> data - :always - (-> (blob/decode) - (assoc :id (:id file)) - (pmg/migrate-data)) + [file libraries changes skip-validate] + (let [validate (fn [file] + (when (and (cf/flags :file-validation) + (not skip-validate)) + (val/validate-file file libraries :throw? true)))] + (-> file + (update :revn inc) + (update :data (fn [data] + (cond-> data + :always + (-> (blob/decode) + (assoc :id (:id file)) + (pmg/migrate-data)) - (and (contains? ffeat/*current* "components/v2") - (not (contains? ffeat/*previous* "components/v2"))) - (ctf/migrate-to-components-v2) + (and (contains? ffeat/*current* "components/v2") + (not (contains? ffeat/*previous* "components/v2"))) + (ctf/migrate-to-components-v2) - :always - (-> (cp/process-changes changes) - (blob/encode))))))) + :always + (cp/process-changes changes)))) + (d/tap-r validate) + (update :data blob/encode)))) (defn- take-snapshot? "Defines the rule when file `data` snapshot should be saved." diff --git a/backend/test/backend_tests/helpers.clj b/backend/test/backend_tests/helpers.clj index fd067aedb..20c7941a9 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/backend/test/backend_tests/helpers.clj @@ -67,7 +67,8 @@ :enable-smtp :enable-quotes :enable-fdata-storage-pointer-map - :enable-fdata-storage-objets-map]) + :enable-fdata-storage-objets-map + :disable-file-validation]) (def test-init-sql ["alter table project_profile_rel set unlogged;\n" diff --git a/frontend/src/debug.cljs b/frontend/src/debug.cljs index ea8e8a7e1..34ce6c80b 100644 --- a/frontend/src/debug.cljs +++ b/frontend/src/debug.cljs @@ -412,7 +412,8 @@ :revn (:revn file) :session-id sid :changes changes - :features features}] + :features features + :skip-validate true}] (->> (rp/cmd! :update-file params) (rx/tap #(dom/reload-current-window)))))))))