diff --git a/src/module.c b/src/module.c index 2854d157a..ff274825c 100644 --- a/src/module.c +++ b/src/module.c @@ -4147,10 +4147,7 @@ int RM_ListPush(RedisModuleKey *key, int where, RedisModuleString *ele) { if (!(key->mode & REDISMODULE_WRITE)) return REDISMODULE_ERR; if (key->value && key->value->type != OBJ_LIST) return REDISMODULE_ERR; - if (key->iter) { - listTypeReleaseIterator(key->iter); - key->iter = NULL; - } + if (key->iter) moduleFreeKeyIterator(key); if (key->value == NULL) moduleCreateEmptyKey(key,REDISMODULE_KEYTYPE_LIST); listTypePush(key->value, ele, (where == REDISMODULE_LIST_HEAD) ? LIST_HEAD : LIST_TAIL); @@ -4180,10 +4177,7 @@ RedisModuleString *RM_ListPop(RedisModuleKey *key, int where) { errno = EBADF; return NULL; } - if (key->iter) { - listTypeReleaseIterator(key->iter); - key->iter = NULL; - } + if (key->iter) moduleFreeKeyIterator(key); robj *ele = listTypePop(key->value, (where == REDISMODULE_LIST_HEAD) ? LIST_HEAD : LIST_TAIL); robj *decoded = getDecodedObject(ele); @@ -4246,8 +4240,7 @@ int RM_ListSet(RedisModuleKey *key, long index, RedisModuleString *value) { listTypeReplace(&key->u.list.entry, value); /* A note in quicklist.c forbids use of iterator after insert, so * probably also after replace. */ - listTypeReleaseIterator(key->iter); - key->iter = NULL; + moduleFreeKeyIterator(key); return REDISMODULE_OK; } else { return REDISMODULE_ERR; @@ -4292,8 +4285,7 @@ int RM_ListInsert(RedisModuleKey *key, long index, RedisModuleString *value) { int where = index < 0 ? LIST_TAIL : LIST_HEAD; listTypeInsert(&key->u.list.entry, value, where); /* A note in quicklist.c forbids use of iterator after insert. */ - listTypeReleaseIterator(key->iter); - key->iter = NULL; + moduleFreeKeyIterator(key); return REDISMODULE_OK; } else { return REDISMODULE_ERR; @@ -4314,7 +4306,24 @@ int RM_ListInsert(RedisModuleKey *key, long index, RedisModuleString *value) { int RM_ListDelete(RedisModuleKey *key, long index) { if (moduleListIteratorSeek(key, index, REDISMODULE_WRITE)) { listTypeDelete(key->iter, &key->u.list.entry); - moduleDelKeyIfEmpty(key); + if (moduleDelKeyIfEmpty(key)) return REDISMODULE_OK; + if (listTypeNext(key->iter, &key->u.list.entry)) { + /* After delete entry at position 'index', we need to update + * 'key->u.list.index' according to the following cases: + * 1) [1, 2, 3] => dir: forward, index: 0 => [2, 3] => index: still 0 + * 2) [1, 2, 3] => dir: forward, index: -3 => [2, 3] => index: -2 + * 3) [1, 2, 3] => dir: reverse, index: 2 => [1, 2] => index: 1 + * 4) [1, 2, 3] => dir: reverse, index: -1 => [1, 2] => index: still -1 */ + listTypeIterator *li = key->iter; + int reverse = li->direction == LIST_HEAD; + if (key->u.list.index < 0) + key->u.list.index += reverse ? 0 : 1; + else + key->u.list.index += reverse ? -1 : 0; + } else { + /* Reset list iterator if the next entry doesn't exist. */ + moduleFreeKeyIterator(key); + } return REDISMODULE_OK; } else { return REDISMODULE_ERR; diff --git a/tests/modules/list.c b/tests/modules/list.c index 727b05d6f..401b2d802 100644 --- a/tests/modules/list.c +++ b/tests/modules/list.c @@ -50,7 +50,8 @@ int list_getall(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { * The number of occurrences of "i" and "r" in cmdstr) should correspond to the * number of args after cmdstr. * - * The reply is the number of edits (inserts + replaces + deletes) performed. + * Reply with a RESP3 Map, containing the number of edits (inserts, replaces, deletes) + * performed, as well as the last index and the entry it points to. */ int list_edit(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { if (argc < 3) return RedisModule_WrongArity(ctx); @@ -92,7 +93,7 @@ int list_edit(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { } /* Iterate over the chars in cmdstr (edit instructions) */ - long long num_edits = 0; + long long num_inserts = 0, num_deletes = 0, num_replaces = 0; long index = reverse ? -1 : 0; RedisModuleString *value; @@ -102,17 +103,17 @@ int list_edit(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { value = argv[argpos++]; assert(RedisModule_ListInsert(key, index, value) == REDISMODULE_OK); index += reverse ? -1 : 1; - num_edits++; + num_inserts++; break; case 'd': /* delete */ assert(RedisModule_ListDelete(key, index) == REDISMODULE_OK); - num_edits++; + num_deletes++; break; case 'r': /* replace */ value = argv[argpos++]; assert(RedisModule_ListSet(key, index, value) == REDISMODULE_OK); index += reverse ? -1 : 1; - num_edits++; + num_replaces++; break; case 'k': /* keep */ index += reverse ? -1 : 1; @@ -120,7 +121,22 @@ int list_edit(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { } } - RedisModule_ReplyWithLongLong(ctx, num_edits); + RedisModuleString *v = RedisModule_ListGet(key, index); + RedisModule_ReplyWithMap(ctx, v ? 5 : 4); + RedisModule_ReplyWithCString(ctx, "i"); + RedisModule_ReplyWithLongLong(ctx, num_inserts); + RedisModule_ReplyWithCString(ctx, "d"); + RedisModule_ReplyWithLongLong(ctx, num_deletes); + RedisModule_ReplyWithCString(ctx, "r"); + RedisModule_ReplyWithLongLong(ctx, num_replaces); + RedisModule_ReplyWithCString(ctx, "index"); + RedisModule_ReplyWithLongLong(ctx, index); + if (v) { + RedisModule_ReplyWithCString(ctx, "entry"); + RedisModule_ReplyWithString(ctx, v); + RedisModule_FreeString(ctx, v); + } + RedisModule_CloseKey(key); return REDISMODULE_OK; } diff --git a/tests/unit/moduleapi/list.tcl b/tests/unit/moduleapi/list.tcl index 34a19767c..0c440555d 100644 --- a/tests/unit/moduleapi/list.tcl +++ b/tests/unit/moduleapi/list.tcl @@ -1,5 +1,17 @@ set testmodule [file normalize tests/modules/list.so] +# The following arguments can be passed to args: +# i -- the number of inserts +# d -- the number of deletes +# r -- the number of replaces +# index -- the last index +# entry -- The entry pointed to by index +proc verify_list_edit_reply {reply argv} { + foreach {k v} $argv { + assert_equal [dict get $reply $k] $v + } +} + start_server {tags {"modules"}} { r module load $testmodule @@ -39,31 +51,73 @@ start_server {tags {"modules"}} { test {Module list insert & delete} { r del k r rpush k x y z - r list.edit k ikikdi foo bar baz + verify_list_edit_reply [r list.edit k ikikdi foo bar baz] {i 3 index 5} r list.getall k } {foo x bar y baz} test {Module list insert & delete, neg index} { r del k r rpush k x y z - r list.edit k REVERSE ikikdi foo bar baz + verify_list_edit_reply [r list.edit k REVERSE ikikdi foo bar baz] {i 3 index -6} r list.getall k } {baz y bar z foo} test {Module list set while iterating} { r del k r rpush k x y z - r list.edit k rkr foo bar + verify_list_edit_reply [r list.edit k rkr foo bar] {r 2 index 3} r list.getall k } {foo y bar} test {Module list set while iterating, neg index} { r del k r rpush k x y z - r list.edit k reverse rkr foo bar + verify_list_edit_reply [r list.edit k reverse rkr foo bar] {r 2 index -4} r list.getall k } {bar y foo} + test {Module list - list entry and index should be updated when deletion} { + set original_config [config_get_set list-max-listpack-size 1] + + # delete from start (index 0) + r del l + r rpush l x y z + verify_list_edit_reply [r list.edit l dd] {d 2 index 0 entry z} + assert_equal [r list.getall l] {z} + + # delete from start (index -3) + r del l + r rpush l x y z + verify_list_edit_reply [r list.edit l reverse kkd] {d 1 index -3} + assert_equal [r list.getall l] {y z} + + # # delete from tail (index 2) + r del l + r rpush l x y z + verify_list_edit_reply [r list.edit l kkd] {d 1 index 2} + assert_equal [r list.getall l] {x y} + + # # delete from tail (index -1) + r del l + r rpush l x y z + verify_list_edit_reply [r list.edit l reverse dd] {d 2 index -1 entry x} + assert_equal [r list.getall l] {x} + + # # delete from middle (index 1) + r del l + r rpush l x y z + verify_list_edit_reply [r list.edit l kdd] {d 2 index 1} + assert_equal [r list.getall l] {x} + + # # delete from middle (index -2) + r del l + r rpush l x y z + verify_list_edit_reply [r list.edit l reverse kdd] {d 2 index -2} + assert_equal [r list.getall l] {z} + + config_set list-max-listpack-size $original_config + } + test "Unload the module - list" { assert_equal {OK} [r module unload list] }