diff --git a/backend/src/app/rpc/commands/files_update.clj b/backend/src/app/rpc/commands/files_update.clj index 85a0a58b1f..a9563934a6 100644 --- a/backend/src/app/rpc/commands/files_update.clj +++ b/backend/src/app/rpc/commands/files_update.clj @@ -434,6 +434,32 @@ (l/error :hint "file validation error" :cause cause)))) +(defn- get-added-page-ids + "Ids of the pages that `:add-page` changes carry whole under `:page`. + + That form has no top-level `:id`, so `extract-affected-ids` cannot see the + page and `validate-file-affected` does not check it. Pages whose `:id` is + not a uuid are skipped: `process-changes` accepts them and there is nothing + to look up in the resulting file." + [changes] + (into [] + (comp (keep (fn [{:keys [type id page]}] + (when (and (= type :add-page) + (nil? id) + (uuid? (:id page))) + (:id page)))) + (distinct)) + changes)) + +(defn- validate-added-pages + "Referential check of the pages added through the `{:page ...}` form of + `:add-page`. Returns the errors, or nil when no such page was added." + [file libs changes] + (when-let [page-ids (seq (get-added-page-ids changes))] + (val/validate-file-affected file libs + (mapv (fn [id] {:type :add-page :id id}) + page-ids)))) + (defn- process-changes-and-validate [cfg file changes skip-validate] (let [should-resolve-libs? @@ -481,6 +507,21 @@ (record-validation-timing cfg :process-changes-ms "process-changes" (inst-ms (tproc)))))] (binding [pmap/*tracked* nil] + ;; Pages added through the `{:page ...}` form have no top-level `:id`, + ;; so the affected check below does not see them. Check them here in + ;; soft mode until the hard check learns to collect them. + (when should-resolve-libs? + (try + (when-let [errors (validate-added-pages file libs changes)] + (l/error :hint "added page validation error" + :file-id (str (:id file)) + :page-ids (->> errors (map :page-id) distinct (mapv str)) + :codes (->> errors (map :code) distinct (mapv name)))) + (catch Throwable cause + (l/error :hint "added page validation error" + :file-id (str (:id file)) + :cause cause)))) + (when (contains? cf/flags :soft-file-validation) (let [t (ct/tpoint)] (try diff --git a/backend/test/backend_tests/files_update_add_page_validation_test.clj b/backend/test/backend_tests/files_update_add_page_validation_test.clj new file mode 100644 index 0000000000..e6b0e94871 --- /dev/null +++ b/backend/test/backend_tests/files_update_add_page_validation_test.clj @@ -0,0 +1,136 @@ +;; 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 INC Sucursal en España SL + +(ns backend-tests.files-update-add-page-validation-test + (:require + [app.common.features :as cfeat] + [app.common.files.validate :as val] + [app.common.types.page :as ctp] + [app.common.types.shape :as cts] + [app.common.uuid :as uuid] + [app.config :as cf] + [app.rpc :as-alias rpc] + [app.rpc.commands.files-update :as files-update] + [backend-tests.helpers :as th] + [clojure.test :as t])) + +(t/use-fixtures :once th/state-init) +(t/use-fixtures :each th/database-reset) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; Helpers +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(defn- page-with-dangling-parent + "A page whose only shape points at a parent that is not in the page, and + that the page tree cannot reach from the root. It passes the file schema + (geometry is valid) but fails the referential check." + [page-id] + (let [shape (cts/setup-shape + {:id (uuid/next) + :name "rect" + :type :rect + :parent-id (uuid/next) + :frame-id uuid/zero})] + (-> (ctp/make-empty-page {:id page-id :name "Page 2"}) + (assoc-in [:objects (:id shape)] shape)))) + +(defn- file-with + [page] + {:id (uuid/next) + :features #{"components/v2"} + :data {:pages [(:id page)] + :pages-index {(:id page) page}}}) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; Collection of the pages the affected check does not see +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(t/deftest get-added-page-ids-collects-only-whole-page-adds + (let [page-id (uuid/next)] + (t/is (= [page-id] + (@#'files-update/get-added-page-ids + [{:type :add-page :page {:id page-id}}]))) + + (t/testing "an editor-form add-page is left to the affected check" + (t/is (empty? (@#'files-update/get-added-page-ids + [{:type :add-page :id page-id :name "Page 2"}])))) + + (t/testing "pages without a uuid id are ignored" + (t/is (empty? (@#'files-update/get-added-page-ids + [{:type :add-page :page {:id "not-a-uuid"}}])))))) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; Soft validation of the collected pages +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(t/deftest validate-added-pages-reports-a-dangling-parent + (let [page-id (uuid/next) + page (page-with-dangling-parent page-id) + errors (@#'files-update/validate-added-pages + (file-with page) + {} + [{:type :add-page :page page}])] + (t/is (seq errors)) + (t/is (contains? (set (map :code errors)) :parent-not-found)) + (t/is (= #{page-id} (set (map :page-id errors)))))) + +(t/deftest validate-added-pages-skips-editor-form-adds + (let [page-id (uuid/next) + page (page-with-dangling-parent page-id)] + (t/is (nil? (@#'files-update/validate-added-pages + (file-with page) + {} + [{:type :add-page :id page-id :name "Page 2"}]))))) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; update-file accepts the page and reports the error +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(t/deftest update-file-accepts-page-added-with-a-whole-page-value + (let [profile (th/create-profile* 1 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)}) + page-id (uuid/next) + page (page-with-dangling-parent page-id) + calls (atom []) + validate val/validate-file-affected] + + (with-redefs [cf/flags (conj cf/flags :file-validation) + val/validate-file-affected + (fn [file libs changes] + (let [errors (validate file libs changes)] + (swap! calls conj [changes errors]) + errors))] + + (let [out (th/command! {::th/type :update-file + ::rpc/profile-id (:id profile) + :id (:id file) + :session-id (uuid/next) + :revn 0 + :vern 0 + :features cfeat/supported-features + :changes [{:type :add-page :page page}]})] + + (t/testing "the save is accepted" + (t/is (nil? (:error out)))) + + (t/testing "the page is stored" + (let [out (th/command! {::th/type :get-file + ::rpc/profile-id (:id profile) + :id (:id file)})] + (t/is (nil? (:error out))) + (t/is (some? (get-in out [:result :data :pages-index page-id]))))) + + (t/testing "the added page is checked in soft mode" + (let [[changes errors] + (some (fn [[changes :as entry]] + (when (= [{:type :add-page :id page-id}] changes) + entry)) + @calls)] + (t/is (some? changes)) + (t/is (contains? (set (map :code errors)) :parent-not-found)) + (t/is (= #{page-id} (set (map :page-id errors))))))))))