Merge pull request #198 from metosin/exception-logging-2

Exception logging 2

Author: Deraen

CHANGELOG.md

@@ -1,3 +1,8 @@
+## Unreleased
+
+- Added `compojure.api.exception/with-logging` helper to add logging to exception handlers.
+  - Check extended wiki guide on [exception handling](https://github.com/metosin/compojure-api/wiki/Exception-handling#logging)
+
 ## 0.24.4 (13.1.2016)
 
 **[compare](https://github.com/metosin/compojure-api/compare/0.24.3...0.24.4)**

project.clj

@@ -30,6 +30,7 @@
                              [lein-ring "0.9.7"]
                              [funcool/codeina "0.3.0"]]
                    :dependencies [[org.clojure/clojure "1.7.0"]
+                                  [slingshot "0.12.2"]
                                   [peridot "0.4.2"]
                                   [javax.servlet/servlet-api "2.5"]
                                   [midje "1.8.3"]

src/compojure/api/exception.clj

@@ -16,7 +16,7 @@
 
    Error response only contains class of the Exception so that it won't accidentally
    expose secret details."
-  [^Exception e _ _]
+  [^Exception e data req]
   (logging/log! :error e (.getMessage e))
   (internal-server-error {:type "unknown-exception"
                           :class (.getName (.getClass e))}))
@@ -34,29 +34,38 @@
 
 (defn response-validation-handler
   "Creates error response based on Schema error."
-  [_ data _]
+  [e data req]
   (internal-server-error {:errors (stringify-error (su/error-val data))}))
 
 (defn request-validation-handler
   "Creates error response based on Schema error."
-  [_ data _]
+  [e data req]
   (bad-request {:errors (stringify-error (su/error-val data))}))
 
 (defn schema-error-handler
   "Creates error response based on Schema error."
-  [_ data _]
+  [e data req]
   ; FIXME: Why error is not wrapped to ErrorContainer here?
   (bad-request {:errors (stringify-error (:error data))}))
 
 (defn request-parsing-handler
-  [^Exception ex _ _]
+  [^Exception ex data req]
   (let [cause (.getCause ex)]
     (bad-request {:type (cond
                           (instance? JsonParseException cause) "json-parse-exception"
                           (instance? ParserException cause) "yaml-parse-exception"
                           :else "parse-exception")
                   :message (.getMessage cause)})))
 
+;;
+;; Logging
+;;
+
+(defn with-logging [handler]
+  (fn [^Exception e data req]
+    (logging/log! :error e (.getMessage e))
+    (handler e data req)))
+
 ;;
 ;; Mappings from other Exception types to our base types
 ;;

src/compojure/api/meta.clj

@@ -9,7 +9,6 @@
             [ring.swagger.common :refer :all]
             [ring.swagger.json-schema :as js]
             [ring.util.http-response :refer [internal-server-error]]
-            [slingshot.slingshot :refer [throw+]]
             [schema.core :as s]
             [schema.coerce :as sc]
             [schema.utils :as su]
@@ -88,7 +87,8 @@
           (let [coerce (coercer (value-of schema) matcher)
                 body (coerce (:body response))]
             (if (su/error? body)
-              (throw+ (assoc body :type ::ex/response-validation))
+              (throw (ex-info "Response validation error"
+                              (assoc body :type ::ex/response-validation)))
               (assoc response
                 ::serializable? true
                 :body body)))
@@ -105,7 +105,7 @@
        (let [coerce# (~+compojure-api-coercer+ ~schema matcher#)
              result# (coerce# value#)]
          (if (su/error? result#)
-           (throw+ (assoc result# :type ::ex/request-validation))
+           (throw (ex-info "Request validation failed" (assoc result# :type ::ex/request-validation)))
            result#))
        value#)))
 

src/compojure/api/middleware.clj

@@ -12,7 +12,6 @@
             [ring.swagger.middleware :as rsm]
             [ring.swagger.coerce :as rsc]
             [ring.util.http-response :refer :all]
-            [slingshot.slingshot :refer [try+ throw+]]
             [schema.core :as s])
   (:import [com.fasterxml.jackson.core JsonParseException]
            [org.yaml.snakeyaml.parser ParserException]))
@@ -39,9 +38,9 @@
 
 (def rethrow-exceptions? ::rethrow-exceptions?)
 
-(defn- call-error-handler [error-handler error error-type request]
+(defn- call-error-handler [error-handler error data request]
   (try
-    (error-handler error error-type request)
+    (error-handler error data request)
     (catch clojure.lang.ArityException e
       (println "WARNING: Error-handler arity has been changed.")
       (error-handler error))))
@@ -55,18 +54,16 @@
   (let [default-handler (get handlers ::ex/default ex/safe-handler)]
     (assert (fn? default-handler) "Default exception handler must be a function.")
     (fn [request]
-      (try+
+      (try
         (handler request)
-        (catch (get % :type) {:keys [type] :as data}
-          (let [type (or (get ex/legacy-exception-types type) type)]
-            (if-let [handler (get handlers type)]
-              (call-error-handler handler (:throwable &throw-context) data request)
-              (call-error-handler default-handler (:throwable &throw-context) data request))))
-        (catch Object _
-          ; FIXME: Used for validate
-          (if (rethrow-exceptions? request)
-            (throw+)
-            (call-error-handler default-handler (:throwable &throw-context) nil request)))))))
+        (catch Throwable e
+          (let [{:keys [type] :as data} (ex-data e)
+                type (or (get ex/legacy-exception-types type) type)
+                handler (or (get handlers type) default-handler)]
+            ; FIXME: Used for validate
+            (if (rethrow-exceptions? request)
+              (throw e)
+              (call-error-handler handler e data request))))))))
 
 ;;
 ;; Component integration
@@ -144,8 +141,8 @@
   ;; i.e. (handler req) is inside try-catch. If r-m-f was changed to catch only
   ;; exceptions from parsing the request, we wouldn't need to check the exception class.
   (if (or (instance? JsonParseException e) (instance? ParserException e))
-    (throw+ {:type ::ex/request-parsing} e)
-    (throw+ e)))
+    (throw (ex-info "Error parsing request" {:type ::ex/request-parsing} e))
+    (throw e)))
 
 (defn serializable?
   "Predicate which returns true if the response body is serializable.
@@ -184,6 +181,9 @@
                                        :compojure.api.exception/response-validation compojure.api.exception/response-validation-handler
                                        :compojure.api.exception/default             compojure.api.exception/safe-handler}
 
+                                      Note: Because the handlers are merged into default handlers map, to disable default handler you
+                                      need to provide `nil` value as handler.
+
                                       Note: To catch Schema errors use {:schema.core/error compojure.api.exception/schema-error-handler}
 
                                       Note: Adding an alias for exception namespace makes it easier to define these options.

test/compojure/api/middleware_test.clj

@@ -1,9 +1,11 @@
 (ns compojure.api.middleware-test
   (:require [compojure.api.middleware :refer :all]
+            [compojure.api.exception :as ex]
             [midje.sweet :refer :all]
             [ring.util.http-response :refer [ok]]
             [ring.util.http-status :as status]
-            ring.util.test)
+            ring.util.test
+            [slingshot.slingshot :refer [throw+]])
   (:import [java.io PrintStream ByteArrayOutputStream]))
 
 (defmacro without-err
@@ -38,15 +40,43 @@
 
     (ring.util.test/string-input-stream "foobar") false false))
 
