Finish postponed SCAN changes (#501)

Commit 07ed0ea introduced some SCAN improvements, but some
changes were postponed to a later version (8.0), which this PR finishes:

1. Prepare to move the TYPE filtering to the scan callback as well. this
was put on hold since it has side effects that can be considered a
breaking change, which is that we will not attempt to do lazy expire
(delete) a key that was filtered by not matching the TYPE (changing it
would mean TYPE filter starts behaving the same as MATCH filter already
does in that respect).
2. when the specified key TYPE filter is an unknown type, server will
reply a error immediately instead of doing a full scan that comes back
empty handed.

Fixes #235

Release notes:

> SCAN: Expired keys that don't match the TYPE argument for the SCAN are
no longer deleted by SCAN

Signed-off-by: Viktor Söderqvist <[email protected]>

src/db.c

@@ -885,11 +885,10 @@ void scanCallback(void *privdata, const dictEntry *de) {
     serverAssert(!((data->type != LLONG_MAX) && o));
 
     /* Filter an element if it isn't the type we want. */
-    /* TODO: uncomment in version 8.0
     if (!o && data->type != LLONG_MAX) {
         robj *rval = dictGetVal(de);
         if (!objectTypeCompare(rval, data->type)) return;
-    }*/
+    }
 
     /* Filter element if it does not match the pattern. */
     sds keysds = dictGetKey(de);
@@ -1034,9 +1033,8 @@ void scanGenericCommand(client *c, robj *o, unsigned long long cursor) {
             typename = c->argv[i+1]->ptr;
             type = getObjectTypeByName(typename);
             if (type == LLONG_MAX) {
-                /* TODO: uncomment in version 8.0
                 addReplyErrorFormat(c, "unknown type name '%s'", typename);
-                return; */
+                return;
             }
             i+= 2;
         } else if (!strcasecmp(c->argv[i]->ptr, "novalues")) {
@@ -1195,15 +1193,6 @@ void scanGenericCommand(client *c, robj *o, unsigned long long cursor) {
         while ((ln = listNext(&li))) {
             sds key = listNodeValue(ln);
             initStaticStringObject(kobj, key);
-            /* Filter an element if it isn't the type we want. */
-            /* TODO: remove this in version 8.0 */
-            if (typename) {
-                robj* typecheck = lookupKeyReadWithFlags(c->db, &kobj, LOOKUP_NOTOUCH|LOOKUP_NONOTIFY);
-                if (!typecheck || !objectTypeCompare(typecheck, type)) {
-                    listDelNode(keys, ln);
-                }
-                continue;
-            }
             if (expireIfNeeded(c->db, &kobj, 0) != KEY_VALID) {
                 listDelNode(keys, ln);
             }

tests/unit/scan.tcl

@@ -100,7 +100,8 @@ proc test_scan {type} {
 
     test "{$type} SCAN unknown type" {
         r flushdb
-        # make sure that passive expiration is triggered by the scan
+        # Check that passive expiration is not triggered by the scan on an
+        # unknown key type
         r debug set-active-expire 0
 
         populate 1000
@@ -109,25 +110,10 @@ proc test_scan {type} {
 
         after 2
 
-        # TODO: remove this in server version 8.0
-        set cur 0
-        set keys {}
-        while 1 {
-            set res [r scan $cur type "string1"]
-            set cur [lindex $res 0]
-            set k [lindex $res 1]
-            lappend keys {*}$k
-            if {$cur == 0} break
-        }
+        assert_error "*unknown type name*" {r scan 0 type "string1"}
 
-        assert_equal 0 [llength $keys]
-        # make sure that expired key have been removed by scan command
-        assert_equal 1000 [scan [regexp -inline {keys\=([\d]*)} [r info keyspace]] keys=%d]
-
-        # TODO: uncomment in server version 8.0
-        #assert_error "*unknown type name*" {r scan 0 type "string1"}
-        # expired key will be no touched by scan command
-        #assert_equal 1001 [scan [regexp -inline {keys\=([\d]*)} [r info keyspace]] keys=%d]
+        # expired key is not touched by scan command
+        assert_equal 1001 [scan [regexp -inline {keys\=([\d]*)} [r info keyspace]] keys=%d]
         r debug set-active-expire 1
     } {OK} {needs:debug}
 
@@ -191,11 +177,8 @@ proc test_scan {type} {
 
         assert_equal 1000 [llength $keys]
 
-        # make sure that expired key have been removed by scan command
-        assert_equal 1000 [scan [regexp -inline {keys\=([\d]*)} [r info keyspace]] keys=%d]
-        # TODO: uncomment in server version 8.0
         # make sure that only the expired key in the type match will been removed by scan command
-        #assert_equal 1001 [scan [regexp -inline {keys\=([\d]*)} [r info keyspace]] keys=%d]
+        assert_equal 1001 [scan [regexp -inline {keys\=([\d]*)} [r info keyspace]] keys=%d]
 
         r debug set-active-expire 1
     } {OK} {needs:debug}