github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-10-28 21:40:34 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Check pages added with a full page value in soft mode

Browse Source 
An :add-page change can carry a whole page under :page instead of an
id and a name. extract-affected-ids reads the page to check from the
change's top-level :id, so that form left the new page unchecked and
a referential error in it was stored.

Collect those page ids after the changes are applied and run the
affected check on them in soft mode: log the file id, the page ids
and the error codes, and never refuse the save. This is the first
step of the two-step rollout. The follow-up moves the page id into
extract-affected-ids and makes the check hard. The counters of
#12665 decide when to flip it.

Relates to #12666

AI-assisted-by: claude-opus-5-5, deepseek-v4.1-flash
Signed-off-by: Álvaro Tejero Cantero <alvorithm@teje.ro>
This commit is contained in:
Álvaro Tejero Cantero 2026-10-28 08:33:52 +01:00
parent f6f9df9a1a
commit 5638a0f105
No known key found for this signature in database
2 changed files with 177 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
backend/src/app/rpc/commands/files_update.clj
View File

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

136
backend/test/backend_tests/files_update_add_page_validation_test.clj Normal file
View File

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