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

Regression? #1957

Closed
dblock opened this issue Jan 8, 2020 · 6 comments
Closed

Regression? #1957

dblock opened this issue Jan 8, 2020 · 6 comments
Assignees
Labels

Comments

@dblock
Copy link
Member

dblock commented Jan 8, 2020

See #1953 (comment)

@dnesteryuk
Copy link
Member

Unfortunately, it isn't easy to fix. I would recommend to drop my commit from the master and do

git push origin -f

or I can open another PR where I will restore the ReverseStackableValues. 😞

@dm1try
Copy link
Member

dm1try commented Jan 9, 2020

or I can open another PR where I will restore the ReverseStackableValues. 😞

👍, an explicit revert is more suitable for the master

@dblock
Copy link
Member Author

dblock commented Jan 9, 2020

Yeah, someone please PR a revert.

@dnesteryuk Care to explain the problem?

dblock pushed a commit that referenced this issue Jan 9, 2020
@dm1try
Copy link
Member

dm1try commented Jan 9, 2020

closed via #1961

@dm1try dm1try closed this as completed Jan 9, 2020
@dnesteryuk
Copy link
Member

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 😞

@dblock
Copy link
Member Author

dblock commented Jan 9, 2020

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!

basjanssen pushed a commit to basjanssen/grape that referenced this issue Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants