search

redis / redis-rb

A Ruby client library for Redis
MIT License
3.98k stars 1.03k forks source link

hgetall throwing error when upgrading to 5.0 #1298

Closed billybonks closed 1 day ago

billybonks commented 1 day ago

A bit late with this report, given that the 5.0 compatibility issue is closed.

I am currently using 4.8.1

the following code throws ArgumentError: element has wrong array length (expected 2, was 1) at redis/commands.rb:59 https://github.com/redis/redis-rb/blob/master/lib/redis/commands.rb#L59

when debugging in the hashify lambda I can see that the value for value is {"token"=>"asdf"}

hset('wonk', 'token', 'asdf')
hgetall('wonk')

I have tried the earliest and the latest version of redis-rb and I get the same effect

I am using redis_version:7.2.6

byroot commented 1 day ago

This issue is more likely to be with the Ruby version, but 4.x is EOL.

billybonks commented 1 day ago

@byroot i just updated ruby from 3.3.4 -> 3.3.6 and the issue continues to persist

byroot commented 1 day ago

My point was that your Ruby was too new, not too old.

Anyway, you should upgrade to redis-rb 5.x, I'm no longer doing anything with 4.x.

billybonks commented 1 day ago

absolutely this is the blocker to upgrading to version 5.x, trying to overcome this error to upgrade. this was working fine on 4.x so why would updating to 5.x but leaving the version the same cause the error? i blamed as many parts of the code involved and they have not changed in 2years

byroot commented 1 day ago

Wait, this error is on 5.x? You initial report wasn't clear at all.

when debugging in the hashify lambda I can see that the value for value is {"token"=>"asdf"}

How are you instantiating Redis, this suggest you are passing protocol: 3, which you shouldn't.

billybonks commented 1 day ago

no worries, sorry about the clarity

I am using protocol 3, based on what you are saying sounds like support for this in v4.x was unintentional and 5.x fixed that unintentionally.

The reason why I am using protocol 3 is because sidekiq required it with its latest major version

byroot commented 1 day ago

The reason why I am using protocol 3 is because sidekiq required it with its latest major version

It's entirely orthogonal. Sidekiq uses it's own pool of raw redis-client instances. Just remove protocol: 3 and you'll be fine.

billybonks commented 1 day ago

👍🏿 that does fix the issue thanks, and thanks for the swift responses.

I have a few questions

  1. Does this lib only intend support for RESP2 and are there any reasons (I have not looked into the difference between the two just assume newer is better 😆

  2. Are you open to a pr that throws an error when resp3 is used? so this does not happen to others as well.

byroot commented 1 day ago
  1. Does this lib only intend support for RESP2

Yes.

are there any reasons

Mostly lack of interest, and lots of work for little gain. One would need to go over all the methods and make sure they do the right thing. Some of the casting is centralized so might not be as bad as I expect, but I don't see much value.

Are you open to a pr that throws an error when resp3 is used?

Yes, the argument should be rejected. PR welcome.