fetlife / rollout

Feature flippers.
MIT License
2.89k stars 211 forks source link

Deprecation warning after upgrading to redis gem version 4.2.0 #148

Closed mattbrictson closed 4 years ago

mattbrictson commented 4 years ago

After upgrading to version 4.2.0 of the redis gem, I see these deprecation warnings:

`Redis#exists(key)` will return an Integer in redis-rb 4.3, if you want to keep the old behavior, use `exists?` instead.
To opt-in to the new behavior now you can set Redis.exists_returns_integer = true.
(rollout-2.4.5/lib/rollout.rb:299:in `exists?’)

Seems like rollout should use exists? for redis 4.2.0 and higher.

mattbrictson commented 4 years ago

Perhaps related, if I run bundle update on the rollout repository itself and run rspec, rollout's specs currently fail:

``` Failures: 1) Rollout Check if feature exists it should return true if the feature is exist Failure/Error: @storage.exists(key(feature)) NoMethodError: undefined method `>' for true:TrueClass # ./lib/rollout.rb:299:in `exists?' # ./spec/rollout_spec.rb:669:in `block (3 levels) in ' 2) Rollout Check if feature exists it should return false if the feature is not exist Failure/Error: @storage.exists(key(feature)) NoMethodError: undefined method `>' for false:FalseClass # ./lib/rollout.rb:299:in `exists?' # ./spec/rollout_spec.rb:673:in `block (3 levels) in ' Finished in 0.58978 seconds (files took 0.16322 seconds to load) 83 examples, 2 failures Failed examples: rspec ./spec/rollout_spec.rb:667 # Rollout Check if feature exists it should return true if the feature is exist rspec ./spec/rollout_spec.rb:672 # Rollout Check if feature exists it should return false if the feature is not exist ```

Edit: this is unrelated. It is a separate bug in fakeredis.

reneklacan commented 4 years ago

@mattbrictson thank you for opening the issue.

I would be happy to fix it but that error is actually coming from the redis-rb gem itself from

From: /Users/rene/.rbenv/versions/2.6.3/lib/ruby/gems/2.6.0/gems/redis-4.2.1/lib/redis.rb:606 Redis#exists?:

    603: def exists?(*keys)
    604:   synchronize do |client|
    605:     client.call([:exists, *keys]) do |value|
 => 606:       value > 0
    607:     end
    608:   end
    609: end

[2] pry(#<Redis>)> value
=> true

value is boolean and they are comparing it with 0

If I'm not mistaken, they first need to actually fix #exists? method before we can update rollout.

mattbrictson commented 4 years ago

I think that is actually due to a bug in fakeredis. If you use the redis 4.2.1 gem directly (i.e. without loading fakeredis), it works as expected:

$ irb
>> require "redis"
true
>> Redis::VERSION
"4.2.1"
>> redis = Redis.new(uri: "redis://127.0.0.1/1")
>> redis.exists?("foo")
false
>> 

If you load fakeredis, that is when the error occurs:

$ irb
>> require "redis"
true
>> require "fakeredis"
true
>> redis = Redis.new(uri: "redis://127.0.0.1/1")
>> redis.exists?("foo")
NoMethodError (undefined method `>' for false:FalseClass)
reneklacan commented 4 years ago

@mattbrictson ah, you are right, I completely forgot we use fakeredis and not a real one. Created PR to fix this issue and also to stop using fakeredis

reneklacan commented 4 years ago

Released as part of 2.4.6