From b978abbf022b1810031b5aaa171ef0899f7fe77d Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Sun, 13 Jun 2010 11:16:18 +0200 Subject: [PATCH] small refactor of SMOVE and tests for SMOVE on sets with different encoding --- redis.c | 51 +++++++++++++---------- tests/unit/type/set.tcl | 89 ++++++++++++++++++++++++++++------------- 2 files changed, 92 insertions(+), 48 deletions(-) diff --git a/redis.c b/redis.c index c1df3293..51ca6d2a 100644 --- a/redis.c +++ b/redis.c @@ -5694,37 +5694,46 @@ static void sremCommand(redisClient *c) { } static void smoveCommand(redisClient *c) { - robj *srcset, *dstset; - + robj *srcset, *dstset, *ele; srcset = lookupKeyWrite(c->db,c->argv[1]); dstset = lookupKeyWrite(c->db,c->argv[2]); + ele = c->argv[3]; - /* If the source key does not exist return 0, if it's of the wrong type - * raise an error */ - if (srcset == NULL || srcset->type != REDIS_SET) { - addReply(c, srcset ? shared.wrongtypeerr : shared.czero); - return; - } - /* Error if the destination key is not a set as well */ - if (dstset && dstset->type != REDIS_SET) { - addReply(c,shared.wrongtypeerr); - return; - } - /* Remove the element from the source set */ - if (!setTypeRemove(srcset,c->argv[3])) { - /* Key not found in the src set! return zero */ + /* If the source key does not exist return 0 */ + if (srcset == NULL) { addReply(c,shared.czero); return; } - if (setTypeSize(srcset) == 0 && srcset != dstset) - dbDelete(c->db,c->argv[1]); + + /* If the source key has the wrong type, or the destination key + * is set and has the wrong type, return with an error. */ + if (checkType(c,srcset,REDIS_SET) || + (dstset && checkType(c,dstset,REDIS_SET))) return; + + /* If srcset and dstset are equal, SMOVE is a no-op */ + if (srcset == dstset) { + addReply(c,shared.cone); + return; + } + + /* If the element cannot be removed from the src set, return 0. */ + if (!setTypeRemove(srcset,ele)) { + addReply(c,shared.czero); + return; + } + + /* Remove the src set from the database when empty */ + if (setTypeSize(srcset) == 0) dbDelete(c->db,c->argv[1]); server.dirty++; - /* Add the element to the destination set */ + + /* Create the destination set when it doesn't exist */ if (!dstset) { - dstset = setTypeCreate(c->argv[3]); + dstset = setTypeCreate(ele); dbAdd(c->db,c->argv[2],dstset); } - setTypeAdd(dstset,c->argv[3]); + + /* An extra key has changed when ele was successfully added to dstset */ + if (setTypeAdd(dstset,ele)) server.dirty++; addReply(c,shared.cone); } diff --git a/tests/unit/type/set.tcl b/tests/unit/type/set.tcl index c4fd4d76..5b8d961e 100644 --- a/tests/unit/type/set.tcl +++ b/tests/unit/type/set.tcl @@ -180,38 +180,73 @@ start_server {tags {"set"}} { } } - test {SMOVE basics} { - r sadd myset1 a - r sadd myset1 b - r sadd myset1 c - r sadd myset2 x - r sadd myset2 y - r sadd myset2 z - r smove myset1 myset2 a - list [lsort [r smembers myset2]] [lsort [r smembers myset1]] - } {{a x y z} {b c}} + proc setup_move {} { + r del myset3 myset4 + create_set myset1 {1 a b} + create_set myset2 {2 3 4} + assert_encoding hashtable myset1 + assert_encoding intset myset2 + } - test {SMOVE non existing key} { - list [r smove myset1 myset2 foo] [lsort [r smembers myset2]] [lsort [r smembers myset1]] - } {0 {a x y z} {b c}} + test "SMOVE basics - from regular set to intset" { + # move a non-integer element to an intset should convert encoding + setup_move + assert_equal 1 [r smove myset1 myset2 a] + assert_equal {1 b} [lsort [r smembers myset1]] + assert_equal {2 3 4 a} [lsort [r smembers myset2]] + assert_encoding hashtable myset2 - test {SMOVE non existing src set} { - list [r smove noset myset2 foo] [lsort [r smembers myset2]] - } {0 {a x y z}} + # move an integer element should not convert the encoding + setup_move + assert_equal 1 [r smove myset1 myset2 1] + assert_equal {a b} [lsort [r smembers myset1]] + assert_equal {1 2 3 4} [lsort [r smembers myset2]] + assert_encoding intset myset2 + } - test {SMOVE non existing dst set} { - list [r smove myset2 myset3 y] [lsort [r smembers myset2]] [lsort [r smembers myset3]] - } {1 {a x z} y} + test "SMOVE basics - from intset to regular set" { + setup_move + assert_equal 1 [r smove myset2 myset1 2] + assert_equal {1 2 a b} [lsort [r smembers myset1]] + assert_equal {3 4} [lsort [r smembers myset2]] + } - test {SMOVE wrong src key type} { + test "SMOVE non existing key" { + setup_move + assert_equal 0 [r smove myset1 myset2 foo] + assert_equal {1 a b} [lsort [r smembers myset1]] + assert_equal {2 3 4} [lsort [r smembers myset2]] + } + + test "SMOVE non existing src set" { + setup_move + assert_equal 0 [r smove noset myset2 foo] + assert_equal {2 3 4} [lsort [r smembers myset2]] + } + + test "SMOVE from regular set to non existing destination set" { + setup_move + assert_equal 1 [r smove myset1 myset3 a] + assert_equal {1 b} [lsort [r smembers myset1]] + assert_equal {a} [lsort [r smembers myset3]] + assert_encoding hashtable myset3 + } + + test "SMOVE from intset to non existing destination set" { + setup_move + assert_equal 1 [r smove myset2 myset3 2] + assert_equal {3 4} [lsort [r smembers myset2]] + assert_equal {2} [lsort [r smembers myset3]] + assert_encoding intset myset3 + } + + test "SMOVE wrong src key type" { r set x 10 - catch {r smove x myset2 foo} err - format $err - } {ERR*} + assert_error "ERR*wrong kind*" {r smove x myset2 foo} + } - test {SMOVE wrong dst key type} { + test "SMOVE wrong dst key type" { r set x 10 - catch {r smove myset2 x foo} err - format $err - } {ERR*} + assert_error "ERR*wrong kind*" {r smove myset2 x foo} + } }