✨ Improve error handling

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]}
 

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)

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)))

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

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)