:only Strategy is Broken

[jherdman]

During test runs I namespace all Redis operations under "test:* using the redis-namespace Gem. Let's check to see if it works if I configure only: ["test*"]:

$ redis-cli set 'foo' 'bar'
OK
$ redis-cli keys '*'
1) "foo"
$ bin/rspec path/to/spec
# ASSUME TESTS RUN...
$ redis-cli keys '*'
(empty array)

Uh oh! We've accidentally wiped every single key in the Redis DB. This is a bummer for my development environment.

I've tracked this down to this bit of code:

database_cleaner-redis/lib/database_cleaner/redis/deletion.rb

Lines 26 to 29 in e8389c2

	only = expand_keys(@only)
	except = expand_keys(@except)
	only = connection.keys if only.none?
	(only - except)

1. On L26 we don't find any keys in the "test:*" namespace as expected. Ergo only == []
2. except: isn't configured, ergo except == []
3. only.none? is true, therefore we go ahead and find all keys in the Redis DB (bug!!)
4. (only - except) returns every single key in the Redis DB, and they're then dutifully wiped out

A reproduction PR: #13

I was hoping we could simply drop L28 but it seems like there's existing behaviour that relies upon this logic.

[niborg]

We are also running into this issue.

[jherdman]

@niborg the solution I've adopted is to avoid this gem entirely for Redis. The solution I've settled on for my use case as follows:

1. Namespace my keys based upon the environment (e.g. my-project:test:foo — I'm using https://rubygems.org/gems/redis-namespace)
2. Wipe keys for an entire namespace between test runs

The entire solution is maybe 10-15 lines of code including configuration.

[niborg]

@jherdman I added a PR in #15 that should fix the issue.

[satoruk]

Hi @niborg, I appreciate your efforts on this PR. After reviewing your changes, I had a thought about another approach that might also work well, and in some ways, it might improve upon the solution. It's just an idea, and I'd love to hear your thoughts on it. Let me know what you think!

require "database_cleaner/redis/deletion"

class DatabaseCleaner::Redis::Deletion
  private

  def keys_to_delete
    only = @only.none? ? @connection.keys : expand_keys(@only)
    except = expand_keys(@except)
    (only - except)
  end
end

[niborg]

Hi @satoruk that solution is fine, and I support it if you want to proceed with it (it is less code, which is attractive!). However, it is worth considering the my rationale as set forth in the PR:

Resolves bug that would wipe the entire Redis db when an only specification is provided but there are no matching keys in the db. This is achieved by defaulting the only and except arguments to be nil, which signals that they have not been set whereas implementation before was ambiguous. And if only has not been set, then the code knows to use all Redis keys.

Your solution leaves a bit of an awkward API. For example, DatabaseCleaner::Redis::Deletion.new(only: []) suggests by its syntax that it will not delete any keys, but it actually deletes every key. I admit this usage is "weird" and probably would only occur by accident or by someone unfamiliar with database cleaning and this library, but the end result (deleting every key in redis) is pretty unfortunate that it is nice to avoid.

But in short, please go with whatever solution you prefer. I'd happily use either approach.