Refactor XINFO handler

Fixes #2119

5 files changed, 46 insertions, 35 deletions

diff --git a/redis_cluster.stub.php b/redis_cluster.stub.php
index d4d9518a..b0792bd0 100644
--- a/redis_cluster.stub.php
+++ b/redis_cluster.stub.php
@@ -344,7 +344,7 @@ class RedisCluster {
 
     public function xgroup(string $operation, string $key = null, string $arg1 = null, string $arg2 = null, bool $arg3 = false): mixed;
 
-    public function xinfo(string $operation, string $arg1 = null, string $arg2 = null): mixed;
+    public function xinfo(string $operation, ?string $arg1 = null, ?string $arg2 = null, int $count = -1): mixed;
 
     public function xlen(string $key): int;
 
diff --git a/redis_cluster_arginfo.h b/redis_cluster_arginfo.h
index f3357408..3c5a1e9f 100644
--- a/redis_cluster_arginfo.h
+++ b/redis_cluster_arginfo.h
@@ -1,5 +1,5 @@
 /* This is a generated file, edit the .stub.php file instead.
- * Stub hash: 39c0741e5bf358e116f5ed2caa35c1ca226fd593 */
+ * Stub hash: 396a72b8937928cf3ed504a2a8063f5090e7196b */
 
 ZEND_BEGIN_ARG_INFO_EX(arginfo_class_RedisCluster___construct, 0, 0, 1)
 	ZEND_ARG_TYPE_INFO(0, name, IS_STRING, 1)
@@ -742,8 +742,9 @@ ZEND_END_ARG_INFO()
 
 ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_RedisCluster_xinfo, 0, 1, IS_MIXED, 0)
 	ZEND_ARG_TYPE_INFO(0, operation, IS_STRING, 0)
-	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, arg1, IS_STRING, 0, "null")
-	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, arg2, IS_STRING, 0, "null")
+	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, arg1, IS_STRING, 1, "null")
+	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, arg2, IS_STRING, 1, "null")
+	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, count, IS_LONG, 0, "-1")
 ZEND_END_ARG_INFO()
 
 #define arginfo_class_RedisCluster_xlen arginfo_class_RedisCluster_decr
diff --git a/redis_cluster_legacy_arginfo.h b/redis_cluster_legacy_arginfo.h
index fb31eefe..dd9e3196 100644
--- a/redis_cluster_legacy_arginfo.h
+++ b/redis_cluster_legacy_arginfo.h
@@ -1,5 +1,5 @@
 /* This is a generated file, edit the .stub.php file instead.
- * Stub hash: 39c0741e5bf358e116f5ed2caa35c1ca226fd593 */
+ * Stub hash: 396a72b8937928cf3ed504a2a8063f5090e7196b */
 
 ZEND_BEGIN_ARG_INFO_EX(arginfo_class_RedisCluster___construct, 0, 0, 1)
 	ZEND_ARG_INFO(0, name)
@@ -644,6 +644,7 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_class_RedisCluster_xinfo, 0, 0, 1)
 	ZEND_ARG_INFO(0, operation)
 	ZEND_ARG_INFO(0, arg1)
 	ZEND_ARG_INFO(0, arg2)
+	ZEND_ARG_INFO(0, count)
 ZEND_END_ARG_INFO()
 
 #define arginfo_class_RedisCluster_xlen arginfo_class_RedisCluster__prefix
