Test infra, handle RESP3 attributes and big-numbers and bools (#9235)

- promote the code in DEBUG PROTOCOL to addReplyBigNum
- DEBUG PROTOCOL ATTRIB skips the attribute when client is RESP2
- networking.c addReply for push and attributes generate assertion when
  called on a RESP2 client, anything else would produce a broken
  protocol that clients can't handle.

(cherry picked from commit 6a5bac3)
(cherry picked from commit 7f38aa8)

src/debug.c

@@ -654,7 +654,7 @@ NULL
         } else if (!strcasecmp(name,"double")) {
             addReplyDouble(c,3.14159265359);
         } else if (!strcasecmp(name,"bignum")) {
-            addReplyProto(c,"(1234567999999999999999999999999999999\r\n",40);
+            addReplyBigNum(c,"1234567999999999999999999999999999999",37);
         } else if (!strcasecmp(name,"null")) {
             addReplyNull(c);
         } else if (!strcasecmp(name,"array")) {
@@ -670,11 +670,13 @@ NULL
                 addReplyBool(c, j == 1);
             }
         } else if (!strcasecmp(name,"attrib")) {
-            addReplyAttributeLen(c,1);
-            addReplyBulkCString(c,"key-popularity");
-            addReplyArrayLen(c,2);
-            addReplyBulkCString(c,"key:123");
-            addReplyLongLong(c,90);
+            if (c->resp >= 3) {
+                addReplyAttributeLen(c,1);
+                addReplyBulkCString(c,"key-popularity");
+                addReplyArrayLen(c,2);
+                addReplyBulkCString(c,"key:123");
+                addReplyLongLong(c,90);
+            }
             /* Attributes are not real replies, so a well formed reply should
              * also have a normal reply type after the attribute. */
             addReplyBulkCString(c,"Some real reply following the attribute");

src/networking.c

@@ -589,14 +589,13 @@ void setDeferredSetLen(client *c, void *node, long length) {
 }
 
 void setDeferredAttributeLen(client *c, void *node, long length) {
-    int prefix = c->resp == 2 ? '*' : '|';
-    if (c->resp == 2) length *= 2;
-    setDeferredAggregateLen(c,node,length,prefix);
+    serverAssert(c->resp >= 3);
+    setDeferredAggregateLen(c,node,length,'|');
 }
 
 void setDeferredPushLen(client *c, void *node, long length) {
-    int prefix = c->resp == 2 ? '*' : '>';
-    setDeferredAggregateLen(c,node,length,prefix);
+    serverAssert(c->resp >= 3);
+    setDeferredAggregateLen(c,node,length,'>');
 }
 
 /* Add a double as a bulk reply */
@@ -625,6 +624,16 @@ void addReplyDouble(client *c, double d) {
     }
 }
 
+void addReplyBigNum(client *c, const char* num, size_t len) {
+    if (c->resp == 2) {
+        addReplyBulkCBuffer(c, num, len);
+    } else {
+        addReplyProto(c,"(",1);
+        addReplyProto(c,num,len);
+        addReply(c,shared.crlf);
+    }
+}
+
 /* Add a long double as a bulk reply, but uses a human readable formatting
  * of the double instead of exposing the crude behavior of doubles to the
  * dear user. */
