-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Regression? #1957
Comments
Unfortunately, it isn't easy to fix. I would recommend to drop my commit from the master and do
or I can open another PR where I will restore the ReverseStackableValues. 😞 |
👍, an explicit revert is more suitable for the |
Yeah, someone please PR a revert. @dnesteryuk Care to explain the problem? |
closed via #1961 |
When rescue handlers get added, every handler is added to a separate hash. [{ RuntimeError => handler }, { StandardError => handler }] Even though they are defined in one endpoint rescue_from RuntimeError do
# catch
end
rescue_from StandardError do
# catch
end So, after reverting this array, we got the wrong order. If handlers defined in an endpoint were in one hash [{ RuntimeError => handler, StandardError => handler }] after adding inherited values it would look like this: [
{ RuntimeError => inheritedHandler },
{ RuntimeError => handler, StandardError => handler }
] my solution would've worked. However, the stackable values doesn't work this way. Actually, any structure doesn't work this way. I tried a few other ideas, but they didn't work because of namespaces and copies of settings, there are lots of things happening with settings. Grape really shuffles them a lot. I am sorry guys for troubles 😞 |
No trouble at all. Bug was caught before release. A test case was added (thanks @dmitry!) that ensures rescue handlers happen in predictable order, which was clearly a miss and potentially a serious regression in the future because of some other change. I call this a win! |
See #1953 (comment)
The text was updated successfully, but these errors were encountered: