Remove rescue_from StandardError in Api::BaseController #2139
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, there was a before_destroy on LegacyUser which would prevent delting users which had an attached order by raiseing DestroyWithOrdersError. LegacyUser shouldn't be used in real stores, so this is pointless. This replaces that check with dependent: :restrict_with_exception inside of UserMethods, which will restrict deletions of any user model (like Spree::User from solidus_auth_devise). This also updates the API and admin (the only places which handled this) to understand DeleteRestrictionError instead, and to return a better error response.
This avoids only rendering the exception through the default error_during_processing, which I am working on removing.
This test wasn't testing anything useful because any exception would be swallowed by error_during_processing. Inevitably, this test was failing because an order in a state which doesn't exist is invalid and state_machines was throwing an error on `can_complete?` This regression test existed because previously some code needed to check if there was a template available for the specific state the order was in. That behaviour was removed in 3af26e2, so we no longer need this specific regression test.
Previously Api::BaseController would swallow all exceptions (or all StandardError as of Solidus 1.0) and silently render an exception using error_during_processing. This has long hid errors in specs (see the last few commits for examples) as well as in production usage. This removes the rescue_from, the error_during_processing method, and the configurable error_notifier. Instead, errors bubble up and are handled by Rails middleware, as they should be. This allows exceptions to be seen in API specs, stack traces to be seen in development, and errors to be reported to error collection services in production.
jhawthorn
force-pushed
the
remove_api_rescue
branch
from
021a414 to
4c57b31
Compare
|
Oh, thank you so much. When I used to work with the API, I always had to start with commenting out this rescue. And when I forget .. thanks a lot! |
This was referenced Aug 14, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously
Api::BaseControllerwould swallow all exceptions (or allStandardErroras of Solidus 1.0) and silently render an exception usingerror_during_processing.This has long hid errors in specs (see the last few commits for examples) as well as in production usage.
This removes the
rescue_from StandardError, theerror_during_processingmethod, and the configurableerror_notifier. Instead, errors bubble up and are handled by Rails middleware, as they would be normally.This allows exceptions to be seen in API specs, stack traces to be seen in development, and errors to be reported to error collection services in production.
This will change the responses for unhandled exceptions:
Previously, unhandled exceptions would return
422 unprocessable entity(which is just wrong) with JSON and anexceptionkey.Now unhandled exceptions will return
500 server error(which is correct) and JSON with anerrorskey (if not requesting from a web browser in development, which would instead display a nice error page)I feel this is an unavoidable API change to the responses, and a good one.
This builds on #2138 and #2136, and includes a few other commits to make the removal possible.