Place generated OPTIONS ahead of user defined routes

[arempe93]

Fix for #1260

This change places the generated OPTIONS routes ahead of user defined routes, so that 404 handling does not interfere.

I also noticed a problem when doing this, declaring a route with :any as the http verb would declare an OPTIONS route and a 405 Not Allowed route for ALL methods. It would just be below the user defined route in the stack, and therefore never called. This, however, interfered with my change so I fixed it to map to all methods properly in Grape::API#add_head_not_allowed_methods_and_options_methods

[arempe93]

I think there might actually be a slightly better way to do this. I will revisit tomorrow

[dblock]

@arempe93 Thanks. Let us know how you feel about this PR. Either way please do squash the commits and update CHANGELOG. Possibly also update README.

[arempe93]

Ok - I think this is ready for review now

[dblock]

There'a focus: true still here, remove.

[dblock]

There's a behavior change here when you use route :any with OPTIONS, it's possible that some people accidentally relied on broken behavior, I think you should also add a paragraph into UPGRADING, thanks!

[dblock]

Code looks good FYI, nice work.

[arempe93]

I believe this change should address your comments. I left in the part about Not Allowed methods because the following spec would also fail before my changes.

it 'should route 405' do
  subject.get :hello do
    'Hello World'
  end
  subject.route :any, '*path' do
    error! :not_found, 404
  end

  post '/hello'

  expect(last_response.status).to eql 405
end

Let me know if there needs to be any other documentation

[dblock]

Merged via e54b8b0 (I added a period in the CHANGELOG, OCD ;)).

[dblock]

This introduced a regression, see #1307. I am not sure what the right fix is (yet).

[dblock]

@arempe93 I have reverted this fix in 5aa6554. Couldn't fix the last spec in that commit. Reopening #1260 as well.

[arempe93]

@dblock the problem is (using the example from spec/grape/api/namespace_parameters_in_route_spec) is that me/:id should get a 405 for a GET, but that can also match me/pending. And it does match that first because of how this PR re-ordered the route stack - like this

# PR 1263 ordering
version=, method=PUT, path=/me/pending(.:format)
version=, method=POST, path=/me/pending(.:format)
version=, method=DELETE, path=/me/pending(.:format)
version=, method=PATCH, path=/me/pending(.:format)
version=, method=OPTIONS, path=/me/pending(.:format)
version=, method=GET, path=/me/:x(.:format)    # first match, incorrect (405)
version=, method=POST, path=/me/:x(.:format)
version=, method=DELETE, path=/me/:x(.:format)
version=, method=PATCH, path=/me/:x(.:format)
version=, method=HEAD, path=/me/:x(.:format)
version=, method=OPTIONS, path=/me/:x(.:format)
version=, method=PUT, path=/me/:id(.:format)
version=, method=GET, path=/me/pending(.:format)

compared to master ordering

# original ordering
version=, method=PUT, path=/me/:id(.:format)
version=, method=GET, path=/me/pending(.:format)   # first match, correct (200)
version=, method=OPTIONS, path=/me/:x(.:format)
version=, method=GET, path=/me/:x(.:format)
version=, method=POST, path=/me/:x(.:format)
version=, method=DELETE, path=/me/:x(.:format)
version=, method=PATCH, path=/me/:x(.:format)
version=, method=HEAD, path=/me/:x(.:format)
version=, method=OPTIONS, path=/me/pending(.:format)
version=, method=PUT, path=/me/pending(.:format)
version=, method=POST, path=/me/pending(.:format)
version=, method=DELETE, path=/me/pending(.:format)
version=, method=PATCH, path=/me/pending(.:format)

But this is complicated, because even if we do some complicated ordering where the 405/options are placed after each route, this can still be incorrect

# concept complicated ordering
version=, method=PUT, path=/me/:id(.:format)
version=, method=OPTIONS, path=/me/:x(.:format)
version=, method=GET, path=/me/:x(.:format)   # still first match, incorrect (405)
version=, method=POST, path=/me/:x(.:format)
version=, method=DELETE, path=/me/:x(.:format)
version=, method=PATCH, path=/me/:x(.:format)
version=, method=HEAD, path=/me/:x(.:format)
version=, method=GET, path=/me/pending(.:format)
version=, method=OPTIONS, path=/me/pending(.:format)
version=, method=PUT, path=/me/pending(.:format)
version=, method=POST, path=/me/pending(.:format)
version=, method=DELETE, path=/me/pending(.:format)
version=, method=PATCH, path=/me/pending(.:format)

The only solution I see to make 405 work is to dump them all after the routes...which was the point of this PR to fix 404 handling. So I am unsure of how to fix it, short of re-ordering your routes in your grape project and having some more complicated route ordering.

[dblock]

@arempe93 I totally understand the problem (and your explanation is correct). Of the two evils I chose the lesser, not to have a regression. I think we need a much smarter fix here that doesn't create these other routes, but inspects the routing table to figure out if there's a match with a different verb and generates a 405 accordingly.

[arempe93]

ok I will look into a solution more in that direction when I finally have the free time again