🐛 Fix several issues related to pointer-map and components-v2

backend/src/app/rpc/commands/files.clj

@@ -201,15 +201,15 @@
                      ::db/check-deleted? false})]
     (blob/decode (:content row))))
 
-(defn load-all-pointers!
+(defn- load-all-pointers!
-  [data]
+  [{:keys [data] :as file}]
   (doseq [[_id page] (:pages-index data)]
     (when (pmap/pointer-map? page)
       (pmap/load! page)))
   (doseq [[_id component] (:components data)]
     (when (pmap/pointer-map? component)
       (pmap/load! component)))
-  data)
+  file)
 
 (defn persist-pointers!
   [conn file-id]
@@ -247,22 +247,44 @@
 ;; QUERY COMMANDS
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
-(defn handle-file-features
+(defn handle-file-features!
-  [{:keys [features] :as file} client-features]
+  [conn {:keys [id features data] :as file} client-features]
+
   (when (and (contains? features "components/v2")
              (not (contains? client-features "components/v2")))
     (ex/raise :type :restriction
               :code :feature-mismatch
               :feature "components/v2"
               :hint "file has 'components/v2' feature enabled but frontend didn't specifies it"))
-  (cond-> file
-    (and (contains? client-features "components/v2")
-         (not (contains? features "components/v2")))
-    (update :data ctf/migrate-to-components-v2)
 
-    (and (contains? features "storage/pointer-map")
+  ;; NOTE: this operation is needed because the components migration
-         (not (contains? client-features "storage/pointer-map")))
+  ;; generates a new page with random id which is returned to the
-    (process-pointers deref)))
+  ;; client; without persisting the migration this can cause that two
+  ;; simultaneous clients can have a different view of the file data
+  ;; and end persisting two pages with main components and breaking
+  ;; the whole file
+  (let [file (if (and (contains? client-features "components/v2")
+                      (not (contains? features "components/v2")))
+               (binding [pmap/*tracked* (atom {})]
+                 (let [data        (ctf/migrate-to-components-v2 data)
+                       features    (conj features "components/v2")
+                       modified-at (dt/now)]
+                   (db/update! conn :file
+                               {:data (blob/encode data)
+                                :modified-at modified-at
+                                :features features}
+                               {:id id})
+                   (persist-pointers! conn id)
+                   (-> file
+                       (assoc :modified-at modified-at)
+                       (assoc :features features)
+                       (assoc :data data))))
+               file)]
+
+    (cond-> file
+      (and (contains? features "storage/pointer-map")
+           (not (contains? client-features "storage/pointer-map")))
+      (process-pointers deref))))
 
 ;; --- COMMAND QUERY: get-file (by id)
 
@@ -306,10 +328,18 @@
   ;; here we check if client requested features are supported
   (check-features-compatibility! client-features)
   (binding [pmap/*load-fn* (partial load-pointer conn id)]
-    (-> (db/get-by-id conn :file id)
+    (let [file  (-> (db/get-by-id conn :file id)
-        (decode-row)
+                    (decode-row)
-        (pmg/migrate-file)
+                    (pmg/migrate-file))
-        (handle-file-features client-features))))
+
+          file  (handle-file-features! conn file client-features)]
+
+      ;; NOTE: if migrations are applied, probably new pointers generated so
+      ;; instead of persiting them on each get-file, we just resolve them until
+      ;; user updates the file and permanently persists the new pointers
+      (cond-> file
+        (pmg/migrated? file)
+        (process-pointers deref)))))
 
 (defn get-minimal-file
   [{:keys [::db/pool] :as cfg} id]
@@ -543,7 +573,7 @@
 
 ;; --- COMMAND QUERY: get-file-libraries
 
-(def ^:private sql:file-libraries
+(def ^:private sql:get-file-libraries
   "WITH RECURSIVE libs AS (
      SELECT fl.*, flr.synced_at
        FROM file AS fl
@@ -556,7 +586,6 @@
        JOIN libs AS l ON (flr.file_id = l.id)
    )
    SELECT l.id,
-          l.data,
           l.features,
           l.project_id,
           l.created_at,
@@ -569,33 +598,24 @@
     WHERE l.deleted_at IS NULL OR l.deleted_at > now();")
 
 (defn get-file-libraries
-  [conn file-id client-features]
+  [conn file-id]
-  (check-features-compatibility! client-features)
+  (into []
-  (->> (db/exec! conn [sql:file-libraries file-id])
+        (comp
-       (map decode-row)
+         (map #(assoc % :is-indirect false))
-       (map #(assoc % :is-indirect false))
+         (map decode-row))
-       (map (fn [{:keys [id] :as row}]
+        (db/exec! conn [sql:get-file-libraries file-id])))
-              (binding [pmap/*load-fn* (partial load-pointer conn id)]
-                (-> row
-                    ;; TODO: re-enable this dissoc and replace call
-                    ;;       with other that gets files individually
-                    ;;       See task https://tree.taiga.io/project/penpot/task/4904
-                    ;; (update :data dissoc :pages-index)
-                    (handle-file-features client-features)))))
-       (vec)))
 
 (s/def ::get-file-libraries
   (s/keys :req [::rpc/profile-id]
-          :req-un [::file-id]
+          :req-un [::file-id]))
-          :opt-un [::features]))
 
 (sv/defmethod ::get-file-libraries
   "Get libraries used by the specified file."
   {::doc/added "1.17"}
-  [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id file-id features]}]
+  [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id file-id]}]
   (dm/with-open [conn (db/open pool)]
     (check-read-permissions! conn profile-id file-id)
-    (get-file-libraries conn file-id features)))
+    (get-file-libraries conn file-id)))
 
 
 ;; --- COMMAND QUERY: Files that use this File library
@@ -719,31 +739,38 @@
               {:is-shared is-shared}
               {:id id}))
 
+(def sql:get-referenced-files
+  "SELECT f.id
+     FROM file_library_rel AS flr
+    INNER JOIN file AS f ON (f.id = flr.file_id)
+    WHERE flr.library_file_id = ?
+    ORDER BY f.created_at ASC;")
+
 (defn absorb-library
   "Find all files using a shared library, and absorb all library assets
   into the file local libraries"
   [conn {:keys [id] :as params}]
   (let [library (db/get-by-id conn :file id)]
     (when (:is-shared library)
-      (let [ldata (-> library decode-row pmg/migrate-file :data)]
+      (let [ldata (binding [pmap/*load-fn* (partial load-pointer conn id)]
-        (binding [pmap/*load-fn* (partial load-pointer conn id)]
+                    (-> library decode-row load-all-pointers! pmg/migrate-file :data))
-          (load-all-pointers! ldata))
+            rows  (db/exec! conn [sql:get-referenced-files id])]
-
+        (doseq [file-id (map :id rows)]
-        (->> (db/query conn :file-library-rel {:library-file-id id})
+          (binding [pmap/*load-fn* (partial load-pointer conn file-id)
-             (map :file-id)
+                    pmap/*tracked* (atom {})]
-             (keep #(db/get-by-id conn :file % ::db/check-deleted? false))
+            (let [file (-> (db/get-by-id conn :file file-id
-             (map decode-row)
+                                         ::db/check-deleted? false
-             (map pmg/migrate-file)
+                                         ::db/remove-deleted? false)
-             (run! (fn [{:keys [id data revn] :as file}]
+                           (decode-row)
-                     (binding [pmap/*tracked* (atom {})
+                           (load-all-pointers!)
-                               pmap/*load-fn* (partial load-pointer conn id)]
+                           (pmg/migrate-file))
-                       (let [data (ctf/absorb-assets data ldata)]
+                  data (ctf/absorb-assets (:data file) ldata)]
-                         (db/update! conn :file
+              (db/update! conn :file
-                                     {:revn (inc revn)
+                          {:revn (inc (:revn file))
-                                      :data (blob/encode data)
+                           :data (blob/encode data)
-                                      :modified-at (dt/now)}
+                           :modified-at (dt/now)}
-                                     {:id id}))
+                          {:id file-id})
-                       (persist-pointers! conn id)))))))))
+              (persist-pointers! conn file-id))))))))
 
 (s/def ::set-file-shared
   (s/keys :req [::rpc/profile-id]

backend/src/app/rpc/commands/viewer.clj

@@ -27,9 +27,8 @@
   [conn file-id profile-id features]
   (let [file    (files/get-file conn file-id features)
         project (get-project conn (:project-id file))
-        libs    (files/get-file-libraries conn file-id features)
+        libs    (files/get-file-libraries conn file-id)
         users   (comments/get-file-comments-users conn file-id profile-id)
-
         links   (->> (db/query conn :share-link {:file-id file-id})
                      (mapv (fn [row]
                              (-> row

common/src/app/common/pages/migrations.cljc

@@ -37,10 +37,16 @@
        (reduce migrate-fn data (range (:version data 0) to-version))))))
 
 (defn migrate-file
-  [file]
+  [{:keys [id data] :as file}]
-  (-> file
+  (let [data (assoc data :id id)]
-      (update :data assoc :id (:id file))
+    (-> file
-      (update :data migrate-data)))
+        (assoc ::orig-version (:version data))
+        (assoc :data (migrate-data data)))))
+
+(defn migrated?
+  [{:keys [data] :as file}]
+  (> (:version data)
+     (::orig-version file)))
 
 ;; Default handler, noop
 (defmethod migrate :default [data] data)

common/src/app/common/types/components_list.cljc

@@ -30,25 +30,19 @@
   (assoc component :modified-at (dt/now)))
 
 (defn add-component
-  [file-data {:keys [id name path main-instance-id main-instance-page shapes]}]
+  [fdata {:keys [id name path main-instance-id main-instance-page shapes]}]
-  (let [components-v2  (dm/get-in file-data [:options :components-v2])
+  (let [components-v2  (dm/get-in fdata [:options :components-v2])
-        wrap-object-fn feat/*wrap-with-objects-map-fn*]
+        fdata          (update fdata :components assoc id (touch {:id id :name name :path path}))]
-    (cond-> file-data
+    (if components-v2
-      :always
+      (update-in fdata [:components id] assoc
-      (assoc-in [:components id]
-                (touch {:id id
-                        :name name
-                        :path path}))
-
-      (not components-v2)
-      (assoc-in [:components id :objects]
-                (->> shapes
-                     (d/index-by :id)
-                     (wrap-object-fn)))
-      components-v2
-      (update-in [:components id] assoc
                  :main-instance-id main-instance-id
-                 :main-instance-page main-instance-page))))
+                 :main-instance-page main-instance-page)
+
+      (let [wrap-object-fn feat/*wrap-with-objects-map-fn*]
+        (assoc-in fdata [:components id :objects]
+                  (->> shapes
+                       (d/index-by :id)
+                       (wrap-object-fn)))))))
 
 (defn mod-component
   [file-data {:keys [id name path objects annotation]}]

common/src/app/common/types/container.cljc

@@ -173,7 +173,8 @@
    (make-component-instance container component library-data position components-v2 {}))
 
   ([container component library-data position components-v2
-    {:keys [main-instance? force-id] :or {main-instance? false force-id nil}}]
+    {:keys [main-instance? force-id force-frame-id]
+     :or {main-instance? false force-id nil force-frame-id nil}}]
    (let [component-page  (when components-v2
                            (ctpl/get-page library-data (:main-instance-page component)))
          component-shape (if components-v2
@@ -188,10 +189,11 @@
          objects         (:objects container)
          unames          (volatile! (common/retrieve-used-names objects))
 
-         frame-id        (ctst/frame-id-by-position objects
+         frame-id        (or force-frame-id
-                                                    (gpt/add orig-pos delta)
+                             (ctst/frame-id-by-position objects
-                                                    {:skip-components? true}) ; It'd be weird to make an instance
+                                                        (gpt/add orig-pos delta)
-         frame-ids-map   (volatile! {})                                       ; inside other component
+                                                        {:skip-components? true}))
+         frame-ids-map   (volatile! {})
 
          update-new-shape
          (fn [new-shape original-shape]

common/src/app/common/types/file.cljc

@@ -345,7 +345,8 @@
                                                  file-data
                                                  position
                                                  false
-                                                 {:main-instance? true})
+                                                 {:main-instance? true
+                                                  :force-frame-id uuid/zero})
 
                     add-shapes
                     (fn [page]

frontend/src/app/main/data/workspace.cljs

@@ -225,14 +225,17 @@
                                (resolve-pointers id)
                                (rx/map workspace-data-pointers-loaded))
 
-                          (->> (rp/cmd! :get-file-libraries {:file-id id :features features})
+                          (->> (rp/cmd! :get-file-libraries {:file-id id})
                                (rx/mapcat identity)
-                               (rx/mapcat
+                               (rx/merge-map
+                                (fn [{:keys [id]}]
+                                  (rp/cmd! :get-file {:id id :features features})))
+                               (rx/merge-map
                                 (fn [{:keys [id data] :as file}]
                                   (->> (filter (comp t/pointer? val) data)
                                        (resolve-pointers id)
                                        (rx/map #(update file :data merge %)))))
-                               (rx/mapcat
+                               (rx/merge-map
                                 (fn [{:keys [id data] :as file}]
                                   ;; Resolve all pages of each library, if needed
                                   (->> (rx/from (seq (:pages-index data)))