before block called before valid route is checked for.

[alexaltair]

On version 0.16.2, I have the following configured;

class Foos < Grape::API
  include AuthCheck
  ...
  resource :foo do
    ...
    get '/:foo/:id' do
      ...
    end

    # But no POST route
  end
end

module AuthCheck
  extend ActiveSupport::Concern

  included do
    before do
      error!('Unauthorized', 401) unless headers_has_secret
    end
  end
end

A GET request to foo/<foo-id> will either respond with a 200 or a 400 as expected. A POST however will 500;

ArgumentError (uncaught throw :error):
  app/api/my_auth_check.rb:8:in `block (2 levels) in <module:AuthCheck>'

Because it's a throw, this is the only line of the stack trace. If I force a throw and send a GET request I do get a big stack trace, which means the throw is caught in that case. If I write a post block, I get the expected behavior.

It seems like grape is running the before block before it checks to see whether the request has a matching route. Is this the intended behaviour?

[dblock]

I think this is by design, but I can see how it's unclear in https://github.com/ruby-grape/grape#before-and-after. I'd appreciate at least some specs that definitely answer this question if we don't have any, and a README update.

Does before_validation behave differently?

[rosa]

I'm seeing something kind of similar to this, but it's not the same problem. In my case, I was seeing a 500 error when using a wrong request method (for example, calling the /:foo/:id in the issue description with anything different from GET) due to this kind of before filter:

      before do
        require_special_header!
      end

And require_special_header! is something like this:

error!(:special_header_missing, 400) unless request.headers['X-Special-Header'].present?

This ends up raising NoMethodError: undefined method 'headers' for nil:NilClass, and the reason is that before filters are run without passing the env anywhere:

module Grape
  class Router

  ...

  def method_not_allowed(env, methods, endpoint)
      env[Grape::Env::GRAPE_METHOD_NOT_ALLOWED] = true
      current = endpoint.dup
      current.instance_eval do
        @lazy_initialized = false
        lazy_initialize!
        run_filters befores, :before
        @block = proc do
          raise Grape::Exceptions::MethodNotAllowed, header.merge('Allow' => methods)
        end
      end
      current.call(env)
    end

This shouldn't happen with before_validation because we only run namespace_stackable(:befores) and not namespace_stackable(:before_validations), but I haven't tried.

What do you think? Should the env be made available to before filters in that situation?

[rosa]

(In my particular case I'll likely move that kind of headers check to a before_validation filter, after reading again the README on those it looks definitely more correct. But I thought I would mention it just in case it helps 😃 )

[dblock]

@rosa maybe you're onto something, want to try writing a spec for this and seeing whether bringing in env is even possible?

I still would love better documentation on when these are called, it's not obvious that before is called even for non-matching requests.

[rosa]

I was going to add some spec and found out that there is already a spec that shows how before is called in the case of MethodNotAllowed here. I could still add a case to show how before_validation is not invoked in that case. I'll also try to come up with some way of having env in before blocks.

[connorshea]

I think I just struggled with this for the last few days while trying to figure out why a test had broken when everything else was working fine in an upgrade from 0.15.0 to 0.17.0 (0.16.x had a bug with env being unavailable in before blocks, so we had to skip it).

I explain the problem somewhat more in-depth here before I found this issue.

Essentially we have an endpoint delete ":id/hooks/:hook_id" that has a test that tests what happens when no hook_id is provided, i.e. when delete ":id/hooks" is called. In 0.15.0 this returns a 405 error, but in 0.17.0 a 500 error is returned.

I tried to bisect to see when this behavior was introduced, but due to the env bug I mentioned before I wasn't able to run our spec when using a Grape version around 0.16.x, before #1446 was merged.

[connorshea]

So based on the README with the order of before/before_validations/validations/after_validations/etc. this makes sense. However, this seems to be a breaking change from 0.15.0 that is not mentioned in the UPGRADING.md file and caused us quite a bit of headache.

[dblock]

@connorshea If something wasn't explained in UPGRADING PR that?

[connorshea]

I started on the UPGRADING.md PR, but after thinking on it and testing for a bit it seems that this wasn't the source of the problem I was having (though I don't fully understand why after_validations fixes it for 0.17.0 and 0.18.0 while before works again with current master), so I won't open it.

This is the section I was going to add, but I'm pretty sure it's inaccurate:

---

Routes are no longer validated prior to a before block

Previously before blocks would have only allowed valid routes, but the order of validation now makes it so before blocks can run into errors due to invalid routing.

Something like this, which calls a method that uses the route and assumes its validity in a before block:

before { do_something_with_route }

Should become this:

after_validation { do_something_with_route }

See the Before and After section of the README for details.

---

Any chance of a 0.18.1 release so we can upgrade past 0.15.0? :)

[dblock]

You should focus on master only. Explain what "valid" and "invalid" means by providing working examples and try to PR it. We can discuss it there.

Opened #1521 for release, add your +1.

[connorshea]

Thank you @dblock!