From b3d70f255654cb2841308d4ee5eae7d23f5da752 Mon Sep 17 00:00:00 2001
From: Andrey Antukh <niwi@niwi.nz>
Date: Wed, 23 Mar 2022 13:19:35 +0100
Subject: [PATCH] :bug: Fix many issues related to exportation process

---
 exporter/src/app/browser.cljs           | 28 +++++++++++++++-----
 exporter/src/app/config.cljs            | 15 +++++------
 exporter/src/app/handlers.cljs          | 11 +++++---
 exporter/src/app/http.cljs              |  6 ++---
 exporter/src/app/renderer/bitmap.cljs   | 10 +-------
 frontend/src/app/main/data/exports.cljs | 34 +++++++++++++++----------
 frontend/src/app/main/repo.cljs         |  2 +-
 frontend/src/app/main/ui/export.cljs    | 20 ++++++++++++---
 8 files changed, 77 insertions(+), 49 deletions(-)

diff --git a/exporter/src/app/browser.cljs b/exporter/src/app/browser.cljs
index 3f3f3e960..612023fba 100644
--- a/exporter/src/app/browser.cljs
+++ b/exporter/src/app/browser.cljs
@@ -7,8 +7,10 @@
 (ns app.browser
   (:require
    ["generic-pool" :as gp]
+   ["generic-pool/lib/errors" :as gpe]
    ["playwright" :as pw]
    [app.common.data :as d]
+   [app.common.exceptions :as ex]
    [app.common.logging :as l]
    [app.common.uuid :as uuid]
    [app.config :as cf]
@@ -17,6 +19,8 @@
 
 (l/set-level! :trace)
 
+(def TimeoutError gpe/TimeoutError)
+
 ;; --- BROWSER API
 
 (def default-timeout 30000)
@@ -117,21 +121,23 @@
 
 (defn init
   []
-  (l/info :msg "initializing browser pool")
-  (let [opts #js {:max (cf/get :exporter-browser-pool-max 5)
-                  :min (cf/get :exporter-browser-pool-min 0)
+  (let [opts #js {:max (cf/get :browser-pool-max 5)
+                  :min (cf/get :browser-pool-min 0)
                   :testOnBorrow true
                   :evictionRunIntervalMillis 5000
                   :numTestsPerEvictionRun 5
-                  :acquireTimeoutMillis 120000 ; 2min
+                  ;; :acquireTimeoutMillis 120000 ; 2min
+                  :acquireTimeoutMillis 10000 ; 10 s
                   :idleTimeoutMillis 10000}]
+
+    (l/info :hint "initializing browser pool" :opts opts)
     (reset! pool (gp/createPool browser-pool-factory opts))
     (p/resolved nil)))
 
 (defn stop
   []
   (when-let [pool (deref pool)]
-    (l/info :msg "finalizing browser pool")
+    (l/info :hint "finalizing browser pool")
     (p/do!
      (.drain ^js pool)
      (.clear ^js pool))))
@@ -140,6 +146,15 @@
   [p]
   (p/handle p (constantly nil)))
 
+(defn- translate-browser-errors
+  [cause]
+  (if (instance? TimeoutError cause)
+    (ex/raise :type :internal
+              :code :timeout
+              :hint (ex-message cause)
+              :cause cause)
+    (p/rejected cause)))
+
 (defn exec!
   [config handle]
   (letfn [(handle-browser [browser]
@@ -166,5 +181,4 @@
     (when-let [pool (deref pool)]
       (-> (p/do! (.acquire ^js pool))
           (p/then (partial on-acquire pool))
-          (p/catch (fn [cause]
-                     (p/rejected cause)))))))
+          (p/catch translate-browser-errors)))))
diff --git a/exporter/src/app/config.cljs b/exporter/src/app/config.cljs
index cb59fb6ab..bdbccb122 100644
--- a/exporter/src/app/config.cljs
+++ b/exporter/src/app/config.cljs
@@ -26,17 +26,16 @@
    :http-server-port 6061
    :http-server-host "localhost"
    :redis-uri "redis://redis/0"
-   :exporter-domain-whitelist #{"localhost2:3449"}})
+   :exporter-domain-whitelist #{"localhost:3449"}})
 
 (s/def ::http-server-port ::us/integer)
 (s/def ::http-server-host ::us/string)
 (s/def ::public-uri ::us/uri)
 (s/def ::tenant ::us/string)
 (s/def ::host ::us/string)
-
-(s/def ::exporter-domain-whitelist ::us/set-of-str)
-(s/def ::exporter-browser-pool-max ::us/integer)
-(s/def ::exporter-browser-pool-min ::us/integer)
+(s/def ::domain-white-list ::us/set-of-str)
+(s/def ::browser-pool-max ::us/integer)
+(s/def ::browser-pool-min ::us/integer)
 
 (s/def ::config
   (s/keys :opt-un [::public-uri
@@ -44,9 +43,9 @@
                    ::tenant
                    ::http-server-port
                    ::http-server-host
-                   ::exporter-browser-pool-max
-                   ::exporter-browser-pool-min
-                   ::exporter-domain-whitelist]))
+                   ::browser-pool-max
+                   ::browser-pool-min
+                   ::domain-whitelist]))
 
 (defn- read-env
   [prefix]
diff --git a/exporter/src/app/handlers.cljs b/exporter/src/app/handlers.cljs
index 22e15bf1e..ce4870ecc 100644
--- a/exporter/src/app/handlers.cljs
+++ b/exporter/src/app/handlers.cljs
@@ -21,7 +21,7 @@
    [promesa.core :as p]
    [reitit.core :as r]))
 
