Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecation warning after upgrading to redis gem version 4.2.0 #148

Closed
mattbrictson opened this issue Jun 12, 2020 · 5 comments · Fixed by #149
Closed

Deprecation warning after upgrading to redis gem version 4.2.0 #148

mattbrictson opened this issue Jun 12, 2020 · 5 comments · Fixed by #149

Comments

@mattbrictson
Copy link

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
Copy link
Author

mattbrictson commented Jun 12, 2020

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 <top (required)>'

  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 <top (required)>'

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
Copy link
Member

@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
Copy link
Author

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 added a commit that referenced this issue Jun 12, 2020
@reneklacan
Copy link
Member

@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 added a commit that referenced this issue Jun 12, 2020
reneklacan added a commit that referenced this issue Jun 12, 2020
reneklacan added a commit that referenced this issue Jun 12, 2020
reneklacan added a commit that referenced this issue Jun 12, 2020
@reneklacan
Copy link
Member

Released as part of 2.4.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants