Fix wrong order of key/value in Lua map response (#8266)

When a Lua script returns a map to redis (a feature which was added in
redis 6 together with RESP3), it would have returned the value first and
the key second.

If the client was using RESP2, it was getting them out of order, and if
the client was in RESP3, it was getting a map of value => key.
This was happening regardless of the Lua script using redis.setresp(3)
or not.

This also affects a case where the script was returning a map which it got
from from redis by doing something like: redis.setresp(3); return redis.call()

This fix is a breaking change for redis 6.0 users who happened to rely
on the wrong order (either ones that used redis.setresp(3), or ones that
returned a map explicitly).

This commit also includes other two changes in the tests:
1. The test suite now handles RESP3 maps as dicts rather than nested
   lists
2. Remove some redundant (duplicate) tests from tracking.tcl
This commit is contained in:
Oran Agra 2021-01-05 08:29:20 +02:00 committed by GitHub
parent df9c213bb0
commit 2017407b4d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 36 additions and 22 deletions

View File

@ -407,9 +407,9 @@ void luaReplyToRedisReply(client *c, lua_State *lua) {
lua_pushnil(lua); /* Use nil to start iteration. */ lua_pushnil(lua); /* Use nil to start iteration. */
while (lua_next(lua,-2)) { while (lua_next(lua,-2)) {
/* Stack now: table, key, value */ /* Stack now: table, key, value */
luaReplyToRedisReply(c, lua); /* Return value. */ lua_pushvalue(lua,-2); /* Dup key before consuming. */
lua_pushvalue(lua,-1); /* Dup key before consuming. */
luaReplyToRedisReply(c, lua); /* Return key. */ luaReplyToRedisReply(c, lua); /* Return key. */
luaReplyToRedisReply(c, lua); /* Return value. */
/* Stack now: table, key. */ /* Stack now: table, key. */
maplen++; maplen++;
} }

View File

@ -214,20 +214,19 @@ proc ::redis::redis_multi_bulk_read {id fd} {
proc ::redis::redis_read_map {id fd} { proc ::redis::redis_read_map {id fd} {
set count [redis_read_line $fd] set count [redis_read_line $fd]
if {$count == -1} return {} if {$count == -1} return {}
set l {} set d {}
set err {} set err {}
for {set i 0} {$i < $count} {incr i} { for {set i 0} {$i < $count} {incr i} {
if {[catch { if {[catch {
set t {} set k [redis_read_reply $id $fd] ; # key
lappend t [redis_read_reply $id $fd] ; # key set v [redis_read_reply $id $fd] ; # value
lappend t [redis_read_reply $id $fd] ; # value dict set d $k $v
lappend l $t
} e] && $err eq {}} { } e] && $err eq {}} {
set err $e set err $e
} }
} }
if {$err ne {}} {return -code error $err} if {$err ne {}} {return -code error $err}
return $l return $d
} }
proc ::redis::redis_read_line fd { proc ::redis::redis_read_line fd {

View File

@ -563,6 +563,30 @@ start_server {tags {"scripting"}} {
} e } e
set e set e
} {*wrong number*} } {*wrong number*}
test {Script with RESP3 map} {
set expected_dict [dict create field value]
set expected_list [list field value]
# Sanity test for RESP3 without scripts
r HELLO 3
r hset hash field value
set res [r hgetall hash]
assert_equal $res $expected_dict
# Test RESP3 client with script in both RESP2 and RESP3 modes
set res [r eval {redis.setresp(3); return redis.call('hgetall', KEYS[1])} 1 hash]
assert_equal $res $expected_dict
set res [r eval {redis.setresp(2); return redis.call('hgetall', KEYS[1])} 1 hash]
assert_equal $res $expected_list
# Test RESP2 client with script in both RESP2 and RESP3 modes
r HELLO 2
set res [r eval {redis.setresp(3); return redis.call('hgetall', KEYS[1])} 1 hash]
assert_equal $res $expected_list
set res [r eval {redis.setresp(2); return redis.call('hgetall', KEYS[1])} 1 hash]
assert_equal $res $expected_list
}
} }
# Start a new server since the last test in this stanza will kill the # Start a new server since the last test in this stanza will kill the

View File

@ -132,30 +132,21 @@ start_server {tags {"tracking"}} {
test {HELLO 3 reply is correct} { test {HELLO 3 reply is correct} {
set reply [r HELLO 3] set reply [r HELLO 3]
assert {[lindex $reply 2] eq {proto 3}} assert_equal [dict get $reply proto] 3
} }
test {HELLO without protover} { test {HELLO without protover} {
set reply [r HELLO 3] set reply [r HELLO 3]
assert {[lindex $reply 2] eq {proto 3}} assert_equal [dict get $reply proto] 3
set reply [r HELLO] set reply [r HELLO]
assert {[lindex $reply 2] eq {proto 3}} assert_equal [dict get $reply proto] 3
set reply [r HELLO]
assert {[lindex $reply 2] eq {proto 3}}
set reply [r HELLO 2] set reply [r HELLO 2]
assert {[lindex $reply 4] eq "proto"} assert_equal [dict get $reply proto] 2
assert {[lindex $reply 5] eq 2}
set reply [r HELLO] set reply [r HELLO]
assert {[lindex $reply 4] eq "proto"} assert_equal [dict get $reply proto] 2
assert {[lindex $reply 5] eq 2}
set reply [r HELLO]
assert {[lindex $reply 4] eq "proto"}
assert {[lindex $reply 5] eq 2}
# restore RESP3 for next test # restore RESP3 for next test
r HELLO 3 r HELLO 3