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

add Content-Type header to the 405 response #343

Merged
merged 4 commits into from
Mar 4, 2013

Conversation

gustavosaume
Copy link
Contributor

Rack throws Rack::Lint::LintError after calling an undefined method for an endpoint, it expects a Content-Type in the response.

Expected response for a PUT request to a defined endpoint that doesn't allow PUT:

HTTP/1.1 405 Method Not Allowed
Allow: OPTIONS, GET, HEAD
Cache-Control: no-cache
Connection: close
Server: thin 1.5.0 codename Knife
Transfer-Encoding: Identity

Current response:

HTTP/1.1 500 Internal Server Error
Content-Length: 90362
Content-Type: text/html
Connection: close
Server: thin 1.5.0 codename Knife

Stack trace

gems/rack-1.4.5/lib/rack/lint.rb:19:in `assert'
gems/rack-1.4.5/lib/rack/lint.rb:476:in `check_content_type'
gems/rack-1.4.5/lib/rack/lint.rb:54:in `_call'
gems/rack-1.4.5/lib/rack/lint.rb:36:in `call'
gems/rack-1.4.5/lib/rack/showexceptions.rb:24:in `call'
gems/rack-1.4.5/lib/rack/commonlogger.rb:33:in `call'
gems/sinatra-1.3.4/lib/sinatra/base.rb:161:in `call'
gems/rack-1.4.5/lib/rack/chunked.rb:43:in `call'
gems/rack-1.4.5/lib/rack/content_length.rb:14:in `call'
gems/thin-1.5.0/lib/thin/connection.rb:81:in `block in pre_process'
gems/thin-1.5.0/lib/thin/connection.rb:79:in `catch'
gems/thin-1.5.0/lib/thin/connection.rb:79:in `pre_process'
gems/thin-1.5.0/lib/thin/connection.rb:54:in `process'
gems/thin-1.5.0/lib/thin/connection.rb:39:in `receive_data'
gems/eventmachine-1.0.0/lib/eventmachine.rb:187:in `run_machine'
gems/eventmachine-1.0.0/lib/eventmachine.rb:187:in `run'
gems/thin-1.5.0/lib/thin/backends/base.rb:63:in `start'
gems/thin-1.5.0/lib/thin/server.rb:159:in `start'
gems/rack-1.4.5/lib/rack/handler/thin.rb:13:in `run'
gems/rack-1.4.5/lib/rack/server.rb:268:in `start'
gems/rack-1.4.5/lib/rack/server.rb:137:in `start'
gems/rack-1.4.5/bin/rackup:4:in `<top (required)>'
bin/rackup:19:in `load'
bin/rackup:19:in `<main>'
bin/ruby_noexec_wrapper:14:in `eval'
bin/ruby_noexec_wrapper:14:in `<main>'

@dblock
Copy link
Member

dblock commented Feb 22, 2013

Should this be a text/html content-type rather than blank? Please also update CHANGELOG. Thank you.

@gustavosaume
Copy link
Contributor Author

We actually thought about that but there is no content in the 405 response, so it doesn't makes sense to set a Content-Type. Setting it to Blank prevents Rack throwing the error plus it doesn't get included in the actual response header.

@dblock
Copy link
Member

dblock commented Feb 22, 2013

I read everything I could about this and I don't think an empty content-type is RFC-compatible. I am sure Rack::Lint has a good reason to raise an error though, so we should try to figure this out.

I asked SO What should the content-type be for a 4xx error without a body?

@gustavosaume
Copy link
Contributor Author

yes, an empty Content-Type in the header is definitely not RFC compliant. But setting the Content-Type as an empty string in the code causes Rack to exclude it from the header entirely, which is RFC compliant.

This may be a bit of a hack, since the problem is really with Rack::Lint. Rack::Lint should only require Content-Type if there is actually content. Maybe we should submit a patch to Rack::Lint.

@gustavosaume
Copy link
Contributor Author

This has been discussed before in the Rack issues but their solution doesn't seemed to actually fix this issue. (from Rack issues).

@dblock
Copy link
Member

dblock commented Feb 25, 2013

I think I would accept a Content-Type text/plain here, but I would like other places in Grape to be reviewed where we return no content-type (if that exists).

@gustavosaume
Copy link
Contributor Author

agreed, lets see if the guys from rack have a comment.

@dblock
Copy link
Member

dblock commented Mar 4, 2013

I am ready to merge this. Should we do the same for OPTIONS that returns a 204 right above that code?

@dblock dblock merged commit 43b321b into ruby-grape:master Mar 4, 2013
@dblock
Copy link
Member

dblock commented Mar 4, 2013

Merged in 27d15e2.

@gustavosaume
Copy link
Contributor Author

I don't think it would be necessary for the 204 since Rack doesn't enforce that validation for that type of response, and because it means "No Content" don't think it will be changing in the near future.

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.

2 participants