Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Raise exception on Cache.clear call with string argument #11904

Closed
wants to merge 1 commit into from

3 participants

Egor Lynko Paul Nikitochkin Yves Senn
Egor Lynko

Options argument from clear(options) in all bundled cache implementations
wasn't used at all, so calling clear('some_string') just flushes cache
without any warning. But this method can be easily confused with delete,
exactly what happened on my project. So I think it's better to check
if options is a hash, when it provided, and raise ArgumentError if not.

Egor Lynko flexoid Raise exception on Cache.clear call with string
Options argument from `clear(options)` in all bundled cache implementations
wasn't used at all, so calling `clear('some_string')` just flushes cache
without any warning. But this method can be easily confused with `delete`,
exactly what happened on my project. So I think it's better to check
if `options` is a hash, when it provided.
172b2d3
Paul Nikitochkin

For this PR, I think need to add Changelog

Egor Lynko

Sure, I will, I just want to know first what people think about this PR.

Yves Senn senny closed this in dd493d3
Yves Senn
Owner

I don't think code is the solution to this problem. clear is very widely used to perform a complete wipe. The docs of the method are very clear: Clear the entire cache. Be careful with this method since it could affect other processes if shared cache is being used.. And options is used throughout the Rails framework to name a Hash with symbolized keys. I pushed a small doc patch to state the fact that options is a Hash (dd493d3) but I don't think we need to add code.

@flexoid thanks for your work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 15, 2013
  1. Egor Lynko

    Raise exception on Cache.clear call with string

    flexoid authored
    Options argument from `clear(options)` in all bundled cache implementations
    wasn't used at all, so calling `clear('some_string')` just flushes cache
    without any warning. But this method can be easily confused with `delete`,
    exactly what happened on my project. So I think it's better to check
    if `options` is a hash, when it provided.
This page is out of date. Refresh to see the latest.
4 activesupport/lib/active_support/cache.rb
View
@@ -588,6 +588,10 @@ def save_block_result_to_cache(name, options)
write(name, result, options)
result
end
+
+ def validate_for_hash(options) # :nodoc:
+ raise ArgumentError, 'Options must be a hash' unless options.is_a?(Hash)
+ end
end
# This class is used to represent cache entries. Cache entries have a value and an optional
5 activesupport/lib/active_support/cache/file_store.rb
View
@@ -22,10 +22,11 @@ def initialize(cache_path, options = nil)
extend Strategy::LocalCache
end
- # Deletes all items from the cache. In this case it deletes all the entries in the specified
- # file store directory except for .gitkeep. Be careful which directory is specified in your
+ # Deletes all items from the cache. In this case it deletes all the entries in the specified
+ # file store directory except for .gitkeep. Be careful which directory is specified in your
# config file when using +FileStore+ because everything in that directory will be deleted.
def clear(options = nil)
+ validate_for_hash(options) if options
root_dirs = Dir.entries(cache_path).reject {|f| (EXCLUDED_DIRS + [".gitkeep"]).include?(f)}
FileUtils.rm_r(root_dirs.collect{|f| File.join(cache_path, f)})
end
1  activesupport/lib/active_support/cache/mem_cache_store.rb
View
@@ -109,6 +109,7 @@ def decrement(name, amount = 1, options = nil) # :nodoc:
# Clear the entire cache on all memcached servers. This method should
# be used with care when shared cache is being used.
def clear(options = nil)
+ validate_for_hash(options) if options
@data.flush_all
rescue Dalli::DalliError => e
logger.error("DalliError (#{e}): #{e.message}") if logger
1  activesupport/lib/active_support/cache/memory_store.rb
View
@@ -29,6 +29,7 @@ def initialize(options = nil)
end
def clear(options = nil)
+ validate_for_hash(options) if options
synchronize do
@data.clear
@key_access.clear
4 activesupport/test/caching_test.rb
View
@@ -422,6 +422,10 @@ def test_really_long_keys
assert_equal({key => "bar"}, @cache.read_multi(key))
assert @cache.delete(key)
end
+
+ def test_clear_exception_with_string_as_options_argument
+ assert_raises(ArgumentError) { @cache.clear('string') }
+ end
end
# https://rails.lighthouseapp.com/projects/8994/tickets/6225-memcachestore-cant-deal-with-umlauts-and-special-characters
Something went wrong with that request. Please try again.