New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add openCursor(key, primaryKey) and continuePrimaryKey #14

Open
inexorabletash opened this Issue May 19, 2015 · 16 comments

Comments

Projects
None yet
3 participants
@inexorabletash
Member

inexorabletash commented May 19, 2015

https://www.w3.org/Bugs/Public/show_bug.cgi?id=20257

partial interface IDBIndex {
  IDBRequest openCursor(optional any query, optional IDBCursorDirection direction, IDBIndexOpenCursorOptions options);
  IDBRequest openKeyCursor(optional any query, optional IDBCursorDirection direction, IDBIndexOpenCursorOptions options);
}

dictionary IDBIndexOpenCursorOptions {
  any key;
  any primaryKey;
}

partial interface IDBCursor {
  void continuePrimaryKey(optional any key, optional any primaryKey);
}

If primaryKey is specified in options but key is not, throw.

  • cursor iteration operation
  • continuePrimaryKey()
  • IDBIndex.openCursor()
  • IDBIndex.openKeyCursor()

@inexorabletash inexorabletash referenced this issue May 21, 2015

Closed

Import issues from Bugzilla #15

14 of 14 tasks complete
@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash May 28, 2015

Member

Need to specify what happens if you call continuePrimaryKey() on a cursor where the source is an object store. Throw?

I'm updating the experimental impl in Blink to throw InvalidAccessError in this case, pending further discussion.

Member

inexorabletash commented May 28, 2015

Need to specify what happens if you call continuePrimaryKey() on a cursor where the source is an object store. Throw?

I'm updating the experimental impl in Blink to throw InvalidAccessError in this case, pending further discussion.

dstockwell pushed a commit to dstockwell/blink that referenced this issue May 29, 2015

IndexedDB: continuePrimaryKey() on object store cursor should throw
Behind the --enable-experimental-web-platform-features flag, Blink
supports a new continuePrimaryKey() method on IDBCursor. This is
intended for use on indexes to advance a cursor to a specific (key,
primaryKey) pair. It's not fully specified yet only hashed out in
bugs[1] (hence, experimental). Calling the method on a cursor which
has an object store rather than an index as its source hits a DCHECK.

Pending a full spec for the feature, have the method throw if the
cursor's source is not an index.

[1] w3c/IndexedDB#14

R=cmumford@chromium.org
BUG=493443
TEST=idbcursor_continueprimarykey.html

Review URL: https://codereview.chromium.org/1160853004

git-svn-id: svn://svn.chromium.org/blink/trunk@196108 bbb929c8-8fbe-4397-9dbb-9b2b20218538
@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Jun 5, 2015

Member

What should the behavior of continuePrimaryKey() be with "prevunique" iteration? Given this index:

  • r1 = key: 'a', primary key: 1
  • r2 = key: 'a', primary key: 2
  • r3 = key: 'b', primary key: 1

If direction is "prevunique" and the cursor is at r3, continue('a') is specified to end up at r1 (the first record with matching key).

Cases:

  • If direction is "prevunique" and the cursor is at r3, continue('a', 1) should end up at... ?
    • r1 is obvious
  • If direction is "prevunique" and the cursor is at r3, continue('a', 2) should end up at... ?
    • r2 seems to make the most sense here, but r1 would be consistent too.
  • If direction is "prevunique" and the cursor is at r3, continue('a', 3) should end up at... ?
    • to me, r2 seems sensible here, but my intuition may be wonky
Member

inexorabletash commented Jun 5, 2015

What should the behavior of continuePrimaryKey() be with "prevunique" iteration? Given this index:

  • r1 = key: 'a', primary key: 1
  • r2 = key: 'a', primary key: 2
  • r3 = key: 'b', primary key: 1

If direction is "prevunique" and the cursor is at r3, continue('a') is specified to end up at r1 (the first record with matching key).

Cases:

  • If direction is "prevunique" and the cursor is at r3, continue('a', 1) should end up at... ?
    • r1 is obvious
  • If direction is "prevunique" and the cursor is at r3, continue('a', 2) should end up at... ?
    • r2 seems to make the most sense here, but r1 would be consistent too.
  • If direction is "prevunique" and the cursor is at r3, continue('a', 3) should end up at... ?
    • to me, r2 seems sensible here, but my intuition may be wonky
@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash
Member

inexorabletash commented Jun 6, 2015

@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Jun 6, 2015

Member

Initial work in a4e4d2b

Member