-(l/set-level! :info)
+(l/set-level! :debug)
 
 (defn on-error
   [error exchange]
@@ -52,8 +52,10 @@
           (assoc :response/headers {"content-type" "application/transit+json"}))
 
       :else
-      (do
-        (l/error :msg "Unexpected error" :cause error)
+      (let [data {:type :server-error
+                  :hint (ex-message error)
+                  :data data}]
+        (l/error :hint "unexpected internal error" :cause error)
         (-> exchange
             (assoc :response/status 500)
             (assoc :response/body (t/encode data))
@@ -77,7 +79,7 @@
 
 (defn validate-uri!
   [uri]
-  (let [white-list (cf/get :exporter-domain-whitelist #{})
+  (let [white-list (cf/get :domain-white-list #{})
         default    (cf/get :public-uri)]
     (when-not (or (contains? white-list (u/get-domain uri))
                   (= (u/get-domain default) (u/get-domain uri)))
@@ -88,6 +90,7 @@
 (defn handler
   [{:keys [:request/params] :as exchange}]
   (let [{:keys [cmd uri] :as params} (us/conform ::params params)]
+    (l/debug :hint "process-request" :cmd cmd)
     (some-> uri validate-uri!)
     (case cmd
       :get-resource  (resources/handler exchange)
diff --git a/exporter/src/app/http.cljs b/exporter/src/app/http.cljs
index 846a8e28d..1e3512ab7 100644
--- a/exporter/src/app/http.cljs
+++ b/exporter/src/app/http.cljs
@@ -159,10 +159,10 @@
         server  (create-server handler)
         port    (cf/get :http-server-port 6061)]
     (.listen server port)
-    (l/info :msg "welcome to penpot"
+    (l/info :hint "welcome to penpot"
             :module "exporter"
             :version (:full @cf/version))
-    (l/info :msg "starting http server" :port port)
+    (l/info :hint "starting http server" :port port)
     (reset! instance server)))
 
 (defn stop
@@ -170,6 +170,6 @@
   (if-let [server @instance]
     (p/create (fn [resolve]
                 (.close server (fn []
-                                 (l/info :msg "shutdown http server")
+                                 (l/info :hint "shutdown http server")
                                  (resolve)))))
     (p/resolved nil)))
diff --git a/exporter/src/app/renderer/bitmap.cljs b/exporter/src/app/renderer/bitmap.cljs
index 40ae11847..67f4b965f 100644
--- a/exporter/src/app/renderer/bitmap.cljs
+++ b/exporter/src/app/renderer/bitmap.cljs
@@ -58,23 +58,15 @@
 (s/def ::object-id ::us/uuid)
 (s/def ::scale ::us/number)
 (s/def ::token ::us/string)
-(s/def ::origin ::us/string)
 (s/def ::uri ::us/uri)
 
 (s/def ::params
   (s/keys :req-un [::name ::suffix ::type ::object-id ::page-id ::scale ::token ::file-id]
-          :opt-un [::origin ::uri]))
+          :opt-un [::uri]))
 
 (defn render
   [params]
   (us/verify ::params params)
-  (when (and (:origin params)
-             (not (contains? (cf/get :origin-white-list) (:origin params))))
-    (ex/raise :type :validation
-              :code :invalid-origin
-              :hint "invalid origin"
-              :origin (:origin params)))
-
   (p/let [content (screenshot-object params)]
     {:data content
      :name (str (:name params)
diff --git a/frontend/src/app/main/data/exports.cljs b/frontend/src/app/main/data/exports.cljs
index 4451cfd4d..d9d787dc4 100644
--- a/frontend/src/app/main/data/exports.cljs
+++ b/frontend/src/app/main/data/exports.cljs
@@ -7,6 +7,7 @@
 (ns app.main.data.exports
   (:require
    [app.common.data.macros :as dm]
+   [app.common.uuid :as uuid]
    [app.main.data.modal :as modal]
    [app.main.data.workspace.persistence :as dwp]
    [app.main.data.workspace.state-helpers :as wsh]
@@ -117,7 +118,7 @@
                              :filename filename})))))))
 
 (defn- initialize-export-status
-  [exports filename resource-id query]
+  [exports filename resource-id query-name]
   (ptk/reify ::initialize-export-status
     ptk/UpdateEvent
     (update [_ state]
@@ -131,7 +132,7 @@
                             :exports exports
                             :filename filename
                             :last-update (dt/now)
-                            :query query}))))
+                            :query-name query-name}))))
 
 (defn- update-export-status
   [{:keys [progress status resource-id name] :as data}]
@@ -163,7 +164,7 @@
   (ptk/reify ::request-simple-export
     ptk/UpdateEvent
     (update [_ state]
-      (update state :export assoc :in-progress true))
+      (update state :export assoc :in-progress true :id uuid/zero))
 
     ptk/WatchEvent
     (watch [_ state _]
@@ -175,11 +176,17 @@
          (->> (rp/query! :export-shapes-simple params)
               (rx/map (fn [data]
                         (dom/trigger-download filename data)
-                        (fn [state]
-                          (dissoc state :export))))))))))
+                        (clear-export-state uuid/zero)))
+              (rx/catch (fn [cause]
+                          (prn "KKKK" cause)
+                          (rx/concat
+                           (rx/of (clear-export-state uuid/zero))
+                           (rx/throw cause))))))))))
 
 (defn request-multiple-export
-  [{:keys [filename exports query] :as params}]
+  [{:keys [filename exports query-name]
+    :or {query-name :export-shapes-multiple}
+    :as params}]
   (ptk/reify ::request-multiple-export
     ptk/WatchEvent
     (watch [_ state _]
@@ -200,9 +207,9 @@
                  (rx/share))
 
             stoper
-            (->> progress-stream
-                 (rx/filter #(or (= "ended" (:status %))
-                                 (= "error" (:status %)))))]
+            (rx/filter #(or (= "ended" (:status %))
+                            (= "error" (:status %)))
+                       progress-stream)]
 
         (swap! st/ongoing-tasks conj :export)
 