+(def default-options (:exceptions api-middleware-defaults))
+
 (facts "wrap-exceptions"
   (with-out-str
-    (without-err
-      (let [exception (RuntimeException. "kosh")
-            exception-class (.getName (.getClass exception))
-            failure (fn [_] (throw exception))]
-
-        (fact "converts exceptions into safe internal server errors"
-          ((wrap-exceptions failure (:handlers (:exceptions api-middleware-defaults))) ..request..)
-          => (contains {:status status/internal-server-error
-                        :body (contains {:class exception-class
-                                         :type "unknown-exception"})}))))))
+   (without-err
+     (let [exception (RuntimeException. "kosh")
+           exception-class (.getName (.getClass exception))
+           handler (-> (fn [_] (throw exception))
+                       (wrap-exceptions default-options))]
+
+       (fact "converts exceptions into safe internal server errors"
+         (handler {}) => (contains {:status status/internal-server-error
+                                    :body (contains {:class exception-class
+                                                     :type "unknown-exception"})})))))
+
+  (with-out-str
+   (without-err
+     (fact "Slingshot exception map type can be matched"
+       (let [handler (-> (fn [_] (throw+ {:type ::test} (RuntimeException. "kosh")))
+                         (wrap-exceptions (assoc-in default-options [:handlers ::test] (fn [ex _ _] {:status 500 :body "hello"}))))]
+         (handler {}) => (contains {:status status/internal-server-error
+                                    :body "hello"})))))
+
+  (without-err
+    (fact "Default handler logs exceptions to console"
+      (let [handler (-> (fn [_] (throw (RuntimeException. "kosh")))
+                        (wrap-exceptions default-options))]
+        (with-out-str (handler {})) => "ERROR kosh\n")))
+
+  (without-err
+    (fact "Default request-parsing handler does not log messages"
+      (let [handler (-> (fn [_] (throw (ex-info "Error parsing request" {:type ::ex/request-parsing} (RuntimeException. "Kosh"))))
+                        (wrap-exceptions default-options))]
+        (with-out-str (handler {})) => "")))
+
+  (without-err
+    (fact "Logging can be added to a exception handler"
+      (let [handler (-> (fn [_] (throw (ex-info "Error parsing request" {:type ::ex/request-parsing} (RuntimeException. "Kosh"))))
+                        (wrap-exceptions (assoc-in default-options [:handlers ::ex/request-parsing] (ex/with-logging ex/request-parsing-handler))))]
+        (with-out-str (handler {})) => "ERROR Error parsing request\n"))))