Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix do_not_route_options! handling: do not respond to OPTIONS if disabled! #1119

Closed
wants to merge 2 commits into from

Conversation

jf
Copy link
Contributor

@jf jf commented Aug 23, 2015

FYI: so apparently this wasn't caught by spec/grape/dsl/routing_spec.rb. Not too sure how to fix that.

@dblock
Copy link
Member

dblock commented Aug 23, 2015

First, there should be a test for the fixed behavior, please.

@dblock
Copy link
Member

dblock commented Aug 23, 2015

If this is caused by #1120 then the CHANGELOG should be a bit clearer.

@jf
Copy link
Contributor Author

jf commented Aug 23, 2015

No, it's not caused by #1120. #1120 is a separate issue. Just to be clear, I can fix #1119 with this PR. But #1120 still remains.

@dblock
Copy link
Member

dblock commented Aug 27, 2015

Thanks @jf. The only think I am missing in this PR is a test for #1119 being fixed.

@jf
Copy link
Contributor Author

jf commented Aug 28, 2015

Can I get some clarification on that?

"Test for #1119 being fixed":

  • would that refer to my 2nd commit for this PR which fixed the Travis build (jf@cb44238)?
  • are you looking for something less brittle and not reliant on a fixed ordering of the options as per now? (Because that's what broke Travis. A change in the order)
  • if you're talking about putting something into spec/grape/dsl/routing_spec.rb, now that I've had the chance to see it, would you say that Travis has highlighted that I should be looking to fix spec/grape/api_spec.rb instead (which I have sort of done; but am happy to make the test little brittle as mentioned) ?

@dblock
Copy link
Member

dblock commented Aug 29, 2015

The order of headers isn't the problem in the bug. There should be a test (in any spec appropriate) that reproduces the actual problem, aka that the API responds to OPTIONS when it shouldn't. That test should fail without the fix. Does this help?

@arempe93
Copy link
Contributor

arempe93 commented Feb 2, 2016

@jf Is the objective of this PR to remove OPTIONS from the Allow header when do_not_route_options! is active?

@jf
Copy link
Contributor Author

jf commented Feb 3, 2016

Yup. That was the spirit of jf@3f3c0ba. OPTIONS should not be be advertised as an allowed method when do_not_route_options! is set. Thanks for completing this! Don't know why I didnt describe the patch better, but at least now with the test, I know that things work as intended. Thanks!

@dblock
Copy link
Member

dblock commented Feb 3, 2016

So this can be closed in favor of #1266. @jf reopen if I got it wrong.

@dblock dblock closed this Feb 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants