Release 5.0.0: known compatibility issues

[byroot]

This issue aim to maintain a list of known compatibility issues with redis-rb 5.0, as well as links to where they are being handled.

If you have an error not yet reported it after upgrading, feel free to comment and I'll triage it.

• [X] NoMethodError: undefined method 'drivers' for Redis::Commands::Connection:Module (https://github.com/redis/redis-rb/issues/1140)
  • Caused by older versions of Sidekiq, is worked around by redis-rb 5.0.1
• [x] Rails Action Cable and RedisCacheStore compatibility:
  • [X] Fixed in Rails 7-0-stable branch
  • [x] Will be released as part of Rails 7.0.4.
• [x] Resque compatibility (Unknown keyword: thread_safe)
  • This one will be tricky, best to lock redis-rb for now (https://github.com/resque/resque/issues/1821)
• [x] kredis compatibility
  • [X] The PR is merged, no changed was needed aside from the gemspec.
  • [x] Still need a release.

[jason-hobbs]

I bundle updated and am now getting: Error loading the 'redis' Action Cable pubsub adapter. Missing a gem it depends on? can't activate redis (>= 3, < 5), already activated redis-5.0.1. Make sure all dependencies are added to Gemfile.

Never mind, just saw the Rails 7.0.4 fix.

[tonobo]

Howdy guys, redis-client gem won't support reconnect_delay and reconnect_delay_max arguments, is it planned to adopt this?

[byroot]

won't support reconnect_delay and reconnect_delay_max arguments, is it planned to adopt this?

No, this feature was wonky, so I replaced it by reconnect_attempts accepting a list of sleep durations, e.g:

Redis.new(reconnect_attempts: [
  0, # retry once immediately
  0.1, # retry a second time after sleeping 100ms
  0.5, # retry a third time after sleeping 500ms more.
])

I think it's way way easier to reason about it with this format.

If you absolutely want reconnect_delay and reconnect_delay_max, you can implement a function that takes these two parameters and return an array of durations like redis 5 expects.

[runephilosof-karnovgroup]

Rails 7.0.4 has been released (Sep 10th), so I guess you can check that checkmark (in the issue description)

[byroot]

Thanks, all known outstanding issues are fixed. I'll leave open in case someone else find something.

[jmthomas]

@byroot It seems like the redis-client code does not allow Boolean parameters yet the original redis gem defines them. Here is a relavent stack trace:

TypeError : Unsupported command argument type: FalseClass 
/usr/lib/ruby/gems/3.1.0/gems/redis-client-0.10.0/lib/redis_client/command_builder.rb:37:in `block in generate'
/usr/lib/ruby/gems/3.1.0/gems/redis-client-0.10.0/lib/redis_client/command_builder.rb:28:in `map!'
/usr/lib/ruby/gems/3.1.0/gems/redis-client-0.10.0/lib/redis_client/command_builder.rb:28:in `generate'
/usr/lib/ruby/gems/3.1.0/gems/redis-client-0.10.0/lib/redis_client.rb:212:in `call_v'
/usr/lib/ruby/gems/3.1.0/gems/redis-5.0.5/lib/redis/client.rb:73:in `call_v'
/usr/lib/ruby/gems/3.1.0/gems/redis-5.0.5/lib/redis.rb:167:in `block in send_command'
/usr/lib/ruby/gems/3.1.0/gems/redis-5.0.5/lib/redis.rb:166:in `synchronize'
/usr/lib/ruby/gems/3.1.0/gems/redis-5.0.5/lib/redis.rb:166:in `send_command'
/usr/lib/ruby/gems/3.1.0/gems/redis-5.0.5/lib/redis/commands/streams.rb:57:in `xadd' 

When calling xadd with something like:

redis.xadd(topic, msg_hash, id: '*', approximate: true)

Looks like the redis-client code doesn't allow Boolean keyword arguments: https://github.com/redis-rb/redis-client/blob/master/lib/redis_client/command_builder.rb#L28

[jmthomas]

I'm also seeing an error trying to call call on the client:

NoMethodError : undefined method `call' for #<Redis::Client redis://openc3-redis-ephemeral:6380/0> synchronize { |client| client.call(args) } ^^^^^ Did you mean? call_v

We have client code which tries to use call like so:

class Redis
  def xtrim_minid(key, minid, approximate: true, limit: nil)
    args = [:xtrim, key, :MINID, (approximate ? '~' : nil), minid].compact
    args.concat([:LIMIT, limit]) if limit
    synchronize { |client| client.call(args) }
  end
end

Mostly because the redis client had not implemented xtrim. Has call been deprecated?

[byroot]

@jmthomas indeed and that is by design. Redis 4.x would call to_s on all arguments, and this was the source of many errors and mistakes. (See the recent GitLab CVE).

If you know an argument is safe, you can call to_s on it yourself.

[byroot]

As for client.call is was never public API.

If you want to add your own commands you can look at the send_command helper (sorry hard to link on mobile)

[jmthomas]

So I guess the issue is that your documentation in the code is now incorrect: https://github.com/redis/redis-rb/blob/fdf61e578ae8438e466961cfda91aa9727f914b5/lib/redis/commands/streams.rb#L45. It indicates taking a Boolean and the example actually shows approximate: true. I've been using the code as my API guide so hopefully this will get cleaned up at some point. I'll start calling it with simply:

redis.xadd(topic, msg_hash, id: '*', approximate: 'true')

[jmthomas]

@byroot I really think you need to support Boolean and nil arguments or anything in the message hash that is Boolean or nil will throw this error. It seems like a simple change to redis-client here: https://github.com/redis-rb/redis-client/blob/master/lib/redis_client/command_builder.rb#L34

was:

 when Integer, Float

should be:

when Integer, Float, Boolean, NilClass

[byroot]

@jmthomas no. This is not an oversight, it's a deliberate change.

From the Redis point of view commands are list of strings, it's ok to cast Numerics to string because they have a simple canonical representation, but that's not true of booleans and nil.

It's the caller responsability to do the casting, e.g. msg_hash.transform_values(&:to_s)

[jmthomas]

Makes sense. We were able to fix our own client in this PR by adding a bunch of .to_s. Seems like there's still a bunch of work to do in the APIs to eliminate booleans that were being passed through. Let me know if there's a ticket open for this or how I can help.

[byroot]

Let me know if there's a ticket open for this or how I can help.

I think there is a misunderstanding here. redis-rb won't cast booleans for you, it's your responsability to do it.

e.g. if you had

redis.set("foo", true)

You need to change it in your app to:

redis.set("foo", true.to_s)

No, if you really want to keep that old behavior (bad idea IMO), you can pass a custom command builder.

[jpcamara]

@jmthomas indeed and that is by design. Redis 4.x would call to_s on all arguments, and this was the source of many errors and mistakes. (See the recent GitLab CVE).

If you know an argument is safe, you can call to_s on it yourself.

@byroot Any chance you could post a link to the specific CVE? I've tried searching around and I haven't found one that clearly lines up with your comment. I'm updating some code related to this change and I don't want to blindly apply .to_s without understanding what the related CVE was

[byroot]

Yeah, sorry I should have linked to more info. Here's the GitLab issue: https://gitlab.com/gitlab-org/gitlab/-/issues/371098

In short they were passing API responses as Sawyer::Resource (a bit like Hashie or OpenStruct), so if the payload contains a to_s field...

[byroot]

Another reason why I'm convince this is the right move, is that I've seen several instance of:

redis.mapped_hmset("hash", "key" => "val", "other-key" => nil)
redis.hgetall("hash") # => { "key" => "val", "other-key" => "" }

Which is really wonky.

[jpcamara]

Yeah, sorry I should have linked to more info. Here's the GitLab issue: https://gitlab.com/gitlab-org/gitlab/-/issues/371098

In short they were passing API responses as Sawyer::Resource (a bit like Hashie or OpenStruct), so if the payload contains a to_s field...

😵‍💫 yikes!

Thanks for the reference - definitely changes how I'd want to update related code

[byroot]

5.x has been out for a while now, I think we can close this.