From 23b8f5a1d199ddc65d513ad118097a645809faed Mon Sep 17 00:00:00 2001 From: Paul Iannazzo Date: Wed, 20 Jul 2022 10:07:53 -0400 Subject: [PATCH] FIX error-interceptor-ctx keeps ctx state of non-erroring interceptors or :ctx from cause previous: `(:ctx data)` was used to get `ctx`, or `ctx` started from scratch on error for error-interceptors-chain however, if a resource has `(throw (ex-info "..." {:ctx yada-ctx}))` then the `:ctx` is likely on the cause `(ex-cause e)`, so `(some->> e ex-cause ex-data :ctx)` gets the `ctx` if a user added it to `ex-info` this isn't good enough, as if an error is thrown without user doing `(try catch (throw (ex-info ...))` then we don't have the `ctx` anymore, but we probably want the `ctx` for error reporting the below code iterates through each interceptor and returns the last ctx state before the interceptor chain throws. so now our error-interceptor-chain has whatever `ctx` that existed when the error happened. ``` (let [partial-ctx @(d/loop [ctx ctx [interceptor & interceptors-rest] (:interceptor-chain ctx)] (let [next-ctx (interceptor ctx)] (if (instance? manifold.deferred.ErrorDeferred next-ctx) ctx (d/recur next-ctx interceptors-rest))))]) ``` --- src/yada/handler.clj | 95 ++++++++++++++++++++++++-------------------- 1 file changed, 53 insertions(+), 42 deletions(-) diff --git a/src/yada/handler.clj b/src/yada/handler.clj index c5edf7cb..806c9556 100644 --- a/src/yada/handler.clj +++ b/src/yada/handler.clj @@ -111,52 +111,63 @@ ;; Normal resources (-> (apply d/chain ctx (:interceptor-chain ctx)) - (d/catch java.lang.Exception (fn [e] (error-handler e) - (let [data (error-data e)] - (let [status (or (:status data) 500)] - - (let [custom-response (get* (:responses resource) status) - rep (rep/select-best-representation - (:request ctx) - (if custom-response - (or (:produces custom-response) [{:media-type "text/plain" - :charset "UTF-8"}]) - error-representations) - )] - - (apply d/chain - (cond-> (or (:ctx data) ctx) - e (assoc :error e) - ;; true (merge (select-keys ctx [:id :request :method])) - status (assoc-in [:response :status] status) - (:headers data) (assoc-in [:response :headers] (:headers data)) - - rep (assoc-in [:response :produces] rep) - - ;; This primes the body data in case the representation is nil - (contains? data :body) - (assoc-in [:response :body] (:body data)) - - (contains? data :cookies) - (assoc-in [:response :cookies] (:cookies data)) - - ;; This could override [:response :body] - (and (not (contains? data :body)) (not (:response custom-response))) - (standard-error status e rep) - - ;; This could override [:response :body] - (and (not (contains? data :body)) (:response custom-response)) - (custom-error (:response custom-response) rep) - - true set-content-length) - - (:error-interceptor-chain ctx) - - )))))))))) + (let [data (error-data e) + status (or (:status data) 500) + custom-response (get* (:responses resource) status) + ;; create CTX that is based on interceptors before error + partial-ctx (d/loop [ctx ctx + [interceptor & interceptors-rest] (:interceptor-chain ctx)] + (let [next-ctx (interceptor ctx)] + (if (instance? manifold.deferred.ErrorDeferred next-ctx) + ctx + (d/recur next-ctx interceptors-rest)))) + ;; update ctx based on error cause, error, or non-erroring interceptors + ctx (or + (some->> e ex-cause ex-data :ctx) ;; ctx field via user made (throw (ex-info "..." {:ctx ctx})) + (:ctx data) ;; not sure if this is ever valid + @partial-ctx ;; get the last ctx that didn't fail an interceptor + ;; initial ctx that request was made with, discard any :interceptor-chain changes + ctx) + rep (rep/select-best-representation + (:request ctx) + (if custom-response + (or (:produces custom-response) [{:media-type "text/plain" + :charset "UTF-8"}]) + error-representations)) + ] + (apply d/chain + ;; this is all to do with handling an error, we can put this in the error-interceptor-chain + (cond-> (or (:ctx data) ctx) + e (assoc :error e) + ;; true (merge (select-keys ctx [:id :request :method])) + status (assoc-in [:response :status] status) + (:headers data) (assoc-in [:response :headers] (:headers data)) + + rep (assoc-in [:response :produces] rep) + + ;; This primes the body data in case the representation is nil + (contains? data :body) + (assoc-in [:response :body] (:body data)) + + (contains? data :cookies) + (assoc-in [:response :cookies] (:cookies data)) + + ;;FIXME: This could override [:response :body] + (and (not (contains? data :body)) (not (:response custom-response))) + (standard-error ,, status e rep) + + ;;FIXME: This could override [:response :body] + ;; this should be done in the error-interceptor-chain, because we lose access to the body + (and (not (contains? data :body)) (:response custom-response)) + (custom-error ,, (:response custom-response) rep) + + true (set-content-length ,,)) + + (:error-interceptor-chain ctx))))))))) (defn handle-request "Handle Ring request"