OPTIONS requests do not go through the middleware stack

[benschwarz]

I want to extend the response that an API will return for OPTIONS requests — I started by writing a middleware:

class MyAPI < Grape::API
  use Grape::Middleware::OptionsException
  mount ApiExample
end

class Grape::Middleware::OptionsException < Grape::Middleware::Base
  def call(env)
    raise env.inspect
  end
end

A get request will through the error:

~ ⚡  curl -I  http://localhost:9292/super/special\?id\=1                                                         
HTTP/1.1 500 Internal Server Error 
Content-Type: text/html
Content-Length: 87978
Server: WEBrick/1.3.1 (Ruby/1.9.3/2012-12-25)
Date: Wed, 29 May 2013 01:49:50 GMT
Connection: Keep-Alive

but an options request will not:

~ ⚡  curl -I -X OPTIONS http://localhost:9292/super/special\?id\=1                                               
HTTP/1.1 204 No Content 
Allow: OPTIONS, GET, HEAD
Server: WEBrick/1.3.1 (Ruby/1.9.3/2012-12-25)
Date: Wed, 29 May 2013 01:50:22 GMT
Connection: close

Any thoughts, @mbleigh ?

[dblock]

Check out /~https://github.com/intridea/grape/blob/master/lib/grape/api.rb#L504, it will respond to OPTIONS. There's a way to disable this behavior with a do_not_route_options! in the API declaration.

Hope this helps. Please do use the mailing list for questions.

[benschwarz]

@dblock, so you're saying that if I use:

class MyAPI < Grape::API
  do_not_route_options!
  use Grape::Middleware::OptionsException

  mount ApiExample
end

Then an options request should go through to the middleware, right? (It doesn't)

[benschwarz]

Ok, additional information @dblock —

Using do_not_route_options! does indeed render status 405, however, the request does not get passed through to middleware.

… it also still sets the Allow: header too.

[dblock]

I'll reopen as a bug. Do you think you could write a test within Grape for something like this? I'll take a look.

[benschwarz]

Hmm, I can try…
I guess I can register a middleware for a given test and have it fail when the middleware isn't hit?

[dblock]

Anything that fails that you expect to succeed :)

[benschwarz]

@dblock Talking over what should happen a little more:

• OPTIONS is enabled by default
• Disabling OPTIONS using do_not_route_options! should:
  • Let the request go through the middleware stack
  • If nothing collects the response, a 405 will be issued

Anything else?

[benschwarz]

ping @dblock

Should I have a stab at implementing this?

[dblock]

Don't hesitate!

[benschwarz]

@dblock, I've added some commits that move OPTIONS support down to the middleware level—

In addition to this, I've also added a BadMethod middleware, for throwing 405 errors when a given method isn't available for a route.

I haven't added these to the default stack in endpoint.rb because the test suite doesn't appear to execute them correctly? Can you please review?

Also, existing specs covered that a Content-type of text/plain should be added to 405 requests… but I'm not sure why the content-type should be altered?

/ping @mbleigh

[dblock]

I think BadMethod may be a poor naming choice (totally inspired by existing code). What this middleware does is check whether the method is allowed or not, I would rename it to just Method.

[benschwarz]

Agree. Lets rename it to Method

[dblock]

This is very much on the right track. Needless to say I much rather have a middleware implementation for this than the homegrown route mounting one. If you had all specs passing, and added a line to CHANGELOG, I would merge this. If you need help with something, like the question you had on inserting this into the middleware stack, glad to help, just say that you're stuck with a specific thing.

The content-type is something that was definitely by design. See #343.

[benschwarz]

Covered #343

[dblock]

Looks closer. Did you look at the spec failures?

[benschwarz]

I haven't covered the spec failures yet… I'm relying on the allowed_methods_for_route method from Middleware::Base… but it feels pretty "unracky" for a middleware to need to pop up to the application to lookup something.

…What do you think?

[dblock]

You could setup the middleware explicitly at construction time instead and modularize allowed_methods_for_route. But I am not sold that it is useful at this stage, and not writing the code I don't know where the underwater mines are. I think you should leave it this way and think about refactoring it further along - what you have now is already much better than what we have here, it's worth finishing.

[benschwarz]

Understood. I'll ship the first cut. Thanks!

[dblock]

Bump. I still want this!

[benschwarz]

I abandoned this branch because… it didn't really work. Middlewares should not be run with any knowledge of other middlewares up the stack — introspecting the routes in the way that I was attempting wouldn't work consistently because the middleware would need to be loaded / required / ordered in such a way that it was the first middleware on the stack, which I think is a good sign that something wasn't quite right.

[dblock]

Given #498, what do you all think I should do with this issue?

[benschwarz]

Given that there is a working OPTIONS implementation now… then we can probably close this.

[dblock]

Closing.