Permalink
Browse files

Allow fresh_when/stale? to take a record instead of an options hash […

…DHH]
  • Loading branch information...
1 parent 1e51cd9 commit 218c2729384be487b7b743a58ac39753cb5a8856 @dhh dhh committed Dec 1, 2011
View
2 actionpack/CHANGELOG.md
@@ -1,5 +1,7 @@
## Rails 3.2.0 (unreleased) ##
+* Allow fresh_when/stale? to take a record instead of an options hash *DHH*
+
* Assets should use the request protocol by default or default to relative if no request is available *Jonathan del Strother*
* Log "Filter chain halted as CALLBACKNAME rendered or redirected" every time a before callback halts *José Valim*
View
53 actionpack/lib/action_controller/metal/conditional_get.rb
@@ -23,8 +23,27 @@ module ConditionalGet
# This will render the show template if the request isn't sending a matching etag or
# If-Modified-Since header and just a <tt>304 Not Modified</tt> response if there's a match.
#
- def fresh_when(options)
- options.assert_valid_keys(:etag, :last_modified, :public)
+ # You can also just pass a record where last_modified will be set by calling updated_at and the etag by passing the object itself. Example:
+ #
+ # def show
+ # @article = Article.find(params[:id])
+ # fresh_when(@article)
+ # end
+ #
+ # When passing a record, you can still set whether the public header:
+ #
+ # def show
+ # @article = Article.find(params[:id])
+ # fresh_when(@article, :public => true)
+ # end
+ def fresh_when(record_or_options, additional_options = {})
+ if record_or_options.is_a? Hash
+ options = record_or_options
+ options.assert_valid_keys(:etag, :last_modified, :public)
+ else
+ record = record_or_options
+ options = { :etag => record, :last_modified => record.try(:updated_at) }.merge(additional_options)
@stephencelis
stephencelis added a line comment Dec 1, 2011

Is it necessary to set both the ETag and the Last-Modified headers? If the ETag header is set, isn't Last-Modified just taking up space in the HTTP headers? In other words, is it redundant to set both, or are there reasons to?

@dhh
Ruby on Rails member
dhh added a line comment Dec 2, 2011

Some clients support only one or the other.

@eloyesp
eloyesp added a line comment Jan 3, 2012

I think it could be better to use case instead of if because it is easier to extend, it could catch errors (when it is not a hash nor a record), and it could be easier to read.

case record_or_options
 when Hash
   options = record_or_options
   options.assert_valid_keys(:etag, :last_modified, :public)
  when ActiveRecord::Base
    record  = record_or_options
    options = { :etag => record, :last_modified => record.try(:updated_at) }.merge(additional_options)
  else
    raise ArgumentError, "It must be a Hash or a Record"
end
@josevalim
Ruby on Rails member
josevalim added a line comment Jan 3, 2012

Your case example does not duck type.

@eloyesp
eloyesp added a line comment Jan 4, 2012

thanks! I've learned something today. :)

case record_or_options
when Hash
  options = record_or_options
  options.assert_valid_keys(:etag, :last_modified, :public)
else
  record  = record_or_options
  options = { :etag => record, :last_modified => record.try(:updated_at) }.merge(additional_options)
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
response.etag = options[:etag] if options[:etag]
response.last_modified = options[:last_modified] if options[:last_modified]
@@ -55,8 +74,34 @@ def fresh_when(options)
# end
# end
# end
- def stale?(options)
- fresh_when(options)
+ #
+ # You can also just pass a record where last_modified will be set by calling updated_at and the etag by passing the object itself. Example:
+ #
+ # def show
+ # @article = Article.find(params[:id])
+ #
+ # if stale?(@article)
+ # @statistics = @article.really_expensive_call
+ # respond_to do |format|
+ # # all the supported formats
+ # end
+ # end
+ # end
+ #
+ # When passing a record, you can still set whether the public header:
+ #
+ # def show
+ # @article = Article.find(params[:id])
+ #
+ # if stale?(@article, :public => true)
+ # @statistics = @article.really_expensive_call
+ # respond_to do |format|
+ # # all the supported formats
+ # end
+ # end
+ # end
+ def stale?(record_or_options, additional_options = {})
+ fresh_when(record_or_options, additional_options)
!request.fresh?(response)
end
View
46 actionpack/test/controller/render_test.rb
@@ -50,12 +50,28 @@ def conditional_hello
end
end
+ def conditional_hello_with_record
+ record = Struct.new(:updated_at, :cache_key).new(Time.now.utc.beginning_of_day, "foo/123")
+
+ if stale?(record)
+ render :action => 'hello_world'
+ end
+ end
+
def conditional_hello_with_public_header
if stale?(:last_modified => Time.now.utc.beginning_of_day, :etag => [:foo, 123], :public => true)
render :action => 'hello_world'
end
end
+ def conditional_hello_with_public_header_with_record
+ record = Struct.new(:updated_at, :cache_key).new(Time.now.utc.beginning_of_day, "foo/123")
+
+ if stale?(record, :public => true)
+ render :action => 'hello_world'
+ end
+ end
+
def conditional_hello_with_public_header_and_expires_at
expires_in 1.minute
if stale?(:last_modified => Time.now.utc.beginning_of_day, :etag => [:foo, 123], :public => true)
@@ -1440,6 +1456,36 @@ def test_request_modified
assert_equal @last_modified, @response.headers['Last-Modified']
end
+
+ def test_responds_with_last_modified_with_record
+ get :conditional_hello_with_record
+ assert_equal @last_modified, @response.headers['Last-Modified']
+ end
+
+ def test_request_not_modified_with_record
+ @request.if_modified_since = @last_modified
+ get :conditional_hello_with_record
+ assert_equal 304, @response.status.to_i
+ assert_blank @response.body
+ assert_equal @last_modified, @response.headers['Last-Modified']
+ end
+
+ def test_request_not_modified_but_etag_differs_with_record
+ @request.if_modified_since = @last_modified
+ @request.if_none_match = "234"
+ get :conditional_hello_with_record
+ assert_response :success
+ end
+
+ def test_request_modified_with_record
+ @request.if_modified_since = 'Thu, 16 Jul 2008 00:00:00 GMT'
+ get :conditional_hello_with_record
+ assert_equal 200, @response.status.to_i
+ assert_present @response.body
+ assert_equal @last_modified, @response.headers['Last-Modified']
+ end
+
+
def test_request_with_bang_gets_last_modified
get :conditional_hello_with_bangs
assert_equal @last_modified, @response.headers['Last-Modified']

0 comments on commit 218c272

Please sign in to comment.