Redis::exists should no longer use Boolify

[scottyschup]

In the Redis documentation for exists, as of Redis 3.0.3 exists may take multiple key names and returns a count of the number of keys that exist.

Using the redis-rb gem v3.3.3, the result of Redis::exists with an array of keys returns true if and only if just one of the keys exist. false is returned if none of the keys exist or if multiple keys do because Boolify looks for a non-nil value equal to 1.

To reproduce:

r = Redis.new
r.mset 'test1', 1, 'test2', 2
r.exists ['test1', 'nonexistant']
#=> true
r.exists ['test1', 'test2']
#=> false

So either Boolify could be adjusted to look for values >= 1, or (what makes more sense to me) Boolify should not be used in this function so that the return value is the count of the number of keys found as is described in the Redis docs.

[kumekay]

I've created a new pull request #736 to fix this issue.
It differs from #728 that it doesn't change return type of exists method. It always returns boolean:
true if all passed keys exist in database, and false if at least one is missing.

[kumekay]

If there is a real demand for number of existed keys, it can be implemented as separate method, exists_count, for example

[vgmoose]

Any update on the status of this? It looks like there are three in-progress PRs for providing functionality to accept multiple keys:

#728 by @ybinzu - uses exists, returns the count if multiple keys are provided, nil if 0 are found
#736 by @kumekay - uses exists, returns true if all provided keys exist, false if one doesn't
#741 by @ericproulx - adds mexists, returns the count of how many provided keys exist (and never nil)

Thanks to all involved and sorry for any unwanted pings

[micahbf]

Just ran into this issue as well.
Can I do anything to move this forward?

[scottyschup]

Because this issue was not resolved by the time I needed it to be, I just ended up writing a helper method in my RedisClient class to use in place of Redis#exists. It was something like:

class RedisClient
...
  @client = Redis.new
...
  def exists(keys)
    res = keys.inject(0) { |key, count| count +=1 if @client.exists(key) }
    res == 0 ? nil : res
  end
end

[byroot]

Fixed in 3257527

[simonrussell]

@byroot This resolution seems okay, but probably changing the return type should be a major version bump (5.0.0) as this is a breaking change (I'm referrering to the warning issued when you call exists) -- a lot of gems relying on this client might not (should not) be restricting the minor versions.

[simonrussell]

Also this causes gems like Sidekiq to write a lot of junk to the console -- if there was a way to turn off the message that would be good.

[byroot]

if there was a way to turn off the message that would be good.

This should do:

Redis.send(:alias_method, :exists, :exists?)

[simonrussell]

I'd probably rather a way that was a little more future-proof :) I've reverted the minor upgrade in my codebases because it's a lot of extra logging.

My suggestion would be to piggy back on the Redis.exists_returns_integer stuff, and if you set Redis.exists_returns_integer = false then that goes back to the old behaviour (possibly with a single warning when you set it); you could make it die if you set this in the next major version.

(the default value would be nil which could possibly still trigger this logging behaviour)

I'm happy to do a PR. I'd also like to know your thoughts on the major version bump; it's not backwards compatible so it seems like a major version bump is required.

[byroot]

Redis.exists_returns_integer stuff, and if you set Redis.exists_returns_integer = false then that goes back to the old behaviour

Sure, let's see how your PR look.

it's not backwards compatible so it seems like a major version bump is required.

Semver is not gospel, and redis-rb doesn't claim to follow semver anyway. That change is absolutely trivial to handle, and it will be a while before the behavior change. It doesn't require a major version bump.

[simonrussell]

Semver isn't gospel, but for people like me who aren't actually using this gem directly it keeps things simpler. It's a trivial change, but this gem is a dependency of a lot of others. Who knows what their gemspecs say; I'd say it's reasonably likely that a bunch of mysterious bugs will appear if the next release comes out with it.

I'd say that if redis-rb isn't following semver then the question would be why not? It's definitely far from perfect (what is a "breaking change" certainly is subjective sometimes) but it's better than nothing.

[byroot]

then the question would be why not?

Because you then have the exact inverse issue to the one you described. Since a lots of gem depends on redis and set strict major version requirements, you then need to patch hundreds of gems to get the community to move to the new major version. In the meantime you are locked in between with lots of incompatible dependencies.

The Redis 4.0 was a big pain, I don't want to repeat that for such a small change that's trivial to fix.

As an aside Sidekiq literally merged the fix for it in record time: sidekiq/sidekiq#4591 it's just a matter of waiting for the release, in the meantime staying on 4.1.x version or simply setting $VERBOSE = nil is fine.

[simonrussell]

Yes, that's a valid concern definitely -- I wasn't implying it was an easy situation. And yes, at least Sidekiq is an active project, so getting updates is easy. Perhaps a more conservative option would have been to make the change in 5.0 but add another method that did the multi key thing (like some of the other PRs).

Anyway, my PR is there if you're interested.

[mperham]

I would have preferred to see the warning only if multiple keys are passed to exists.

• exists(key) => Bool
• exists(array) => Warning + existing behavior
• exists_count(array | key) => Int
• exists?(key) => Bool

This would minimize warnings and only trigger on code that is arguably using the API incorrectly. wdyt?

[byroot]

@mperham until 4.2 redis-rb wasn't accepting multiple keys for exists(). So I don't quite understand what you are referring to.

One option I played with was for exists() to return an int, except for 0 where it would return nil instead, so that you could keep using it in boolean context. But then I figured it would trick people of in cases such as exists('blah') > 1.

[mperham]

I guess I don't understand why the code should allow Arrays with exists. If you stick with exists(key) => Bool and exists_count(array) => Int you get no breaks, no deprecations, no problems. Am I missing a use case?

[byroot]

I guess I don't understand why the code should allow Arrays with exists.

It doesn't, it only takes variadic arguments.

If you stick with exists(key) => Bool and exists_count(array) => Int you get no breaks, no deprecations, no problems. Am I missing a use case?

Yes, several PRs proposed to offer a distinct method (exists_count, mexists, etc), but so far the redis-rb interface maps directly to commands, so it would mean introducing a method, then asking people to change their code in the next major anyway.

I'd much rather go through a soft deprecation.

[scottyschup]

I guess I don't understand why the code should allow Arrays with exists.

The issue was originally raised because commands in the gem generally follow the Redis documentation for identically named commands, but in this case, the redis-rb gem departs from Redis behavior. This creates a barrier to use (admittedly small).

I raised the issue with the hope that a resolution would bring the gem's behavior back into alignment with the Redis documentation. I think only one or two of the proposed solutions have attempted to resolve the issue with this goal in mind. But to me, those seem like the best solutions. It will create minor issues for existing users, but will make the gem more intuitive and prevent confusion for new users.