Skip to content

Add test for OPTIONS and HEAD requests with catch-all#1326

Merged
dblock merged 2 commits into
ruby-grape:masterfrom
namusyaka:fix-1057
Mar 16, 2016
Merged

Add test for OPTIONS and HEAD requests with catch-all#1326
dblock merged 2 commits into
ruby-grape:masterfrom
namusyaka:fix-1057

Conversation

@namusyaka

Copy link
Copy Markdown
Contributor

ref #1057
I've cherry-picked @ekampp's commit and fixed a little bit.
Thoughts?

ekampp and others added 2 commits March 17, 2016 01:06
This illustrates the issue described in ruby-grape#1056 by adding a options and
head request when the catch-all route is enabled.
@ekampp

ekampp commented Mar 16, 2016

Copy link
Copy Markdown
Contributor

@namusyaka

Copy link
Copy Markdown
Contributor Author

Well, OPTIONS seems not to have the restriction of status codes.
Our default OPTIONS routes are defined at this line, and this means that a response of the route will be empty.
So I guess 204 status is reasonable, right?

@ekampp

ekampp commented Mar 16, 2016

Copy link
Copy Markdown
Contributor

204 is reasonable for an empty response. But I'm reading that it should return 200 OK. But either way getting this to pass would be an improvement 👍

@dblock

dblock commented Mar 16, 2016

Copy link
Copy Markdown
Member

Very good, the passing spec is a merge. Lets close the related bug and review whether we want to change the status code here (open new issues as appropriate).

dblock added a commit that referenced this pull request Mar 16, 2016
Add test for OPTIONS and HEAD requests with catch-all
@dblock dblock merged commit f5d3ddc into ruby-grape:master Mar 16, 2016
@dblock

dblock commented Mar 16, 2016

Copy link
Copy Markdown
Member

This possibly needs a CHANGELOG entry since I believe this was a known bug now fixed, @namusyaka please.

namusyaka added a commit to namusyaka/grape that referenced this pull request Mar 16, 2016
dblock added a commit that referenced this pull request Mar 16, 2016
@namusyaka namusyaka deleted the fix-1057 branch March 22, 2016 12:26
przbadu pushed a commit to przbadu/grape that referenced this pull request Apr 5, 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.

3 participants