ruby-grape / grape

An opinionated framework for creating REST-like APIs in Ruby.
http://www.ruby-grape.org
MIT License
9.86k stars 1.22k forks source link

'context' undefined in rescue_from in 2.1.0 #2451

Closed mscrivo closed 1 month ago

mscrivo commented 1 month ago

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

krismichalski commented 1 month ago

I believe it's because of https://github.com/ruby-grape/grape/pull/2363/ the Grape::Middleware::Helpers module is no longer eager loaded. Adding helpers Grape::Middleware::Helpers fixes the issue.

ericproulx commented 1 month ago

@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 commented 1 month ago

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

dblock commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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

ericproulx commented 1 month ago

@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 commented 1 month ago

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

dblock commented 1 month ago

Thanks for fixing this @ericproulx.