Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Mime type corrections #367

Merged
merged 1 commit into from

3 participants

@raggi
Owner
  • HTTP 1.0 and 1.1 do not have MUST for Content-Type requirements, they have "should" (not SHOULD).
  • They also have text describing how clients should handle this header being missing.
  • Whilst it might be a "good idea" to auto set this, it's actually problematic elsewhere.

Opened pull request for discussion.

@chneukirchen @rkh @rtomayko @wycats @tenderlove please weigh in.

If I don't get responses by 1.5, I will merge this in there.

  • I'm also considering changing the default mime to nil in Rack::Mime, thoughts on that?
@raggi
Owner

References #316 and #366.

@chneukirchen

If its not a SHOULD, dont write SHOULD.

And I'd prefer text/html to stay the default for Rack::Response.

@raggi
Owner

@chneukirchen not sure what your first sentence means.

WRT Rack::Response defaults - cannot happen if we allow for no Content-Type, as it's preventing that from being testable through MockResponse. In essence, it's not "what the application intends".

I suspect you're thinking about ease of use. I think some of the API changes in 2.0 would be more appropriate for ease of use than to enforce things that are potentially incorrect. At the end of the day, our primary use cases are Frameworks and Server authors, not folks "tinkering" with basic apps in lambdas. The latter can be better serviced, but I don't think that should be the concern of the core library / primary APIs.

Example:

run lambda { |env| Rack::Response.html("<h1>ohai</h1>") }

Rather than making Rack::Response.new difficult to use for framework<->server interconnects in certain cases.

@chneukirchen

You said its "should" not "SHOULD" but the comment in lint.rb says "SHOULD".

(And then, Rack::Response never was made to be used by frameworks, but I guess that's the way it is now.)

@raggi
Owner

@chneukirchen lint.rb is wrong. and should is not MUST.

@raggi
Owner

Correction. 1.1 has SHOULD.

Any HTTP/1.1 message containing an entity-body SHOULD include a Content-Type
header field defining the media type of that body. If and only if the media type
is not given by a Content-Type field, the recipient MAY attempt to guess the
media type via inspection of its content and/or the name extension(s) of the URI
used to identify the resource. If the media type remains unknown, the recipient
SHOULD treat it as type "application/octet-stream".

@raggi raggi Correct some of the mime type issues. References #316 and #366.
HTTP 1.0 and 1.1 do not have MUST for Content-Type requirements, they have "should" (not SHOULD). They also have text describing how clients should handle this header being missing.
3623d04
@raggi
Owner

Just rebased this ready for merge.

@raggi raggi merged commit c23edf4 into from
@raggi raggi deleted the branch
@rkh

@header = Utils::HeaderHash.new(header)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 30, 2012
  1. @raggi

    Correct some of the mime type issues. References #316 and #366.

    raggi authored raggi committed
    HTTP 1.0 and 1.1 do not have MUST for Content-Type requirements, they have "should" (not SHOULD). They also have text describing how clients should handle this header being missing.
This page is out of date. Refresh to see the latest.
View
19 lib/rack/file.rb
@@ -21,9 +21,10 @@ class File
alias :to_path :path
- def initialize(root, headers={})
+ def initialize(root, headers={}, default_mime = 'text/plain')
@root = root
@headers = headers
+ @default_mime = default_mime
end
def call(env)
@@ -70,17 +71,15 @@ def _call(env)
def serving(env)
last_modified = F.mtime(@path).httpdate
return [304, {}, []] if env['HTTP_IF_MODIFIED_SINCE'] == last_modified
- response = [
- 200,
- {
- "Last-Modified" => last_modified,
- "Content-Type" => Mime.mime_type(F.extname(@path), 'text/plain')
- },
- env["REQUEST_METHOD"] == "HEAD" ? [] : self
- ]
+
+ headers = { "Last-Modified" => last_modified }
+ mime = Mime.mime_type(F.extname(@path), @default_mime)
+ headers["Content-Type"] = mime if mime
# Set custom headers
- @headers.each { |field, content| response[1][field] = content } if @headers
+ @headers.each { |field, content| headers[field] = content } if @headers
+
+ response = [ 200, headers, env["REQUEST_METHOD"] == "HEAD" ? [] : self ]
# NOTE:
# We check via File::size? whether this file provides size info
View
7 lib/rack/lint.rb
@@ -474,9 +474,10 @@ def check_content_type(status, headers)
return
end
}
- assert("No Content-Type header found") {
- Rack::Utils::STATUS_WITH_NO_ENTITY_BODY.include? status.to_i
- }
+ # This is a SHOULD in HTTP 1.0 and 1.1:
+ # assert("No Content-Type header found") {
+ # Rack::Utils::STATUS_WITH_NO_ENTITY_BODY.include? status.to_i
+ # }
end
## === The Content-Length
View
3  lib/rack/response.rb
@@ -21,8 +21,7 @@ class Response
def initialize(body=[], status=200, header={})
@status = status.to_i
- @header = Utils::HeaderHash.new("Content-Type" => "text/html").
- merge(header)
+ @header = Utils::HeaderHash.new.merge(header)
@chunked = "chunked" == @header['Transfer-Encoding']
@writer = lambda { |x| @body << x }
View
21 test/spec_file.rb
@@ -189,4 +189,25 @@ def file(*args)
res['Content-Length'].should.equal "193"
end
+ should "default to a mime type of text/plain" do
+ req = Rack::MockRequest.new(Rack::Lint.new(Rack::File.new(DOCROOT)))
+ res = req.get "/cgi/test"
+ res.should.be.successful
+ res['Content-Type'].should.equal "text/plain"
+ end
+
+ should "allow the default mime type to be set" do
+ req = Rack::MockRequest.new(Rack::Lint.new(Rack::File.new(DOCROOT, nil, 'application/octet-stream')))
+ res = req.get "/cgi/test"
+ res.should.be.successful
+ res['Content-Type'].should.equal "application/octet-stream"
+ end
+
+ should "not set Content-Type if the mime type is not set" do
+ req = Rack::MockRequest.new(Rack::Lint.new(Rack::File.new(DOCROOT, nil, nil)))
+ res = req.get "/cgi/test"
+ res.should.be.successful
+ res['Content-Type'].should.equal nil
+ end
+
end
View
12 test/spec_lint.rb
@@ -234,12 +234,12 @@ def result.name
end
should "notice content-type errors" do
- lambda {
- Rack::Lint.new(lambda { |env|
- [200, {"Content-length" => "0"}, []]
- }).call(env({}))
- }.should.raise(Rack::Lint::LintError).
- message.should.match(/No Content-Type/)
+ # lambda {
+ # Rack::Lint.new(lambda { |env|
+ # [200, {"Content-length" => "0"}, []]
+ # }).call(env({}))
+ # }.should.raise(Rack::Lint::LintError).
+ # message.should.match(/No Content-Type/)
[100, 101, 204, 205, 304].each do |status|
lambda {
View
25 test/spec_mime.rb
@@ -0,0 +1,25 @@
+require 'rack/mime'
+
+describe Rack::Mime do
+
+ it "should return the fallback mime-type for files with no extension" do
+ fallback = 'image/jpg'
+ Rack::Mime.mime_type(File.extname('no_ext'), fallback).should == fallback
+ end
+
+ it "should always return 'application/octet-stream' for unknown file extensions" do
+ unknown_ext = File.extname('unknown_ext.abcdefg')
+ Rack::Mime.mime_type(unknown_ext).should == 'application/octet-stream'
+ end
+
+ it "should return the mime-type for a given extension" do
+ # sanity check. it would be infeasible test every single mime-type.
+ Rack::Mime.mime_type(File.extname('image.jpg')).should == 'image/jpeg'
+ end
+
+ it "should support null fallbacks" do
+ Rack::Mime.mime_type('.nothing', nil).should == nil
+ end
+
+end
+
View
6 test/spec_response.rb
@@ -6,7 +6,7 @@
response = Rack::Response.new
status, header, body = response.finish
status.should.equal 200
- header.should.equal "Content-Type" => "text/html"
+ header.should.equal({})
body.each { |part|
part.should.equal ""
}
@@ -14,7 +14,7 @@
response = Rack::Response.new
status, header, body = *response
status.should.equal 200
- header.should.equal "Content-Type" => "text/html"
+ header.should.equal({})
body.each { |part|
part.should.equal ""
}
@@ -37,7 +37,7 @@
it "can set and read headers" do
response = Rack::Response.new
- response["Content-Type"].should.equal "text/html"
+ response["Content-Type"].should.equal nil
response["Content-Type"] = "text/plain"
response["Content-Type"].should.equal "text/plain"
end
Something went wrong with that request. Please try again.