What is expected before/after workflow for 405/Method not found?

[jnardone]

When in the workflow should a 405 be returned for the wrong method? At what point with regards to the various handlers should the code exit and return?

Based on searches of various issues (including #1402) I would expect that the "before" handler would run, and that's it.

However, that does not seem to be the case. It appears that "before", "before_validation", and "after_validation" are run before I get the 405. This leads to weird issues where common code in these handlers gets called (and perhaps returns a different error, e.g. a 404 for invalid route data) even though the more fundamental issue is the 405.

(It is also interesting to note that in 0.15.0 headers set in before/before_validation/after_validation are returned with the 405 error, but in 0.16.2 only the headers set in 'before' are returned, even though all 3 blocks are still called.)

module Foo
  class API < Grape::API
    prefix 'api'
    version 'v1', using: :path
    format :json
    do_not_route_head!

    before do
      puts 'in before'
      header 'x-b', 'before'
    end

    before_validation do
      puts 'in before_validation'
      header 'x-bv', 'before_validation'
    end

    after_validation do
      puts 'in after_validation'
      header 'x-av', 'after_validation'
    end

    after do
      puts 'in after'
      header 'x-a', 'after'
    end

    get do
      puts "in do"
    end
  end
end

If I do a PUT to /api/v1 I will only get the "x-b" header (expected) but "before", "before_validation", "after_validation" will all run (unexpected). However, there is no documented explanation of what should happen. So... what should happen?

[jlfaber]

I added some debug trace to the grape specs, and it looks like the before filter is actually getting called twice: once by the method_not_allowed method in router.rb, and then again as part of the endpoint evaluation in endpoint.rb. For some reason, the side-effects (like header setting) of the latter call are not "sticking". This also means the other two filters (before_validation and after_validation) are also getting called stealthily.

Here's a spec that demonstrates the issue.

it 'skips before_validation filter on bad method' do
  subject.namespace :test do
    before { header 'X-Custom-Header1', 'foo' }
    before_validation { header 'X-Custom-Header2', 'foo' }
    # before_validation { raise 'THIS SHOULD NOT HAPPEN' }
    get
  end

  post 'test'
  expect(last_response.status).to eql 405
  expect(last_response.headers['X-Custom-Header1']).to eql 'foo'
  expect(last_response.headers['X-Custom-Header2']).to be_nil
end

This version does not fail, so it appears that before_validation is not getting called. But if you swap in the commented line, you'll see the spec fail because of the raised Error. So, apparently, before_validation IS getting called, but the header setting it does is being ignored.

I can understand the need for running before regardless of whether the method is allowed, but I'm not as clear on why one would want the other filter blocks to execute as well. It does look like there are explicit steps taken to bypass the parameter validation itself when a 405 is occurring. And, of course, it makes no sense at all to run before twice.

So I'll second @jnardone 's question above: what's the official answer on which filters are supposed to run in a 405 situation?

[dblock]

This looks like a bug. Retry on HEAD and PR the spec that fails please (with raise).

[jlfaber]

Added #1426 with two new (failing) specs that demonstrate how I think it should work. Specifically, in a 405 situation, I think that before should run exactly once and no other filters should run at all.

I feel bound to mention that this is not the way it apparently used to work in 0.15.0. As @jnardone noted above, in that release, the validation related filters apparently ran even on a 405 RC. But the docs are unclear about how it's supposed to behave and there were no test specs to define the expectation.

If there is a desire to restore the previous behavior (which most likely broke in conjunction with the big routing changes), the first new spec in the PR would need to change to reflect that expectation.

But, regardless of which way that decision goes, the second new spec should remain as is and be fixed since it demonstrates the before filter getting called multiple times. And that's clearly a problem.

cc/ @jnardone @dblock

[jnardone]

Mostly - in both 0.15.0 and 0.16.2 the before, before_validation, and after_validation events are run before the 405 is returned. However, in 0.16.2 some behavior of "before_validation" and "after_validation" don't take effect (at least, the "headers" command). This makes things very confusing: which code should we expect to get called for the 405 handler, and what effects does that code actually have on what is returned to the client?

(And yes, the double-call of "before" in the new routing code also appears to be a bug.)

[dblock]

Thanks all, @namusyaka you might want to look at this if you have time. I'll queue this up too, but no promises. Pull requests accepted of course!

[dblock]

Closed via #1465.