From 15d09eb0d4a2d750e3905b058c428e98f70f0f48 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 24 Feb 2025 11:05:16 +0100 Subject: [PATCH] :bug: Fix incorrect data id assignation on creating a snapshot (#5934) * :paperclip: Set proper name to relink-refs mechanism function * :bug: Fix incorrect id assignation on snapshot file resolution * :recycle: Use uniform api for file retrieval on file snapshot code --- backend/src/app/binfile/common.clj | 35 +----- backend/src/app/binfile/migrations.clj | 45 ++++++++ backend/src/app/binfile/v1.clj | 3 +- backend/src/app/binfile/v3.clj | 5 +- .../src/app/rpc/commands/files_snapshot.clj | 104 +++++++++--------- backend/src/app/srepl/helpers.clj | 15 ++- .../backend_tests/rpc_file_snapshot_test.clj | 1 + common/src/app/common/files/helpers.cljc | 9 +- 8 files changed, 120 insertions(+), 97 deletions(-) create mode 100644 backend/src/app/binfile/migrations.clj diff --git a/backend/src/app/binfile/common.clj b/backend/src/app/binfile/common.clj index 0db55f664..95fae4dfe 100644 --- a/backend/src/app/binfile/common.clj +++ b/backend/src/app/binfile/common.clj @@ -20,7 +20,6 @@ [app.config :as cf] [app.db :as db] [app.db.sql :as sql] - [app.features.components-v2 :as feat.compv2] [app.features.fdata :as feat.fdata] [app.features.file-migrations :as feat.fmigr] [app.loggers.audit :as-alias audit] @@ -307,7 +306,7 @@ update-shapes (fn [data {:keys [page-id shape-id]}] - (d/update-in-when data [:pages-index page-id :objects shape-id] cfh/relink-media-refs lookup-index)) + (d/update-in-when data [:pages-index page-id :objects shape-id] cfh/relink-refs lookup-index)) file (update file :data #(reduce update-shapes % media-refs))] @@ -375,7 +374,7 @@ replace the old :component-file reference with the new ones, using the provided file-index." [data] - (cfh/relink-media-refs data lookup-index)) + (cfh/relink-refs data lookup-index)) (defn- relink-media "A function responsible of process the :media attr of file data and @@ -523,33 +522,3 @@ (l/error :hint "file schema validation error" :cause result)))) (insert-file! cfg file opts))) - -(defn register-pending-migrations! - "All features that are enabled and requires explicit migration are - added to the state for a posterior migration step." - [cfg {:keys [id features] :as file}] - (doseq [feature (-> (::features cfg) - (set/difference cfeat/no-migration-features) - (set/difference cfeat/backend-only-features) - (set/difference features))] - (vswap! *state* update :pending-to-migrate (fnil conj []) [feature id])) - - file) - -(defn apply-pending-migrations! - "Apply alredy registered pending migrations to files" - [cfg] - (doseq [[feature file-id] (-> *state* deref :pending-to-migrate)] - (case feature - "components/v2" - (feat.compv2/migrate-file! cfg file-id - :validate? (::validate cfg true) - :skip-on-graphic-error? true) - - "fdata/shape-data-type" - nil - - (ex/raise :type :internal - :code :no-migration-defined - :hint (str/ffmt "no migation for feature '%' on file importation" feature) - :feature feature)))) diff --git a/backend/src/app/binfile/migrations.clj b/backend/src/app/binfile/migrations.clj new file mode 100644 index 000000000..fb860c85b --- /dev/null +++ b/backend/src/app/binfile/migrations.clj @@ -0,0 +1,45 @@ +;; 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 + +(ns app.binfile.migrations + "A binfile related migrations handling" + (:require + [app.binfile.common :as bfc] + [app.common.exceptions :as ex] + [app.common.features :as cfeat] + [app.features.components-v2 :as feat.compv2] + [clojure.set :as set] + [cuerdas.core :as str])) + +(defn register-pending-migrations! + "All features that are enabled and requires explicit migration are + added to the state for a posterior migration step." + [cfg {:keys [id features] :as file}] + (doseq [feature (-> (::features cfg) + (set/difference cfeat/no-migration-features) + (set/difference cfeat/backend-only-features) + (set/difference features))] + (vswap! bfc/*state* update :pending-to-migrate (fnil conj []) [feature id])) + + file) + +(defn apply-pending-migrations! + "Apply alredy registered pending migrations to files" + [cfg] + (doseq [[feature file-id] (-> bfc/*state* deref :pending-to-migrate)] + (case feature + "components/v2" + (feat.compv2/migrate-file! cfg file-id + :validate? (::validate cfg true) + :skip-on-graphic-error? true) + + "fdata/shape-data-type" + nil + + (ex/raise :type :internal + :code :no-migration-defined + :hint (str/ffmt "no migation for feature '%' on file importation" feature) + :feature feature)))) diff --git a/backend/src/app/binfile/v1.clj b/backend/src/app/binfile/v1.clj index 0f94df969..2f70ed50d 100644 --- a/backend/src/app/binfile/v1.clj +++ b/backend/src/app/binfile/v1.clj @@ -9,6 +9,7 @@ (:refer-clojure :exclude [assert]) (:require [app.binfile.common :as bfc] + [app.binfile.migrations :as bfm] [app.common.data :as d] [app.common.data.macros :as dm] [app.common.exceptions :as ex] @@ -473,7 +474,7 @@ (read-section options)))) [:v1/metadata :v1/files :v1/rels :v1/sobjects]) - (bfc/apply-pending-migrations! cfg) + (bfm/apply-pending-migrations! cfg) ;; Knowing that the ids of the created files are in index, ;; just lookup them and return it as a set diff --git a/backend/src/app/binfile/v3.clj b/backend/src/app/binfile/v3.clj index dcda35455..dc6bf7b80 100644 --- a/backend/src/app/binfile/v3.clj +++ b/backend/src/app/binfile/v3.clj @@ -9,6 +9,7 @@ (:refer-clojure :exclude [read]) (:require [app.binfile.common :as bfc] + [app.binfile.migrations :as bfm] [app.common.data :as d] [app.common.data.macros :as dm] [app.common.exceptions :as ex] @@ -735,7 +736,7 @@ (bfc/process-file))] - (bfc/register-pending-migrations! cfg file) + (bfm/register-pending-migrations! cfg file) (bfc/save-file! cfg file ::db/return-keys false) file-id'))) @@ -915,7 +916,7 @@ (import-file-media cfg) (import-file-thumbnails cfg) - (bfc/apply-pending-migrations! cfg) + (bfm/apply-pending-migrations! cfg) ids))))))) diff --git a/backend/src/app/rpc/commands/files_snapshot.clj b/backend/src/app/rpc/commands/files_snapshot.clj index 43e3f1c95..71689560a 100644 --- a/backend/src/app/rpc/commands/files_snapshot.clj +++ b/backend/src/app/rpc/commands/files_snapshot.clj @@ -6,6 +6,7 @@ (ns app.rpc.commands.files-snapshot (:require + [app.binfile.common :as bfc] [app.common.exceptions :as ex] [app.common.logging :as l] [app.common.schema :as sm] @@ -22,7 +23,6 @@ [app.rpc.quotes :as quotes] [app.storage :as sto] [app.util.blob :as blob] - [app.util.pointer-map :as pmap] [app.util.services :as sv] [app.util.time :as dt] [cuerdas.core :as str])) @@ -58,26 +58,6 @@ (files/check-read-permissions! conn profile-id file-id) (get-file-snapshots conn file-id)))) -(def ^:private sql:get-file - "SELECT f.*, - p.id AS project_id, - p.team_id AS team_id - FROM file AS f - INNER JOIN project AS p ON (p.id = f.project_id) - WHERE f.id = ?") - -(defn- get-file - [cfg file-id] - (let [file (->> (db/exec-one! cfg [sql:get-file file-id]) - (feat.fdata/resolve-file-data cfg))] - (binding [pmap/*load-fn* (partial feat.fdata/load-pointer cfg file-id)] - (-> file - (update :data blob/decode) - (update :data feat.fdata/process-pointers deref) - (update :data feat.fdata/process-objects (partial into {})) - (update :data assoc ::id file-id) - (update :data blob/encode))))) - (defn- generate-snapshot-label [] (let [ts (-> (dt/now) @@ -87,49 +67,53 @@ (str "snapshot-" ts))) (defn create-file-snapshot! - [cfg profile-id file-id label] - (let [file (get-file cfg file-id) + [cfg file & {:keys [label created-by deleted-at profile-id] + :or {deleted-at :default + created-by :system}}] + + (assert (#{:system :user :admin} created-by) + "expected valid keyword for created-by") + + (let [conn + (db/get-connection cfg) - ;; NOTE: final user never can provide label as `:system` - ;; keyword because the validator implies label always as - ;; string; keyword is used for signal a special case created-by - (if (= label :system) - "system" - "user") + (name created-by) deleted-at - (if (= label :system) + (cond + (= deleted-at :default) (dt/plus (dt/now) (cf/get-deletion-delay)) + + (dt/instant? deleted-at) + deleted-at + + :else nil) label - (if (= label :system) - (str "internal/snapshot/" (:revn file)) - (or label (generate-snapshot-label))) + (or label (generate-snapshot-label)) snapshot-id - (uuid/next)] + (uuid/next) - (-> cfg - (assoc ::quotes/profile-id profile-id) - (assoc ::quotes/project-id (:project-id file)) - (assoc ::quotes/team-id (:team-id file)) - (assoc ::quotes/file-id (:id file)) - (quotes/check! {::quotes/id ::quotes/snapshots-per-file} - {::quotes/id ::quotes/snapshots-per-team})) + data + (blob/encode (:data file)) + + features + (db/encode-pgarray (:features file) conn "text")] (l/debug :hint "creating file snapshot" - :file-id (str file-id) + :file-id (str (:id file)) :id (str snapshot-id) :label label) (db/insert! cfg :file-change {:id snapshot-id :revn (:revn file) - :data (:data file) + :data data :version (:version file) - :features (:features file) + :features features :profile-id profile-id :file-id (:id file) :label label @@ -146,12 +130,25 @@ (sv/defmethod ::create-file-snapshot {::doc/added "1.20" - ::sm/params schema:create-file-snapshot} - [cfg {:keys [::rpc/profile-id file-id label]}] - (db/tx-run! cfg - (fn [{:keys [::db/conn] :as cfg}] - (files/check-edition-permissions! conn profile-id file-id) - (create-file-snapshot! cfg profile-id file-id label)))) + ::sm/params schema:create-file-snapshot + ::db/transaction true} + [{:keys [::db/conn] :as cfg} {:keys [::rpc/profile-id file-id label]}] + (files/check-edition-permissions! conn profile-id file-id) + (let [file (bfc/get-file cfg file-id) + project (db/get-by-id cfg :project (:project-id file))] + + (-> cfg + (assoc ::quotes/profile-id profile-id) + (assoc ::quotes/project-id (:project-id file)) + (assoc ::quotes/team-id (:team-id project)) + (assoc ::quotes/file-id (:id file)) + (quotes/check! {::quotes/id ::quotes/snapshots-per-file} + {::quotes/id ::quotes/snapshots-per-team})) + + (create-file-snapshot! cfg file + {:label label + :profile-id profile-id + :created-by :user}))) (defn restore-file-snapshot! [{:keys [::db/conn ::mbus/msgbus] :as cfg} file-id snapshot-id] @@ -237,8 +234,11 @@ (db/tx-run! cfg (fn [{:keys [::db/conn] :as cfg}] (files/check-edition-permissions! conn profile-id file-id) - (create-file-snapshot! cfg profile-id file-id :system) - (restore-file-snapshot! cfg file-id id)))) + (let [file (bfc/get-file cfg file-id)] + (create-file-snapshot! cfg file + {:profile-id profile-id + :created-by :system}) + (restore-file-snapshot! cfg file-id id))))) (def ^:private schema:update-file-snapshot [:map {:title "update-file-snapshot"} diff --git a/backend/src/app/srepl/helpers.clj b/backend/src/app/srepl/helpers.clj index 2ea26e3bb..d8293253e 100644 --- a/backend/src/app/srepl/helpers.clj +++ b/backend/src/app/srepl/helpers.clj @@ -15,7 +15,8 @@ [app.features.components-v2 :as feat.comp-v2] [app.main :as main] [app.rpc.commands.files :as files] - [app.rpc.commands.files-snapshot :as fsnap])) + [app.rpc.commands.files-snapshot :as fsnap] + [app.util.time :as dt])) (def ^:dynamic *system* nil) @@ -96,8 +97,11 @@ (let [conn (db/get-connection system)] (->> (feat.comp-v2/get-and-lock-team-files conn team-id) (reduce (fn [result file-id] - (fsnap/create-file-snapshot! system nil file-id label) - (inc result)) + (let [file (fsnap/get-file-snapshots system file-id)] + (fsnap/create-file-snapshot! system file + {:label label + :created-by :admin}) + (inc result))) 0)))) (defn restore-team-snapshot! @@ -143,7 +147,10 @@ (cfv/validate-file-schema! file')) (when (string? label) - (fsnap/create-file-snapshot! system nil file-id label)) + (fsnap/create-file-snapshot! system file + {:label label + :deleted-at (dt/in-future {:days 30}) + :created-by :admin})) (let [file' (update file' :revn inc)] (bfc/update-file! system file') diff --git a/backend/test/backend_tests/rpc_file_snapshot_test.clj b/backend/test/backend_tests/rpc_file_snapshot_test.clj index 90e7366ee..1da63c4f3 100644 --- a/backend/test/backend_tests/rpc_file_snapshot_test.clj +++ b/backend/test/backend_tests/rpc_file_snapshot_test.clj @@ -97,6 +97,7 @@ (th/db-query :file-change {:file-id (:id file)} {:order-by [:created-at]})] + (t/is (= 2 (count rows))) (t/is (= "user" (:created-by row1))) (t/is (= "system" (:created-by row2))))) diff --git a/common/src/app/common/files/helpers.cljc b/common/src/app/common/files/helpers.cljc index c3f56695d..c429cd749 100644 --- a/common/src/app/common/files/helpers.cljc +++ b/common/src/app/common/files/helpers.cljc @@ -570,10 +570,9 @@ (into xform:collect-media-refs (vals (:components data))) (into (keys (:media data))))) -(defn relink-media-refs - "A function responsible to analyze all file data and replace the - old :component-file reference with the new ones, using the provided - file-index." +(defn relink-refs + "A function responsible to analyze the file data or shape for references + and apply lookup-index on it." [data lookup-index] (letfn [(process-map-form [form] (cond-> form @@ -724,7 +723,7 @@ (defn split-by-last-period "Splits a string into two parts: - the text before and including the last period, + the text before and including the last period, and the text after the last period." [s] (if-let [last-period (str/last-index-of s ".")]