inexorabletash commented Jun 6, 2015

Initial work in a4e4d2b

@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Jun 11, 2015

Member

Another option is to throw if continuePrimaryKey() is used with "nextunique" or "prevunique" since the behavior is a bit odd.

Member

inexorabletash commented Jun 11, 2015

Another option is to throw if continuePrimaryKey() is used with "nextunique" or "prevunique" since the behavior is a bit odd.

@yathit

This comment has been minimized.

Show comment
Hide comment
@yathit

yathit Jun 16, 2015

@inexorabletash +1 throw InvalidAccessError

yathit commented Jun 16, 2015

@inexorabletash +1 throw InvalidAccessError

@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Jun 16, 2015

Member

@yathit do you mean for the object store source case, or for "nextunique" / "prevunique", or both?

Member

inexorabletash commented Jun 16, 2015

@yathit do you mean for the object store source case, or for "nextunique" / "prevunique", or both?

@yathit

This comment has been minimized.

Show comment
Hide comment
@yathit

yathit commented Jun 16, 2015

@inexorabletash both cases.

@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash
Member

inexorabletash commented Jun 17, 2015

sgtm!

dstockwell pushed a commit to dstockwell/blink that referenced this issue Jun 18, 2015

IndexedDB: Disallow continuePrimaryKey on unique cursors
The experimental 'continuePrimaryKey()' method had undefined behavior
when the cursor was a 'unique' cursor (skipping over duplicate
primary keys). After some spec discussion[1], it seems like forbidding
the use of the method on such cursors is the safest approach.

[1] w3c/IndexedDB#14

R=cmumford@chromium.org
BUG=497454

Review URL: https://codereview.chromium.org/1188343002

git-svn-id: svn://svn.chromium.org/blink/trunk@197309 bbb929c8-8fbe-4397-9dbb-9b2b20218538
@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Jun 19, 2015

Member

