From 075ee496e82bd0ed1b97b9ff2595715114ee60aa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= <viktor.soderqvist@est.tech>
Date: Fri, 1 Sep 2023 13:14:44 +0200
Subject: [PATCH] Fix code for XREAD and XREADGROUP and add test case (#187)

The code for finding keys in XREAD and XREADGROUP commands was untested and incorrect. Here it is fixed and a tests are added.
---
 command.c            | 33 +++++++++++++++++----------------
 tests/ut_parse_cmd.c | 26 ++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/command.c b/command.c
index 9b42513..856504f 100644
--- a/command.c
+++ b/command.c
@@ -274,28 +274,29 @@ void redis_parse_cmd(struct cmd *r) {
             goto error;
         }
 
-        /* Skip forward to the 'startfrom' arg index. */
+        /* Skip forward to the 'startfrom' arg index, then search for the keyword. */
         arg = arg1;
         arglen = arg1_len;
-        while (argidx < startfrom) {
+        while (argidx < (int)rnarg - 1) {
             if ((p = redis_parse_bulk(p, end, &arg, &arglen)) == NULL)
                 goto error; /* Keyword not provided, thus no keys. */
-            argidx++;
+            if (argidx++ < startfrom)
+                continue; /* Keyword can't appear in a position before 'startfrom' */
+            if (!strncasecmp(keyword, arg, arglen)) {
+                /* Keyword found. Now the first key is the next arg. */
+                if ((p = redis_parse_bulk(p, end, &arg, &arglen)) == NULL)
+                    goto error;
+                struct keypos *kpos = hiarray_push(r->keys);
+                if (kpos == NULL)
+                    goto oom;
+                kpos->start = arg;
+                kpos->end = arg + arglen;
+                goto done;
+            }
         }
 
-        /* Check that we found the keyword. */
-        if (strncasecmp(keyword, arg, arglen))
-            goto error;
-
-        /* Now find key is the next arg. */
-        if ((p = redis_parse_bulk(p, end, &arg, &arglen)) == NULL)
-            goto error;
-        struct keypos *kpos = hiarray_push(r->keys);
-        if (kpos == NULL)
-            goto oom;
-        kpos->start = arg;
-        kpos->end = arg + arglen;
-        goto done;
+        /* Keyword not provided. */
+        goto error;
     }
 
     /* Find first key arg. */
diff --git a/tests/ut_parse_cmd.c b/tests/ut_parse_cmd.c
index 60e411a..d662be9 100644
--- a/tests/ut_parse_cmd.c
+++ b/tests/ut_parse_cmd.c
@@ -131,6 +131,30 @@ void test_redis_parse_cmd_xgroup_destroy_ok(void) {
     command_destroy(c);
 }
 
+void test_redis_parse_cmd_xreadgroup_ok(void) {
+    struct cmd *c = command_get();
+    /* Use group name and consumer name "streams" just to try to confuse the
+     * parser. The parser shouldn't mistake those for the STREAMS keyword. */
+    int len = redisFormatCommand(
+        &c->cmd, "XREADGROUP GROUP streams streams COUNT 1 streams mystream >");
+    ASSERT_MSG(len >= 0, "Format command error");
+    c->clen = len;
+    redis_parse_cmd(c);
+    ASSERT_KEYS(c, "mystream");
+    command_destroy(c);
+}
+
+void test_redis_parse_cmd_xread_ok(void) {
+    struct cmd *c = command_get();
+    int len = redisFormatCommand(&c->cmd,
+                                 "XREAD BLOCK 42 STREAMS mystream another $ $");
+    ASSERT_MSG(len >= 0, "Format command error");
+    c->clen = len;
+    redis_parse_cmd(c);
+    ASSERT_KEYS(c, "mystream");
+    command_destroy(c);
+}
+
 int main(void) {
     test_redis_parse_error_nonresp();
     test_redis_parse_cmd_get();
@@ -140,5 +164,7 @@ int main(void) {
     test_redis_parse_cmd_xgroup_no_subcommand();
     test_redis_parse_cmd_xgroup_destroy_no_key();
     test_redis_parse_cmd_xgroup_destroy_ok();
+    test_redis_parse_cmd_xreadgroup_ok();
+    test_redis_parse_cmd_xread_ok();
     return 0;
 }