@@ -698,14 +707,13 @@ void addReplySetLen(client *c, long length) {
 }
 
 void addReplyAttributeLen(client *c, long length) {
-    int prefix = c->resp == 2 ? '*' : '|';
-    if (c->resp == 2) length *= 2;
-    addReplyAggregateLen(c,length,prefix);
+    serverAssert(c->resp >= 3);
+    addReplyAggregateLen(c,length,'|');
 }
 
 void addReplyPushLen(client *c, long length) {
-    int prefix = c->resp == 2 ? '*' : '>';
-    addReplyAggregateLen(c,length,prefix);
+    serverAssert(c->resp >= 3);
+    addReplyAggregateLen(c,length,'>');
 }
 
 void addReplyNull(client *c) {

src/server.h

@@ -1695,6 +1695,7 @@ void addReplyErrorSds(client *c, sds err);
 void addReplyError(client *c, const char *err);
 void addReplyStatus(client *c, const char *status);
 void addReplyDouble(client *c, double d);
+void addReplyBigNum(client *c, const char* num, size_t len);
 void addReplyHumanLongDouble(client *c, long double d);
 void addReplyLongLong(client *c, long long ll);
 void addReplyArrayLen(client *c, long length);

tests/support/redis.tcl

@@ -244,27 +244,46 @@ proc ::redis::redis_read_null fd {
     return {}
 }
 
+proc ::redis::redis_read_bool fd {
+    set v [redis_read_line $fd]
+    if {$v == "t"} {return 1}
+    if {$v == "f"} {return 0}
+    return -code error "Bad protocol, '$v' as bool type"
+}
+
 proc ::redis::redis_read_reply {id fd} {
     if {$::redis::readraw($id)} {
         return [redis_read_line $fd]
     }
 
-    set type [read $fd 1]
-    switch -exact -- $type {
-        _ {redis_read_null $fd}
-        : -
-        + {redis_read_line $fd}
-        - {return -code error [redis_read_line $fd]}
-        $ {redis_bulk_read $fd}
-        > -
-        * {redis_multi_bulk_read $id $fd}
-        % {redis_read_map $id $fd}
-        default {
-            if {$type eq {}} {
-                set ::redis::fd($id) {}
-                return -code error "I/O error reading reply"
+    while {1} {
+        set type [read $fd 1]
+        switch -exact -- $type {
+            _ {return [redis_read_null $fd]}
+            : -
+            ( -
+            + {return [redis_read_line $fd]}
+            , {return [expr {double([redis_read_line $fd])}]}
+            # {return [redis_read_bool $fd]}
+            - {return -code error [redis_read_line $fd]}
+            $ {return [redis_bulk_read $fd]}
+            > -
+            ~ -
+            * {return [redis_multi_bulk_read $id $fd]}
+            % {return [redis_read_map $id $fd]}
+            | {
+                # ignore attributes for now (nowhere to store them)
+                redis_read_map $id $fd
+                continue
+            }
+            default {
+                if {$type eq {}} {
+                    catch {close $fd}
+                    set ::redis::fd($id) {}
+                    return -code error "I/O error reading reply"
+                }
+                return -code error "Bad protocol, '$type' as reply type byte"
             }
-            return -code error "Bad protocol, '$type' as reply type byte"
         }
     }
 }

tests/unit/protocol.tcl

@@ -136,6 +136,60 @@ start_server {tags {"protocol"}} {
 
     # check the connection still works
     assert_equal [r ping] {PONG}
+
+    test {RESP3 attributes} {
+        r hello 3
+        set res [r debug protocol attrib]
+        # currently the parser in redis.tcl ignores the attributes
+
+        # restore state
+        r hello 2
+        set _ $res
+    } {Some real reply following the attribute}
+
+    test {RESP3 attributes readraw} {
+        r hello 3
+        r readraw 1
+        r deferred 1
+
+        r debug protocol attrib
+        assert_equal [r read] {|1}
+        assert_equal [r read] {$14}
+        assert_equal [r read] {key-popularity}
+        assert_equal [r read] {*2}
+        assert_equal [r read] {$7}
+        assert_equal [r read] {key:123}
+        assert_equal [r read] {:90}
+        assert_equal [r read] {$39}
+        assert_equal [r read] {Some real reply following the attribute}
+
+        # restore state
+        r readraw 0
+        r deferred 0
+        r hello 2
+        set _ {}
+    } {}
+
+    test {RESP3 attributes on RESP2} {
+        r hello 2
+        set res [r debug protocol attrib]
+        set _ $res
+    } {Some real reply following the attribute}
+
+    test "test big number parsing" {
+        r hello 3
+        r debug protocol bignum
+    } {1234567999999999999999999999999999999}
+
+    test "test bool parsing" {
+        r hello 3
+        assert_equal [r debug protocol true] 1
+        assert_equal [r debug protocol false] 0
+        r hello 2
+        assert_equal [r debug protocol true] 1
+        assert_equal [r debug protocol false] 0
+        set _ {}
+    } {}
 }
 
 start_server {tags {"regression"}} {