In the openCursor() case, is the lower-bound of the key range implicitly the index key if primaryKey is specified (and thus it's an error if a open lower bound is specified), or should the range and starting position be decoupled and require both the key and primaryKey to be explicit specified e.g. openCursor(null, null, {key: x, primaryKey: y}) ?

Member

inexorabletash commented Jun 19, 2015

In the openCursor() case, is the lower-bound of the key range implicitly the index key if primaryKey is specified (and thus it's an error if a open lower bound is specified), or should the range and starting position be decoupled and require both the key and primaryKey to be explicit specified e.g. openCursor(null, null, {key: x, primaryKey: y}) ?

@yathit

This comment has been minimized.

Show comment
Hide comment
@yathit

yathit Jun 20, 2015

Agree. IDBIndexOpenCursorOptions should be a complete cursor position.

dictionary IDBIndexOpenCursorOptions {
  IDBKey key, // nullable, default to lower-bound
  IDBKey primaryKey; // nullable, default to first
}

Using openCursor becomes easier. Let the keyRange unchanged and open with different starting position, without requiring keyRange to be met with cursor position.

yathit commented Jun 20, 2015

Agree. IDBIndexOpenCursorOptions should be a complete cursor position.

dictionary IDBIndexOpenCursorOptions {
  IDBKey key, // nullable, default to lower-bound
  IDBKey primaryKey; // nullable, default to first
}

Using openCursor becomes easier. Let the keyRange unchanged and open with different starting position, without requiring keyRange to be met with cursor position.

dstockwell pushed a commit to dstockwell/chromium that referenced this issue Sep 23, 2015

IndexedDB: continuePrimaryKey() on object store cursor should throw
Behind the --enable-experimental-web-platform-features flag, Blink
supports a new continuePrimaryKey() method on IDBCursor. This is
intended for use on indexes to advance a cursor to a specific (key,
primaryKey) pair. It's not fully specified yet only hashed out in
bugs[1] (hence, experimental). Calling the method on a cursor which
has an object store rather than an index as its source hits a DCHECK.

Pending a full spec for the feature, have the method throw if the
cursor's source is not an index.

[1] w3c/IndexedDB#14

R=cmumford@chromium.org
BUG=493443
TEST=idbcursor_continueprimarykey.html

Review URL: https://codereview.chromium.org/1160853004

git-svn-id: svn://svn.chromium.org/blink/trunk@196108 bbb929c8-8fbe-4397-9dbb-9b2b20218538

dstockwell pushed a commit to dstockwell/chromium that referenced this issue Sep 23, 2015

IndexedDB: Disallow continuePrimaryKey on unique cursors
The experimental 'continuePrimaryKey()' method had undefined behavior
when the cursor was a 'unique' cursor (skipping over duplicate
primary keys). After some spec discussion[1], it seems like forbidding
the use of the method on such cursors is the safest approach.

[1] w3c/IndexedDB#14

R=cmumford@chromium.org
BUG=497454

Review URL: https://codereview.chromium.org/1188343002

git-svn-id: svn://svn.chromium.org/blink/trunk@197309 bbb929c8-8fbe-4397-9dbb-9b2b20218538
@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Feb 19, 2016

Member

Should we throw if primaryKey is specified but key is not? (I vote yes.)

Should we define any special behavior if options are used on a unique index? (I vote no.)

Member

inexorabletash commented Feb 19, 2016

Should we throw if primaryKey is specified but key is not? (I vote yes.)

Should we define any special behavior if options are used on a unique index? (I vote no.)

@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Feb 19, 2016

Member

For posterity, here's how you'd write a fallback if these methods didn't exist:

  var options = {key: 2, primaryKey: 2};

  var req = idx.openCursor(null, 'next', options);
  req.onsuccess = function() {
    var cursor = req.result;
    if (!cursor) return;

    if (indexedDB.cmp(cursor.key, options.key) < 0 ||
      (indexedDB.cmp(cursor.key, options.key) === 0 &&
        indexedDB.cmp(cursor.primaryKey, options.primaryKey) < 0)) {

          // if you have continuePrimaryKey but not openCursor(..., options) for some reason
          if (cursor.continuePrimaryKey) {
            cursor.continuePrimaryKey(options.key, options.primaryKey);
            return;
          }

          // If index key is too low we can jump directly to that.
          if (indexedDB.cmp(cursor.key, options.key) < 0) {
            cursor.continue();
            return;
          }

          // Otherwise we're stuck iterating until we hit the right primary key.
          cursor.continue();
          return;
    }

    // The cursor is positioned at appropriate record - use it!
    cursor.continue();
  };
Member

inexorabletash commented Feb 19, 2016

For posterity, here's how you'd write a fallback if these methods didn't exist:

  var options = {key: 2, primaryKey: 2};

  var req = idx.openCursor(null, 'next', options);
  req.onsuccess = function() {
    var cursor = req.result;
    if (!cursor) return;

    if (indexedDB.cmp(cursor.key, options.key) < 0 ||
      (indexedDB.cmp(cursor.key, options.key) === 0 &&
        indexedDB.cmp(cursor.primaryKey, options.primaryKey) < 0)) {

          // if you have continuePrimaryKey but not openCursor(..., options) for some reason
          if (cursor.continuePrimaryKey) {
            cursor.continuePrimaryKey(options.key, options.primaryKey);
            return;
          }

          // If index key is too low we can jump directly to that.
          if (indexedDB.cmp(cursor.key, options.key) < 0) {
            cursor.continue();
            return;
          }

          // Otherwise we're stuck iterating until we hit the right primary key.
          cursor.continue();
          return;
    }

    // The cursor is positioned at appropriate record - use it!
    cursor.continue();
  };

@inexorabletash inexorabletash added cursors and removed queries labels May 5, 2016

@pwnall pwnall referenced this issue Feb 17, 2017

Closed

IndexedDB 2.0 features review #153

3 of 3 tasks complete
@pwnall

This comment has been minimized.

Show comment
Hide comment
@pwnall

pwnall Mar 9, 2017

Should this issue be closed? I'm pretty sure Chrome, Firefox and Safari are shipping this.

pwnall commented Mar 9, 2017

Should this issue be closed? I'm pretty sure Chrome, Firefox and Safari are shipping this.

@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Mar 9, 2017

Member

openCursor() with primaryKey specified is still a proposal - no-one implements that.

Currently you need to open the cursor against key then maybe call continuePrimaryKey() to advance it to the right primaryKey on the first iteration.

Member

inexorabletash commented Mar 9, 2017

openCursor() with primaryKey specified is still a proposal - no-one implements that.

Currently you need to open the cursor against key then maybe call continuePrimaryKey() to advance it to the right primaryKey on the first iteration.

@pwnall

This comment has been minimized.

Show comment
Hide comment
@pwnall

pwnall Mar 9, 2017

Thanks for explaining! I missed that first part.

pwnall commented Mar 9, 2017

Thanks for explaining! I missed that first part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment