Merge pull request #3672 from penpot/hiru-automatic-validation

🎉 Do file validation on each file change

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"

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
 

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 libraries changes skip-validate]
-  (-> file
+  (let [validate (fn [file]
-      (update :revn inc)
+                   (when (and (cf/flags :file-validation)
-      (update :data (fn [data]
+                              (not skip-validate))
-                      (cond-> data
+                     (val/validate-file file libraries :throw? true)))]
-                        :always
+    (-> file
-                        (-> (blob/decode)
+        (update :revn inc)
-                            (assoc :id (:id file))
+        (update :data (fn [data]
-                            (pmg/migrate-data))
+                        (cond-> data
+                          :always
+                          (-> (blob/decode)
+                              (assoc :id (:id file))
+                              (pmg/migrate-data))
 
-                        (and (contains? ffeat/*current* "components/v2")
+                          (and (contains? ffeat/*current* "components/v2")
-                             (not (contains? ffeat/*previous* "components/v2")))
+                               (not (contains? ffeat/*previous* "components/v2")))
-                        (ctf/migrate-to-components-v2)
+                          (ctf/migrate-to-components-v2)
 
-                        :always
+                          :always
-                        (-> (cp/process-changes changes)
+                          (cp/process-changes changes))))
-                            (blob/encode)))))))
+        (d/tap-r validate)
+        (update :data blob/encode))))
 
 (defn- take-snapshot?
   "Defines the rule when file `data` snapshot should be saved."

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"

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)))))))))