From 9abf4b126c29d36765284c51561e4d52c6119fb5 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 24 Mar 2022 12:25:07 +0100 Subject: [PATCH] :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)