Bug 84207

Summary: IndexedDB: Support Array-type key paths
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: WebCore Misc.Assignee: Joshua Bell <jsbell>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alecflett, dglazkov, dgrogan, fishd, haraken, jamesr, japhet, ojan, tkent+wkapi, tony, vsevik, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 85298    
Bug Blocks: 84303    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Joshua Bell 2012-04-17 16:21:53 PDT
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).
Comment 1 Joshua Bell 2012-04-24 16:55:54 PDT
Created attachment 138691 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-24 16:58:29 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Joshua Bell 2012-04-24 17:42:39 PDT
Comment on attachment 138691 [details]
Patch

Whoops, this should have applied to a different bug. The ChangeLog confused webkit-patch upload.
Comment 4 Joshua Bell 2012-04-30 15:39:53 PDT
Created attachment 139532 [details]
Patch
Comment 5 Joshua Bell 2012-04-30 15:40:47 PDT
dgrogan@, alecflett@ - can you do an initial pass on this?
Comment 6 WebKit Review Bot 2012-04-30 16:39:03 PDT
Comment on attachment 139532 [details]
Patch

Attachment 139532 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12589380
Comment 7 David Grogan 2012-04-30 16:44:17 PDT
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?
Comment 8 Joshua Bell 2012-04-30 17:19:16 PDT
(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.
Comment 9 Joshua Bell 2012-05-21 17:31:21 PDT
Created attachment 143147 [details]
Patch
Comment 10 Joshua Bell 2012-05-21 17:37:56 PDT
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.
Comment 11 Kentaro Hara 2012-05-21 17:53:11 PDT
Comment on attachment 143147 [details]
Patch

r+ for the V8 change.
Comment 12 Joshua Bell 2012-05-22 10:44:45 PDT
tony@ - r? on the overall patch?
Comment 13 WebKit Review Bot 2012-05-22 11:47:10 PDT
Comment on attachment 143147 [details]
Patch

Clearing flags on attachment: 143147

Committed r118011: <http://trac.webkit.org/changeset/118011>
Comment 14 WebKit Review Bot 2012-05-22 11:47:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 David Grogan 2012-05-22 12:39:48 PDT
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]});
Comment 16 Joshua Bell 2012-05-22 12:43:59 PDT
(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.
Comment 17 David Grogan 2012-05-22 12:53:06 PDT
(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.
Comment 18 Joshua Bell 2012-05-22 13:07:43 PDT
(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.