jedis icon indicating copy to clipboard operation
jedis copied to clipboard

Fix return value of HRANDFIELD With Values when count is negative

Open BalaKaza opened this issue 2 years ago • 3 comments

### Expected behavior

hrandfieldWithValues(String key, long count) should return the count equivalent entries in case of a negative (absolute value of count) or a positive count value.

Actual behavior

hrandfieldWithValues(String key, long count) currently returns a Map<String, String> which does not account for the duplicates that can be returned(https://redis.io/commands/hrandfield/) when a negative count is given. This results in an inaccurate number (less than the count) of random entries being returned when there is a negative count.

Steps to reproduce:

  1. Create a Hash Set with size 5
  2. Call hrandfieldWithValues(String key, long count) with a count of -3.
  3. Assert for the size of the result.
Map<String, String> hash = createHash(5); 
jedis.hset(KEY, hash); 
long count = -3L; 
Map<String, String> result = jedis.hrandfieldWithValues(KEY, count); 
assertThat(result.size()).isEqualTo(Math.abs(count));

Redis / Jedis Configuration

Jedis version: 3.7.0

Redis version: 6.2.0

Java version: 8

BalaKaza avatar Jun 09 '22 21:06 BalaKaza

As per Redis documentation -

When the count is a negative value, the behavior changes as follows:

Repeating fields are possible. Exactly count fields, or an empty array if the hash is empty (non-existing key), are always returned. The order of fields in the reply is truly random.

So output may contain repeating fields when the count is a negative value. Current implementation uses "Map<String, String>" for response builder which will not account for repeating fields.

@sazzad16 A map which stores collection of values for a key will work good here ? Something like this -

public static final Builder<Map<String, List<String>>> STRING_MULTI_MAP = new Builder<Map<String, List<String>>>() {
    @Override
    @SuppressWarnings("unchecked")
    public Map<String, List<String>> build(Object data) {
      final List<byte[]> flatHash = (List<byte[]>) data;
      final Map<String, List<String>> hash = new HashMap<>(flatHash.size() / 2, 1);
      final Iterator<byte[]> iterator = flatHash.iterator();
      while (iterator.hasNext()) {
        String key = STRING.build(iterator.next());
        String value = STRING.build(iterator.next());
        List<String> list = hash.containsKey(key) ? hash.get(key) : new ArrayList<>();
        list.add(value);
        hash.put(key, list);
      }

      return hash;
    }

    @Override
    public String toString() {
      return "Map<String, List<String>>";
    }

};

The new return type will be "Map<String, List<String>>". Count of collections for all keys will be equivalent to the absolute value of input count.

s-sathish avatar Jun 15 '22 12:06 s-sathish

@s-sathish It would be simpler and better to return a List of pairs.

sazzad16 avatar Jun 15 '22 12:06 sazzad16

@sazzad16 Thanks. Yes, List of pairs works good here.

s-sathish avatar Jun 15 '22 14:06 s-sathish

Resolved by #3425 and #3430

sazzad16 avatar May 21 '23 14:05 sazzad16