♻️ Change how shapes are validated after changes apply operation

CHANGES.md

@@ -64,6 +64,7 @@ penpot on-premise you will need to apply the same changes on your own
 - Add toggle for switching boolean property values [Taiga #12341](https://tree.taiga.io/project/penpot/us/12341)
 - Make the file export process more reliable [Taiga #12555](https://tree.taiga.io/project/penpot/us/12555)
 - Add auth flow changes [Taiga #12333](https://tree.taiga.io/project/penpot/us/12333)
+- Add new shape validation mechanism for shapes [Github #7696](https://github.com/penpot/penpot/pull/7696)
 
 ### :bug: Bugs fixed
 

common/src/app/common/files/changes.cljc

@@ -463,35 +463,16 @@
 
 ;; Changes Processing Impl
 
-(defn validate-shapes!
+#_:clj-kondo/ignore
-  [data-old data-new items]
+(defn- validate-shape
-  (letfn [(validate-shape! [[page-id id]]
+  [{:keys [id] :as shape} page-id]
-            (let [shape-old (dm/get-in data-old [:pages-index page-id :objects id])
+  (when-not (cts/valid-shape? shape)
-                  shape-new (dm/get-in data-new [:pages-index page-id :objects id])]
+    (ex/raise :type :assertion
-
+              :code :data-validation
-              ;; If object has changed or is new verify is correct
+              :hint (str "invalid shape found '" id "'")
-              (when (and (some? shape-new)
+              :page-id page-id
-                         (not= shape-old shape-new))
+              :shape-id id
-                (when-not (and (cts/valid-shape? shape-new)
+              ::sm/explain (cts/explain-shape shape))))
-                               (cts/shape? shape-new))
-                  (ex/raise :type :assertion
-                            :code :data-validation
-                            :hint (str "invalid shape found after applying changes on file "
-                                       (:id data-new))
-                            :file-id (:id data-new)
-                            ::sm/explain (cts/explain-shape shape-new))))))]
-
-    (->> (into #{} (map :page-id) items)
-         (mapcat (fn [page-id]
-                   (filter #(= page-id (:page-id %)) items)))
-         (mapcat (fn [{:keys [type id page-id] :as item}]
-                   (sequence
-                    (map (partial vector page-id))
-                    (case type
-                      (:add-obj :mod-obj :del-obj) (cons id nil)
-                      (:mov-objects :reg-objects)  (:shapes item)
-                      nil))))
-         (run! validate-shape!))))
 
 (defn- process-touched-change
   [data {:keys [id page-id component-id]}]
@@ -518,14 +499,8 @@
      (check-changes items))
 
    (binding [*touched-changes* (volatile! #{})]
-     (let [result (reduce #(or (process-change %1 %2) %1) data items)
+     (let [result (reduce #(or (process-change %1 %2) %1) data items)]
-           result (reduce process-touched-change result @*touched-changes*)]
+       (reduce process-touched-change result @*touched-changes*)))))
-       ;; Validate result shapes (only on the backend)
-       ;;
-       ;; TODO: (PERF) add changed shapes tracking and only validate
-       ;; the tracked changes instead of iterate over all shapes
-       #?(:clj (validate-shapes! data result items))
-       result))))
 
 ;; --- Comment Threads
 
@@ -613,9 +588,10 @@
 
 (defmethod process-change :add-obj
   [data {:keys [id obj page-id component-id frame-id parent-id index ignore-touched]}]
-  (let [update-container
+  ;; NOTE: we only perform hard validation on backend
-        (fn [container]
+  #?(:clj (validate-shape obj page-id))
-          (ctst/add-shape id obj container frame-id parent-id index ignore-touched))]
+
+  (let [update-container #(ctst/add-shape id obj % frame-id parent-id index ignore-touched)]
 
     (when *state*
       (swap! *state* collect-shape-media-refs obj page-id))
@@ -638,6 +614,9 @@
       (when (and *state* page-id)
         (swap! *state* collect-shape-media-refs shape page-id))
 
+      ;; NOTE: we only perform hard validation on backend
+      #?(:clj (validate-shape shape page-id))
+
       (assoc objects id shape))
 
     objects))
@@ -720,22 +699,24 @@
 
           (update-group [group objects]
             (let [lookup   (d/getf objects)
-                  children (get group :shapes)]
+                  children (get group :shapes)
-              (cond
+                  group    (cond
-                ;; If the group is empty we don't make any changes. Will be removed by a later process
+                             ;; If the group is empty we don't make any changes. Will be removed by a later process
-                (empty? children)
+                             (empty? children)
-                group
+                             group
 
-                (= :bool (:type group))
+                             (= :bool (:type group))
-                (path/update-bool-shape group objects)
+                             (path/update-bool-shape group objects)
 
-                (:masked-group group)
+                             (:masked-group group)
-                (->> (map lookup children)
+                             (->> (map lookup children)
-                     (set-mask-selrect group))
+                                  (set-mask-selrect group))
 
-                :else
+                             :else
-                (->> (map lookup children)
+                             (->> (map lookup children)
-                     (gsh/update-group-selrect group)))))]
+                                  (gsh/update-group-selrect group)))]
+              #?(:clj (validate-shape group page-id))
+              group))]
 
     (if page-id
       (d/update-in-when data [:pages-index page-id :objects] reg-objects)
@@ -817,6 +798,11 @@
                 (not (cfh/frame-shape? obj))
                 (as-> $$ (reduce (partial update-frame-id frame-id) $$ (:shapes obj))))))
 
+          (validate-shape [objects #_:clj-kondo/ignore shape-id]
+            #?(:clj (when-let [shape (get objects shape-id)]
+                      (validate-shape shape page-id)))
+            objects)
+
           (move-objects [objects]
             (let [parent (get objects parent-id)]
               ;; Do not proceed with the move if parent does not
@@ -841,7 +827,10 @@
 
                     ;; Ensure that all shapes of the new parent has a
                     ;; correct link to the topside frame.
-                    (reduce (partial update-frame-id frame-id) $ shapes)))
+                    (reduce (partial update-frame-id frame-id) $ shapes)
+
+                    ;; Perform validation of the affected shapes
+                    (reduce validate-shape $ shapes)))
 
                 objects)))]