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

'context' undefined in rescue_from in 2.1.0 #2451

Closed
mscrivo opened this issue Jun 17, 2024 · 9 comments · Fixed by #2453
Closed

'context' undefined in rescue_from in 2.1.0 #2451

mscrivo opened this issue Jun 17, 2024 · 9 comments · Fixed by #2453
Labels

Comments

@mscrivo
Copy link
Contributor

mscrivo commented Jun 17, 2024

After updating from 2.0 to 2.1, some of our error handling tests broke. We have various error handlers that would catch errors globally and log some info and we used the context for that. For example:

rescue_from :all do |e|
  log_context = {
    useragent: context.env['HTTP_USER_AGENT'],
    method: context.env['REQUEST_METHOD'],
    url: context.env['PATH_INFO'],
    querystring: context.env['QUERY_STRING'],
    ip: context.env['HTTP_X_REAL_IP'],
    accept: context.env['HTTP_ACCEPT'],
    content_type: context.env['CONTENT_TYPE'],
    params: context.params,
  }
  logger.log(e, log_context)
  error!(e, e.status)
end

But now with 2.1, we're getting:

NameError: undefined local variable or method `context' for #<#<Class:0x0000000323776e08>:0x0000000324f45700>
    ruby/lib/grape/error_handling.rb:103:in `block in <module:ErrorHandling>'

I'll try to dig in tomorrow to see if I can figure out which commit caused it, but figured I'd give folks a heads up

@dblock dblock added the bug? label Jun 18, 2024
@krismichalski
Copy link

I believe it's because of #2363 the Grape::Middleware::Helpers module is no longer eager loaded. Adding helpers Grape::Middleware::Helpers fixes the issue.

@ericproulx
Copy link
Contributor

ericproulx commented Jun 19, 2024

@krismichalski your close but the helper is included and loaded in Grape::Middleware::Base and Grape::Middleware::Auth::Base

Nonetheless, I think I know what's the issue, since #2377, rescue_from are endpoint.instance_exec instead of instance_exec. Since the endpoint doesn't have the helper included, adding helpers Grape::Middleware::Helpers makes it work.

@krismichalski
Copy link

oh, I didn't notice it was loaded there, sorry for the wrong accusation ^^" I'm glad the root cause was discovered 👍

@dblock
Copy link
Member

dblock commented Jun 19, 2024

@krismichalski Looks like this isn't a bug? Can you please take a look at https://github.com/ruby-grape/grape/blob/master/UPGRADING.md and make sure it accurately describes this situation for the next person?

@ericproulx
Copy link
Contributor

@krismichalski Looks like this isn't a bug? Can you please take a look at https://github.com/ruby-grape/grape/blob/master/UPGRADING.md and make sure it accurately describes this situation for the next person?

For me, it feels like a bug since we narrowed the context of all rescue_from to the endpoint. I've made a fix which is very simple

@ericproulx
Copy link
Contributor

@mscrivo could you checkout my branch and see if it works ?

@ericproulx
Copy link
Contributor

@mscrivo @krismichalski since the rescue_from's block has the context of the endpoint, you should be able to just call env directly instead of context.env.

@dblock I think we should add it back like in my PR and add deprecation on future releases.

@mscrivo
Copy link
Contributor Author

mscrivo commented Jun 19, 2024

@ericproulx your branch seems to work great, our tests are passing

@dblock
Copy link
Member

dblock commented Jun 19, 2024

Thanks for fixing this @ericproulx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants