mirror of
https://github.com/sinatra/sinatra
synced 2023-03-27 23:18:01 -04:00
Add default_content_type setting
Historically, Sinatra::Response defaults to a text/html Content-Type. However, in practice, Sinatra immediately clears this attribute after instantiating a new Sinatra::Response object, so this default is some- what suspect. Let's break this behavior and have Sinatra::Response be Content-Type-less by default, and update the tests to reflect this. Next, let's introduce a new default_content_type setting that will be applied (instead of text/html) if the Content-Type is not set before the response is finalized. This will be great for e.g. JSON API developers. Let's also make it nil-able to indicate that a default Content-Type should never be applied. Wherever Sinatra is emitting HTML, e.g. in error pages, force the Content-Type to text/html. Finally, clean up the error-handling logic to behave as follows: * Set the X-Cascade header as early as possible. * If an error block matches and returns a value, stop processing and return that value. * If we have a not found or bad request error, inspect the exception (if any) for an error message and present it as the response body if it exists, or present a default error message. The remaining logic is the same otherwise. This should make error handlers simpler to write and behave more consistently by not polluting the body with a default message when none may be necessary.
This commit is contained in:
parent
28505410ec
commit
2128dcfb31
4 changed files with 68 additions and 18 deletions
|
@ -2264,6 +2264,15 @@ set :protection, :session => true
|
||||||
used for built-in server.
|
used for built-in server.
|
||||||
</dd>
|
</dd>
|
||||||
|
|
||||||
|
<dt>default_content_type</dt>
|
||||||
|
<dd>
|
||||||
|
Content-Type to assume if unknown (defaults to <tt>"text/html"</tt>). Set
|
||||||
|
to <tt>nil</tt> to not set a default Content-Type on every response; when
|
||||||
|
configured so, you must set the Content-Type manually when emitting content
|
||||||
|
or the user-agent will have to sniff it (or, if <tt>nosniff</tt> is enabled
|
||||||
|
in Rack::Protection::XSSHeader, assume <tt>application/octet-stream</tt>).
|
||||||
|
</dd>
|
||||||
|
|
||||||
<dt>default_encoding</dt>
|
<dt>default_encoding</dt>
|
||||||
<dd>Encoding to assume if unknown (defaults to <tt>"utf-8"</tt>).</dd>
|
<dd>Encoding to assume if unknown (defaults to <tt>"utf-8"</tt>).</dd>
|
||||||
|
|
||||||
|
|
|
@ -159,10 +159,6 @@ module Sinatra
|
||||||
# http://rubydoc.info/github/rack/rack/master/Rack/Response/Helpers
|
# http://rubydoc.info/github/rack/rack/master/Rack/Response/Helpers
|
||||||
class Response < Rack::Response
|
class Response < Rack::Response
|
||||||
DROP_BODY_RESPONSES = [204, 304]
|
DROP_BODY_RESPONSES = [204, 304]
|
||||||
def initialize(*)
|
|
||||||
super
|
|
||||||
headers['Content-Type'] ||= 'text/html'
|
|
||||||
end
|
|
||||||
|
|
||||||
def body=(value)
|
def body=(value)
|
||||||
value = value.body while Rack::Response === value
|
value = value.body while Rack::Response === value
|
||||||
|
@ -176,6 +172,8 @@ module Sinatra
|
||||||
def finish
|
def finish
|
||||||
result = body
|
result = body
|
||||||
|
|
||||||
|
headers.delete "Content-Type" if headers["Content-Type"].nil?
|
||||||
|
|
||||||
if drop_content_info?
|
if drop_content_info?
|
||||||
headers.delete "Content-Length"
|
headers.delete "Content-Length"
|
||||||
headers.delete "Content-Type"
|
headers.delete "Content-Type"
|
||||||
|
@ -189,7 +187,7 @@ module Sinatra
|
||||||
if calculate_content_length?
|
if calculate_content_length?
|
||||||
# if some other code has already set Content-Length, don't muck with it
|
# if some other code has already set Content-Length, don't muck with it
|
||||||
# currently, this would be the static file-handler
|
# currently, this would be the static file-handler
|
||||||
headers["Content-Length"] = body.inject(0) { |l, p| l + p.bytesize }.to_s
|
headers["Content-Length"] = body.map(&:bytesize).reduce(0, :+).to_s
|
||||||
end
|
end
|
||||||
|
|
||||||
[status.to_i, headers, result]
|
[status.to_i, headers, result]
|
||||||
|
@ -940,15 +938,14 @@ module Sinatra
|
||||||
@response = Response.new
|
@response = Response.new
|
||||||
template_cache.clear if settings.reload_templates
|
template_cache.clear if settings.reload_templates
|
||||||
|
|
||||||
@response['Content-Type'] = nil
|
|
||||||
invoke { dispatch! }
|
invoke { dispatch! }
|
||||||
invoke { error_block!(response.status) } unless @env['sinatra.error']
|
invoke { error_block!(response.status) } unless @env['sinatra.error']
|
||||||
|
|
||||||
unless @response['Content-Type']
|
unless @response['Content-Type']
|
||||||
if Array === body and body[0].respond_to? :content_type
|
if Array === body && body[0].respond_to?(:content_type)
|
||||||
content_type body[0].content_type
|
content_type body[0].content_type
|
||||||
else
|
elsif default = settings.default_content_type
|
||||||
content_type :html
|
content_type default
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -1089,10 +1086,10 @@ module Sinatra
|
||||||
# Attempt to serve static files from public directory. Throws :halt when
|
# Attempt to serve static files from public directory. Throws :halt when
|
||||||
# a matching file is found, returns nil otherwise.
|
# a matching file is found, returns nil otherwise.
|
||||||
def static!(options = {})
|
def static!(options = {})
|
||||||
return if (public_dir = settings.public_folder).nil?
|
return if (public_dir = settings.public_folder).nil?
|
||||||
path = "#{public_dir}#{URI_INSTANCE.unescape(request.path_info)}"
|
path = "#{public_dir}#{URI_INSTANCE.unescape(request.path_info)}"
|
||||||
return unless valid_path?(path)
|
return unless valid_path?(path)
|
||||||
|
|
||||||
path = File.expand_path(path)
|
path = File.expand_path(path)
|
||||||
return unless File.file?(path)
|
return unless File.file?(path)
|
||||||
|
|
||||||
|
@ -1160,19 +1157,27 @@ module Sinatra
|
||||||
|
|
||||||
status(500) unless status.between? 400, 599
|
status(500) unless status.between? 400, 599
|
||||||
|
|
||||||
boom_message = boom.message if boom.message && boom.message != boom.class.name
|
|
||||||
if server_error?
|
if server_error?
|
||||||
dump_errors! boom if settings.dump_errors?
|
dump_errors! boom if settings.dump_errors?
|
||||||
raise boom if settings.show_exceptions? and settings.show_exceptions != :after_handler
|
raise boom if settings.show_exceptions? and settings.show_exceptions != :after_handler
|
||||||
elsif not_found?
|
elsif not_found?
|
||||||
headers['X-Cascade'] = 'pass' if settings.x_cascade?
|
headers['X-Cascade'] = 'pass' if settings.x_cascade?
|
||||||
body boom_message || '<h1>Not Found</h1>'
|
|
||||||
elsif bad_request?
|
|
||||||
body boom_message || '<h1>Bad Request</h1>'
|
|
||||||
end
|
end
|
||||||
|
|
||||||
res = error_block!(boom.class, boom) || error_block!(status, boom)
|
if res = error_block!(boom.class, boom) || error_block!(status, boom)
|
||||||
return res if res or not server_error?
|
return res
|
||||||
|
end
|
||||||
|
|
||||||
|
if not_found? || bad_request?
|
||||||
|
if boom.message && boom.message != boom.class.name
|
||||||
|
body boom.message
|
||||||
|
else
|
||||||
|
content_type 'text/html'
|
||||||
|
body '<h1>' + (not_found? ? 'Not Found' : 'Bad Request') + '</h1>'
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
return unless server_error?
|
||||||
raise boom if settings.raise_errors? or settings.show_exceptions?
|
raise boom if settings.raise_errors? or settings.show_exceptions?
|
||||||
error_block! Exception, boom
|
error_block! Exception, boom
|
||||||
end
|
end
|
||||||
|
@ -1813,6 +1818,7 @@ module Sinatra
|
||||||
set :add_charset, %w[javascript xml xhtml+xml].map { |t| "application/#{t}" }
|
set :add_charset, %w[javascript xml xhtml+xml].map { |t| "application/#{t}" }
|
||||||
settings.add_charset << /^text\//
|
settings.add_charset << /^text\//
|
||||||
set :mustermann_opts, {}
|
set :mustermann_opts, {}
|
||||||
|
set :default_content_type, 'text/html'
|
||||||
|
|
||||||
# explicitly generating a session secret eagerly to play nice with preforking
|
# explicitly generating a session secret eagerly to play nice with preforking
|
||||||
begin
|
begin
|
||||||
|
|
|
@ -3,7 +3,7 @@
|
||||||
require File.expand_path('../helper', __FILE__)
|
require File.expand_path('../helper', __FILE__)
|
||||||
|
|
||||||
class ResponseTest < Minitest::Test
|
class ResponseTest < Minitest::Test
|
||||||
setup { @response = Sinatra::Response.new }
|
setup { @response = Sinatra::Response.new([], 200, { 'Content-Type' => 'text/html' }) }
|
||||||
|
|
||||||
def assert_same_body(a, b)
|
def assert_same_body(a, b)
|
||||||
assert_equal a.to_enum(:each).to_a, b.to_enum(:each).to_a
|
assert_equal a.to_enum(:each).to_a, b.to_enum(:each).to_a
|
||||||
|
|
|
@ -163,6 +163,41 @@ class SettingsTest < Minitest::Test
|
||||||
assert_equal :foo, @base.settings.environment
|
assert_equal :foo, @base.settings.environment
|
||||||
end
|
end
|
||||||
|
|
||||||
|
describe 'default_content_type' do
|
||||||
|
it 'defaults to html' do
|
||||||
|
assert_equal 'text/html', @base.default_content_type
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'can be changed' do
|
||||||
|
@base.set :default_content_type, 'application/json'
|
||||||
|
@base.get('/') { '{"a":1}' }
|
||||||
|
@app = @base
|
||||||
|
get '/'
|
||||||
|
assert_equal 200, status
|
||||||
|
assert_equal 'application/json', response.content_type
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'can be disabled' do
|
||||||
|
@base.set :default_content_type, nil
|
||||||
|
@base.error(404) { "" }
|
||||||
|
@app = @base
|
||||||
|
get '/'
|
||||||
|
assert_equal 404, status
|
||||||
|
assert_nil response.content_type
|
||||||
|
assert_empty body
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'may emit content without a content-type (to be sniffed)' do
|
||||||
|
@base.set :default_content_type, nil
|
||||||
|
@base.get('/') { raise Sinatra::BadRequest, "This is a drill" }
|
||||||
|
@app = @base
|
||||||
|
get '/'
|
||||||
|
assert_equal 400, status
|
||||||
|
assert_nil response.content_type
|
||||||
|
assert_body "This is a drill"
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
describe 'methodoverride' do
|
describe 'methodoverride' do
|
||||||
it 'is disabled on Base' do
|
it 'is disabled on Base' do
|
||||||
assert ! @base.method_override?
|
assert ! @base.method_override?
|
||||||
|
|
Loading…
Reference in a new issue