diff --git a/redis_commands.c b/redis_commands.c
index a3dc4c9e..9a2395dc 100644
--- a/redis_commands.c
+++ b/redis_commands.c
@@ -5624,42 +5624,38 @@ int redis_xgroup_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
 int redis_xinfo_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
                      char **cmd, int *cmd_len, short *slot, void **ctx)
 {
-    char *op, *key, *arg = NULL;
-    size_t oplen, keylen, arglen;
+    zend_string *op = NULL, *key = NULL, *arg = NULL;
+    smart_string cmdstr = {0};
     zend_long count = -1;
-    int argc = ZEND_NUM_ARGS();
-    char fmt[] = "skssl";
-
-    if (argc > 4 || zend_parse_parameters(ZEND_NUM_ARGS(), "s|ssl",
-                                          &op, &oplen, &key, &keylen, &arg,
-                                          &arglen, &count) == FAILURE)
-    {
-        return FAILURE;
-    }
 
-    /* Handle everything except XINFO STREAM */
-    if (strncasecmp(op, "STREAM", 6) != 0) {
-        fmt[argc] = '\0';
-        *cmd_len = REDIS_CMD_SPPRINTF(cmd, "XINFO", fmt, op, oplen, key, keylen,
-                                      arg, arglen);
-        return SUCCESS;
-    }
+    ZEND_PARSE_PARAMETERS_START(1, 4)
+        Z_PARAM_STR(op)
+        Z_PARAM_OPTIONAL
+        Z_PARAM_STR_OR_NULL(key)
+        Z_PARAM_STR_OR_NULL(arg)
+        Z_PARAM_LONG(count)
+    ZEND_PARSE_PARAMETERS_END_EX(return FAILURE);
 
-    /* 'FULL' is the only legal option to XINFO STREAM */
-    if (argc > 2 && strncasecmp(arg, "FULL", 4) != 0) {
-        php_error_docref(NULL, E_WARNING, "'%s' is not a valid option for XINFO STREAM", arg);
+    if ((arg != NULL && key == NULL) || (count != -1 && (key == NULL || arg == NULL))) {
+        php_error_docref(NULL, E_WARNING, "Cannot pass a non-null optional argument after a NULL one.");
         return FAILURE;
     }
 
-    /* If we have a COUNT bump the argument count to account for the 'COUNT' literal */
-    if (argc == 4) argc++;
+    REDIS_CMD_INIT_SSTR_STATIC(&cmdstr, 1 + (key != NULL) + (arg != NULL) + (count > -1 ? 2 : 0), "XINFO");
+    redis_cmd_append_sstr_zstr(&cmdstr, op);
 
-    fmt[argc] = '\0';
+    if (key != NULL)
+        redis_cmd_append_sstr_key(&cmdstr, ZSTR_VAL(key), ZSTR_LEN(key), redis_sock, slot);
+    if (arg != NULL)
+        redis_cmd_append_sstr_zstr(&cmdstr, arg);
 
-    /* Build our XINFO STREAM variant */
-    *cmd_len = REDIS_CMD_SPPRINTF(cmd, "XINFO", fmt, "STREAM", 6, key, keylen,
-                                  "FULL", 4, "COUNT", 5, count);
+    if (count > -1) {
+        REDIS_CMD_APPEND_SSTR_STATIC(&cmdstr, "COUNT");
+        redis_cmd_append_sstr_long(&cmdstr, count);
+    }
 
+    *cmd = cmdstr.c;
+    *cmd_len = cmdstr.len;
     return SUCCESS;
 }
 
diff --git a/tests/RedisTest.php b/tests/RedisTest.php
index 4023c37b..044c7884 100644
--- a/tests/RedisTest.php
+++ b/tests/RedisTest.php
@@ -6742,9 +6742,9 @@ class Redis_Test extends TestSuite
 
     public function testXInfo()
     {
-        if (!$this->minVersionCheck("5.0")) {
-            return $this->markTestSkipped();
-        }
+        if (!$this->minVersionCheck("5.0"))
+            $this->markTestSkipped();
+
         /* Create some streams and groups */
         $stream = 's';
         $groups = ['g1' => 0, 'g2' => 0];
@@ -6767,6 +6767,12 @@ class Redis_Test extends TestSuite
             $this->assertTrue(is_array($info[$key]));
         }
 
+        /* Ensure that default/NULL arguments are ignored */
+        $info = $this->redis->xInfo('STREAM', $stream, NULL);
+        $this->assertTrue(is_array($info));
+        $info = $this->redis->xInfo('STREAM', $stream, NULL, -1);
+        $this->assertTrue(is_array($info));
+
         /* XINFO STREAM FULL [COUNT N] Requires >= 6.0.0 */
         if (!$this->minVersionCheck("6.0"))
             return;
@@ -6791,6 +6797,13 @@ class Redis_Test extends TestSuite
             $n = isset($info['entries']) ? count($info['entries']) : 0;
             $this->assertEquals($n, $this->redis->xLen($stream));
         }
+
+        /* Make sure we can't erroneously send non-null args after null ones */
+        $this->redis->clearLastError();
+        $this->assertFalse(@$this->redis->xInfo('FOO', NULL, 'fail', 25));
+        $this->assertEquals(NULL, $this->redis->getLastError());
+        $this->assertFalse(@$this->redis->xInfo('FOO', NULL, NULL, -2));
+        $this->assertEquals(NULL, $this->redis->getLastError());
     }
 
     /* Regression test for issue-1831 (XINFO STREAM on an empty stream) */