@@ -212,11 +219,11 @@
 
          ;; Launch the exportation process and stores the resource id
          ;; locally.
-         (->> (rp/query! query params)
+         (->> (rp/query! query-name params)
               (rx/tap (fn [{:keys [id]}]
                         (vreset! resource-id id)))
               (rx/map (fn [{:keys [id]}]
-                        (initialize-export-status exports filename id query))))
+                        (initialize-export-status exports filename id query-name))))
 
          ;; We proceed to update the export state with incoming
          ;; progress updates. We delay the stoper for give some time
@@ -245,6 +252,7 @@
   (ptk/reify ::retry-last-export
     ptk/WatchEvent
     (watch [_ state _]
-      (let [{:keys [exports filename query]} (:export state)]
-        (rx/of (request-multiple-export {:exports exports :filename filename :query query}))))))
+      (let [params (select-keys (:export state) [:filename :exports :query-name])]
+        (when (seq params)
+          (rx/of (request-multiple-export params)))))))
 
diff --git a/frontend/src/app/main/repo.cljs b/frontend/src/app/main/repo.cljs
index afbdde5f8..2e3a1a970 100644
--- a/frontend/src/app/main/repo.cljs
+++ b/frontend/src/app/main/repo.cljs
@@ -126,7 +126,7 @@
   [_ params]
   (send-export-command :cmd :export-shapes :params params :blob? false))
 
-(defmethod query :export-frames
+(defmethod query :export-frames-multiple
   [_ params]
   (send-export-command :cmd :export-frames :params (assoc params :uri (str base-uri)) :blob? false))
 
diff --git a/frontend/src/app/main/ui/export.cljs b/frontend/src/app/main/ui/export.cljs
index e872d28d1..5a2b2d471 100644
--- a/frontend/src/app/main/ui/export.cljs
+++ b/frontend/src/app/main/ui/export.cljs
@@ -22,7 +22,7 @@
    [rumext.alpha :as mf]))
 
 (mf/defc export-multiple-dialog
-  [{:keys [exports filename title query no-selection]}]
+  [{:keys [exports filename title query-name no-selection]}]
   (let [lstate          (mf/deref refs/export)
         in-progress?    (:in-progress lstate)
 
@@ -43,7 +43,10 @@
         (fn [event]
           (dom/prevent-default event)
           (st/emit! (modal/hide)
-                    (de/request-multiple-export {:filename filename :exports enabled-exports :query query})))
+                    (de/request-multiple-export
+                     {:filename filename
+                      :exports enabled-exports
+                      :query-name query-name})))
 
         on-toggle-enabled
         (fn [index]
@@ -143,14 +146,23 @@
    ::mf/register-as :export-shapes}
   [{:keys [exports filename]}]
   (let [title (tr "dashboard.export-shapes.title")]
-    (export-multiple-dialog {:exports exports :filename filename :title title :query :export-shapes-multiple :no-selection shapes-no-selection})))
+    [:& export-multiple-dialog
+     {:exports exports
+      :filename filename
+      :title title
+      :query-name :export-shapes-multiple
+      :no-selection shapes-no-selection}]))
 
 (mf/defc export-frames
   {::mf/register modal/components
    ::mf/register-as :export-frames}
   [{:keys [exports filename]}]
   (let [title (tr "dashboard.export-frames.title")]
-    (export-multiple-dialog {:exports exports :filename filename :title title :query :export-frames})))
+    [:& export-multiple-dialog
+     {:exports exports
+      :filename filename
+      :title title
+      :query-name :export-frames-multiple}]))
 
 (mf/defc export-progress-widget
   {::mf/wrap [mf/memo]}