Skip to content
Browse files

properly implement If-Matches and If-None-Matches handling according …

…to RFC 2616, fixes #355
  • Loading branch information...
1 parent 3ac3a29 commit 48e167317ea9d15c957af8e0e600cc837e46c0a4 @rkh rkh committed Sep 17, 2011
Showing with 647 additions and 54 deletions.
  1. +18 −0 README.rdoc
  2. +28 −10 lib/sinatra/base.rb
  3. +4 −0 test/helper.rb
  4. +597 −44 test/helpers_test.rb
View
18 README.rdoc
@@ -1018,6 +1018,24 @@ try {rack-cache}[http://rtomayko.github.com/rack-cache/]:
Use the <tt>:static_cache_control</tt> setting (see below) to add
<tt>Cache-Control</tt> header info to static files.
+According to RFC 2616 your application should behave differently if the
+If-Matches of If-None-Matches header is set to <tt>*</tt> depending on whether
+the resource requested is already in existence. Sinatra assumes resources for
+safe (like get) and idempotent (like put) requests are already in existence,
+whereas other resources (for instance for post requests), are treated as new
+resources. You can change this behavior by passing in a <tt>:new_resource</tt>
+option:
+
+ get '/create' do
+ etag '', :new_resource => true
+ Article.create
+ erb :new_article
+ end
+
+If you still want to use a weak ETag, pass in a <tt>:kind</tt> option:
+
+ etag '', :new_resource => true, :kind => :weak
+
=== Sending Files
For sending files, you can use the <tt>send_file</tt> helper method:
View
38 lib/sinatra/base.rb
@@ -355,8 +355,12 @@ def last_modified(time)
return unless time
time = time_for time
response['Last-Modified'] = time.httpdate
- # compare based on seconds since epoch
- halt 304 if Time.httpdate(env['HTTP_IF_MODIFIED_SINCE']).to_i >= time.to_i
+
+ unless env['HTTP_IF_NONE_MATCH']
+ # compare based on seconds since epoch
+ since = Time.httpdate(env['HTTP_IF_MODIFIED_SINCE']).to_i
+ halt 304 if since >= time.to_i
+ end
rescue ArgumentError
end
@@ -369,22 +373,36 @@ def last_modified(time)
# When the current request includes an 'If-None-Match' header with a
# matching etag, execution is immediately halted. If the request method is
# GET or HEAD, a '304 Not Modified' response is sent.
- def etag(value, kind = :strong)
- raise ArgumentError, ":strong or :weak expected" unless [:strong,:weak].include?(kind)
+ def etag(value, options = {})
+ # Before touching this code, please double check RFC 2616 14.24 and 14.26.
+ options = {:kind => options} unless Hash === options
+ kind = options[:kind] || :strong
+ new_resource = options.fetch(:new_resource) { request.post? }
+
+ unless [:strong, :weak].include?(kind)
+ raise ArgumentError, ":strong or :weak expected"
+ end
+
value = '"%s"' % value
value = 'W/' + value if kind == :weak
response['ETag'] = value
- if etags = env['HTTP_IF_NONE_MATCH']
- etags = etags.split(/\s*,\s*/)
- if etags.include?(value) or etags.include?('*')
- halt 304 if request.safe?
- else
- halt 412 unless request.safe?
+ if success? or status == 304
+ if etag_matches? env['HTTP_IF_NONE_MATCH'], new_resource
+ halt(request.safe? ? 304 : 412)
+ end
+
+ if env['HTTP_IF_MATCH']
+ halt 412 unless etag_matches? env['HTTP_IF_MATCH'], new_resource
end
end
end
+ def etag_matches?(list, new_resource = request.post?)
+ return !new_resource if list == '*'
+ list.to_s.split(/\s*,\s*/).include? response['ETag']
+ end
+
# Sugar for redirect (example: redirect back)
def back
request.referer
View
4 test/helper.rb
@@ -72,6 +72,10 @@ def assert_body(value)
assert_equal value.lstrip.gsub(/\s*\n\s*/, ""), body.lstrip.gsub(/\s*\n\s*/, "")
end
+ def assert_status(expected)
+ assert_equal Integer(expected), Integer(status)
+ end
+
def assert_like(a,b)
pattern = /id=['"][^"']*["']|\s+/
assert_equal a.strip.gsub(pattern, ""), b.strip.gsub(pattern, "")
View
641 test/helpers_test.rb
@@ -960,69 +960,622 @@ def obj.is_a?(thing) 60.is_a?(thing) end
end
describe 'etag' do
- setup do
- mock_app {
- get '/' do
- body { 'Hello World' }
- etag 'FOO'
- 'Boo!'
+ context "safe requests" do
+ it 'returns 200 for normal requests' do
+ mock_app do
+ get '/' do
+ etag 'foo'
+ 'ok'
+ end
end
- post '/' do
- etag 'FOO'
- 'Matches!'
+ get('/')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ context "If-None-Match" do
+ it 'returns 304 when If-None-Match is *' do
+ mock_app do
+ get '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_NONE_MATCH' => '*')
+ assert_status 304
+ assert_body ''
end
- }
- end
- it 'sets the ETag header' do
- get '/'
- assert_equal '"FOO"', response['ETag']
- end
+ it 'returns 200 when If-None-Match is * for new resources' do
+ mock_app do
+ get '/' do
+ etag 'foo', :new_resource => true
+ 'ok'
+ end
+ end
- it 'returns a body when conditional get misses' do
- get '/'
- assert_equal 200, status
- assert_equal 'Boo!', body
- end
+ get('/', {}, 'HTTP_IF_NONE_MATCH' => '*')
+ assert_status 200
+ assert_body 'ok'
+ end
- it 'returns a body when posting with no If-None-Match header' do
- post '/'
- assert_equal 200, status
- assert_equal 'Matches!', body
- end
+ it 'returns 304 when If-None-Match is * for existing resources' do
+ mock_app do
+ get '/' do
+ etag 'foo', :new_resource => false
+ 'ok'
+ end
+ end
- it 'returns a body when conditional post matches' do
- post '/', {}, { 'HTTP_IF_NONE_MATCH' => '"FOO"' }
- assert_equal 200, status
- assert_equal 'Matches!', body
- end
+ get('/', {}, 'HTTP_IF_NONE_MATCH' => '*')
+ assert_status 304
+ assert_body ''
+ end
- it 'halts with 412 when conditional post misses' do
- post '/', {}, { 'HTTP_IF_NONE_MATCH' => '"BAR"' }
- assert_equal 412, status
- assert_equal '', body
+ it 'returns 304 when If-None-Match is the etag' do
+ mock_app do
+ get '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_NONE_MATCH' => '"foo"')
+ assert_status 304
+ assert_body ''
+ end
+
+ it 'returns 304 when If-None-Match includes the etag' do
+ mock_app do
+ get '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar", "foo"')
+ assert_status 304
+ assert_body ''
+ end
+
+ it 'returns 200 when If-None-Match does not include the etag' do
+ mock_app do
+ get '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar"')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'ignores If-Modified-Since if If-None-Match does not match' do
+ mock_app do
+ get '/' do
+ etag 'foo'
+ last_modified Time.at(0)
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar"')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'does not change a status code other than 2xx or 304' do
+ mock_app do
+ get '/' do
+ status 499
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_NONE_MATCH' => '"foo"')
+ assert_status 499
+ assert_body 'ok'
+ end
+
+ it 'does change 2xx status codes' do
+ mock_app do
+ get '/' do
+ status 299
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_NONE_MATCH' => '"foo"')
+ assert_status 304
+ assert_body ''
+ end
+
+ it 'does not send a body on 304 status codes' do
+ mock_app do
+ get '/' do
+ status 304
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_NONE_MATCH' => '"foo"')
+ assert_status 304
+ assert_body ''
+ end
+ end
+
+ context "If-Match" do
+ it 'returns 200 when If-Match is the etag' do
+ mock_app do
+ get '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_MATCH' => '"foo"')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 200 when If-Match includes the etag' do
+ mock_app do
+ get '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_MATCH' => '"foo", "bar"')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 200 when If-Match is *' do
+ mock_app do
+ get '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_MATCH' => '*')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 412 when If-Match is * for new resources' do
+ mock_app do
+ get '/' do
+ etag 'foo', :new_resource => true
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_MATCH' => '*')
+ assert_status 412
+ assert_body ''
+ end
+
+ it 'returns 200 when If-Match is * for existing resources' do
+ mock_app do
+ get '/' do
+ etag 'foo', :new_resource => false
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_MATCH' => '*')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 412 when If-Match does not include the etag' do
+ mock_app do
+ get '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ get('/', {}, 'HTTP_IF_MATCH' => '"bar"')
+ assert_status 412
+ assert_body ''
+ end
+ end
end
- it 'halts when a conditional GET matches' do
- get '/', {}, { 'HTTP_IF_NONE_MATCH' => '"FOO"' }
- assert_equal 304, status
- assert_equal '', body
+ context "idempotent requests" do
+ it 'returns 200 for normal requests' do
+ mock_app do
+ put '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ put('/')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ context "If-None-Match" do
+ it 'returns 412 when If-None-Match is *' do
+ mock_app do
+ put '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ put('/', {}, 'HTTP_IF_NONE_MATCH' => '*')
+ assert_status 412
+ assert_body ''
+ end
+
+ it 'returns 200 when If-None-Match is * for new resources' do
+ mock_app do
+ put '/' do
+ etag 'foo', :new_resource => true
+ 'ok'
+ end
+ end
+
+ put('/', {}, 'HTTP_IF_NONE_MATCH' => '*')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 412 when If-None-Match is * for existing resources' do
+ mock_app do
+ put '/' do
+ etag 'foo', :new_resource => false
+ 'ok'
+ end
+ end
+
+ put('/', {}, 'HTTP_IF_NONE_MATCH' => '*')
+ assert_status 412
+ assert_body ''
+ end
+
+ it 'returns 412 when If-None-Match is the etag' do
+ mock_app do
+ put '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ put('/', {}, 'HTTP_IF_NONE_MATCH' => '"foo"')
+ assert_status 412
+ assert_body ''
+ end
+
+ it 'returns 412 when If-None-Match includes the etag' do
+ mock_app do
+ put '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ put('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar", "foo"')
+ assert_status 412
+ assert_body ''
+ end
+
+ it 'returns 200 when If-None-Match does not include the etag' do
+ mock_app do
+ put '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ put('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar"')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'ignores If-Modified-Since if If-None-Match does not match' do
+ mock_app do
+ put '/' do
+ etag 'foo'
+ last_modified Time.at(0)
+ 'ok'
+ end
+ end
+
+ put('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar"')
+ assert_status 200
+ assert_body 'ok'
+ end
+ end
+
+ context "If-Match" do
+ it 'returns 200 when If-Match is the etag' do
+ mock_app do
+ put '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ put('/', {}, 'HTTP_IF_MATCH' => '"foo"')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 200 when If-Match includes the etag' do
+ mock_app do
+ put '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ put('/', {}, 'HTTP_IF_MATCH' => '"foo", "bar"')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 200 when If-Match is *' do
+ mock_app do
+ put '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ put('/', {}, 'HTTP_IF_MATCH' => '*')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 412 when If-Match is * for new resources' do
+ mock_app do
+ put '/' do
+ etag 'foo', :new_resource => true
+ 'ok'
+ end
+ end
+
+ put('/', {}, 'HTTP_IF_MATCH' => '*')
+ assert_status 412
+ assert_body ''
+ end
+
+ it 'returns 200 when If-Match is * for existing resources' do
+ mock_app do
+ put '/' do
+ etag 'foo', :new_resource => false
+ 'ok'
+ end
+ end
+
+ put('/', {}, 'HTTP_IF_MATCH' => '*')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 412 when If-Match does not include the etag' do
+ mock_app do
+ put '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ put('/', {}, 'HTTP_IF_MATCH' => '"bar"')
+ assert_status 412
+ assert_body ''
+ end
+ end
end
- it 'should handle multiple ETag values in If-None-Match header' do
- get '/', {}, { 'HTTP_IF_NONE_MATCH' => '"BAR", *' }
- assert_equal 304, status
- assert_equal '', body
+ context "post requests" do
+ it 'returns 200 for normal requests' do
+ mock_app do
+ post '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ post('/')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ context "If-None-Match" do
+ it 'returns 200 when If-None-Match is *' do
+ mock_app do
+ post '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ post('/', {}, 'HTTP_IF_NONE_MATCH' => '*')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 200 when If-None-Match is * for new resources' do
+ mock_app do
+ post '/' do
+ etag 'foo', :new_resource => true
+ 'ok'
+ end
+ end
+
+ post('/', {}, 'HTTP_IF_NONE_MATCH' => '*')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 412 when If-None-Match is * for existing resources' do
+ mock_app do
+ post '/' do
+ etag 'foo', :new_resource => false
+ 'ok'
+ end
+ end
+
+ post('/', {}, 'HTTP_IF_NONE_MATCH' => '*')
+ assert_status 412
+ assert_body ''
+ end
+
+ it 'returns 412 when If-None-Match is the etag' do
+ mock_app do
+ post '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ post('/', {}, 'HTTP_IF_NONE_MATCH' => '"foo"')
+ assert_status 412
+ assert_body ''
+ end
+
+ it 'returns 412 when If-None-Match includes the etag' do
+ mock_app do
+ post '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ post('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar", "foo"')
+ assert_status 412
+ assert_body ''
+ end
+
+ it 'returns 200 when If-None-Match does not include the etag' do
+ mock_app do
+ post '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ post('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar"')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'ignores If-Modified-Since if If-None-Match does not match' do
+ mock_app do
+ post '/' do
+ etag 'foo'
+ last_modified Time.at(0)
+ 'ok'
+ end
+ end
+
+ post('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar"')
+ assert_status 200
+ assert_body 'ok'
+ end
+ end
+
+ context "If-Match" do
+ it 'returns 200 when If-Match is the etag' do
+ mock_app do
+ post '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ post('/', {}, 'HTTP_IF_MATCH' => '"foo"')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 200 when If-Match includes the etag' do
+ mock_app do
+ post '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ post('/', {}, 'HTTP_IF_MATCH' => '"foo", "bar"')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 412 when If-Match is *' do
+ mock_app do
+ post '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ post('/', {}, 'HTTP_IF_MATCH' => '*')
+ assert_status 412
+ assert_body ''
+ end
+
+ it 'returns 412 when If-Match is * for new resources' do
+ mock_app do
+ post '/' do
+ etag 'foo', :new_resource => true
+ 'ok'
+ end
+ end
+
+ post('/', {}, 'HTTP_IF_MATCH' => '*')
+ assert_status 412
+ assert_body ''
+ end
+
+ it 'returns 200 when If-Match is * for existing resources' do
+ mock_app do
+ post '/' do
+ etag 'foo', :new_resource => false
+ 'ok'
+ end
+ end
+
+ post('/', {}, 'HTTP_IF_MATCH' => '*')
+ assert_status 200
+ assert_body 'ok'
+ end
+
+ it 'returns 412 when If-Match does not include the etag' do
+ mock_app do
+ post '/' do
+ etag 'foo'
+ 'ok'
+ end
+ end
+
+ post('/', {}, 'HTTP_IF_MATCH' => '"bar"')
+ assert_status 412
+ assert_body ''
+ end
+ end
end
it 'uses a weak etag with the :weak option' do
- mock_app {
+ mock_app do
get '/' do
etag 'FOO', :weak
"that's weak, dude."
end
- }
+ end
get '/'
assert_equal 'W/"FOO"', response['ETag']
end

0 comments on commit 48e1673

Please sign in to comment.
Something went wrong with that request. Please try again.