Add ability for interceptors to handle exceptions in lacinia #101
Conversation
| (if (instance? Throwable result) | ||
| (do (log/error :event :execution-exception | ||
| :ex result) | ||
| ;; TODO: Async exceptions should also go into the error interceptor |
There was a problem hiding this comment.
This is the TODO that should be resolved.
There was a problem hiding this comment.
When thats done, probably also remove the go in favour of a normal chan. If you have suggestions on the pedestal async stuff, let me know, otherwise I'll invest some time (probably) over the weekend.
There was a problem hiding this comment.
Hey there. This has been resolved.
After talking to the Pedestal people, it is indeed correct that this function throwable->ex-info is private, but you can assemble the same information from the context map + which interceptor will assoc the error in the context map. Its sad that we have to duplicate this bit of pedestal functionality, but we do now have the same error handling on the sync- and async paths that behaves exactly as it would when it was pedestal generating the wrapped exception.
(The last fact you can verify, replace the assoc'ing of the error with a throw, and the synchronous test should still succeed)
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).
lennartbuit
force-pushed
the
error-handling
branch
from
2624f09
to
667c4b8
Compare
This change unifies the error handling of async-query-executor-handler and query-execution-handler. Exceptions raised in either will end up in error interceptors down the interceptor chain.
lennartbuit
force-pushed
the
error-handling
branch
from
36f643b
to
0378ffd
Compare
|
Merged back the changes you made on master. That means this branch is now adding the error handling to both the 'old' pedestal ns and the 'new' pedestal2 ns. As far as I am concerned, this is ready to go, so please let me know what you think ^^! |
| Based on the (private) `io.pedestal.interceptor.chain/throwable->ex-info` function of pedestal" | ||
| [{::chain/keys [execution-id] :as context} exception interceptor-name stage] | ||
| (let [exception-str (pr-str (type exception)) | ||
| msg (str exception-str " in Interceptor " interceptor-name " - " (.getMessage exception)) |
There was a problem hiding this comment.
Some nitpicks, and I'm dissapointed that pedestal doesn't make it easier to indicate a failure in an async interceptor ... wouldn't it be neat if you could just return a Throwable? But I think as a whole this is an improvement, as we prefer to use interceptors to customize behavior of lacinia-pedestal.
| @@ -125,19 +133,20 @@ | |||
| This comes last in the interceptor chain." | |||
| (interceptor | |||
| {:name ::query-executor | |||
| :enter internal/on-enter-query-excecutor})) | |||
| :enter (internal/on-enter-query-excecutor ::query-executor :enter)})) | |||
There was a problem hiding this comment.
Also; I just noticed the typo, let me fix that
Yeah so was I, and that was my line of reasoning as well. It's easy to handle an exception on the sync path, but you don't get the same goodies on the async path. So I really felt like I was doing something wrong... but yeah I asked and no. Anyhow, applied your suggestions :)! |
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 error response 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 occurring 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).
Besides fixing the asynchronous error handling case, it may also be beneficial to move the default error logging to its own separate interceptor. Our use case will directly send these to Rollbar, so the default logging is not required. Another benefit is that exceptions occurring inside the pedestal interceptors can also be logged.
I have verified that these changes work in our application and am eager to make changes if so desired :)!