Add ability for interceptors to handle exceptions in lacinia

This change enables the use of standard pedestal error handling for errors
raised in the interceptor stack or in lacinia itself. It does so by moving
response error generation to the very end of the (outbound) interceptor stack.
End users can then inject interceptors prior to it to handle or log exceptions
as desired.

As it stands, this is only applicable to the synchronous query executor. I
would also like to invoke the same error interceptors with exceptions
occurring on the asynchronous path, but am currently unable to figure out how
exceptions occurriing in channels should be handled in pedestal.

If we do figure that out, we can simplify some of the code here as there are
now two error paths (one for sync, one for async).

src/com/walmartlabs/lacinia/pedestal.clj

@@ -15,7 +15,7 @@
 (ns com.walmartlabs.lacinia.pedestal
   "Defines Pedestal interceptors and supporting code."
   (:require
-    [clojure.core.async :refer [chan put!]]
+    [clojure.core.async :refer [go chan put!]]
     [cheshire.core :as cheshire]
     [io.pedestal.interceptor :refer [interceptor]]
     [clojure.string :as str]
@@ -230,6 +230,16 @@
   [exception]
   {:errors [(util/as-error-map exception)]})
 
+(def error-response-interceptor
+  "Returns an internal server error response when an exception was not handled in prior interceptors.
+
+   This must come after [[json-response-interceptor]], as the error still needs to be converted to json."
+  (interceptor
+   {:name ::error-response
+    :error (fn [context ex]
+             (let [{:keys [exception]} (ex-data ex)]
+               (assoc context :response (failure-response 500 (as-errors exception)))))}))
+
 (defn ^:private parse-query-document
   [context compiled-schema q operation-name]
   (try
@@ -316,23 +326,16 @@
 
 (defn ^:private apply-result-to-context
   [context result]
-  ;; Lacinia changed the contract here is 0.36.0 (to support timeouts), the result
-  ;; maybe an exception thrown during initial processing of the query.
-  (if (instance? Throwable result)
-    (do
-      (log/error :event :execution-exception
-                 :ex result)
-      (assoc context :response (failure-response 500 (as-errors result))))
-    ;; When :data is missing, then a failure occurred during parsing or preparing
-    ;; the request, which indicates a bad request, rather than some failure
-    ;; during execution.
-    (let [status (if (contains? result :data)
-                   200
-                   400)
-          response {:status status
-                    :headers {}
-                    :body result}]
-      (assoc context :response response))))
+  ;; When :data is missing, then a failure occurred during parsing or preparing
+  ;; the request, which indicates a bad request, rather than some failure
+  ;; during execution.
+  (let [status (if (contains? result :data)
+                 200
+                 400)
+        response {:status status
+                  :headers {}
+                  :body result}]
+    (assoc context :response response)))
 
 (defn ^:private execute-query
   [context]
@@ -359,22 +362,37 @@
                 (resolve/on-deliver! resolver-result
                                      (fn [result]
                                        (deliver *result result)))
-                (apply-result-to-context context @*result)))}))
+                (let [result @*result]
+                  ;; Lacinia changed the contract here is 0.36.0 (to support timeouts), the result
+                  ;; maybe an exception thrown during initial processing of the query.
+                  (if (instance? Throwable result)
+                    (do (log/error :event :execution-exception
+                                   :ex result)
+                        ;; Raise the exception again for another interceptor to handle.
+                        ;; If unhandled, eventually ends up in error-response-interceptor
+                        (throw result))
+                    (apply-result-to-context context result)))))}))
 
 (def ^{:added "0.2.0"} async-query-executor-handler
   "Async variant of [[query-executor-handler]] which returns a channel that conveys the
   updated context."
   (interceptor
     {:name ::async-query-executor
      :enter (fn [context]
-              (let [ch (chan 1)
-                    resolver-result (execute-query context)]
+              (let [resolver-result (execute-query context)
+                    *result         (promise)]
                 (resolve/on-deliver! resolver-result
                                      (fn [result]
-                                       (->> result
-                                            (apply-result-to-context context)
-                                            (put! ch))))
-                ch))}))
+                                       (deliver *result result)))
+                (go (let [result @*result]
+                      ;; Lacinia changed the contract here is 0.36.0 (to support timeouts), the result
+                      ;; maybe an exception thrown during initial processing of the query.
+                      (if (instance? Throwable result)
+                        (do (log/error :event :execution-exception
+                                       :ex result)
+                            ;; TODO: Async exceptions should also go into the error interceptor
+                            (assoc context :response (failure-response 500 (as-errors result))))
+                        (apply-result-to-context context result))))))}))
 
 (def ^{:added "0.3.0"} disallow-subscriptions-interceptor
   "Handles requests for subscriptions.  Subscription requests must only be sent to the subscriptions web-socket, not the
@@ -390,6 +408,7 @@
   "Returns the default set of GraphQL interceptors, as a seq:
 
     * ::json-response [[json-response-interceptor]]
+    * ::error-response [[error-response-interceptor]]
     * ::graphql-data [[graphql-data-interceptor]]
     * ::status-conversion [[status-conversion-interceptor]]
     * ::missing-query [[missing-query-interceptor]]
@@ -407,6 +426,7 @@
   {:added "0.7.0"}
   [compiled-schema options]
   [json-response-interceptor
+   error-response-interceptor
    graphql-data-interceptor
    status-conversion-interceptor
    missing-query-interceptor

test/com/walmartlabs/lacinia/pedestal_error_test.clj

@@ -0,0 +1,65 @@
+; Copyright (c) 2017-present Walmart, Inc.
+;
+; Licensed under the Apache License, Version 2.0 (the "License")
+; you may not use this file except in compliance with the License.
+; You may obtain a copy of the License at
+;
+;     http://www.apache.org/licenses/LICENSE-2.0
+;
+; Unless required by applicable law or agreed to in writing, software
+; distributed under the License is distributed on an "AS IS" BASIS,
+; WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+; See the License for the specific language governing permissions and
+; limitations under the License.
+
+(ns com.walmartlabs.lacinia.pedestal-error-test
+  (:require
+   [clojure.test :refer [deftest is use-fixtures]]
+   [com.walmartlabs.lacinia.pedestal :refer [default-interceptors inject] :as lp]
+   [com.walmartlabs.lacinia.test-utils :refer [test-server-fixture
+                                               send-request]]
+   [clojure.spec.test.alpha :as stest]
+   [io.pedestal.interceptor :refer [interceptor]]))
+
+(stest/instrument)
+
+(defn proof-interceptor
+  "Interceptor that records how often its error function is called"
+  [proof]
+  (interceptor
+   {:name ::proof
+    :error (fn [_ ex]
+             (swap! proof inc)
+             (throw ex))}))
+
+(def prior-error-proof (atom 0))
+(def post-error-proof (atom 0))
+
+(use-fixtures :once (test-server-fixture {:graphiql true}
+                                         (fn [schema]
+                                           {:interceptors (-> (default-interceptors schema {})
+                                                              ;; Should not be called
+                                                              (inject (proof-interceptor prior-error-proof)
+                                                                      :before ::lp/error-response)
+                                                              ;; Should be called
+                                                              (inject (proof-interceptor post-error-proof)
+                                                                      :after  ::lp/error-response))})))
+
+(deftest can-return-failure-response
+  (let [response (send-request "{ fail }")]
+    (is (= {:body {:errors [{:message "resolver exception"}]}
+            :status 500}
+           (select-keys response [:status :body])))))
+
+(deftest calls-post-error-interceptor
+  (reset! prior-error-proof 0)
+  (reset! post-error-proof 0)
+  (send-request "{ fail }")
+  (is (= 1 @post-error-proof)))
+
+(deftest skips-prior-error-interceptor
+  (reset! prior-error-proof 0)
+  (reset! post-error-proof 0)
+  (send-request "{ fail }")
+  (is (= 0 @prior-error-proof)))
+