Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Properly cache value when it is "false" #2010

Merged
merged 1 commit into from

7 participants

@hasclass

Rails.cache does not properly cache "false", it returns nil instead.

This is because the value is checked with an

if value
# ...
else
value = nil # => sets false values to nil
end

I changed it to:
if defined?(value)

Example:

Rails.cache.fetch('foo') { false }
=> false # uncached value

Rails.cache.fetch('foo') { false }
=> nil # from cache

@pzac

Yeah

@sikachu
Collaborator

Nice catch! :+1:

@spastorino spastorino merged commit 4f7ca00 into rails:master
@jamestyj

Any chance getting this patch into Rails 3.1?

@jamestyj jamestyj referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@vijaydev
Collaborator

@jamestyj This will be available in 3.1.2

@jonleighton
Collaborator

@vijaydev cherry picking this into 3-1-stable broke the build. the implementation in master has changed since this PR was merged. additionally, the test in this PR actually passes without the fix, so I think even master exhibits this bug still. (also, in the first line of this patch, defined?(value) will always be truthy as it is an argument to the method.)

I am going to revert the cherry-pick for now. I am planning to push 3.1.2rc1 quite soon, but I can wait for a fix for this if you intend to work on it?

@hasclass

@jonleighton This is fixed in master (by using @value = Marshal.dump(value)).

@vijaydev
Collaborator

Fixed in cd392fd and passes Travis. Good to go for 3.1.4 :)

@jamestyj

Finally, thanks!

@wkj wkj referenced this pull request from a commit
@jonleighton jonleighton Revert "Properly cache value when it is "false""
This reverts commit 551f313.

Reason: see
#2010 (comment)
e52dc33
@ttosch ttosch referenced this pull request from a commit
@jonleighton jonleighton Revert "Properly cache value when it is "false""
This reverts commit 551f313.

Reason: see
#2010 (comment)
40f8d9f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 8, 2011
  1. @hasclass
This page is out of date. Refresh to see the latest.
View
4 activesupport/lib/active_support/cache.rb
@@ -557,7 +557,7 @@ def initialize(value, options = {})
@expires_in = options[:expires_in]
@expires_in = @expires_in.to_f if @expires_in
@created_at = Time.now.to_f
- if value
+ if defined?(value)
if should_compress?(value, options)
@value = Zlib::Deflate.deflate(Marshal.dump(value))
@compressed = true
@@ -576,7 +576,7 @@ def raw_value
# Get the value stored in the cache.
def value
- if @value
+ if defined?(@value)
val = compressed? ? Marshal.load(Zlib::Inflate.inflate(@value)) : @value
unless val.frozen?
val.freeze rescue nil
View
5 activesupport/test/caching_test.rb
@@ -188,6 +188,11 @@ def test_should_read_and_write_nil
assert_equal nil, @cache.read('foo')
end
+ def test_should_read_and_write_false
+ assert_equal true, @cache.write('foo', false)
+ assert_equal false, @cache.read('foo')
+ end
+
def test_read_multi
@cache.write('foo', 'bar')
@cache.write('fu', 'baz')
Something went wrong with that request. Please try again.