Bug 62288 – IndexedDB createObjectStore should throw if key path contains a space

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mark Pilgrim (Google)]

Created attachment 96430 [details]
test case

http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#key-path-construct states "Note that spaces are not allowed within a key path." Further, http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBDatabase-createObjectStore-IDBObjectStore-DOMString-name-Object-optionalParameters states that "if keyPath is a string and is not a valid key path then a NON_TRANSIENT_ERR error must be thrown."

This test tries to create an object store with a space in the key path.

Expected behavior: throw webkitIDBDatabaseException.NON_TRANSIENT_ERR
Actual behavior: no exception thrown, objectstore created

Test case attached.

[Kentaro Hara]

I would like to try to fix this bug but is it OK?

[David Grogan]

Sure, as long as Mark doesn't already have a fix waiting.

[Mark Pilgrim (Google)]

I do not. Feel free to take it.

[Kentaro Hara]

I have one question.The specification says "A valid key path is either the empty string, a JavaScript identifier, or multiple Javascript identifiers separated by periods (Note that spaces are not allowed within a key path.)". 

However, as far as I see the current key path parser (WebCore/storage/IDBKeyPath.cpp) and its tests (storage/indexdb/keyPath.html, WebKit/chromium/tests/IDBKeyPathTest.cpp, WebCore/bindings/v8/IDBBindingUtilities.cpp), array access in the key path seems to be allowed, like "foo.bar[2]", "foo.bar[   2   ]", "foo.bar[2][3][[[[4]]]]" and (even) "[123]". I guess that there must be some reason for the parser allowing the array access in the key path, but I cannot yet find the reason... 

Is it OK to just remove all array access from the tests and write the parser strictly according to the above specification? Or should I leave the array access in the parser? In the latter case, I think that more specification is necessary to write the parser (e.g. "[[[ 123 ]]]" should not be accepted as a valid key path, but it is allowed in the current parser).

[Hans Wennborg]

(In reply to comment #4)
> I have one question.The specification says "A valid key path is either the empty string, a JavaScript identifier, or multiple Javascript identifiers separated by periods (Note that spaces are not allowed within a key path.)". 
> 
> However, as far as I see the current key path parser (WebCore/storage/IDBKeyPath.cpp) and its tests (storage/indexdb/keyPath.html, WebKit/chromium/tests/IDBKeyPathTest.cpp, WebCore/bindings/v8/IDBBindingUtilities.cpp), array access in the key path seems to be allowed, like "foo.bar[2]", "foo.bar[   2   ]", "foo.bar[2][3][[[[4]]]]" and (even) "[123]". I guess that there must be some reason for the parser allowing the array access in the key path, but I cannot yet find the reason... 
> 
> Is it OK to just remove all array access from the tests and write the parser strictly according to the above specification? Or should I leave the array access in the parser? In the latter case, I think that more specification is necessary to write the parser (e.g. "[[[ 123 ]]]" should not be accepted as a valid key path, but it is allowed in the current parser).

Hi Kentaro, yes feel free to take on this bug.

The spec text about what constitutes a valid key path is pretty recent. It was discussed here: http://www.w3.org/Bugs/Public/show_bug.cgi?id=11269 and here: http://lists.w3.org/Archives/Public/public-webapps/2011JanMar/0859.html

We should make sure our tests use valid key paths, and update the parser to follow the spec.

[Kentaro Hara]

Created attachment 97785 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97785 [details]
Patch

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

New failing tests:
storage/indexeddb/keypath-as-array.html
storage/indexeddb/keyPath.html

[WebKit Review Bot]

Created attachment 97787 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hans Wennborg]

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

> LayoutTests/storage/indexeddb/keypath-basics.html:49
> +    testKeyPath = [null, undefined, '', 'foo', 'foo.bar.baz'];

maybe rename testKeyPath to testKeyPaths? (here, and in a couple of more places below)

> Source/WebCore/storage/IDBDatabase.cpp:87
> +    IDBParseKeyPath(keyPath, keyPathElements, error);

