Bug 84207 – IndexedDB: Support Array-type key paths

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joshua Bell]

The Indexed DB spec allows key paths for object stores and indexes to be either strings or Arrays of strings. When evaluating a key path which is an Array against object to produce a key, the result is a key which is an Array of the evaluation of the strings as key paths against the object. (This is most useful when creating indexes, as you can can create a compound key.) Some restrictions apply: Array-type key paths cannot be used for object stores with key generators (i.e. autoIncrement: true).

[Joshua Bell]

Created attachment 138691 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Joshua Bell]

Comment on attachment 138691 [details]
Patch

Whoops, this should have applied to a different bug. The ChangeLog confused webkit-patch upload.

[Joshua Bell]

Created attachment 139532 [details]
Patch

[Joshua Bell]

dgrogan@, alecflett@ - can you do an initial pass on this?

[WebKit Review Bot]

Comment on attachment 139532 [details]
Patch

Attachment 139532 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12589380

[David Grogan]

Comment on attachment 139532 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139532&action=review

> Source/WebCore/ChangeLog:8
> +        String for holding/conveying key paths. Implements spec logic for evaluating

Is it possible to split the string->IDBKeyPath conversion from the array-keypath logic so that this patch isn't as big?

[Joshua Bell]

(In reply to comment #7)
> (From update of attachment 139532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139532&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        String for holding/conveying key paths. Implements spec logic for evaluating
> 
> Is it possible to split the string->IDBKeyPath conversion from the array-keypath logic so that this patch isn't as big?

Yeah... it's not a huge win in reducing complexity - mostly just drops the tests. (The actual logic change in this whole less than 20 lines!). Looks like the string->IDBKeyPath conversion patch would be about 100k (vs. the current 137k). Stay tuned.

[Joshua Bell]

Created attachment 143147 [details]
Patch

[Joshua Bell]

Okie dokie, this is the actual functional change described in the spec.

dgrogan@ - can you take a look?

haraken@ - binding/v8 changes should be non-controversial. There's a FIXME in IDBObjectStore.cpp to work around http://webkit.org/b/84217 - it's definitely non-optimal, but I don't want to tackle the whole WebIDL overloading spec yet.

[Kentaro Hara]

Comment on attachment 143147 [details]
Patch

r+ for the V8 change.

[Joshua Bell]

tony@ - r? on the overall patch?

[WebKit Review Bot]

Comment on attachment 143147 [details]
Patch

Clearing flags on attachment: 143147

Committed r118011: <http://trac.webkit.org/changeset/118011>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Grogan]

Comment on attachment 143147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143147&action=review

> LayoutTests/storage/indexeddb/resources/keypath-arrays.js:34
> +        evalAndExpectException("db.createObjectStore('store-with-generator', {keyPath: ['a', 'b'], autoIncrement: true})", "DOMException.INVALID_ACCESS_ERR");

We can't support this because we can't use an array as a property, i.e. we can't assign obj.[1,2] = generatedKey ?

> LayoutTests/storage/indexeddb/resources/keypath-arrays.js:35
> +        evalAndExpectException("store.createIndex('index-multientry', ['e', 'f'], {multiEntry: true})", "DOMException.NOT_SUPPORTED_ERR");

We can't do array keypaths and multiEntry==true because this would be meaningless?

index = store.createIndex('index-multientry', ['e', 'f'], {multiEntry: true});
store.put({a: 1, b: 2, c: 3, d: 4, e: 5, f: [6, 7]});

[Joshua Bell]

(In reply to comment #15)
> (From update of attachment 143147 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143147&action=review
> 
> > LayoutTests/storage/indexeddb/resources/keypath-arrays.js:34
> > +        evalAndExpectException("db.createObjectStore('store-with-generator', {keyPath: ['a', 'b'], autoIncrement: true})", "DOMException.INVALID_ACCESS_ERR");
> 
> We can't support this because we can't use an array as a property, i.e. we can't assign obj.[1,2] = generatedKey ?

Correct. This was raised as a spec issue and the spec was updated to explicitly forbid this:

"If the optionalParameters parameter is specified, and autoIncrement is set to true, and the keyPath parameter is specified to the empty string, or specified to an Array, this function must throw a InvalidAccessError exception."

> > LayoutTests/storage/indexeddb/resources/keypath-arrays.js:35
> > +        evalAndExpectException("store.createIndex('index-multientry', ['e', 'f'], {multiEntry: true})", "DOMException.NOT_SUPPORTED_ERR");
> 
> We can't do array keypaths and multiEntry==true because this would be meaningless?
> 
> index = store.createIndex('index-multientry', ['e', 'f'], {multiEntry: true});
> store.put({a: 1, b: 2, c: 3, d: 4, e: 5, f: [6, 7]});

The spec says: "If keyPath is and [sic] Array and the multiEntry property in the optionalParameters is true, then a DOMException of type NotSupportedError must be thrown."

I actually find this restriction a bit odd, since it logically could work - the array-type keypath would generate an array-type key, which if multiEntry were true would generate multiple index entries.

[David Grogan]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 143147 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=143147&action=review
> > > LayoutTests/storage/indexeddb/resources/keypath-arrays.js:35
> > > +        evalAndExpectException("store.createIndex('index-multientry', ['e', 'f'], {multiEntry: true})", "DOMException.NOT_SUPPORTED_ERR");
> > 
> > We can't do array keypaths and multiEntry==true because this would be meaningless?
> > 
> > index = store.createIndex('index-multientry', ['e', 'f'], {multiEntry: true});
> > store.put({a: 1, b: 2, c: 3, d: 4, e: 5, f: [6, 7]});
> 
> The spec says: "If keyPath is and [sic] Array and the multiEntry property in the optionalParameters is true, then a DOMException of type NotSupportedError must be thrown."
> 
> I actually find this restriction a bit odd, since it logically could work - the array-type keypath would generate an array-type key, which if multiEntry were true would generate multiple index entries.

What keys would be in the index in the example from my last comment?  [5,6] and [5,7]?  That could get out of control, but could also be useful, if the specified keypaths yielded long arrays.

[Joshua Bell]

(In reply to comment #17)
> > > index = store.createIndex('index-multientry', ['e', 'f'], {multiEntry: true});
> > > store.put({a: 1, b: 2, c: 3, d: 4, e: 5, f: [6, 7]});
> 
> What keys would be in the index in the example from my last comment?  [5,6] and [5,7]?  That could get out of control, but could also be useful, if the specified keypaths yielded long arrays.

Following the spec for evaluating the key path:

input key path: ['e', 'f']
input value: {a: 1, b: 2, c: 3, d: 4, e: 5, f: [6, 7]}
output key: [5, [6, 7]]

... which would yield index entries for:
index key: 5
index key: [6, 7]

So no explosion, but not necessarily all that useful.