LibWeb/IDB: Handle cursor iteration more correctly

Author: https://github.com/stelar7
Commit: 684c543ddb
Pull-request: https://github.com/LadybirdBrowser/ladybird/pull/6628
Reviewed-by: https://github.com/lpas
Reviewed-by: https://github.com/trflynn89

Libraries/LibWeb/IndexedDB/Internal/Algorithms.cpp

@@ -1599,13 +1599,9 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
         // * If key is defined:
         if (key) {
             // * The record’s key is greater than or equal to key.
-            auto is_greater_than_or_equal = record.visit(
-                [](Empty) { VERIFY_NOT_REACHED(); },
-                [key](auto const& inner_record) {
-                    return Key::greater_than(inner_record.key, *key) || Key::equals(inner_record.key, *key);
-                });
-
-            if (!is_greater_than_or_equal)
+            if (!record.visit([&](auto const& inner_record) {
+                    return Key::greater_than_or_equal(inner_record.key, *key);
+                }))
                 return false;
         }
 
@@ -1613,13 +1609,18 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
         if (primary_key) {
             auto const& inner_record = record.get<IndexRecord>();
 
-            // * The record’s key is equal to key and the record’s value is greater than or equal to primaryKey
-            if (!(Key::equals(inner_record.key, *key) && (Key::greater_than(inner_record.value, *primary_key) || Key::equals(inner_record.value, *primary_key))))
-                return false;
-
-            // * The record’s key is greater than key.
-            if (!Key::greater_than(inner_record.key, *key))
-                return false;
+            // * If the record’s key is equal to key:
+            if (Key::equals(inner_record.key, *key)) {
+                // * The record’s value is greater than or equal to primaryKey
+                if (!Key::greater_than_or_equal(inner_record.value, *primary_key))
+                    return false;
+            }
+            // * Else:
+            else {
+                // * The record’s key is greater than key.
+                if (!Key::greater_than(inner_record.key, *key))
+                    return false;
+            }
         }
 
         // * If position is defined and source is an object store:
@@ -1635,73 +1636,60 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
         if (position && source.has<GC::Ref<Index>>()) {
             auto const& inner_record = record.get<IndexRecord>();
 
-            // * The record’s key is equal to position and the record’s value is greater than object store position
-            if (!(Key::equals(inner_record.key, *position) && (Key::greater_than(inner_record.value, *object_store_position))))
-                return false;
-
-            // * The record’s key is greater than position.
-            if (!Key::greater_than(inner_record.key, *position))
-                return false;
+            // * If the record’s key is equal to position:
+            if (Key::equals(inner_record.key, *position)) {
+                // * The record’s value is greater than object store position
+                if (!Key::greater_than(inner_record.value, *object_store_position))
+                    return false;
+            }
+            // * Else:
+            else {
+                // * The record’s key is greater than position.
+                if (!Key::greater_than(inner_record.key, *position))
+                    return false;
+            }
         }
 
         // * The record’s key is in range.
-        auto is_in_range = record.visit(
-            [](Empty) { VERIFY_NOT_REACHED(); },
-            [range](auto const& inner_record) {
+        return record.visit(
+            [&](auto const& inner_record) {
                 return range->is_in_range(inner_record.key);
             });
-
-        return is_in_range;
     };
 
     auto next_unique_requirements = [&](Variant<ObjectStoreRecord, IndexRecord> const& record) -> bool {
         // * If key is defined:
         if (key) {
             // * The record’s key is greater than or equal to key.
-            auto is_greater_than_or_equal = record.visit(
-                [](Empty) { VERIFY_NOT_REACHED(); },
-                [key](auto const& inner_record) {
-                    return Key::greater_than(inner_record.key, *key) || Key::equals(inner_record.key, *key);
-                });
-
-            if (!is_greater_than_or_equal)
+            if (!record.visit([&](auto const& inner_record) {
+                    return Key::greater_than_or_equal(inner_record.key, *key);
+                }))
                 return false;
         }
 
         // * If position is defined:
         if (position) {
             // * The record’s key is greater than position.
-            auto is_greater_than_position = record.visit(
-                [](Empty) { VERIFY_NOT_REACHED(); },
-                [position](auto const& inner_record) {
-                    return Key::greater_than(inner_record.key, *position) || Key::equals(inner_record.key, *position);
-                });
-
-            if (!is_greater_than_position)
+            if (!record.visit([&](auto const& inner_record) {
+                    return Key::greater_than(inner_record.key, *position);
+                }))
                 return false;
         }
 
         // * The record’s key is in range.
-        auto is_in_range = record.visit(
-            [](Empty) { VERIFY_NOT_REACHED(); },
-            [range](auto const& inner_record) {
+        return record.visit(
+            [&](auto const& inner_record) {
                 return range->is_in_range(inner_record.key);
             });
-
-        return is_in_range;
     };
 
     auto prev_requirements = [&](Variant<ObjectStoreRecord, IndexRecord> const& record) -> bool {
         // * If key is defined:
         if (key) {
             // * The record’s key is less than or equal to key.
-            auto is_less_than_or_equal = record.visit(
-                [](Empty) { VERIFY_NOT_REACHED(); },
-                [key](auto const& inner_record) {
-                    return Key::less_than(inner_record.key, *key) || Key::equals(inner_record.key, *key);
-                });
-
-            if (!is_less_than_or_equal)
+            if (!record.visit([&](auto const& inner_record) {
+                    return Key::less_than_or_equal(inner_record.key, *key);
+                }))
                 return false;
         }
 
@@ -1709,13 +1697,18 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
         if (primary_key) {
             auto const& inner_record = record.get<IndexRecord>();
 
-            // * The record’s key is equal to key and the record’s value is less than or equal to primaryKey
-            if (!(Key::equals(inner_record.key, *key) && (Key::less_than(inner_record.value, *primary_key) || Key::equals(inner_record.value, *primary_key))))
-                return false;
-
-            // * The record’s key is less than key.
-            if (!Key::less_than(inner_record.key, *key))
-                return false;
+            // * If the record’s key is equal to key:
+            if (Key::equals(inner_record.key, *key)) {
+                // * The record’s value is less than or equal to primaryKey
+                if (!Key::less_than_or_equal(inner_record.value, *primary_key))
+                    return false;
+            }
+            // * Else:
+            else {
+                // * The record’s key is less than key.
+                if (!Key::less_than(inner_record.key, *key))
+                    return false;
+            }
         }
 
         // * If position is defined and source is an object store:
@@ -1731,60 +1724,51 @@ GC::Ptr<IDBCursor> iterate_a_cursor(JS::Realm& realm, GC::Ref<IDBCursor> cursor,
         if (position && source.has<GC::Ref<Index>>()) {
             auto const& inner_record = record.get<IndexRecord>();
 
-            // * The record’s key is equal to position and the record’s value is less than object store position
-            if (!(Key::equals(inner_record.key, *position) && Key::less_than(inner_record.value, *object_store_position)))
-                return false;
-
-            // * The record’s key is less than position.
-            if (!Key::less_than(inner_record.key, *position))
-                return false;
+            // * If the record’s key is equal to position:
+            if (Key::equals(inner_record.key, *position)) {
+                // * The record’s value is less than object store position
+                if (!Key::less_than(inner_record.value, *object_store_position))
+                    return false;
+            }
+            // Else:
+            else {
+                // * The record’s key is less than position.
+                if (!Key::less_than(inner_record.key, *position))
+                    return false;
+            }
         }
 
         // * The record’s key is in range.
-        auto is_in_range = record.visit(
-            [](Empty) { VERIFY_NOT_REACHED(); },
-            [range](auto const& inner_record) {
+        return record.visit(
+            [&](auto const& inner_record) {
                 return range->is_in_range(inner_record.key);
             });
-
-        return is_in_range;
     };
 
     auto prev_unique_requirements = [&](Variant<ObjectStoreRecord, IndexRecord> const& record) -> bool {
         // * If key is defined:
         if (key) {
             // * The record’s key is less than or equal to key.
-            auto is_less_than_or_equal = record.visit(
-                [](Empty) { VERIFY_NOT_REACHED(); },
-                [key](auto const& inner_record) {
-                    return Key::less_than(inner_record.key, *key) || Key::equals(inner_record.key, *key);
-                });
-
-            if (!is_less_than_or_equal)
+            if (!record.visit([&](auto const& inner_record) {
+                    return Key::less_than_or_equal(inner_record.key, *key);
+                }))
                 return false;
         }
 
         //* If position is defined:
         if (position) {
             // * The record’s key is less than position.
-            auto is_less_than_position = record.visit(
-                [](Empty) { VERIFY_NOT_REACHED(); },
-                [position](auto const& inner_record) {
-                    return Key::less_than(inner_record.key, *position) || Key::equals(inner_record.key, *position);
-                });
-
-            if (!is_less_than_position)
+            if (!record.visit([&](auto const& inner_record) {
+                    return Key::less_than(inner_record.key, *position);
+                }))
                 return false;
         }
 
         // * The record’s key is in range.
-        auto is_in_range = record.visit(
-            [](Empty) { VERIFY_NOT_REACHED(); },
-            [range](auto const& inner_record) {
+        return record.visit(
+            [&](auto const& inner_record) {
                 return range->is_in_range(inner_record.key);
             });
-
-        return is_in_range;
     };
 
     // 9. While count is greater than 0:

Libraries/LibWeb/IndexedDB/Internal/Key.h

@@ -69,6 +69,8 @@ public:
     [[nodiscard]] static bool equals(GC::Ref<Key> a, GC::Ref<Key> b) { return compare_two_keys(a, b) == 0; }
     [[nodiscard]] static bool less_than(GC::Ref<Key> a, GC::Ref<Key> b) { return compare_two_keys(a, b) < 0; }
     [[nodiscard]] static bool greater_than(GC::Ref<Key> a, GC::Ref<Key> b) { return compare_two_keys(a, b) > 0; }
+    [[nodiscard]] static bool less_than_or_equal(GC::Ref<Key> a, GC::Ref<Key> b) { return compare_two_keys(a, b) <= 0; }
+    [[nodiscard]] static bool greater_than_or_equal(GC::Ref<Key> a, GC::Ref<Key> b) { return compare_two_keys(a, b) >= 0; }
 
     AK::String dump() const;
 

Tests/LibWeb/Text/expected/wpt-import/IndexedDB/idbcursor-advance-invalid.any.txt

@@ -2,11 +2,10 @@ Harness status: OK
 
 Found 6 tests
 
-4 Pass
-2 Fail
-Fail	IDBCursor.advance() - invalid - attempt to call advance twice
+6 Pass
+Pass	IDBCursor.advance() - invalid - attempt to call advance twice
 Pass	IDBCursor.advance() - invalid - pass something other than number
 Pass	IDBCursor.advance() - invalid - pass null/undefined
 Pass	IDBCursor.advance() - invalid - missing argument
 Pass	IDBCursor.advance() - invalid - pass negative numbers
-Fail	IDBCursor.advance() - invalid - got value not set on exception
+Pass	IDBCursor.advance() - invalid - got value not set on exception

Tests/LibWeb/Text/expected/wpt-import/IndexedDB/idbcursor-advance.any.txt

@@ -2,11 +2,10 @@ Harness status: OK
 
 Found 6 tests
 
-2 Pass
-4 Fail
-Fail	IDBCursor.advance() - advances
-Fail	IDBCursor.advance() - advances backwards
+6 Pass
+Pass	IDBCursor.advance() - advances
+Pass	IDBCursor.advance() - advances backwards
 Pass	IDBCursor.advance() - skip far forward
-Fail	IDBCursor.advance() - within range
+Pass	IDBCursor.advance() - within range
 Pass	IDBCursor.advance() - within single key range
-Fail	IDBCursor.advance() - within single key range, with several results
+Pass	IDBCursor.advance() - within single key range, with several results

Tests/LibWeb/Text/expected/wpt-import/IndexedDB/idbcursor-continue.any.txt

@@ -2,11 +2,10 @@ Harness status: OK
 
 Found 6 tests
 
-2 Pass
-4 Fail
-Fail	IDBCursor.continue() - continues
-Fail	IDBCursor.continue() - with given key
+6 Pass
+Pass	IDBCursor.continue() - continues
+Pass	IDBCursor.continue() - with given key
 Pass	IDBCursor.continue() - skip far forward
-Fail	IDBCursor.continue() - within range
+Pass	IDBCursor.continue() - within range
 Pass	IDBCursor.continue() - within single key range
-Fail	IDBCursor.continue() - within single key range, with several results
+Pass	IDBCursor.continue() - within single key range, with several results

Tests/LibWeb/Text/expected/wpt-import/IndexedDB/idbcursor-direction-index-keyrange.any.txt

@@ -0,0 +1,9 @@
+Harness status: OK
+
+Found 4 tests
+
+4 Pass
+Pass	IDBCursor direction - index with keyrange - next
+Pass	IDBCursor direction - index with keyrange - prev
+Pass	IDBCursor direction - index with keyrange - nextunique
+Pass	IDBCursor direction - index with keyrange - prevunique

Tests/LibWeb/Text/expected/wpt-import/IndexedDB/idbcursor-request-source.any.txt

@@ -2,12 +2,11 @@ Harness status: OK
 
 Found 8 tests
 
-6 Pass
-2 Fail
+8 Pass
 Pass	IDBObjectStore::openCursor's request source must be the IDBObjectStore instance that opened the cursor
 Pass	IDBObjectStore::openKeyCursor's request source must be the IDBObjectStore instance that opened the cursor
-Fail	IDBIndex::openCursor's request source must be the IDBIndex instance that opened the cursor
-Fail	IDBIndex::openKeyCursor's request source must be the IDBIndex instance that opened the cursor
+Pass	IDBIndex::openCursor's request source must be the IDBIndex instance that opened the cursor
+Pass	IDBIndex::openKeyCursor's request source must be the IDBIndex instance that opened the cursor
 Pass	The source of the request from IDBObjectStore::update() is the cursor itself
 Pass	The source of the request from IDBObjectStore::delete() is the cursor itself
 Pass	The source of the request from IDBIndex::update() is the cursor itself

Tests/LibWeb/Text/input/wpt-import/IndexedDB/idbcursor-direction-index-keyrange.any.html

@@ -0,0 +1,15 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>IDBCursor direction - index with keyrange</title>
+<script>
+self.GLOBAL = {
+  isWindow: function() { return true; },
+  isWorker: function() { return false; },
+  isShadowRealm: function() { return false; },
+};
+</script>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+<script src="resources/support.js"></script>
+<div id=log></div>
+<script src="../IndexedDB/idbcursor-direction-index-keyrange.any.js"></script>

Tests/LibWeb/Text/input/wpt-import/IndexedDB/idbcursor-direction-index-keyrange.any.js

@@ -0,0 +1,55 @@
+// META: global=window,worker
+// META: title=IDBCursor direction - index with keyrange
+// META: script=resources/support.js
+
+// Spec: https://w3c.github.io/IndexedDB/#cursor-iteration-operation
+
+'use strict';
+
+let records = [1337, 'Alice', 'Bob', 'Bob', 'Greg', 'Åke', ['Anne']];
+let cases = [
+  {dir: 'next', expect: ['Alice:1', 'Bob:2', 'Bob:3', 'Greg:4']},
+  {dir: 'prev', expect: ['Greg:4', 'Bob:3', 'Bob:2', 'Alice:1']},
+  {dir: 'nextunique', expect: ['Alice:1', 'Bob:2', 'Greg:4']},
+  {dir: 'prevunique', expect: ['Greg:4', 'Bob:2', 'Alice:1']}
+];
+
+cases.forEach(function(testcase) {
+  let dir = testcase.dir;
+  let expect = testcase.expect;
+  indexeddb_test(
+      function(t, db, tx) {
+        let objStore = db.createObjectStore('test');
+        objStore.createIndex('idx', 'name');
+
+        for (let i = 0; i < records.length; i++) {
+          objStore.add({name: records[i]}, i);
+        }
+      },
+      function(t, db) {
+        let count = 0;
+        let rq = db.transaction('test', 'readonly')
+                     .objectStore('test')
+                     .index('idx')
+                     .openCursor(IDBKeyRange.bound('AA', 'ZZ'), dir);
+        rq.onsuccess = t.step_func(function(e) {
+          let cursor = e.target.result;
+          if (!cursor) {
+            assert_equals(count, expect.length, 'cursor runs');
+            t.done();
+            return;
+          }
+          assert_equals(
+              cursor.value.name + ':' + cursor.primaryKey, expect[count],
+              'cursor.value');
+          count++;
+          cursor.continue();
+        });
+        rq.onerror = t.step_func(function(e) {
+          e.preventDefault();
+          e.stopPropagation();
+          assert_unreached('rq.onerror - ' + e.message);
+        });
+      },
+      'IDBCursor direction - index with keyrange - ' + dir);
+});