could you factor this out into a function such as "bool IDBKeyPathIsValid(const String& path)" in IDBKeyPath.cpp/h?

> Source/WebCore/storage/IDBKeyPath.h:40
> +        IsNamed

There doesn't seem to be any point in keeping this enum around if an IDBKeyPathElement is always a name.

> Source/WebCore/storage/IDBObjectStore.cpp:138
> +    }

see comment in IDBDatabase.cpp. It would be nice if we could just have:

if (keyPath.isNull() || !IDBIsValidKeyPath(keyPath)) {
    ec = ...;
    return 0;
}

> Source/WebKit/chromium/tests/IDBKeyPathTest.cpp:60
>              ASSERT_TRUE(false) << "Invalid IDBKeyPathElement type";

this loop should be simpler now that an element is always a name, right?


Thanks for working on this!

[Kentaro Hara]

I found that storage/indexeddb/keypath-as-array.html fails in this patch because the current keypath parser does not support an array of keypath. The reason why the previous code passes keypath-as-array.html is just that the previous code did not at all check |keypath| argument of createObjectStore() (i.e. if an array ['foo', 'bar'] is passed to |keypath|, it is converted to a string "['foo', 'bar']" and the string is registered as a keypath of the created ObjectStore, without being checked if the string is a valid keypath or not). 

As far as I see, the current ObjectStore implementation does not support multiple keypaths in the first place. Thus, I would like to remove keypath-as-array.html in another patch (https://bugs.webkit.org/show_bug.cgi?id=63054) and then come back to this bug again.

[Hans Wennborg]

(In reply to comment #10)
> As far as I see, the current ObjectStore implementation does not support multiple keypaths in the first place. Thus, I would like to remove keypath-as-array.html in another patch (https://bugs.webkit.org/show_bug.cgi?id=63054) and then come back to this bug again.

Sounds good to me.

[Kentaro Hara]

I created a patch (not uploaded yet), but found that storage/indexeddb/mozilla/create-index-null-name.html fails with my patch. I would like to address this issue in another bug (https://bugs.webkit.org/show_bug.cgi?id=63921) and then come back to this bug again.

[Kentaro Hara]

Created attachment 99711 [details]
Patch

[Kentaro Hara]

(In reply to comment #9)
> View in context: https://bugs.webkit.org/attachment.cgi?id=97785&action=review
> 
> > LayoutTests/storage/indexeddb/keypath-basics.html:49
> > +    testKeyPath = [null, undefined, '', 'foo', 'foo.bar.baz'];
> 
> maybe rename testKeyPath to testKeyPaths? (here, and in a couple of more places below)

Done.

> 
> > Source/WebCore/storage/IDBDatabase.cpp:87
> > +    IDBParseKeyPath(keyPath, keyPathElements, error);
> 
> could you factor this out into a function such as "bool IDBKeyPathIsValid(const String& path)" in IDBKeyPath.cpp/h?

Done.

> 
> > Source/WebCore/storage/IDBKeyPath.h:40
> > +        IsNamed
> 
> There doesn't seem to be any point in keeping this enum around if an IDBKeyPathElement is always a name.

Done. I replaced IDBKeyPathElement with String.

> 
> > Source/WebCore/storage/IDBObjectStore.cpp:138
> > +    }
> 
> see comment in IDBDatabase.cpp. It would be nice if we could just have:
> 
> if (keyPath.isNull() || !IDBIsValidKeyPath(keyPath)) {
>     ec = ...;
>     return 0;
> }

Done.

> 
> > Source/WebKit/chromium/tests/IDBKeyPathTest.cpp:60
> >              ASSERT_TRUE(false) << "Invalid IDBKeyPathElement type";
> 
> this loop should be simpler now that an element is always a name, right?

Done.

[WebKit Review Bot]

Comment on attachment 99711 [details]
Patch

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

New failing tests:
storage/indexeddb/keyPath.html

[WebKit Review Bot]

Created attachment 99712 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kentaro Hara]

What exception should be returned when null or undefined is passed to createObjectStore() or createIndex() is debatable.

Mark said that TypeError is best (see https://bugs.webkit.org/show_bug.cgi?id=62414), but I guess that NON_TRANSIENT_ERR may be best (and I do so in my latest patch). There is no definite reason but the reasons are as follows:

- The specification (http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBDatabase-createObjectStore-IDBObjectStore-DOMString-name-Object-optionalParameters) says that |keypath| must not be nullable, but does not specify what exception should be thrown when |keypath| is nullable. However, according to the specification, other APIs can throw a kind of database exception when null or undefined is passed to the APIs. For example, bound() throws DATA_ERR, lowerBound() throws DATA_ERR, cmp() throws NON_TRANSIENT_ERR, when null or undefined is passed. In addition, the specification requires that creaetObjectStore() should throw NON_TRANSIENT_ERR when a keypath is string but is not a valid keypath. Thus, I guess that it is not so strange to throw NON_TRANSIENT_ERR for null and undefined cases too.

- As far as I see, there seems to be no place that is trying to throw TypeError from WebCore. Actually, I could not find the exception code for TypeError anywhere in xxxxxxException.h. Maybe we have to write a code for throwing TypeError for each JavaScript engine, provided that we really want to throw TypeError (see https://bugs.webkit.org/show_bug.cgi?id=62414). 

- FYI: Firefox behaves as follows:

createObjectStore(): NS_ERROR_XPC_NOT_ENOUGH_ARGS exception

createObjectStore(null): NS_ERROR_DOM_INDEXEDDB_NON_TRANSIENT_ERR exception

createObjectStore(undefined): NS_ERROR_DOM_INDEXEDDB_NON_TRANSIENT_ERR exception

createIndex("name", {keyPath: undefined}): no exception (definitely wrong!)

createIndex("name", {keyPath: null}): no exception (definitely wrong!)

[Hans Wennborg]

(In reply to comment #17)
> What exception should be returned when null or undefined is passed to createObjectStore() or createIndex() is debatable.

Mark Pilgrim is probably the best to answer this, so please check with him.

LGTM on the rest.

[Kentaro Hara]

> > What exception should be returned when null or undefined is passed to createObjectStore() or createIndex() is debatable.
> 
> Mark Pilgrim is probably the best to answer this, so please check with him.

Mark: Would you please comment on this?

[Kentaro Hara]

Mark: Would you please take a look at it?

[Mark Pilgrim (Google)]

This is my best understanding...

createObjectStore(): should throw TypeError (this should be handled in the IDL, no need for C++ code)

createObjectStore(null): this should behave identically to createObjectStore("null"). Yeah, I'm not happy about it either.

createObjectStore(undefined): should throw TypeError

createIndex("name", {keyPath: undefined}): should throw NON_TRANSIENT_ERR

createIndex("name", {keyPath: null}): should throw NON_TRANSIENT_ERR

cite for the last 2: "If keyPath is an Array and any items in the array is not a valid key path, or if keyPath is a string and is not a valid key path, or if keyPath is and Array and the multientry property in the optionalParameters is true, then a NON_TRANSIENT_ERR error must be thrown." http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBObjectStore-createIndex-IDBIndex-DOMString-name-DOMString-keyPath-Object-IDBIndexOptionalParameters

[Kentaro Hara]

> This is my best understanding...
> 
> (1) createObjectStore(): should throw TypeError (this should be handled in the IDL, no need for C++ code)
> 
> (2) createObjectStore(null): this should behave identically to createObjectStore("null"). Yeah, I'm not happy about it either.
> 
> (3) createObjectStore(undefined): should throw TypeError
> 
> (4) createIndex("name", {keyPath: undefined}): should throw NON_TRANSIENT_ERR
> 
> (5) createIndex("name", {keyPath: null}): should throw NON_TRANSIENT_ERR

Thank you very much for the comments. I agree with (4) and (5), but still feel some suspicion on (1), (2) and (3). 

- At any rate, the essential problem is the lack of the specification. We need to interpret it as appropriately as possible. 

- As for (2), I think that null and "null" are definitely different things. "null" is a valid key-path but null is not a valid key-path. 

- In my opinion, the results for (1), (2) and (3) should be identical (, although throwing TypeError in case of (2) is difficult from the perspective of the current implementation).

- Why do you consider that not NON_TRANSIENT_ERR but TypeError should be thrown in case of (1) and (3)? Are there any other places that are throwing TypeError when a nullable argument is passed? According to the specification, other APIs are throwing a kind of database exception when the nullable argument is passed, for example, bound() throws DATA_ERR, lowerBound() throws DATA_ERR, cmp() throws NON_TRANSIENT_ERR, when null or undefined is passed. 

- In summary...

The current specification: "If keyPath is an Array and any items in the array is not a valid key path, or if keyPath is a string and is not a valid key path, or if keyPath is Array and the multientry property in the optionalParameters is true, then a NON_TRANSIENT_ERR error must be thrown. "

Expected specification in my opinion: "If keyPath is an Array and any items in the array is not a valid key path, or if keyPath is not a valid key path, or if keyPath is Array and the multientry property in the optionalParameters is true, then a NON_TRANSIENT_ERR error must be thrown. "


However, if you would still say that the behaviors you indicated above are desirable, I am willing to follow your idea.

[Kentaro Hara]

Mark: would you please comment on this?

[Kentaro Hara]

OK. In this patch, I would like to follow Mark's idea about what exception should be thrown (, since this is the patch mainly intended to fix the keypath parser).

One question: For createIndex(), as Mark indicated, it is possible to throw TypeError from the IDL layer. However, for createObjectStore(), how can we throw TypeError from the IDL layer? Since the key path of createObjectStore() is passed as an object property (i.e. createObjectStore("name", {keypath: "foo.bar", ...})), I am afraid that there is no way to know the value of keypath and throw TypeError from the IDL layer. (Also, as we discussed, for now there seems to be no way to throw TypeError from C++ code...)

[Dominic Cooney]

(In reply to comment #24)
> OK. In this patch, I would like to follow Mark's idea about what exception should be thrown (, since this is the patch mainly intended to fix the keypath parser).
> 
> One question: For createIndex(), as Mark indicated, it is possible to throw TypeError from the IDL layer. However, for createObjectStore(), how can we throw TypeError from the IDL layer? Since the key path of createObjectStore() is passed as an object property (i.e. createObjectStore("name", {keypath: "foo.bar", ...})), I am afraid that there is no way to know the value of keypath and throw TypeError from the IDL layer. (Also, as we discussed, for now there seems to be no way to throw TypeError from C++ code...)

Using a [Custom] binding it should be possible to throw TypeError for createObjectStore. Let’s talk offline.

[Kentaro Hara]

Created attachment 104639 [details]
Patch

[Kentaro Hara]

I found that the spec about exceptions is now revised and defined.

- createObjectStore("name", {keyPath: undefined}): Creates an object store with no key-path.
- createObjectStore("name", {keyPath: null}): Creates an object store with no key-path.
- createObjectStore("name", {keyPath: ''}): Creates an object store with no key-path.

http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBDatabaseSync-createObjectStore-IDBObjectStoreSync-DOMString-name-Object-IDBDatabaseOptionalParameters says "If the attribute is null, undefined, the empty string, or not present, no key path is specified and thus keys are out-of-line."

- createIndex("name", undefined): Creates an index with a key-path "undefined"
- createIndex("name", null): Creates an index with a key-path "null"

This case hits the step 3 of this spec: http://www.w3.org/TR/WebIDL/#es-DOMString In other words, undefined should be converted to "undefined" and null should be converted to "null", and then those strings are used for a key-path.

[Mark Pilgrim (Google)]

LGTM.

[WebKit Review Bot]

Comment on attachment 104639 [details]
Patch

Clearing flags on attachment: 104639

Committed r93759: <http://trac.webkit.org/changeset/93759>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hans Wennborg]

(In reply to comment #30)
> All reviewed patches have been landed.  Closing bug.

Thanks everyone for getting this worked out, reviewed, and landed!