Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LRS-61 Remove Error Interceptor #71

Merged
merged 11 commits into from
Dec 13, 2021
Merged

LRS-61 Remove Error Interceptor #71

merged 11 commits into from
Dec 13, 2021

Conversation

milt
Copy link
Member

@milt milt commented Dec 10, 2021

LET MY PEOPLE THROW (Kinda)

Exception handling should be delegated to implementations based on their needs regarding logging, security, etc.

We were applying an error handling interceptor erroneously to all the routes but doc, this has been changed (see below)

We also included errors in the return spec for sync lrs methods, but did not catch and ensure these were included for the lib handling to use. Catching and passing of errors has been standardized across all sync method wrappers in lrs.cljc with the exception of consistent-through, which is called by an interceptor and functions differently.

Any error that is not explicitly handled by the lrs layer (a narrow class of specific named errors) will now be properly placed in the pedestal context map. Implementations can apply any error handling they wish by providing an error interceptor in the :wrap-interceptors vector passed to routes/build.

UPDATE: The default for the :wrap-interceptors vector is now [i/error-interceptor] per @kelvinqian00's request, meaning the default behavior will not change for implementations, though it will now also apply to doc routes which it did not before.

  • remove error interceptor from common interceptors
  • add :wrap-interceptors arg to routs build for passing your own global interceptors
  • make return less weird for implementations that want to use it
  • ensure handler fns are properly passing thrown impl errors and not covering what should be 500s

@milt milt marked this pull request as draft December 10, 2021 21:39
Comment on lines +9 to +10
(build {:lrs lrs
:wrap-interceptors [i/error-interceptor]}))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

demonstrates plugging in your own

@milt milt marked this pull request as ready for review December 13, 2021 16:07
@milt milt changed the title LRS-61 Don't Handle Exceptions LRS-61 Remove Error Interceptor Dec 13, 2021
@milt milt requested a review from kelvinqian00 December 13, 2021 18:26
Copy link
Contributor

@kelvinqian00 kelvinqian00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to hold off on approval until this bug is addressed:

Wowww. I’ve confirmed that w/o the error handler cljs also just up and dies, but oddly it also does with the handler, lol. Gonna re-check the handler for cljs compat, just in case
Yeah, that looks like a separate bug in the terminator-injector and not something that started with my branch

Copy link
Contributor

@kelvinqian00 kelvinqian00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misunderstood the implications of the above revelation (i.e. the two issues are not connected); otherwise this looks great.

@milt milt merged commit 31af5d9 into master Dec 13, 2021
@milt milt deleted the LRS-61 branch December 13, 2021 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants