Auto OPTIONS should not check params or call _validation hooks #1505
Conversation
|
Leaving the commits un-squashed to make review easier. Once that's complete, let me know and I'll be happy to squash prior to merge. |
| * [#1503](https://github.com/ruby-grape/grape/pull/1503): Allow to use regexp validator with arrays - [@akoltun](https://github.com/akoltun). | ||
| * [#1507](https://github.com/ruby-grape/grape/pull/1507): Add group attributes for parameter definitions - [@304](https://github.com/304). | ||
| * Your contribution here. | ||
|
|
||
| #### Fixes | ||
|
|
||
| * [#1505](https://github.com/ruby-grape/grape/pull/1505): Run only before hook for automatic OPTIONS - [@jlfaber](https://github.com/jlfaber). |
There was a problem hiding this comment.
This sounds not very english :) I think what you meant to say is that before_validation is not called for OPTIONS, but before is?
|
Okay, I think this is good. Want to squash it before merging? @namusyaka did I miss anything? |
|
@jlfaber This needs a note in UPGRADING and possibly clarification in README. |
|
Updated language in CHANGELOG and added README and UPGRADING content. |
|
@namusyaka This is now yours. I am a little worried that we don't run the |
|
Okay, I'm going to review this in a few days! |
|
|
||
| When a request is made to a resource using an undefined method (HTTP 405) or a request is | ||
| made to the built-in `OPTIONS` handler, only the `before` callbacks associated with the | ||
| resource will be run. Other callbacks (`before_validation`, `after_validation`, `after`) |
There was a problem hiding this comment.
Why don't we call after callback? Is this reasonable?
I couldn't find reason why the callback isn't executed on built-in OPTIONS.
There was a problem hiding this comment.
I think it's quite reasonable. From personal experience, after callbacks sometimes assume that the body code for at least one of the valid methods has run or that parameters exist. Since built-in OPTIONS doesn't run any of that body code and one normally doesn't pass params to OPTIONS, this can be quite surprising and cause mysterious errors.
There was a problem hiding this comment.
The after depended on body may not be required in this case, indeed.
However, the logger on the callback should not be considered?
I wonder if too much care?
There was a problem hiding this comment.
I would add that if before is called, it feels like after should be called too, at least logically.
There was a problem hiding this comment.
That is a good point. If someone tries to build logging using a route-defined after callback, this will defeat that.
However I think the preferred way to do logging would be to use a logger that inherits from Grape::Middleware::Base. Both the before and after callbacks of middleware are still called even for the built-in OPTIONS handler, just as they are when an HTTP 405 is generated because of an invalid method.
There was a problem hiding this comment.
I think I feel strongly that if you have a before you get an after. Is there a good reason to exclude an after? Logging is just an example, but callbacks typically have before, after, around, etc.
There was a problem hiding this comment.
@jlfaber You're right. Indeed, the logging example should be placed in middleware stack however I'm not sure if it's worth breaking compatibility.
Also if I'm newcomer, I'll wonder that both before and after behaviors are different.
There was a problem hiding this comment.
You're the boss. :) I added another commit that restores after but still skips the _validation callbacks as well as parameter processing for the built-in OPTIONS handler.
There was a problem hiding this comment.
@dblock and @namusyaka if you're both happy with the latest set of changes, I'd be happy to squash prior to merge. Just let me know.
| @@ -631,6 +659,9 @@ class DummyFormatClass | |||
| describe 'adds an OPTIONS route that' do | |||
| before do | |||
| subject.before { header 'X-Custom-Header', 'foo' } | |||
| subject.before_validation { header 'X-Custom-Header-2', 'bar' } | |||
| subject.after_validation { header 'X-Custom-Header-3', 'baz' } | |||
There was a problem hiding this comment.
The thing aboutafter callback breaks compatibility.
We should add a spec for the thing even if we can find reasonable reason about it.
There was a problem hiding this comment.
Added a check that 'after' did not run.
No description provided.