VLV: handle empty results correctly

The VLV was wrongly returning an operations error when the list of
results was empty.

Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Pair-programmed-with: Garming Sam <garming@catalyst.net.nz>
Reviewed-by: Garming Sam <garming@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>

source4/dsdb/samdb/ldb_modules/vlv_pagination.c

@@ -355,48 +355,57 @@ static int vlv_results(struct vlv_context *ac)
 	vlv_details = ac->store->vlv_details;
 	sort_details = ac->store->sort_details;
 
-	if (vlv_details->type == 1) {
+	if (ac->store->num_entries != 0) {
-		target = vlv_gt_eq_to_index(ac, ac->store->results, vlv_details,
+		if (vlv_details->type == 1) {
-					    sort_details, &ret);
+			target = vlv_gt_eq_to_index(ac, ac->store->results,
-		if (ret != LDB_SUCCESS) {
+						    vlv_details,
-			return ret;
+						    sort_details, &ret);
+			if (ret != LDB_SUCCESS) {
+				return ret;
+			}
+		} else {
+			target = vlv_calc_real_offset(vlv_details->match.byOffset.offset,
+						      vlv_details->match.byOffset.contentCount,
+						      ac->store->num_entries);
+			if (target == -1) {
+				return LDB_ERR_OPERATIONS_ERROR;
+			}
+		}
+
+		/* send the results */
+		first_i = MAX(target - vlv_details->beforeCount, 0);
+		last_i = MIN(target + vlv_details->afterCount,
+			     ac->store->num_entries - 1);
+
+		for (i = first_i; i <= last_i; i++) {
+			struct ldb_result *result;
+			struct GUID *guid = &ac->store->results[i];
+			ret =  dsdb_search_by_dn_guid(ldb, ac,
+						      &result,
+						      guid,
+						      ac->req->op.search.attrs,
+						      0);
+
+			if (ret == LDAP_NO_SUCH_OBJECT) {
+				/* The thing isn't there, which we quietly
+				   ignore and go on to send an extra one
+				   instead. */
+				if (last_i < ac->store->num_entries - 1) {
+					last_i++;
+				}
+				continue;
+			} else if (ret != LDB_SUCCESS) {
+				return ret;
+			}
+
+			ret = ldb_module_send_entry(ac->req, result->msgs[0],
+						    NULL);
+			if (ret != LDB_SUCCESS) {
+				return ret;
+			}
 		}
 	} else {
-		target = vlv_calc_real_offset(vlv_details->match.byOffset.offset,
+		target = -1;
-					      vlv_details->match.byOffset.contentCount,
-					      ac->store->num_entries);
-		if (target == -1) {
-			return LDB_ERR_OPERATIONS_ERROR;
-		}
-	}
-	/* send the results */
-	first_i = MAX(target - vlv_details->beforeCount, 0);
-	last_i = MIN(target + vlv_details->afterCount, ac->store->num_entries - 1);
-
-	for (i = first_i; i <= last_i; i++) {
-		struct ldb_result *result;
-		struct GUID *guid = &ac->store->results[i];
-		ret =  dsdb_search_by_dn_guid(ldb, ac,
-					      &result,
-					      guid,
-					      ac->req->op.search.attrs,
-					      0);
-
-		if (ret == LDAP_NO_SUCH_OBJECT) {
-			/* The thing isn't there, which we quietly ignore and
-			   go on to send an extra one instead. */
-			if (last_i < ac->store->num_entries - 1) {
-				last_i++;
-			}
-			continue;
-		} else if (ret != LDB_SUCCESS) {
-			return ret;
-		}
-
-		ret = ldb_module_send_entry(ac->req, result->msgs[0], NULL);
-		if (ret != LDB_SUCCESS) {
-			return ret;
-		}
 	}
 
 	/* return result done */
@@ -512,12 +521,14 @@ static int vlv_search_callback(struct ldb_request *req, struct ldb_reply *ares)
 		break;
 
 	case LDB_REPLY_DONE:
-		store->results = talloc_realloc(store, store->results,
+		if (store->num_entries != 0) {
-						struct GUID,
+			store->results = talloc_realloc(store, store->results,
-						store->num_entries);
+							struct GUID,
-		if (store->results == NULL) {
+							store->num_entries);
-			return ldb_module_done(ac->req, NULL, NULL,
+			if (store->results == NULL) {
-					       LDB_ERR_OPERATIONS_ERROR);
+				return ldb_module_done(ac->req, NULL, NULL,
+						       LDB_ERR_OPERATIONS_ERROR);
+			}
 		}
 		store->result_array_size = store->num_entries;
 

source4/dsdb/tests/python/vlv.py

@@ -212,11 +212,12 @@ class VLVTests(samba.tests.TestCase):
         controls = res.controls
         return full_results, controls, sort_control
 
-    def get_expected_order(self, attr):
+    def get_expected_order(self, attr, expression=None):
         """Fetch the whole list sorted on the attribute, using sort only."""
         sort_control = "server_sort:1:0:%s" % attr
         res = self.ldb.search(self.ou,
                               scope=ldb.SCOPE_ONELEVEL,
+                              expression=expression,
                               attrs=[attr],
                               controls=[sort_control])
         results = [x[attr][0] for x in res]
@@ -226,8 +227,8 @@ class VLVTests(samba.tests.TestCase):
         self.ldb.delete(user['dn'])
         del self.users[self.users.index(user)]
 
-    def get_gte_tests_and_order(self, attr):
+    def get_gte_tests_and_order(self, attr, expression=None):
-        expected_order = self.get_expected_order(attr)
+        expected_order = self.get_expected_order(attr, expression=expression)
         gte_users = []
         if attr in self.delicate_keys:
             gte_keys = [
@@ -260,8 +261,10 @@ class VLVTests(samba.tests.TestCase):
                 '\n\t\t',
                 '桑巴',
                 'zzzz',
-                expected_order[len(expected_order) // 2] + ' tail',
             ]
+            if expected_order:
+                gte_keys.append(expected_order[len(expected_order) // 2] + ' tail')
+
         else:
             # "numeric" means positive integers
             # doesn't work with -1, 3.14, ' 3', '9' * 20
@@ -285,7 +288,7 @@ class VLVTests(samba.tests.TestCase):
             self.delete_user(user)
 
         # for sanity's sake
-        expected_order_2 = self.get_expected_order(attr)
+        expected_order_2 = self.get_expected_order(attr, expression=expression)
         self.assertEqual(expected_order, expected_order_2)
 
         # Map gte tests to indexes in expected order. This will break
@@ -377,6 +380,105 @@ class VLVTests(samba.tests.TestCase):
                         self.assertCorrectResults(results, expected_order,
                                                   offset, before, after)
 
+    def run_index_tests_with_expressions(self, expressions):
+        # Here we don't test every before/after combination.
+        attrs = [x for x in self.users[0].keys() if x not in
+                 ('dn', 'objectclass')]
+        for attr in attrs:
+            for expression in expressions:
+                expected_order = self.get_expected_order(attr, expression)
+                sort_control = "server_sort:1:0:%s" % attr
+                res = None
+                n = len(expected_order)
+                for before in range(0, 11):
+                    after = before
+                    for offset in range(max(1, before - 2),
+                                        min(n - after + 2, n)):
+                        if res is None:
+                            vlv_search = "vlv:1:%d:%d:%d:0" % (before, after,
+                                                               offset)
+                        else:
+                            cookie = get_cookie(res.controls)
+                            vlv_search = ("vlv:1:%d:%d:%d:%s:%s" %
+                                          (before, after, offset, n,
+                                           cookie))
+
+                        res = self.ldb.search(self.ou,
+                                              expression=expression,
+                                              scope=ldb.SCOPE_ONELEVEL,
+                                              attrs=[attr],
+                                              controls=[sort_control,
+                                                        vlv_search])
+
+                        results = [x[attr][0] for x in res]
+
+                        self.assertCorrectResults(results, expected_order,
+                                                  offset, before, after)
+
+    def test_server_vlv_with_failing_expression(self):
+        """What happens when we run the VLV on an expression that matches
+        nothing?"""
+        expressions = ["(samaccountname=testferf)",
+                       "(cn=hefalump)",
+                       ]
+        self.run_index_tests_with_expressions(expressions)
+
+    def run_gte_tests_with_expressions(self, expressions):
+        # Here we don't test every before/after combination.
+        attrs = [x for x in self.users[0].keys() if x not in
+                 ('dn', 'objectclass')]
+        for expression in expressions:
+            for attr in attrs:
+                gte_order, expected_order, gte_map = \
+                    self.get_gte_tests_and_order(attr, expression)
+                # In case there is some order dependency, disorder tests
+                gte_tests = gte_order[:]
+                random.seed(2)
+                random.shuffle(gte_tests)
+                res = None
+                sort_control = "server_sort:1:0:%s" % attr
+
+                expected_order = self.get_expected_order(attr, expression)
+                sort_control = "server_sort:1:0:%s" % attr
+                res = None
+                for before in range(0, 11):
+                    after = before
+                    for gte in gte_tests:
+                        if res is not None:
+                            cookie = get_cookie(res.controls)
+                        else:
+                            cookie = None
+                        vlv_search = encode_vlv_control(before=before,
+                                                        after=after,
+                                                        gte=gte,
+                                                        cookie=cookie)
+
+                        res = self.ldb.search(self.ou,
+                                              scope=ldb.SCOPE_ONELEVEL,
+                                              expression=expression,
+                                              attrs=[attr],
+                                              controls=[sort_control,
+                                                        vlv_search])
+
+                        results = [x[attr][0] for x in res]
+                        offset = gte_map.get(gte, len(expected_order))
+
+                        # here offset is 0-based
+                        start = max(offset - before, 0)
+                        end = offset + 1 + after
+
+                        expected_results = expected_order[start: end]
+
+                        self.assertEquals(expected_results, results)
+
+    def test_vlv_gte_with_failing_expression(self):
+        """What happens when we run the VLV on an expression that matches
+        nothing?"""
+        expressions = ["(samaccountname=testferf)",
+                       "(cn=hefalump)",
+                       ]
+        self.run_gte_tests_with_expressions(expressions)
+
     def test_server_vlv_with_cookie_while_adding_and_deleting(self):
         """What happens if we add or remove items in the middle of the VLV?