Make the XREADGROUP optional COUNT and BLOCK arguments nullable.

Change the way we accept COUNT and BLOCK such that a user can pass NULL to mean "no value". This is technically a breaking change, since previously the value `-1` was used for "no value". Fixes #1560
phpredis · Jun 4, 2019 · 0c17bd2 · 0c17bd2
1 parent 6e49417
commit 0c17bd2
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 7 deletions.
diff --git a/redis_commands.c b/redis_commands.c
@@ -3466,23 +3466,30 @@ int redis_xreadgroup_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
     char *group, *consumer;
     size_t grouplen, consumerlen;
     int scount, argc;
-    zend_long count = -1, block = -1;
+    zend_long count, block;
+    zend_bool no_count = 1, no_block = 1;
 
-    if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "ssa|ll", &group,
+    if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "ssa|l!l!", &group,
                               &grouplen, &consumer, &consumerlen, &z_streams,
-                              &count, &block) == FAILURE)
+                              &count, &no_count, &block, &no_block) == FAILURE)
     {
         return FAILURE;
     }
 
+    /* Negative COUNT or BLOCK is illegal so abort immediately */
+    if ((!no_count && count < 0) || (!no_block && block < 0)) {
+        php_error_docref(NULL TSRMLS_CC, E_WARNING, "Negative values for COUNT or BLOCK are illegal.");
+        return FAILURE;
+    }
+
     /* Redis requires at least one stream */
     kt = Z_ARRVAL_P(z_streams);
     if ((scount = zend_hash_num_elements(kt)) < 1) {
         return FAILURE;
     }
 
     /* Calculate argc and start constructing commands */
-    argc = 4 + (2 * scount) + (2 * (count > -1)) + (2 * (block > -1));
+    argc = 4 + (2 * scount) + (2 * !no_count) + (2 * !no_block);
     REDIS_CMD_INIT_SSTR_STATIC(&cmdstr, argc, "XREADGROUP");
 
     /* Group and consumer */
@@ -3491,13 +3498,13 @@ int redis_xreadgroup_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
     redis_cmd_append_sstr(&cmdstr, consumer, consumerlen);
 
     /* Append COUNT if we have it */
-    if (count > -1) {
+    if (!no_count) {
         REDIS_CMD_APPEND_SSTR_STATIC(&cmdstr, "COUNT");
         redis_cmd_append_sstr_long(&cmdstr, count);
     }
 
     /* Append BLOCK argument if we have it */
-    if (block > -1) {
+    if (!no_block) {
         REDIS_CMD_APPEND_SSTR_STATIC(&cmdstr, "BLOCK");
         redis_cmd_append_sstr_long(&cmdstr, block);
     }

diff --git a/tests/RedisTest.php b/tests/RedisTest.php
@@ -5687,12 +5687,29 @@ public function testXReadGroup() {
             }
         }
 
+        /* Test COUNT option with NULL (should be ignored) */
+        $this->addStreamsAndGroups($streams, 3, $groups, NULL);
+        $resp = $this->redis->xReadGroup('g1', 'consumer', $query1, NULL);
+        foreach ($resp as $stream => $smsg) {
+            $this->assertEquals(count($smsg), 3);
+        }
+
         /* Finally test BLOCK with a sloppy timing test */
         $t1 = $this->mstime();
         $qnew = ['{s}-1' => '>', '{s}-2' => '>'];
-        $this->redis->xReadGroup('g1', 'c1', $qnew, -1, 100);
+        $this->redis->xReadGroup('g1', 'c1', $qnew, NULL, 100);
         $t2 = $this->mstime();
         $this->assertTrue($t2 - $t1 >= 100);
+
+        /* Make sure passing NULL to block doesn't block */
+        $t1 = $this->mstime();
+        $this->redis->xReadGroup('g1', 'c1', $qnew, NULL, NULL);
+        $t2 = $this->mstime();
+        $this->assertTrue($t2 - $t1 < 100);
+
+        /* Make sure passing bad values to BLOCK or COUNT immediately fails */
+        $this->assertFalse(@$this->redis->xReadGroup('g1', 'c1', $qnew, -1));
+        $this->assertFalse(@$this->redis->xReadGroup('g1', 'c1', $qnew, NULL, -1));
     }
 
     public function testXPending() {