From 655afa088dac69b08e06948883d68d784d8310d4 Mon Sep 17 00:00:00 2001 From: Alejandro Alonso Date: Fri, 25 Mar 2022 08:21:30 +0100 Subject: [PATCH 01/19] :bug: Fix copy paste inside a text layer leaves pasted text transparent --- CHANGES.md | 1 + frontend/src/app/util/text_editor.cljs | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 2f1fde89e..12838f2ac 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -42,6 +42,7 @@ ### :bug: Bugs fixed +- Copy paste inside a text layer leaves pasted text transparent [Taiga #3096](https://tree.taiga.io/project/penpot/issue/3096) - On dashboard enter on empty search refresh the page [Taiga #2597](https://tree.taiga.io/project/penpot/issue/2597) - Pencil cursor changes when activated [Taiga #2276](https://tree.taiga.io/project/penpot/issue/2276) - Fix icon placement in Mixed message [Taiga #3037](https://tree.taiga.io/project/penpot/issue/3037) diff --git a/frontend/src/app/util/text_editor.cljs b/frontend/src/app/util/text_editor.cljs index 5cf866a9e..28da8d72c 100644 --- a/frontend/src/app/util/text_editor.cljs +++ b/frontend/src/app/util/text_editor.cljs @@ -15,7 +15,8 @@ (defn immutable-map->map [obj] - (into {} (map (fn [[k v]] [(keyword k) v])) (seq obj))) + (let [data (into {} (map (fn [[k v]] [(keyword k) v])) (seq obj))] + (assoc data :fills (js->clj (:fills data) :keywordize-keys true)))) ;; --- DRAFT-JS HELPERS From 32d31da0da6ee3503968f1dc8d46330eb1ca3e2c Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Fri, 25 Mar 2022 11:53:50 +0100 Subject: [PATCH 02/19] :sparkles: Show shortcuts debugging command --- frontend/src/debug.cljs | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/frontend/src/debug.cljs b/frontend/src/debug.cljs index 21b6881a0..968ec2198 100644 --- a/frontend/src/debug.cljs +++ b/frontend/src/debug.cljs @@ -10,8 +10,11 @@ [app.common.pages.helpers :as cph] [app.common.transit :as t] [app.common.uuid :as uuid] + [app.main.data.viewer.shortcuts] [app.main.data.workspace :as dw] [app.main.data.workspace.changes :as dwc] + [app.main.data.workspace.path.shortcuts] + [app.main.data.workspace.shortcuts] [app.main.store :as st] [app.util.object :as obj] [app.util.timers :as timers] @@ -19,7 +22,7 @@ [cljs.pprint :refer [pprint]] [cuerdas.core :as str] [potok.core :as ptk] - [promesa.core :as p])) + [promesa.core :as p] )) (def debug-options #{;; Displays the bounding box for the shapes @@ -301,3 +304,28 @@ [] (st/emit! (dw/toggle-layout-flag :hide-ui))) + + +(defn ^:export shortcuts + [] + + (letfn [(print-shortcuts [shortcuts] + (.table js/console + (->> shortcuts + (map (fn [[key {:keys [command]}]] + [(d/name key) + (if (vector? command) + (str/join " | " command) + command)])) + (into {}) + (clj->js))))] + + (.log js/console "Workspace") + (print-shortcuts app.main.data.workspace.shortcuts/shortcuts) + + (.log js/console "Path") + (print-shortcuts app.main.data.workspace.path.shortcuts/shortcuts) + + (.log js/console "Viewer") + (print-shortcuts app.main.data.viewer.shortcuts/shortcuts)) + nil) From 01194d5e255d373b20f413215ffd2e11f94b402c Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Fri, 25 Mar 2022 12:18:33 +0100 Subject: [PATCH 03/19] :sparkles: Add dashboard to shortcuts --- frontend/src/debug.cljs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/frontend/src/debug.cljs b/frontend/src/debug.cljs index 968ec2198..e7b48bbf0 100644 --- a/frontend/src/debug.cljs +++ b/frontend/src/debug.cljs @@ -10,6 +10,7 @@ [app.common.pages.helpers :as cph] [app.common.transit :as t] [app.common.uuid :as uuid] + [app.main.data.dashboard.shortcuts] [app.main.data.viewer.shortcuts] [app.main.data.workspace :as dw] [app.main.data.workspace.changes :as dwc] @@ -22,7 +23,7 @@ [cljs.pprint :refer [pprint]] [cuerdas.core :as str] [potok.core :as ptk] - [promesa.core :as p] )) + [promesa.core :as p])) (def debug-options #{;; Displays the bounding box for the shapes @@ -319,13 +320,16 @@ command)])) (into {}) (clj->js))))] + (let [style "font-weight: bold; font-size: 1.25rem;"] + (.log js/console "%c Dashboard" style) + (print-shortcuts app.main.data.dashboard.shortcuts/shortcuts) - (.log js/console "Workspace") - (print-shortcuts app.main.data.workspace.shortcuts/shortcuts) + (.log js/console "%c Workspace" style) + (print-shortcuts app.main.data.workspace.shortcuts/shortcuts) - (.log js/console "Path") - (print-shortcuts app.main.data.workspace.path.shortcuts/shortcuts) + (.log js/console "%c Path" style) + (print-shortcuts app.main.data.workspace.path.shortcuts/shortcuts) - (.log js/console "Viewer") - (print-shortcuts app.main.data.viewer.shortcuts/shortcuts)) + (.log js/console "%c Viewer" style) + (print-shortcuts app.main.data.viewer.shortcuts/shortcuts))) nil) From d73ed95719b84df613eb610708dfa31ef4e65420 Mon Sep 17 00:00:00 2001 From: Alejandro Alonso Date: Fri, 25 Mar 2022 13:20:46 +0100 Subject: [PATCH 04/19] :bug: Fix export multiple styles --- frontend/resources/styles/main/partials/modal.scss | 3 ++- frontend/src/app/main/ui/export.cljs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/frontend/resources/styles/main/partials/modal.scss b/frontend/resources/styles/main/partials/modal.scss index 242b5c846..94c5a1e88 100644 --- a/frontend/resources/styles/main/partials/modal.scss +++ b/frontend/resources/styles/main/partials/modal.scss @@ -1464,7 +1464,8 @@ } } - & .unchecked { + & .intermediate, + .unchecked { svg { background-color: $color-gray-10; } diff --git a/frontend/src/app/main/ui/export.cljs b/frontend/src/app/main/ui/export.cljs index b96b8eab9..1b2a02617 100644 --- a/frontend/src/app/main/ui/export.cljs +++ b/frontend/src/app/main/ui/export.cljs @@ -76,7 +76,7 @@ (cond all-checked? [:span.checked i/checkbox-checked] all-unchecked? [:span.unchecked i/checkbox-unchecked] - :else [:span i/checkbox-intermediate])] + :else [:span.intermediate i/checkbox-intermediate])] [:div.field.title (tr "dashboard.export-multiple.selected" (c (count enabled-exports)) (c (count all-exports)))]] From 927dbbfe82b33b8a49b834c89d53f266022e6a5a Mon Sep 17 00:00:00 2001 From: Alejandro Alonso Date: Fri, 25 Mar 2022 13:37:38 +0100 Subject: [PATCH 05/19] :bug: Fix precission on export modal --- frontend/src/app/main/ui/export.cljs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frontend/src/app/main/ui/export.cljs b/frontend/src/app/main/ui/export.cljs index 1b2a02617..f6c17187e 100644 --- a/frontend/src/app/main/ui/export.cljs +++ b/frontend/src/app/main/ui/export.cljs @@ -18,6 +18,7 @@ [app.main.ui.workspace.shapes :refer [shape-wrapper]] [app.util.dom :as dom] [app.util.i18n :as i18n :refer [tr c]] + [app.util.strings :as ust] [cuerdas.core :as str] [rumext.alpha :as mf])) @@ -107,8 +108,8 @@ [:div.field.name (cond-> (:name shape) suffix (str suffix))] (when (:scale export) - [:div.field.scale (dm/str (* width (:scale export)) "x" - (* height (:scale export)) "px ")]) + [:div.field.scale (dm/str (ust/format-precision (* width (:scale export)) 2) "x" + (ust/format-precision (* height (:scale export)) 2) "px ")]) (when (:type export) [:div.field.extension (-> export :type d/name str/upper)])]))] From 1db9b04bfd8b8b72322e5b4081d6a66b735f34f1 Mon Sep 17 00:00:00 2001 From: Alejandro Alonso Date: Fri, 25 Mar 2022 12:56:55 +0100 Subject: [PATCH 06/19] :bug: Fix error when adding gradient stroke to shape --- frontend/src/app/main/data/workspace/colors.cljs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frontend/src/app/main/data/workspace/colors.cljs b/frontend/src/app/main/data/workspace/colors.cljs index f677aee8f..97deb5a06 100644 --- a/frontend/src/app/main/data/workspace/colors.cljs +++ b/frontend/src/app/main/data/workspace/colors.cljs @@ -259,7 +259,11 @@ (assoc :stroke-color-gradient (:gradient attrs)) (contains? attrs :opacity) - (assoc :stroke-opacity (:opacity attrs))) + (assoc :stroke-opacity (:opacity attrs)) + + :always + (d/without-nils)) + attrs (merge attrs color-attrs)] (rx/of (dch/update-shapes ids (fn [shape] From 5817b5fe1986f5dcdead27cf0e12bddf0475929b Mon Sep 17 00:00:00 2001 From: Alejandro Alonso Date: Fri, 25 Mar 2022 12:49:05 +0100 Subject: [PATCH 07/19] :bug: Fix completed export text not shown --- frontend/src/app/main/ui/export.cljs | 2 ++ frontend/translations/en.po | 4 ++++ frontend/translations/es.po | 6 +++++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/frontend/src/app/main/ui/export.cljs b/frontend/src/app/main/ui/export.cljs index f6c17187e..3b273c399 100644 --- a/frontend/src/app/main/ui/export.cljs +++ b/frontend/src/app/main/ui/export.cljs @@ -176,6 +176,7 @@ progress (:progress state) exports (:exports state) total (count exports) + complete? (= progress total) circ (* 2 Math/PI 12) pct (- circ (* circ (/ progress total))) @@ -188,6 +189,7 @@ (not healthy?) clr/warning) title (cond error? (tr "workspace.options.exporting-object-error") + complete? (tr "workspace.options.exporting-complete") healthy? (tr "workspace.options.exporting-object") (not healthy?) (tr "workspace.options.exporting-object-slow")) diff --git a/frontend/translations/en.po b/frontend/translations/en.po index 9ad21b2c0..ce9150436 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -2549,6 +2549,10 @@ msgstr "Suffix" msgid "workspace.options.exporting-object" msgstr "Exporting…" +#: src/app/main/ui/workspace/sidebar/options/menus/exports.cljs, src/app/main/ui/handoff/exports.cljs, src/app/main/ui/workspace/header.cljs +msgid "workspace.options.exporting-complete" +msgstr "Export complete" + #: src/app/main/ui/workspace/sidebar/options/menus/exports.cljs, src/app/main/ui/handoff/exports.cljs, src/app/main/ui/workspace/header.cljs msgid "workspace.options.exporting-object-error" msgstr "Export failed" diff --git a/frontend/translations/es.po b/frontend/translations/es.po index ccc402c43..c09e0e91b 100644 --- a/frontend/translations/es.po +++ b/frontend/translations/es.po @@ -2564,6 +2564,10 @@ msgstr "Sufijo" msgid "workspace.options.exporting-object" msgstr "Exportando..." +#: src/app/main/ui/workspace/sidebar/options/menus/exports.cljs, src/app/main/ui/handoff/exports.cljs, src/app/main/ui/workspace/header.cljs +msgid "workspace.options.exporting-complete" +msgstr "Exportación completa" + #: src/app/main/ui/workspace/sidebar/options/menus/exports.cljs, src/app/main/ui/handoff/exports.cljs, src/app/main/ui/workspace/header.cljs msgid "workspace.options.exporting-object-error" msgstr "Exportación fallida" @@ -3671,4 +3675,4 @@ msgid "workspace.updates.update" msgstr "Actualizar" msgid "workspace.viewport.click-to-close-path" -msgstr "Pulsar para cerrar la ruta" \ No newline at end of file +msgstr "Pulsar para cerrar la ruta" From 27c8f883ff05fd542c651140b2a56a5c6fd5b98a Mon Sep 17 00:00:00 2001 From: Alejandro Alonso Date: Mon, 28 Mar 2022 08:42:03 +0200 Subject: [PATCH 08/19] :bug: Fix ctrl-click on assets --- frontend/src/app/main/data/workspace.cljs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/app/main/data/workspace.cljs b/frontend/src/app/main/data/workspace.cljs index 25de91e04..ada2bcc73 100644 --- a/frontend/src/app/main/data/workspace.cljs +++ b/frontend/src/app/main/data/workspace.cljs @@ -1043,8 +1043,8 @@ [items element] (let [items (or items #{})] (if (contains? items element) - (disj set element) - (conj set element)))) + (disj items element) + (conj items element)))) (defn toggle-selected-assets [asset type] From b91c42e186ad0054d9d23ebc05af955858cb150a Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 21 Mar 2022 09:25:19 +0100 Subject: [PATCH 09/19] :zap: Add performance improvements to file thumbnails Mainly addresing unnecesary object transmission. The new code strips unnecesary data to be transferred from back to front. Additionally it removes some legacy code and simplifies other parts of code. --- backend/src/app/rpc/mutations/files.clj | 2 +- backend/src/app/rpc/queries/files.clj | 91 +++++++++++------------ common/src/app/common/spec/shape.cljc | 4 +- frontend/src/app/main/data/workspace.cljs | 32 +++----- frontend/src/app/main/render.cljs | 25 ------- frontend/src/app/worker/thumbnails.cljs | 36 ++++----- 6 files changed, 72 insertions(+), 118 deletions(-) diff --git a/backend/src/app/rpc/mutations/files.clj b/backend/src/app/rpc/mutations/files.clj index d3c605534..19a00a404 100644 --- a/backend/src/app/rpc/mutations/files.clj +++ b/backend/src/app/rpc/mutations/files.clj @@ -320,7 +320,7 @@ _ (mtx/run! metrics {:id :update-file-changes :inc (count changes)}) ts (dt/now) - file (-> (files/retrieve-data cfg file) + file (-> file (update :revn inc) (update :data (fn [data] ;; Trace the length of bytes of processed data diff --git a/backend/src/app/rpc/queries/files.clj b/backend/src/app/rpc/queries/files.clj index 30e349e8f..9706c9e17 100644 --- a/backend/src/app/rpc/queries/files.clj +++ b/backend/src/app/rpc/queries/files.clj @@ -16,11 +16,9 @@ [app.rpc.queries.projects :as projects] [app.rpc.queries.share-link :refer [retrieve-share-link]] [app.rpc.queries.teams :as teams] - [app.storage.impl :as simpl] [app.util.blob :as blob] [app.util.services :as sv] - [clojure.spec.alpha :as s] - [promesa.core :as p])) + [clojure.spec.alpha :as s])) (declare decode-row) (declare decode-row-xf) @@ -186,25 +184,12 @@ ;; --- Query: File (By ID) -(defn- retrieve-data* - [{:keys [storage] :as cfg} file] - (p/do - (when-let [backend (simpl/resolve-backend storage (:data-backend file))] - (simpl/get-object-bytes backend file)))) - -(defn retrieve-data - [cfg file] - (if (bytes? (:data file)) - file - (p/->> (retrieve-data* cfg file) - (assoc file :data)))) - (defn retrieve-file [{:keys [pool] :as cfg} id] - (p/->> (db/get-by-id pool :file id) - (retrieve-data cfg) + (let [item (db/get-by-id pool :file id)] + (->> item (decode-row) - (pmg/migrate-file))) + (pmg/migrate-file)))) (s/def ::file (s/keys :req-un [::profile-id ::id])) @@ -214,8 +199,8 @@ [{:keys [pool] :as cfg} {:keys [profile-id id] :as params}] (let [perms (get-permissions pool profile-id id)] (check-read-permissions! perms) - (p/-> (retrieve-file cfg id) - (assoc :permissions perms)))) + (-> (retrieve-file cfg id) + (assoc :permissions perms)))) (declare trim-file-data) @@ -233,9 +218,9 @@ [{:keys [pool] :as cfg} {:keys [profile-id id] :as params}] (let [perms (get-permissions pool profile-id id)] (check-read-permissions! perms) - (p/-> (retrieve-file cfg id) - (trim-file-data params) - (assoc :permissions perms)))) + (-> (retrieve-file cfg id) + (trim-file-data params) + (assoc :permissions perms)))) (defn- trim-file-data [file {:keys [page-id object-id]}] @@ -248,9 +233,12 @@ (update :data assoc :pages-index {page-id page}) (update :data assoc :pages [page-id])))) +;; --- FILE THUMBNAIL + (declare strip-frames-with-thumbnails) (declare extract-file-thumbnail) (declare get-first-page-data) +(declare get-thumbnail-data) (s/def ::strip-frames-with-thumbnails ::us/boolean) @@ -258,6 +246,17 @@ (s/keys :req-un [::profile-id ::file-id] :opt-un [::strip-frames-with-thumbnails])) +(sv/defmethod ::page + "Retrieves the first page of the file. Used mainly for render + thumbnails on dashboard. + + DEPRECATED: still here for backward compatibility." + [{:keys [pool] :as cfg} {:keys [profile-id file-id] :as props}] + (check-read-permissions! pool profile-id file-id) + (let [file (retrieve-file cfg file-id) + data (get-first-page-data file props)] + data)) + (s/def ::file-data-for-thumbnail (s/keys :req-un [::profile-id ::file-id] :opt-un [::strip-frames-with-thumbnails])) @@ -267,20 +266,29 @@ thumbnails on dashboard." [{:keys [pool] :as cfg} {:keys [profile-id file-id] :as props}] (check-read-permissions! pool profile-id file-id) - (p/let [file (retrieve-file cfg file-id) - data (get-first-page-data file props) - file-thumbnail (extract-file-thumbnail (get-in file [:data :pages-index]))] + (let [file (retrieve-file cfg file-id)] + (get-thumbnail-data file props))) - (assoc data :file-thumbnail file-thumbnail))) +(defn get-thumbnail-data + [{:keys [data] :as file} props] + (if-let [[page frame] (first + (for [page (-> data :pages-index vals) + frame (-> page :objects cph/get-frames) + :when (:file-thumbnail frame)] + [page frame]))] + (let [objects (->> (cph/get-children-with-self (:objects page) (:id frame)) + (d/index-by :id))] + (cond-> (assoc page :objects objects) + (:strip-frames-with-thumbnails props) + (strip-frames-with-thumbnails) -(sv/defmethod ::page - "Retrieves the first page of the file. Used mainly for render - thumbnails on dashboard." - [{:keys [pool] :as cfg} {:keys [profile-id file-id] :as props}] - (check-read-permissions! pool profile-id file-id) - (p/let [file (retrieve-file cfg file-id) - data (get-first-page-data file props)] - data)) + :always + (assoc :thumbnail-frame frame))) + + (let [page-id (-> data :pages first)] + (cond-> (get-in data [:pages-index page-id]) + (:strip-frames-with-thumbnails props) + (strip-frames-with-thumbnails))))) (defn get-first-page-data [file props] @@ -318,16 +326,6 @@ (update data :objects update-objects))) -(defn extract-file-thumbnail - "Extract the frame marked as file-thumbnail" - [pages] - (->> pages - vals - (mapcat :objects) - vals - (filter :file-thumbnail) - first)) - ;; --- Query: Shared Library Files (def ^:private sql:team-shared-files @@ -384,7 +382,6 @@ [{:keys [pool] :as cfg} is-indirect file-id] (let [xform (comp (map #(assoc % :is-indirect is-indirect)) - (map #(retrieve-data cfg %)) (map decode-row))] (into #{} xform (db/exec! pool [sql:file-libraries file-id])))) diff --git a/common/src/app/common/spec/shape.cljc b/common/src/app/common/spec/shape.cljc index 422c95f1e..e2732a0f4 100644 --- a/common/src/app/common/spec/shape.cljc +++ b/common/src/app/common/spec/shape.cljc @@ -53,6 +53,7 @@ (s/def ::hide-fill-on-export boolean?) +(s/def ::file-thumbnail boolean?) (s/def ::masked-group? boolean?) (s/def ::font-family string?) (s/def ::font-size ::us/safe-integer) @@ -301,7 +302,8 @@ (defmethod shape-spec :frame [_] (s/and ::shape-attrs - (s/keys :opt-un [::hide-fill-on-export]))) + (s/keys :opt-un [::file-thumbnail + ::hide-fill-on-export]))) (s/def ::shape (s/and (s/multi-spec shape-spec :type) diff --git a/frontend/src/app/main/data/workspace.cljs b/frontend/src/app/main/data/workspace.cljs index ada2bcc73..8d85661df 100644 --- a/frontend/src/app/main/data/workspace.cljs +++ b/frontend/src/app/main/data/workspace.cljs @@ -957,35 +957,23 @@ (let [selected (wsh/lookup-selected state)] (rx/of (dch/update-shapes selected #(update % :blocked not))))))) -(defn extract-file-thumbnails-from-page - [state selected page] - (let [extract-frames (fn [page-id] - (let [objects (wsh/lookup-page-objects state page-id)] - (cph/get-frames objects))) - page-id (key page) - frames-with-thumbnail (->> (extract-frames page-id) - (filter (comp true? :file-thumbnail)) - (map :id) - (remove #(some #{%} selected)) - (map #(into {} {:id % :page-id page-id})))] - (when frames-with-thumbnail frames-with-thumbnail))) - - (defn toggle-file-thumbnail-selected [] (ptk/reify ::toggle-file-thumbnail-selected ptk/WatchEvent (watch [_ state _] (let [selected (wsh/lookup-selected state) - pages (get-in state [:workspace-data - :pages-index]) - file-thumbnails (->> pages - (mapcat #(extract-file-thumbnails-from-page state selected %)))] + pages (-> state :workspace-data :pages-index vals) + extract (fn [{:keys [objects id] :as page}] + (->> (cph/get-frames objects) + (filter :file-thumbnail) + (map :id) + (remove selected) + (map (fn [frame-id] [id frame-id]))))] (rx/concat - (rx/from - (for [ft file-thumbnails] - (dch/update-shapes [(:id ft)] #(update % :file-thumbnail not) (:page-id ft) nil))) - (rx/of (dch/update-shapes selected #(update % :file-thumbnail not)))))))) + (rx/from (for [[page-id frame-id] (mapcat extract pages)] + (dch/update-shapes [frame-id] #(dissoc % :file-thumbnail) page-id nil))) + (rx/of (dch/update-shapes selected #(assoc % :file-thumbnail true)))))))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; Navigation diff --git a/frontend/src/app/main/render.cljs b/frontend/src/app/main/render.cljs index 7970bb173..34eebbabc 100644 --- a/frontend/src/app/main/render.cljs +++ b/frontend/src/app/main/render.cljs @@ -214,31 +214,6 @@ [:& shape-wrapper {:shape item :key (:id item)}])))]]])) -(mf/defc file-thumbnail-svg - {::mf/wrap [mf/memo]} - [{:keys [data embed? include-metadata?] :as props - :or {embed? false include-metadata? false}}] - (let [data (assoc data :x 0 :y 0) - vbox (format-viewbox {:width (:width data 0) :height (:height data 0)}) - background-color (get-in data [:options :background] default-color)] - - [:& (mf/provider embed/context) {:value embed?} - [:& (mf/provider export/include-metadata-ctx) {:value include-metadata?} - [:svg {:view-box vbox - :version "1.1" - :xmlns "http://www.w3.org/2000/svg" - :xmlnsXlink "http://www.w3.org/1999/xlink" - :xmlns:penpot (when include-metadata? "https://penpot.app/xmlns") - :style {:width "100%" - :height "100%" - :background background-color}} - - (when include-metadata? - [:& export/export-page {:options (:options data)}]) - - [:> shape-container {:shape data} - [:& frame/frame-thumbnail {:shape data}]]]]])) - (mf/defc frame-svg {::mf/wrap [mf/memo]} [{:keys [objects frame zoom show-thumbnails?] :or {zoom 1} :as props}] diff --git a/frontend/src/app/worker/thumbnails.cljs b/frontend/src/app/worker/thumbnails.cljs index f49e94b1e..5524c087d 100644 --- a/frontend/src/app/worker/thumbnails.cljs +++ b/frontend/src/app/worker/thumbnails.cljs @@ -31,36 +31,28 @@ (defn- request-thumbnail [file-id] - (let [uri (u/join (cfg/get-public-uri) "api/rpc/query/file-data-for-thumbnail") - params {:file-id file-id - :strip-frames-with-thumbnails true}] - (->> (http/send! - {:method :get - :uri uri - :credentials "include" - :query params}) + (let [uri (u/join (cfg/get-public-uri) "api/rpc/query/file-data-for-thumbnail") + params {:file-id file-id + :strip-frames-with-thumbnails true} + request {:method :get + :uri uri + :credentials "include" + :query params}] + (->> (http/send! request) (rx/map http/conditional-decode-transit) (rx/mapcat handle-response)))) -(defonce cache (atom {})) - (defn render-frame - [data ckey] - (let [prev (get @cache ckey)] - (if (= (:data prev) data) - (:result prev) - (let [file-thumbnail (:file-thumbnail data) - elem (if file-thumbnail - (mf/element render/file-thumbnail-svg #js {:data file-thumbnail :width "290" :height "150"}) - (mf/element render/page-svg #js {:data data :width "290" :height "150" :thumbnails? true})) - result (rds/renderToStaticMarkup elem)] - (swap! cache assoc ckey {:data data :result result}) - result)))) + [data] + (let [elem (if-let [frame (:thumbnail-frame data)] + (mf/element render/frame-svg #js {:objects (:objects data) :frame frame}) + (mf/element render/page-svg #js {:data data :width "290" :height "150" :thumbnails? true}))] + (rds/renderToStaticMarkup elem))) (defmethod impl/handler :thumbnails/generate [{:keys [file-id] :as message}] (->> (request-thumbnail file-id) (rx/map (fn [data] - {:svg (render-frame data #{file-id}) + {:svg (render-frame data) :fonts @fonts/loaded})))) From c876534c859d410752e37360a2afa5d1a6ac6aa4 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 22 Mar 2022 17:22:53 +0100 Subject: [PATCH 10/19] :sparkles: Move the dashboard grid thumbnails to backend cache --- backend/src/app/migrations.clj | 3 + .../sql/0066-add-frame-thumbnail-table.sql | 3 + .../sql/0069-add-file-thumbnail-table.sql | 14 ++++ backend/src/app/rpc/helpers.clj | 16 +++++ backend/src/app/rpc/mutations/files.clj | 24 ++++++- backend/src/app/rpc/queries/files.clj | 58 +++++++++++---- frontend/src/app/main/ui/dashboard/grid.cljs | 60 +++------------- frontend/src/app/render.cljs | 1 - frontend/src/app/worker/thumbnails.cljs | 70 +++++++++++++++---- 9 files changed, 167 insertions(+), 82 deletions(-) create mode 100644 backend/src/app/migrations/sql/0069-add-file-thumbnail-table.sql create mode 100644 backend/src/app/rpc/helpers.clj diff --git a/backend/src/app/migrations.clj b/backend/src/app/migrations.clj index 76e6d0d68..7f552a532 100644 --- a/backend/src/app/migrations.clj +++ b/backend/src/app/migrations.clj @@ -214,6 +214,9 @@ {:name "0068-mod-storage-object-table" :fn (mg/resource "app/migrations/sql/0068-mod-storage-object-table.sql")} + + {:name "0069-add-file-thumbnail-table" + :fn (mg/resource "app/migrations/sql/0069-add-file-thumbnail-table.sql")} ]) diff --git a/backend/src/app/migrations/sql/0066-add-frame-thumbnail-table.sql b/backend/src/app/migrations/sql/0066-add-frame-thumbnail-table.sql index 3134cbe21..316a3ee6d 100644 --- a/backend/src/app/migrations/sql/0066-add-frame-thumbnail-table.sql +++ b/backend/src/app/migrations/sql/0066-add-frame-thumbnail-table.sql @@ -8,3 +8,6 @@ CREATE TABLE file_frame_thumbnail ( PRIMARY KEY(file_id, frame_id) ); + +ALTER TABLE file_frame_thumbnail + ALTER COLUMN data SET STORAGE external; diff --git a/backend/src/app/migrations/sql/0069-add-file-thumbnail-table.sql b/backend/src/app/migrations/sql/0069-add-file-thumbnail-table.sql new file mode 100644 index 000000000..d9a3fc2fe --- /dev/null +++ b/backend/src/app/migrations/sql/0069-add-file-thumbnail-table.sql @@ -0,0 +1,14 @@ +CREATE TABLE file_thumbnail ( + file_id uuid NOT NULL REFERENCES file(id) ON DELETE CASCADE, + revn bigint NOT NULL, + created_at timestamptz NOT NULL DEFAULT now(), + updated_at timestamptz NOT NULL DEFAULT now(), + deleted_at timestamptz NULL, + data text NULL, + props jsonb NULL, + PRIMARY KEY(file_id, revn) +); + +ALTER TABLE file_thumbnail + ALTER COLUMN data SET STORAGE external, + ALTER COLUMN props SET STORAGE external; diff --git a/backend/src/app/rpc/helpers.clj b/backend/src/app/rpc/helpers.clj new file mode 100644 index 000000000..f60879e95 --- /dev/null +++ b/backend/src/app/rpc/helpers.clj @@ -0,0 +1,16 @@ +;; 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) UXBOX Labs SL + +(ns app.rpc.helpers + "General purpose RPC helpers." + (:require [app.common.data.macros :as dm])) + +(defn http-cache + [{:keys [max-age]}] + (fn [_ response] + (let [exp (if (integer? max-age) max-age (inst-ms max-age)) + val (dm/fmt "max-age=%" (int (/ exp 1000.0)))] + (update response :headers assoc "cache-control" val)))) diff --git a/backend/src/app/rpc/mutations/files.clj b/backend/src/app/rpc/mutations/files.clj index 19a00a404..cb6bd5aa3 100644 --- a/backend/src/app/rpc/mutations/files.clj +++ b/backend/src/app/rpc/mutations/files.clj @@ -487,14 +487,34 @@ update set data = ?;") (s/def ::data ::us/string) -(s/def ::upsert-frame-thumbnail +(s/def ::upsert-file-frame-thumbnail (s/keys :req-un [::profile-id ::file-id ::frame-id ::data])) -(sv/defmethod ::upsert-frame-thumbnail +(sv/defmethod ::upsert-file-frame-thumbnail [{:keys [pool] :as cfg} {:keys [profile-id file-id frame-id data]}] (db/with-atomic [conn pool] (files/check-edition-permissions! conn profile-id file-id) (db/exec-one! conn [sql:upsert-frame-thumbnail file-id frame-id data data]) nil)) +;; --- Mutation: Upsert file thumbnail +(def sql:upsert-file-thumbnail + "insert into file_thumbnail(file_id, revn, data, props) + values (?, ?, ?, ?) + on conflict(file_id, revn) do + update set data = ?, updated_at=now();") + +(s/def ::revn ::us/integer) +(s/def ::props (s/map-of ::us/keyword any?)) +(s/def ::upsert-file-thumbnail + (s/keys :req-un [::profile-id ::file-id ::revn ::data ::props])) + +(sv/defmethod ::upsert-file-thumbnail + [{:keys [pool] :as cfg} {:keys [profile-id file-id revn data props]}] + (db/with-atomic [conn pool] + (files/check-edition-permissions! conn profile-id file-id) + (let [props (db/tjson (or props {}))] + (db/exec-one! conn [sql:upsert-file-thumbnail + file-id revn data props data]) + nil))) diff --git a/backend/src/app/rpc/queries/files.clj b/backend/src/app/rpc/queries/files.clj index 9706c9e17..935b41c8e 100644 --- a/backend/src/app/rpc/queries/files.clj +++ b/backend/src/app/rpc/queries/files.clj @@ -7,11 +7,14 @@ (ns app.rpc.queries.files (:require [app.common.data :as d] + [app.common.exceptions :as ex] [app.common.pages.helpers :as cph] [app.common.pages.migrations :as pmg] [app.common.spec :as us] [app.common.uuid :as uuid] [app.db :as db] + [app.db.sql :as sql] + [app.rpc.helpers :as rpch] [app.rpc.permissions :as perms] [app.rpc.queries.projects :as projects] [app.rpc.queries.share-link :refer [retrieve-share-link]] @@ -267,7 +270,9 @@ [{:keys [pool] :as cfg} {:keys [profile-id file-id] :as props}] (check-read-permissions! pool profile-id file-id) (let [file (retrieve-file cfg file-id)] - (get-thumbnail-data file props))) + {:data (get-thumbnail-data file props) + :file-id file-id + :revn (:revn file)})) (defn get-thumbnail-data [{:keys [data] :as file} props] @@ -325,7 +330,6 @@ (update data :objects update-objects))) - ;; --- Query: Shared Library Files (def ^:private sql:team-shared-files @@ -424,22 +428,48 @@ (teams/check-read-permissions! pool profile-id team-id) (db/exec! pool [sql:team-recent-files team-id])) +;; --- QUERY: get all file frame thumbnails -;; --- QUERY: get the thumbnail for an frame +(s/def ::file-frame-thumbnails + (s/keys :req-un [::profile-id ::file-id] + :opt-un [::frame-id])) -(def ^:private sql:file-frame-thumbnail - "select data - from file_frame_thumbnail - where file_id = ? - and frame_id = ?") - -(s/def ::file-frame-thumbnail - (s/keys :req-un [::profile-id ::file-id ::frame-id])) - -(sv/defmethod ::file-frame-thumbnail +(sv/defmethod ::file-frame-thumbnails [{:keys [pool]} {:keys [profile-id file-id frame-id]}] (check-read-permissions! pool profile-id file-id) - (db/exec-one! pool [sql:file-frame-thumbnail file-id frame-id])) + (let [params (cond-> {:file-id file-id} + frame-id (assoc :frame-id frame-id)) + rows (db/query pool :file-frame-thumbnail params)] + (d/group-by :frame-id :data rows))) + +;; --- QUERY: get file thumbnail + +(s/def ::revn ::us/integer) + +(s/def ::file-thumbnail + (s/keys :req-un [::profile-id ::file-id] + :opt-un [::revn])) + +(sv/defmethod ::file-thumbnail + [{:keys [pool]} {:keys [profile-id file-id revn]}] + (check-read-permissions! pool profile-id file-id) + (let [sql (sql/select :file-thumbnail + (cond-> {:file-id file-id} + revn (assoc :revn revn)) + {:limit 1 + :order-by [[:revn :desc]]}) + + row (db/exec-one! pool sql)] + + (when-not row + (ex/raise :type :not-found + :code :file-thumbnail-not-found)) + + (with-meta {:data (:data row) + :props (some-> (:props row) db/decode-transit-pgobject) + :revn (:revn row) + :file-id (:file-id row)} + {:transform-response (rpch/http-cache {:max-age (* 1000 60 60)})}))) ;; --- Helpers diff --git a/frontend/src/app/main/ui/dashboard/grid.cljs b/frontend/src/app/main/ui/dashboard/grid.cljs index 195fcee1d..33dd1159d 100644 --- a/frontend/src/app/main/ui/dashboard/grid.cljs +++ b/frontend/src/app/main/ui/dashboard/grid.cljs @@ -27,8 +27,6 @@ [app.util.timers :as ts] [app.util.webapi :as wapi] [beicon.core :as rx] - [cuerdas.core :as str] - [promesa.core :as p] [rumext.alpha :as mf])) (log/set-level! :warn) @@ -38,57 +36,15 @@ (def ^:const CACHE-NAME "penpot") (def ^:const CACHE-URL "https://penpot.app/cache/") - (defn use-thumbnail-cache "Creates some hooks to handle the files thumbnails cache" [file] - - (let [cache-url (str CACHE-URL (:id file) "/" (:revn file) ".svg") - get-thumbnail - (mf/use-callback - (mf/deps cache-url) - (fn [] - (p/let [response (.match js/caches cache-url)] - (when (some? response) - (p/let [blob (.blob response) - svg-content (.text blob) - headers (.-headers response) - fonts-header (or (.get headers "X-PENPOT-FONTS") "") - fonts (into #{} - (remove #(= "" %)) - (str/split fonts-header ","))] - {:svg svg-content - :fonts fonts}))))) - - cache-thumbnail - (mf/use-callback - (mf/deps cache-url) - (fn [{:keys [svg fonts]}] - (p/let [cache (.open js/caches CACHE-NAME) - blob (js/Blob. #js [svg] #js {:type "image/svg"}) - fonts (str/join "," fonts) - headers (js/Headers. #js {"X-PENPOT-FONTS" fonts}) - response (js/Response. blob #js {:headers headers})] - (.put cache cache-url response))))] - - (mf/use-callback - (mf/deps (:id file) (:revn file)) - (fn [] - (->> (rx/from (get-thumbnail)) - (rx/merge-map - (fn [thumb-data] - (log/debug :msg "retrieve thumbnail" :file (:id file) :revn (:revn file) - :cache (if (some? thumb-data) :hit :miss)) - - (if (some? thumb-data) - (rx/of thumb-data) - (->> (wrk/ask! {:cmd :thumbnails/generate - :file-id (:id file)}) - (rx/tap cache-thumbnail))))) - - ;; If we have a problem we delegate to the thumbnail generation - (rx/catch #(wrk/ask! {:cmd :thumbnails/generate - :file-id (:id file)}))))))) + (mf/use-fn + (mf/deps (:id file) (:revn file)) + (fn [] + (wrk/ask! {:cmd :thumbnails/generate + :revn (:revn file) + :file-id (:id file)})))) (mf/defc grid-item-thumbnail {::mf/wrap [mf/memo]} @@ -100,10 +56,10 @@ (mf/deps file) (fn [] (->> (generate) - (rx/subs (fn [{:keys [svg fonts]}] + (rx/subs (fn [{:keys [data fonts] :as params}] (run! fonts/ensure-loaded! fonts) (when-let [node (mf/ref-val container)] - (dom/set-html! node svg))))))) + (dom/set-html! node data))))))) [:div.grid-item-th {:style {:background-color (get-in file [:data :options :background])} :ref container} diff --git a/frontend/src/app/render.cljs b/frontend/src/app/render.cljs index d119796f8..b40802fb9 100644 --- a/frontend/src/app/render.cljs +++ b/frontend/src/app/render.cljs @@ -29,7 +29,6 @@ :version (:full @cf/version) :public-uri (str cf/public-uri)) - (defn- parse-params [loc] (let [href (unchecked-get loc "href")] diff --git a/frontend/src/app/worker/thumbnails.cljs b/frontend/src/app/worker/thumbnails.cljs index 5524c087d..ddd657643 100644 --- a/frontend/src/app/worker/thumbnails.cljs +++ b/frontend/src/app/worker/thumbnails.cljs @@ -16,6 +16,10 @@ [beicon.core :as rx] [rumext.alpha :as mf])) +(defn- not-found? + [{:keys [type]}] + (= :not-found type)) + (defn- handle-response [response] (cond @@ -29,30 +33,70 @@ (rx/throw {:type :unexpected :code (:error response)}))) -(defn- request-thumbnail - [file-id] - (let [uri (u/join (cfg/get-public-uri) "api/rpc/query/file-data-for-thumbnail") +(defn- request-data-for-thumbnail + [file-id revn] + (let [path "api/rpc/query/file-data-for-thumbnail" params {:file-id file-id + :revn revn :strip-frames-with-thumbnails true} request {:method :get - :uri uri + :uri (u/join (cfg/get-public-uri) path) :credentials "include" :query params}] (->> (http/send! request) (rx/map http/conditional-decode-transit) (rx/mapcat handle-response)))) -(defn render-frame - [data] +(defn- request-thumbnail + [file-id revn] + (let [path "api/rpc/query/file-thumbnail" + params {:file-id file-id + :revn revn} + request {:method :get + :uri (u/join (cfg/get-public-uri) path) + :credentials "include" + :query params}] + (->> (http/send! request) + (rx/map http/conditional-decode-transit) + (rx/mapcat handle-response)))) + +(defn- render-thumbnail + [{:keys [data file-id revn] :as params}] (let [elem (if-let [frame (:thumbnail-frame data)] (mf/element render/frame-svg #js {:objects (:objects data) :frame frame}) (mf/element render/page-svg #js {:data data :width "290" :height "150" :thumbnails? true}))] - (rds/renderToStaticMarkup elem))) + {:data (rds/renderToStaticMarkup elem) + :fonts @fonts/loaded + :file-id file-id + :revn revn})) + +(defn- persist-thumbnail + [{:keys [file-id data revn fonts]}] + (let [path "api/rpc/mutation/upsert-file-thumbnail" + params {:file-id file-id + :revn revn + :props {:fonts fonts} + :data data} + request {:method :post + :uri (u/join (cfg/get-public-uri) path) + :credentials "include" + :body (http/transit-data params)}] + (->> (http/send! request) + (rx/map http/conditional-decode-transit) + (rx/mapcat handle-response) + (rx/map (constantly params))))) (defmethod impl/handler :thumbnails/generate - [{:keys [file-id] :as message}] - (->> (request-thumbnail file-id) - (rx/map - (fn [data] - {:svg (render-frame data) - :fonts @fonts/loaded})))) + [{:keys [file-id revn] :as message}] + (letfn [(on-result [{:keys [data props]}] + {:data data + :fonts (:fonts props)}) + + (on-cache-miss [_] + (->> (request-data-for-thumbnail file-id revn) + (rx/map render-thumbnail) + (rx/mapcat persist-thumbnail)))] + + (->> (request-thumbnail file-id revn) + (rx/catch not-found? on-cache-miss) + (rx/map on-result)))) From 1943877b216d66c6a76e77b74329f44e412e221a Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 22 Mar 2022 17:23:41 +0100 Subject: [PATCH 11/19] :sparkles: Simplify d/group-by impl --- backend/src/app/storage.clj | 4 ++-- common/src/app/common/data.cljc | 17 ++++------------- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/backend/src/app/storage.clj b/backend/src/app/storage.clj index 218835149..10e289b7e 100644 --- a/backend/src/app/storage.clj +++ b/backend/src/app/storage.clj @@ -274,7 +274,7 @@ (let [min-age (db/interval min-age) rows (db/exec! conn [sql:retrieve-deleted-objects-chunk min-age cursor])] [(some-> rows peek :created-at) - (some->> (seq rows) (d/group-by' #(-> % :backend keyword) :id) seq)])) + (some->> (seq rows) (d/group-by #(-> % :backend keyword) :id #{}) seq)])) (retrieve-deleted-objects [conn] (->> (d/iteration (fn [cursor] @@ -383,7 +383,7 @@ (mapv #(d/update-when % :metadata db/decode-transit-pgobject)))] (when (seq rows) [(-> rows peek :created-at) - (d/group-by' get-bucket :id rows)]))) + (d/group-by get-bucket :id #{} rows)]))) (retrieve-touched [conn] (->> (d/iteration (fn [cursor] diff --git a/common/src/app/common/data.cljc b/common/src/app/common/data.cljc index eb74bd3a4..08c952d93 100644 --- a/common/src/app/common/data.cljc +++ b/common/src/app/common/data.cljc @@ -597,19 +597,10 @@ (defn group-by - ([kf coll] (group-by kf identity coll)) - ([kf vf coll] - (let [conj (fnil conj [])] - (reduce (fn [result item] - (update result (kf item) conj (vf item))) - {} - coll)))) - -(defn group-by' - "A variant of group-by that uses a set for collecting results." - ([kf coll] (group-by kf identity coll)) - ([kf vf coll] - (let [conj (fnil conj #{})] + ([kf coll] (group-by kf identity [] coll)) + ([kf vf coll] (group-by kf vf [] coll)) + ([kf vf iv coll] + (let [conj (fnil conj iv)] (reduce (fn [result item] (update result (kf item) conj (vf item))) {} From 9582cc0211540b42ab1f3b7d332350462cd418e3 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 22 Mar 2022 17:24:24 +0100 Subject: [PATCH 12/19] :fire: Remove unused code --- frontend/src/app/main/ui.cljs | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/frontend/src/app/main/ui.cljs b/frontend/src/app/main/ui.cljs index 11a76dbbf..151e14b21 100644 --- a/frontend/src/app/main/ui.cljs +++ b/frontend/src/app/main/ui.cljs @@ -110,19 +110,7 @@ :index index :share-id share-id}])) - :render-object - (do - (let [file-id (uuid (get-in route [:path-params :file-id])) - page-id (uuid (get-in route [:path-params :page-id])) - object-id (uuid (get-in route [:path-params :object-id])) - embed? (= (get-in route [:query-params :embed]) "true") - render-texts (get-in route [:query-params :render-texts])] - [:& render/render-object {:file-id file-id - :page-id page-id - :object-id object-id - :embed? embed? - :render-texts? (and (some? render-texts) (= render-texts "true"))}])) - + ;; TODO: maybe move to `app.render` entrypoint (handled by render.html) :render-sprite (do (let [file-id (uuid (get-in route [:path-params :file-id])) From b87e3c22b3d894042d7b14be9482d4c647a30de9 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 22 Mar 2022 17:24:36 +0100 Subject: [PATCH 13/19] :sparkles: Improve worker error handling Use the global error handlers for handle also the worker errors. --- frontend/src/app/main/worker.cljs | 7 ++----- frontend/src/app/worker.cljs | 11 +++++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/frontend/src/app/main/worker.cljs b/frontend/src/app/main/worker.cljs index 8bd60284f..40e07456b 100644 --- a/frontend/src/app/main/worker.cljs +++ b/frontend/src/app/main/worker.cljs @@ -7,19 +7,16 @@ (ns app.main.worker (:require [app.config :as cfg] + [app.main.errors :as err] [app.util.worker :as uw])) -(defn on-error - [error] - (js/console.error "Error on worker" (pr-str error))) - (defonce instance (atom nil)) (defn init! [] (reset! instance - (uw/init cfg/worker-uri on-error))) + (uw/init cfg/worker-uri err/on-error))) (defn ask! [message] diff --git a/frontend/src/app/worker.cljs b/frontend/src/app/worker.cljs index 52bb72451..a37bce958 100644 --- a/frontend/src/app/worker.cljs +++ b/frontend/src/app/worker.cljs @@ -52,10 +52,13 @@ (reply [result] (post {:payload result})) - (reply-error [err] - (.error js/console "error" (pr-str err)) - (post {:error {:data (ex-data err) - :message (ex-message err)}})) + (reply-error [cause] + (if (map? cause) + (post {:error cause}) + (post {:error {:type :unexpected + :code :unhandled-error-on-worker + :hint (ex-message cause) + :data (ex-data cause)}}))) (reply-completed ([] (reply-completed nil)) From 2832736826d31352d3e8aa44f46fafa438227ea7 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 23 Mar 2022 10:59:20 +0100 Subject: [PATCH 14/19] :tada: Add garbage collection task for file thumbnails And additionally, rename the current task to file-gc to match the real purpose of the task. --- backend/src/app/db.clj | 4 +- backend/src/app/main.clj | 6 +- backend/src/app/rpc/mutations/files.clj | 16 +- backend/src/app/rpc/queries/files.clj | 11 +- backend/src/app/tasks/file_gc.clj | 164 ++++++++++++++++++ backend/src/app/tasks/file_media_gc.clj | 139 --------------- backend/test/app/services_files_test.clj | 205 ++++++++++++++++------- backend/test/app/test_helpers.clj | 19 ++- common/src/app/common/data.cljc | 7 +- 9 files changed, 352 insertions(+), 219 deletions(-) create mode 100644 backend/src/app/tasks/file_gc.clj delete mode 100644 backend/src/app/tasks/file_media_gc.clj diff --git a/backend/src/app/db.clj b/backend/src/app/db.clj index 704c7224a..c874fb9cc 100644 --- a/backend/src/app/db.clj +++ b/backend/src/app/db.clj @@ -233,14 +233,14 @@ ([ds table params opts] (exec-one! ds (sql/insert table params opts) - (assoc opts :return-keys true)))) + (merge {:return-keys true} opts)))) (defn insert-multi! ([ds table cols rows] (insert-multi! ds table cols rows nil)) ([ds table cols rows opts] (exec! ds (sql/insert-multi table cols rows opts) - (assoc opts :return-keys true)))) + (merge {:return-keys true} opts)))) (defn update! ([ds table params where] (update! ds table params where nil)) diff --git a/backend/src/app/main.clj b/backend/src/app/main.clj index 4d785de3d..6a88a10c4 100644 --- a/backend/src/app/main.clj +++ b/backend/src/app/main.clj @@ -189,7 +189,7 @@ :pool (ig/ref :app.db/pool) :entries [{:cron #app/cron "0 0 0 * * ?" ;; daily - :task :file-media-gc} + :task :file-gc} {:cron #app/cron "0 0 * * * ?" ;; hourly :task :file-xlog-gc} @@ -231,7 +231,7 @@ :tasks {:sendmail (ig/ref :app.emails/sendmail-handler) :objects-gc (ig/ref :app.tasks.objects-gc/handler) - :file-media-gc (ig/ref :app.tasks.file-media-gc/handler) + :file-gc (ig/ref :app.tasks.file-gc/handler) :file-xlog-gc (ig/ref :app.tasks.file-xlog-gc/handler) :storage-deleted-gc (ig/ref :app.storage/gc-deleted-task) :storage-touched-gc (ig/ref :app.storage/gc-touched-task) @@ -262,7 +262,7 @@ :storage (ig/ref :app.storage/storage) :max-age cf/deletion-delay} - :app.tasks.file-media-gc/handler + :app.tasks.file-gc/handler {:pool (ig/ref :app.db/pool) :max-age cf/deletion-delay} diff --git a/backend/src/app/rpc/mutations/files.clj b/backend/src/app/rpc/mutations/files.clj index cb6bd5aa3..c3014bfc8 100644 --- a/backend/src/app/rpc/mutations/files.clj +++ b/backend/src/app/rpc/mutations/files.clj @@ -58,8 +58,9 @@ (db/insert! conn :file-profile-rel)))) (defn create-file - [conn {:keys [id name project-id is-shared data deleted-at] + [conn {:keys [id name project-id is-shared data deleted-at revn] :or {is-shared false + revn 0 deleted-at nil} :as params}] (let [id (or id (:id data) (uuid/next)) @@ -68,6 +69,7 @@ {:id id :project-id project-id :name name + :revn revn :is-shared is-shared :data (blob/encode data) :deleted-at deleted-at})] @@ -500,13 +502,13 @@ ;; --- Mutation: Upsert file thumbnail (def sql:upsert-file-thumbnail - "insert into file_thumbnail(file_id, revn, data, props) - values (?, ?, ?, ?) + "insert into file_thumbnail (file_id, revn, data, props) + values (?, ?, ?, ?::jsonb) on conflict(file_id, revn) do - update set data = ?, updated_at=now();") + update set data = ?, props=?, updated_at=now();") -(s/def ::revn ::us/integer) -(s/def ::props (s/map-of ::us/keyword any?)) +(s/def ::revn ::us/integer) +(s/def ::props map?) (s/def ::upsert-file-thumbnail (s/keys :req-un [::profile-id ::file-id ::revn ::data ::props])) @@ -516,5 +518,5 @@ (files/check-edition-permissions! conn profile-id file-id) (let [props (db/tjson (or props {}))] (db/exec-one! conn [sql:upsert-file-thumbnail - file-id revn data props data]) + file-id revn data props data props]) nil))) diff --git a/backend/src/app/rpc/queries/files.clj b/backend/src/app/rpc/queries/files.clj index 935b41c8e..db6dfc45e 100644 --- a/backend/src/app/rpc/queries/files.clj +++ b/backend/src/app/rpc/queries/files.clj @@ -440,7 +440,7 @@ (let [params (cond-> {:file-id file-id} frame-id (assoc :frame-id frame-id)) rows (db/query pool :file-frame-thumbnail params)] - (d/group-by :frame-id :data rows))) + (d/index-by :frame-id :data rows))) ;; --- QUERY: get file thumbnail @@ -465,10 +465,11 @@ (ex/raise :type :not-found :code :file-thumbnail-not-found)) - (with-meta {:data (:data row) - :props (some-> (:props row) db/decode-transit-pgobject) - :revn (:revn row) - :file-id (:file-id row)} + (with-meta + {:data (:data row) + :props (some-> (:props row) db/decode-transit-pgobject) + :revn (:revn row) + :file-id (:file-id row)} {:transform-response (rpch/http-cache {:max-age (* 1000 60 60)})}))) ;; --- Helpers diff --git a/backend/src/app/tasks/file_gc.clj b/backend/src/app/tasks/file_gc.clj new file mode 100644 index 000000000..b8669d96b --- /dev/null +++ b/backend/src/app/tasks/file_gc.clj @@ -0,0 +1,164 @@ +;; 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) UXBOX Labs SL + +(ns app.tasks.file-gc + "A maintenance task that is responsible of: purge unused file media, + clean unused frame thumbnails and remove old file thumbnails. The + file is eligible to be garbage collected after some period of + inactivity (the default threshold is 72h)." + (:require + [app.common.data :as d] + [app.common.logging :as l] + [app.common.pages.helpers :as cph] + [app.common.pages.migrations :as pmg] + [app.db :as db] + [app.util.blob :as blob] + [app.util.time :as dt] + [clojure.spec.alpha :as s] + [integrant.core :as ig])) + +(declare ^:private retrieve-candidates) +(declare ^:private process-file) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; HANDLER +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(s/def ::max-age ::dt/duration) + +(defmethod ig/pre-init-spec ::handler [_] + (s/keys :req-un [::db/pool ::max-age])) + +(defmethod ig/init-key ::handler + [_ {:keys [pool] :as cfg}] + (fn [_] + (db/with-atomic [conn pool] + (let [cfg (assoc cfg :conn conn)] + (loop [total 0 + files (retrieve-candidates cfg)] + (if-let [file (first files)] + (do + (process-file cfg file) + (recur (inc total) + (rest files))) + (do + (l/debug :msg "finished processing files" :processed total) + {:processed total}))))))) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; IMPL +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(def ^:private + sql:retrieve-candidates-chunk + "select f.id, + f.data, + f.revn, + f.modified_at + from file as f + where f.has_media_trimmed is false + and f.modified_at < now() - ?::interval + and f.modified_at < ? + order by f.modified_at desc + limit 1 + for update skip locked") + +(defn- retrieve-candidates + [{:keys [conn max-age] :as cfg}] + (let [interval (db/interval max-age) + + get-chunk + (fn [cursor] + (let [rows (db/exec! conn [sql:retrieve-candidates-chunk interval cursor])] + [(some->> rows peek :modified-at) (seq rows)]))] + + (sequence cat (d/iteration get-chunk + :vf second + :kf first + :initk (dt/now))))) + +(defn- collect-used-media + [data] + (let [xform (comp + (map :objects) + (mapcat vals) + (keep (fn [{:keys [type] :as obj}] + (case type + :path (get-in obj [:fill-image :id]) + :image (get-in obj [:metadata :id]) + nil)))) + pages (concat + (vals (:pages-index data)) + (vals (:components data)))] + (-> #{} + (into xform pages) + (into (keys (:media data)))))) + +(defn- clean-file-media! + "Performs the garbage collection of file media objects." + [conn file-id data] + (let [used (collect-used-media data) + unused (->> (db/query conn :file-media-object {:file-id file-id}) + (remove #(contains? used (:id %))))] + + (doseq [mobj unused] + (l/debug :hint "delete file media object" + :id (:id mobj) + :media-id (:media-id mobj) + :thumbnail-id (:thumbnail-id mobj)) + + ;; NOTE: deleting the file-media-object in the database + ;; automatically marks as touched the referenced storage + ;; objects. The touch mechanism is needed because many files can + ;; point to the same storage objects and we can't just delete + ;; them. + (db/delete! conn :file-media-object {:id (:id mobj)})))) + +(defn- collect-frames + [data] + (let [xform (comp + (map :objects) + (mapcat vals) + (filter cph/frame-shape?) + (keep :id)) + pages (concat + (vals (:pages-index data)) + (vals (:components data)))] + (into #{} xform pages))) + +(defn- clean-file-frame-thumbnails! + [conn file-id data] + (let [sql (str "delete from file_frame_thumbnail " + " where file_id=? and not (frame_id=ANY(?))") + ids (->> (collect-frames data) + (db/create-array conn "uuid")) + res (db/exec-one! conn [sql file-id ids])] + (l/debug :hint "delete frame thumbnails" :total (:next.jdbc/update-count res)))) + +(defn- clean-file-thumbnails! + [conn file-id revn] + (let [sql (str "delete from file_thumbnail " + " where file_id=? and revn < ?") + res (db/exec-one! conn [sql file-id revn])] + (l/debug :hint "delete file thumbnails" :total (:next.jdbc/update-count res)))) + +(defn- process-file + [{:keys [conn] :as cfg} {:keys [id data revn modified-at] :as file}] + (l/debug :hint "processing file" :id id :modified-at modified-at) + + (let [data (-> (blob/decode data) + (assoc :id id) + (pmg/migrate-data))] + + (clean-file-media! conn id data) + (clean-file-frame-thumbnails! conn id data) + (clean-file-thumbnails! conn id revn) + + ;; Mark file as trimmed + (db/update! conn :file + {:has-media-trimmed true} + {:id id}) + nil)) diff --git a/backend/src/app/tasks/file_media_gc.clj b/backend/src/app/tasks/file_media_gc.clj deleted file mode 100644 index 40f55f53f..000000000 --- a/backend/src/app/tasks/file_media_gc.clj +++ /dev/null @@ -1,139 +0,0 @@ -;; 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) UXBOX Labs SL - -(ns app.tasks.file-media-gc - "A maintenance task that is responsible to purge the unused media - objects from files. A file is eligible to be garbage collected - after some period of inactivity (the default threshold is 72h)." - (:require - [app.common.logging :as l] - [app.common.pages.helpers :as cph] - [app.common.pages.migrations :as pmg] - [app.db :as db] - [app.util.blob :as blob] - [app.util.time :as dt] - [clojure.spec.alpha :as s] - [integrant.core :as ig])) - -(declare process-file) -(declare retrieve-candidates) - -(s/def ::max-age ::dt/duration) - -(defmethod ig/pre-init-spec ::handler [_] - (s/keys :req-un [::db/pool ::max-age])) - -(defmethod ig/init-key ::handler - [_ {:keys [pool] :as cfg}] - (fn [_] - (db/with-atomic [conn pool] - (let [cfg (assoc cfg :conn conn)] - (loop [n 0] - (let [files (retrieve-candidates cfg)] - (if (seq files) - (do - (run! (partial process-file cfg) files) - (recur (+ n (count files)))) - (do - (l/debug :msg "finished processing files" :processed n) - {:processed n})))))))) - -(def ^:private - sql:retrieve-candidates-chunk - "select f.id, - f.data, - extract(epoch from (now() - f.modified_at))::bigint as age - from file as f - where f.has_media_trimmed is false - and f.modified_at < now() - ?::interval - order by f.modified_at asc - limit 10 - for update skip locked") - - -(defn- retrieve-candidates - [{:keys [conn max-age] :as cfg}] - (let [interval (db/interval max-age)] - (->> (db/exec! conn [sql:retrieve-candidates-chunk interval]) - (mapv (fn [{:keys [age] :as row}] - (assoc row :age (dt/duration {:seconds age}))))))) - -(def ^:private - collect-media-xf - (comp - (map :objects) - (mapcat vals) - (keep (fn [{:keys [type] :as obj}] - (case type - :path (get-in obj [:fill-image :id]) - :image (get-in obj [:metadata :id]) - nil))))) - -(defn- collect-used-media - [data] - (let [pages (concat - (vals (:pages-index data)) - (vals (:components data)))] - (-> #{} - (into collect-media-xf pages) - (into (keys (:media data)))))) - -(def ^:private - collect-frames-xf - (comp - (map :objects) - (mapcat vals) - (filter cph/frame-shape?) - (keep :id))) - -(defn- collect-frames - [data] - (let [pages (concat - (vals (:pages-index data)) - (vals (:components data)))] - (into #{} collect-frames-xf pages))) - -(defn- process-file - [{:keys [conn] :as cfg} {:keys [id data age] :as file}] - (let [data (-> (blob/decode data) - (assoc :id id) - (pmg/migrate-data))] - - (let [used (collect-used-media data) - unused (->> (db/query conn :file-media-object {:file-id id}) - (remove #(contains? used (:id %))))] - - (l/debug :hint "processing file" - :id id - :age age - :to-delete (count unused)) - - ;; Mark file as trimmed - (db/update! conn :file - {:has-media-trimmed true} - {:id id}) - - (doseq [mobj unused] - (l/debug :hint "deleting media object" - :id (:id mobj) - :media-id (:media-id mobj) - :thumbnail-id (:thumbnail-id mobj)) - - ;; NOTE: deleting the file-media-object in the database - ;; automatically marks as touched the referenced storage - ;; objects. The touch mechanism is needed because many files can - ;; point to the same storage objects and we can't just delete - ;; them. - (db/delete! conn :file-media-object {:id (:id mobj)}))) - - (let [sql (str "delete from file_frame_thumbnail " - " where file_id = ? and not (frame_id = ANY(?))") - ids (->> (collect-frames data) - (db/create-array conn "uuid"))] - ;; delete the unused frame thumbnails - (db/exec! conn [sql (:id file) ids])) - - nil)) diff --git a/backend/test/app/services_files_test.clj b/backend/test/app/services_files_test.clj index 64c69feee..3977a1317 100644 --- a/backend/test/app/services_files_test.clj +++ b/backend/test/app/services_files_test.clj @@ -8,6 +8,7 @@ (:require [app.common.uuid :as uuid] [app.db :as db] + [app.db.sql :as sql] [app.http :as http] [app.storage :as sto] [app.test-helpers :as th] @@ -117,7 +118,7 @@ (t/is (= 0 (count result)))))) )) -(t/deftest file-media-gc-task +(t/deftest file-gc-task (letfn [(create-file-media-object [{:keys [profile-id file-id]}] (let [mfile {:filename "sample.jpg" :path (th/tempfile "app/test_files/sample.jpg") @@ -130,6 +131,9 @@ :name "testfile" :content mfile} out (th/mutation! params)] + + ;; (th/print-result! out) + (t/is (nil? (:error out))) (:result out))) @@ -189,7 +193,7 @@ (t/is (= 0 (:delete res)))) ;; run the task immediately - (let [task (:app.tasks.file-media-gc/handler th/*system*) + (let [task (:app.tasks.file-gc/handler th/*system*) res (task {})] (t/is (= 0 (:processed res)))) @@ -198,7 +202,7 @@ (th/sleep 300) ;; run the task again - (let [task (:app.tasks.file-media-gc/handler th/*system*) + (let [task (:app.tasks.file-gc/handler th/*system*) res (task {})] (t/is (= 1 (:processed res)))) @@ -342,7 +346,7 @@ (t/is (th/ex-info? error)) (t/is (th/ex-of-type? error :not-found)))) -(t/deftest deletion-test +(t/deftest deletion (let [task (:app.tasks.objects-gc/handler th/*system*) profile1 (th/create-profile* 1) file (th/create-file* 1 {:project-id (:default-project-id profile1) @@ -410,71 +414,158 @@ )) (t/deftest query-frame-thumbnails - (let [prof (th/create-profile* 1 {:is-active true}) - file (th/create-file* 1 {:profile-id (:id prof) - :project-id (:default-project-id prof) - :is-shared false}) - data {::th/type :file-frame-thumbnail - :profile-id (:id prof) - :file-id (:id file) - :frame-id (uuid/next)}] - - ;;insert an entry on the database with a test value for the thumbnail of this frame - (db/exec-one! th/*pool* - ["insert into file_frame_thumbnail(file_id, frame_id, data) values (?, ?, ?)" - (:file-id data) (:frame-id data) "testvalue"]) - - (let [out (th/query! data)] - (t/is (nil? (:error out))) - (let [result (:result out)] - (t/is (= 1 (count result))) - (t/is (= "testvalue" (:data result))))))) - -(t/deftest insert-frame-thumbnails - (let [prof (th/create-profile* 1 {:is-active true}) - file (th/create-file* 1 {:profile-id (:id prof) - :project-id (:default-project-id prof) - :is-shared false}) - data {::th/type :upsert-frame-thumbnail - :profile-id (:id prof) - :file-id (:id file) - :frame-id (uuid/next) - :data "test insert new value"} - out (th/mutation! data)] - - (t/is (nil? (:error out))) - (t/is (nil? (:result out))) - - ;;retrieve the value from the database and check its content - (let [result (db/exec-one! - th/*pool* - ["select data from file_frame_thumbnail where file_id = ? and frame_id = ?" - (:file-id data) (:frame-id data)])] - (t/is (= "test insert new value" (:data result)))))) - -(t/deftest frame-thumbnails (let [prof (th/create-profile* 1 {:is-active true}) file (th/create-file* 1 {:profile-id (:id prof) :project-id (:default-project-id prof) :is-shared false}) - data {::th/type :upsert-frame-thumbnail + data {::th/type :file-frame-thumbnails + :profile-id (:id prof) + :file-id (:id file) + :frame-id (uuid/next)}] + + ;; insert an entry on the database with a test value for the thumbnail of this frame + (th/db-insert! :file-frame-thumbnail + {:file-id (:file-id data) + :frame-id (:frame-id data) + :data "testvalue"}) + + (let [{:keys [result error] :as out} (th/query! data)] + ;; (th/print-result! out) + (t/is (nil? error)) + (t/is (= 1 (count result))) + (t/is (= "testvalue" (get result (:frame-id data))))))) + +(t/deftest insert-frame-thumbnails + (let [prof (th/create-profile* 1 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + data {::th/type :upsert-file-frame-thumbnail + :profile-id (:id prof) + :file-id (:id file) + :frame-id (uuid/next) + :data "test insert new value"}] + + (let [out (th/mutation! data)] + (t/is (nil? (:error out))) + (t/is (nil? (:result out))) + (let [[result] (th/db-query :file-frame-thumbnail + {:file-id (:file-id data) + :frame-id (:frame-id data)})] + (t/is (= "test insert new value" (:data result))))))) + +(t/deftest upsert-frame-thumbnails + (let [prof (th/create-profile* 1 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + data {::th/type :upsert-file-frame-thumbnail :profile-id (:id prof) :file-id (:id file) :frame-id (uuid/next) :data "updated value"}] - ;;insert an entry on the database with and old value for the thumbnail of this frame - (db/exec-one! th/*pool* - ["insert into file_frame_thumbnail(file_id, frame_id, data) values (?, ?, ?)" - (:file-id data) (:frame-id data) "old value"]) + ;; insert an entry on the database with and old value for the thumbnail of this frame + (th/db-insert! :file-frame-thumbnail + {:file-id (:file-id data) + :frame-id (:frame-id data) + :data "old value"}) (let [out (th/mutation! data)] + ;; (th/print-result! out) + (t/is (nil? (:error out))) (t/is (nil? (:result out))) - ;;retrieve the value from the database and check its content - (let [result (db/exec-one! - th/*pool* - ["select data from file_frame_thumbnail where file_id = ? and frame_id = ?" - (:file-id data) (:frame-id data)])] + ;; retrieve the value from the database and check its content + (let [[result] (th/db-query :file-frame-thumbnail + {:file-id (:file-id data) + :frame-id (:frame-id data)})] (t/is (= "updated value" (:data result))))))) + + +(t/deftest file-thumbnail-ops + (let [prof (th/create-profile* 1 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :revn 2 + :is-shared false}) + data {::th/type :file-thumbnail + :profile-id (:id prof) + :file-id (:id file)}] + + (t/testing "query a thumbnail with single revn" + + ;; insert an entry on the database with a test value for the thumbnail of this frame + (th/db-insert! :file-thumbnail + {:file-id (:file-id data) + :revn 1 + :data "testvalue1"}) + + (let [{:keys [result error] :as out} (th/query! data)] + ;; (th/print-result! out) + (t/is (nil? error)) + (t/is (= 4 (count result))) + (t/is (= "testvalue1" (:data result))) + (t/is (= 1 (:revn result))))) + + (t/testing "query thumbnail with two revisions" + ;; insert an entry on the database with a test value for the thumbnail of this frame + (th/db-insert! :file-thumbnail + {:file-id (:file-id data) + :revn 2 + :data "testvalue2"}) + + (let [{:keys [result error] :as out} (th/query! data)] + ;; (th/print-result! out) + (t/is (nil? error)) + (t/is (= 4 (count result))) + (t/is (= "testvalue2" (:data result))) + (t/is (= 2 (:revn result)))) + + ;; Then query the specific revn + (let [{:keys [result error] :as out} (th/query! (assoc data :revn 1))] + ;; (th/print-result! out) + (t/is (nil? error)) + (t/is (= 4 (count result))) + (t/is (= "testvalue1" (:data result))) + (t/is (= 1 (:revn result))))) + + (t/testing "upsert file-thumbnail" + (let [data {::th/type :upsert-file-thumbnail + :profile-id (:id prof) + :file-id (:id file) + :data "foobar" + :props {:baz 1} + :revn 2} + {:keys [result error] :as out} (th/mutation! data)] + ;; (th/print-result! out) + (t/is (nil? error)) + (t/is (nil? result)))) + + (t/testing "query last result" + (let [{:keys [result error] :as out} (th/query! data)] + ;; (th/print-result! out) + (t/is (nil? error)) + (t/is (= 4 (count result))) + (t/is (= "foobar" (:data result))) + (t/is (= {:baz 1} (:props result))) + (t/is (= 2 (:revn result))))) + + (t/testing "gc task" + ;; make the file eligible for GC waiting 300ms (configured + ;; timeout for testing) + (th/sleep 300) + + ;; run the task again + (let [task (:app.tasks.file-gc/handler th/*system*) + res (task {})] + (t/is (= 1 (:processed res)))) + + ;; Then query the specific revn + (let [{:keys [result error] :as out} (th/query! (assoc data :revn 1))] + (t/is (= :not-found (th/ex-type error))) + (t/is (= :file-thumbnail-not-found (th/ex-code error))))) + )) + + diff --git a/backend/test/app/test_helpers.clj b/backend/test/app/test_helpers.clj index e626f4dd1..5699424a6 100644 --- a/backend/test/app/test_helpers.clj +++ b/backend/test/app/test_helpers.clj @@ -73,7 +73,7 @@ :app.worker/cron :app.worker/worker) (d/deep-merge - {:app.tasks.file-media-gc/handler {:max-age (dt/duration 300)}})) + {:app.tasks.file-gc/handler {:max-age (dt/duration 300)}})) _ (ig/load-namespaces config) system (-> (ig/prep config) (ig/init))] @@ -285,7 +285,8 @@ (let [data (ex-data error)] (cond (= :spec-validation (:code data)) - (expound/printer (:data data)) + (println + (us/pretty-explain data)) (= :service-error (:type data)) (print-error! (.getCause ^Throwable error)) @@ -302,7 +303,7 @@ (println "====> END ERROR")) (do (println "====> START RESPONSE") - (prn result) + (fipp.edn/pprint result) (println "====> END RESPONSE")))) (defn exception? @@ -374,3 +375,15 @@ (.readLine cnsl) nil)) +(defn db-exec! + [sql] + (db/exec! *pool* sql)) + +(defn db-insert! + [& params] + (apply db/insert! *pool* params)) + +(defn db-query + [& params] + (apply db/query *pool* params)) + diff --git a/common/src/app/common/data.cljc b/common/src/app/common/data.cljc index 08c952d93..77f39b694 100644 --- a/common/src/app/common/data.cljc +++ b/common/src/app/common/data.cljc @@ -128,9 +128,10 @@ (defn index-by "Return a indexed map of the collection keyed by the result of executing the getter over each element of the collection." - [getter coll] - (persistent! - (reduce #(assoc! %1 (getter %2) %2) (transient {}) coll))) + ([kf coll] (index-by kf identity coll)) + ([kf vf coll] + (persistent! + (reduce #(assoc! %1 (kf %2) (vf %2)) (transient {}) coll)))) (defn index-of-pred [coll pred] From ec5a4d09b81c1f8b0c790a607bb1e94681038fa1 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 23 Mar 2022 23:05:04 +0100 Subject: [PATCH 15/19] :bug: Fix possible issue that causes exception on node tests --- frontend/src/app/main/errors.cljs | 5 +++-- frontend/src/app/util/globals.js | 4 ++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index 12daf2712..9e8ad584b 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -13,6 +13,7 @@ [app.main.data.messages :as msg] [app.main.data.users :as du] [app.main.store :as st] + [app.util.globals :as glob] [app.util.i18n :refer [tr]] [app.util.router :as rt] [app.util.timers :as ts] @@ -174,6 +175,6 @@ (.preventDefault ^js event) (some-> (unchecked-get event "error") (on-unhandled-error)))] - (.addEventListener js/window "error" on-error) + (.addEventListener glob/window "error" on-error) (fn [] - (.removeEventListener js/window "error" on-error)))) + (.removeEventListener glob/window "error" on-error)))) diff --git a/frontend/src/app/util/globals.js b/frontend/src/app/util/globals.js index 502f70ddf..12841bec7 100644 --- a/frontend/src/app/util/globals.js +++ b/frontend/src/app/util/globals.js @@ -28,6 +28,10 @@ goog.scope(function() { addListener(...args) { }, removeListener(...args) { + }, + addEventListener(...args) { + }, + removeEventListener(...args) { } } } From 9abf4b126c29d36765284c51561e4d52c6119fb5 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 24 Mar 2022 12:25:07 +0100 Subject: [PATCH 16/19] :sparkles: Improve error handling --- backend/deps.edn | 2 +- backend/src/app/http.clj | 61 ++++++++++++----------- backend/src/app/http/errors.clj | 75 +++++++++++++++++------------ backend/src/app/http/middleware.clj | 17 +++---- frontend/src/app/main/errors.cljs | 29 +++++------ 5 files changed, 96 insertions(+), 88 deletions(-) diff --git a/backend/deps.edn b/backend/deps.edn index 2f96d95ba..70251e7fa 100644 --- a/backend/deps.edn +++ b/backend/deps.edn @@ -19,7 +19,7 @@ io.lettuce/lettuce-core {:mvn/version "6.1.6.RELEASE"} java-http-clj/java-http-clj {:mvn/version "0.4.3"} - funcool/yetti {:git/tag "v8.0" :git/sha "ea7162d" + funcool/yetti {:git/tag "v9.0" :git/sha "e09e46c" :git/url "https://github.com/funcool/yetti.git" :exclusions [org.slf4j/slf4j-api]} diff --git a/backend/src/app/http.clj b/backend/src/app/http.clj index 010bc941f..38c9340d0 100644 --- a/backend/src/app/http.clj +++ b/backend/src/app/http.clj @@ -8,6 +8,7 @@ (:require [app.common.data :as d] [app.common.logging :as l] + [app.common.transit :as t] [app.http.doc :as doc] [app.http.errors :as errors] [app.http.middleware :as middleware] @@ -67,7 +68,7 @@ :xnio/dispatch (:executor cfg) :ring/async true} handler (if (some? router) - (wrap-router cfg router) + (wrap-router router) handler) server (yt/server handler (d/without-nils options))] (assoc cfg :server (yt/start! server)))) @@ -81,30 +82,32 @@ [_ respond _] (respond (yrs/response 404))) -(defn- ring-handler - [router] - (fn [request respond raise] - (if-let [match (r/match-by-path router (yrq/path request))] - (let [params (:path-params match) - result (:result match) - handler (or (:handler result) not-found-handler) - request (-> request - (assoc :path-params params) - (update :params merge params))] - (handler request respond raise)) - (not-found-handler request respond raise)))) - (defn- wrap-router - [_ router] - (let [handler (ring-handler router)] + [router] + (letfn [(handler [request respond raise] + (if-let [match (r/match-by-path router (yrq/path request))] + (let [params (:path-params match) + result (:result match) + handler (or (:handler result) not-found-handler) + request (-> request + (assoc :path-params params) + (update :params merge params))] + (handler request respond raise)) + (not-found-handler request respond raise))) + + (on-error [cause request respond] + (let [{:keys [body] :as response} (errors/handle cause request)] + (respond + (cond-> response + (map? body) + (-> (update :headers assoc "content-type" "application/transit+json") + (assoc :body (t/encode-str body {:type :json-verbose})))))))] + (fn [request respond _] - (handler request respond - (fn [cause] - (l/error :hint "unexpected error processing request" - ::l/context (errors/get-context request) - :query-string (yrq/query request) - :cause cause) - (respond (yrs/response 500 "internal server error"))))))) + (try + (handler request respond #(on-error % request respond)) + (catch Throwable cause + (on-error cause request respond)))))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; HTTP ROUTER @@ -130,6 +133,8 @@ (rr/router [["" {:middleware [[middleware/server-timing] [middleware/format-response] + [middleware/params] + [middleware/parse-request] [middleware/errors errors/handle] [middleware/restrict-methods]]} ["/metrics" {:handler (:handler metrics)}] @@ -138,9 +143,7 @@ ["/by-file-media-id/:id" {:handler (:file-objects-handler assets)}] ["/by-file-media-id/:id/thumbnail" {:handler (:file-thumbnails-handler assets)}]] - ["/dbg" {:middleware [[middleware/params] - [middleware/parse-request] - (:middleware session)]} + ["/dbg" {:middleware [(:middleware session)]} ["" {:handler (:index debug)}] ["/error-by-id/:id" {:handler (:retrieve-error debug)}] ["/error/:id" {:handler (:retrieve-error debug)}] @@ -152,15 +155,11 @@ ["/sns" {:handler (:awsns-handler cfg) :allowed-methods #{:post}}]] - ["/ws/notifications" {:middleware [[middleware/params] - [middleware/parse-request] - (:middleware session)] + ["/ws/notifications" {:middleware [(:middleware session)] :handler ws :allowed-methods #{:get}}] ["/api" {:middleware [[middleware/cors] - [middleware/params] - [middleware/parse-request] (:middleware session)]} ["/health" {:handler (:health-check debug)}] ["/_doc" {:handler (doc/handler rpc) diff --git a/backend/src/app/http/errors.clj b/backend/src/app/http/errors.clj index 95d4f9819..bd40ded1d 100644 --- a/backend/src/app/http/errors.clj +++ b/backend/src/app/http/errors.clj @@ -24,8 +24,8 @@ (defn get-context [request] (merge - {:path (:uri request) - :method (:request-method request) + {:path (:path request) + :method (:method request) :params (:params request) :ip-addr (parse-client-ip request) :profile-id (:profile-id request)} @@ -51,9 +51,10 @@ [err _] (let [data (ex-data err) explain (us/pretty-explain data)] - (yrs/response 400 (-> data - (dissoc ::s/problems ::s/value) - (cond-> explain (assoc :explain explain)))))) + (yrs/response :status 400 + :body (-> data + (dissoc ::s/problems ::s/value) + (cond-> explain (assoc :explain explain)))))) (defmethod handle-exception :assertion [error request] @@ -73,26 +74,6 @@ [err _] (yrs/response 404 (ex-data err))) -(defmethod handle-exception :default - [error request] - (let [edata (ex-data error)] - ;; NOTE: this is a special case for the idle-in-transaction error; - ;; when it happens, the connection is automatically closed and - ;; next-jdbc combines the two errors in a single ex-info. We only - ;; need the :handling error, because the :rollback error will be - ;; always "connection closed". - (if (and (ex/exception? (:rollback edata)) - (ex/exception? (:handling edata))) - (handle-exception (:handling edata) request) - (do - (l/error ::l/raw (ex-message error) - ::l/context (get-context request) - :cause error) - (yrs/response 500 {:type :server-error - :code :unexpected - :hint (ex-message error) - :data edata}))))) - (defmethod handle-exception org.postgresql.util.PSQLException [error request] (let [state (.getSQLState ^java.sql.SQLException error)] @@ -101,24 +82,56 @@ :cause error) (cond (= state "57014") - (yrs/response 504 {:type :server-timeout + (yrs/response 504 {:type :server-error :code :statement-timeout :hint (ex-message error)}) (= state "25P03") - (yrs/response 504 {:type :server-timeout + (yrs/response 504 {:type :server-error :code :idle-in-transaction-timeout :hint (ex-message error)}) :else (yrs/response 500 {:type :server-error - :code :psql-exception + :code :unexpected :hint (ex-message error) :state state})))) +(defmethod handle-exception :default + [error request] + (let [edata (ex-data error)] + (cond + ;; This means that exception is not a controlled exception. + (nil? edata) + (do + (l/error ::l/raw (ex-message error) + ::l/context (get-context request) + :cause error) + (yrs/response 500 {:type :server-error + :code :unexpected + :hint (ex-message error)})) + + ;; This is a special case for the idle-in-transaction error; + ;; when it happens, the connection is automatically closed and + ;; next-jdbc combines the two errors in a single ex-info. We + ;; only need the :handling error, because the :rollback error + ;; will be always "connection closed". + (and (ex/exception? (:rollback edata)) + (ex/exception? (:handling edata))) + (handle-exception (:handling edata) request) + + :else + (do + (l/error ::l/raw (ex-message error) + ::l/context (get-context request) + :cause error) + (yrs/response 500 {:type :server-error + :code :unhandled + :hint (ex-message error) + :data edata}))))) (defn handle - [error req] + [error request] (if (or (instance? java.util.concurrent.CompletionException error) (instance? java.util.concurrent.ExecutionException error)) - (handle-exception (.getCause ^Throwable error) req) - (handle-exception error req))) + (handle-exception (.getCause ^Throwable error) request) + (handle-exception error request))) diff --git a/backend/src/app/http/middleware.clj b/backend/src/app/http/middleware.clj index df3d31c76..0464e0037 100644 --- a/backend/src/app/http/middleware.clj +++ b/backend/src/app/http/middleware.clj @@ -6,6 +6,7 @@ (ns app.http.middleware (:require + [app.common.exceptions :as ex] [app.common.logging :as l] [app.common.transit :as t] [app.config :as cf] @@ -45,23 +46,17 @@ (update :params merge params)))) :else - request))) - - (handle-exception [cause] - (let [data {:type :validation - :code :unable-to-parse-request-body - :hint "malformed params"}] - (l/error :hint (ex-message cause) :cause cause) - (yrs/response :status 400 - :headers {"content-type" "application/transit+json"} - :body (t/encode-str data {:type :json-verbose}))))] + request)))] (fn [request respond raise] (try (let [request (process-request request)] (handler request respond raise)) (catch Exception cause - (respond (handle-exception cause))))))) + (raise (ex/error :type :validation + :code :malformed-params + :hint (ex-message cause) + :cause cause))))))) (def parse-request {:name ::parse-request diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index 9e8ad584b..d94e206b7 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -52,18 +52,6 @@ (st/emit! (du/logout {:capture-redirect true})) (ts/schedule 500 (st/emitf (msg/warn msg))))) - -;; That are special case server-errors that should be treated -;; differently. -(derive :not-found ::exceptional-state) -(derive :bad-gateway ::exceptional-state) -(derive :service-unavailable ::exceptional-state) - -(defmethod ptk/handle-error ::exceptional-state - [error] - (ts/schedule - (st/emitf (rt/assign-exception error)))) - ;; Error that happens on an active business model validation does not ;; passes an validation (example: profile can't leave a team). From ;; the user perspective a error flash message should be visualized but @@ -134,9 +122,22 @@ (js/console.error (with-out-str (expound/printer error))) (js/console.groupEnd message))) +;; That are special case server-errors that should be treated +;; differently. + +(derive :not-found ::exceptional-state) +(derive :bad-gateway ::exceptional-state) +(derive :service-unavailable ::exceptional-state) + +(defmethod ptk/handle-error ::exceptional-state + [error] + (ts/schedule + (st/emitf (rt/assign-exception error)))) + ;; This happens when the backed server fails to process the ;; request. This can be caused by an internal assertion or any other ;; uncontrolled error. + (defmethod ptk/handle-error :server-error [{:keys [data hint] :as error}] (let [hint (or hint (:hint data) (:message data)) @@ -146,8 +147,8 @@ (ts/schedule #(st/emit! (msg/show {:content "Something wrong has happened (on backend)." - :type :error - :timeout 3000}))) + :type :error + :timeout 3000}))) (js/console.group msg) (js/console.info info) From 36027583cd98b92e208c02f92c5a0d1369b4577f Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 24 Mar 2022 12:29:08 +0100 Subject: [PATCH 17/19] :paperclip: Minor change on create team instrumentation --- backend/src/app/rpc/mutations/teams.clj | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/src/app/rpc/mutations/teams.clj b/backend/src/app/rpc/mutations/teams.clj index 494204ca8..ce5626d16 100644 --- a/backend/src/app/rpc/mutations/teams.clj +++ b/backend/src/app/rpc/mutations/teams.clj @@ -425,7 +425,6 @@ :email email :hint "looks like the email you invite has been repeatedly reported as spam or permanent bounce")) - (db/exec-one! conn [sql:upsert-team-invitation (:id team) (str/lower email) (name role) token-exp (name role) token-exp]) @@ -438,7 +437,6 @@ :token itoken :extra-data ptoken}))) - ;; --- Mutation: Create Team & Invite Members (s/def ::emails ::us/set-of-emails) @@ -463,7 +461,9 @@ :role role))) (with-meta team - {:before-complete + {::audit/props {:invitations (count emails)} + + :before-complete #(audit-fn :cmd :submit :type "mutation" :name "invite-team-member" From 9ce0497f00d6c12fe2b38dfc59d5798a34ac3f20 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 25 Mar 2022 17:08:19 +0100 Subject: [PATCH 18/19] :sparkles: Add proper error handlings on http middleware --- backend/src/app/http/middleware.clj | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/backend/src/app/http/middleware.clj b/backend/src/app/http/middleware.clj index 0464e0037..125d75e5e 100644 --- a/backend/src/app/http/middleware.clj +++ b/backend/src/app/http/middleware.clj @@ -49,14 +49,14 @@ request)))] (fn [request respond raise] - (try - (let [request (process-request request)] - (handler request respond raise)) - (catch Exception cause - (raise (ex/error :type :validation - :code :malformed-params - :hint (ex-message cause) - :cause cause))))))) + (when-let [request (try + (process-request request) + (catch Exception cause + (raise (ex/error :type :validation + :code :malformed-params + :hint (ex-message cause) + :cause cause))))] + (handler request respond raise))))) (def parse-request {:name ::parse-request From 3301148da6dc67fcb58d51be16ff3125c8cd6a26 Mon Sep 17 00:00:00 2001 From: Eva Date: Mon, 28 Mar 2022 17:06:37 +0200 Subject: [PATCH 19/19] :bug: Fix comments modal remains open on page change --- CHANGES.md | 1 + frontend/src/app/main/ui/workspace/sidebar/sitemap.cljs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 12838f2ac..98ff1b78a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -42,6 +42,7 @@ ### :bug: Bugs fixed +- Fix comments modal when changing pages [Taiga #2597](https://tree.taiga.io/project/penpot/issue/2508) - Copy paste inside a text layer leaves pasted text transparent [Taiga #3096](https://tree.taiga.io/project/penpot/issue/3096) - On dashboard enter on empty search refresh the page [Taiga #2597](https://tree.taiga.io/project/penpot/issue/2597) - Pencil cursor changes when activated [Taiga #2276](https://tree.taiga.io/project/penpot/issue/2276) diff --git a/frontend/src/app/main/ui/workspace/sidebar/sitemap.cljs b/frontend/src/app/main/ui/workspace/sidebar/sitemap.cljs index 942c75ea5..e5ed8cfc2 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/sitemap.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/sitemap.cljs @@ -33,7 +33,7 @@ state (mf/use-state {:menu-open false}) delete-fn (mf/use-callback (mf/deps id) #(st/emit! (dw/delete-page id))) - navigate-fn (mf/use-callback (mf/deps id) #(st/emit! (dw/go-to-page id))) + navigate-fn (mf/use-callback (mf/deps id) #(st/emit! :interrupt (dw/go-to-page id))) on-context-menu (mf/use-callback