mget will fail with 0 arguments

abbccdda commented 2 years ago

Expected behavior

calling mget() https://github.com/redis/jedis/blob/ef8d545be59b350f5e543726cf921aa46bcf2ad9/src/main/java/redis/clients/jedis/Jedis.java#L809 with 0 argument should just work

Actual behavior

it fails with error:

redis.clients.jedis.exceptions.JedisDataException: ERR wrong number of arguments for 'mget' command

    at redis.clients.jedis.Protocol.processError(Protocol.java:132)
    at redis.clients.jedis.Protocol.process(Protocol.java:166)
    at redis.clients.jedis.Protocol.read(Protocol.java:220)
    at redis.clients.jedis.Connection.readProtocolWithCheckingBroken(Connection.java:283)
    at redis.clients.jedis.Connection.getBinaryMultiBulkReply(Connection.java:235)
    at redis.clients.jedis.BinaryJedis.mget(BinaryJedis.java:672)
    at io.rockset.main.cache.redis.JedisClient.getBytesBatch(JedisClient.java:180)

which makes the API no longer valid as accepting 0 or more arguments

Steps to reproduce:

Could be reproduced with a unit test

Redis / Jedis Configuration

Jedis version:

Redis version:

Java version:

sazzad16 commented 2 years ago

@abbccdda We take that the user would handle not calling with 0 arguments.

abbccdda commented 2 years ago

@sazzad16 is this documented somewhere? Could you add a meta comment to draw user's attention?

sazzad16 commented 2 years ago

@abbccdda

is this documented somewhere?

I'm not sure. It may (or may not) exist in Wiki.

Could you add a meta comment to draw user's attention?

It'll take some time. Sorry.

Sometimes we just depend on Redis doc (i.e. MGET) for command clarification. We're trying to improve doc but it's going to take some (or more) time.

Akaame commented 2 years ago

One way to go about this change public functions as

public List<Foo> bar(final String key) { }
public List<Foo> bar(final String key, final String... keys) { }

// or directly
public List<Foo> bar(final String key, final String... keys) { }

But that requires a prepend operation to the array when you are passing this list to an internal call (CommandArguments::keys for example) and overall a huge code change as most of the commands have this at-least-one element variable arg behavior.

github-actions[bot] commented 8 months ago

This issue is marked stale. It will be closed in 30 days if it is not updated.

redis / jedis