From 8149476f66d6e1e2531e622fd35e81cf3c0cd87c Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Wed, 14 Jul 2021 19:14:31 +0300 Subject: [PATCH] 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 6a5bac309e868deef749c36949723b415de2496f) --- src/debug.c | 14 ++++++----- src/networking.c | 28 +++++++++++++-------- src/server.h | 1 + tests/support/redis.tcl | 52 +++++++++++++++++++++++++-------------- tests/unit/protocol.tcl | 54 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 115 insertions(+), 34 deletions(-) diff --git a/src/debug.c b/src/debug.c index 098ce6ef7..f521490a7 100644 --- a/src/debug.c +++ b/src/debug.c @@ -721,7 +721,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")) { @@ -737,11 +737,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"); diff --git a/src/networking.c b/src/networking.c index d59e49592..51c58ff20 100644 --- a/src/networking.c +++ b/src/networking.c @@ -649,14 +649,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 */ @@ -685,6 +684,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. */ @@ -756,14 +765,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) { diff --git a/src/server.h b/src/server.h index 64d08db60..f57a720e9 100644 --- a/src/server.h +++ b/src/server.h @@ -1840,6 +1840,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); diff --git a/tests/support/redis.tcl b/tests/support/redis.tcl index 285b53574..978163e98 100644 --- a/tests/support/redis.tcl +++ b/tests/support/redis.tcl @@ -247,30 +247,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} - , {expr {double([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 {}} { - catch {close $fd} - 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" } } } diff --git a/tests/unit/protocol.tcl b/tests/unit/protocol.tcl index ffc751f8f..a3d8f7e89 100644 --- a/tests/unit/protocol.tcl +++ b/tests/unit/protocol.tcl @@ -136,6 +136,60 @@ start_server {tags {"protocol network"}} { # 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"}} {