Last Comment Bug 694138 - IndexedDB: Allow passing an array as keypath
: IndexedDB: Allow passing an array as keypath
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
:
Mentors:
Depends on: 692614
Blocks: idb
  Show dependency treegraph
 
Reported: 2011-10-12 14:08 PDT by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2015-12-01 05:43 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to fix (27.04 KB, patch)
2011-12-19 14:47 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
jvarga: review+
bent.mozilla: review+
Details | Diff | Splinter Review
Fixes review comments and the index.keyPath property (5.86 KB, patch)
2011-12-19 16:48 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Fixes review comments and the index.keyPath property (6.39 KB, patch)
2011-12-19 17:05 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
bent.mozilla: review+
Details | Diff | Splinter Review
Add support on objectStores too (22.45 KB, patch)
2011-12-20 01:47 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
jvarga: review+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-12 14:08:54 PDT
The items in the array should be stringified. When evaluating the "keypath" we should evaluate all parts of the array and concatenate the result into an array.

This should work both on objectStore and index keypaths.
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-19 14:47:31 PST
Created attachment 582968 [details] [diff] [review]
Patch to fix

This is working but lacks fixing index.keyPath to return an array rather than a string when a index is created with an array of keypaths. I'll do a second patch for that but wanted to let you start reviewing this for now
Comment 2 Jan Varga [:janv] 2011-12-19 16:48:18 PST
Comment on attachment 582968 [details] [diff] [review]
Patch to fix

we discussed all this stuff on IRC, Jonas addressed all my comments
r=janv
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-19 16:48:52 PST
Created attachment 583008 [details] [diff] [review]
Fixes review comments and the index.keyPath property

The one thing this interdiff doesn't contain is some review comments for Key::AppendArrayItem. Accidentally merged them into the base patch.
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-19 17:05:07 PST
Created attachment 583022 [details] [diff] [review]
Fixes review comments and the index.keyPath property

This one works!
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-19 17:08:10 PST
Comment on attachment 583022 [details] [diff] [review]
Fixes review comments and the index.keyPath property

Ben, if you get to this one before Jan, feel free to just go for it
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-19 19:57:54 PST
Comment on attachment 582968 [details] [diff] [review]
Patch to fix

Review of attachment 582968 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/IDBFactory.cpp
@@ +304,2 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    if (!keyPath.IsEmpty() && keyPath.First() == ',') {

Sneaky! This could probably use a comment ;)

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1660,5 @@
> +      return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> +    }
> +
> +    if (!length) {
> +      return NS_ERROR_DOM_INVALID_ACCESS_ERR;

Wait, what? According to the spec I think you want SYNTAX_ERR?

::: dom/indexedDB/Key.h
@@ +152,5 @@
>      EncodeNumber(double(aInt), eFloat);
>      TrimBuffer();
>    }
>  
> +  nsresult SetFromJSVal(JSContext* aCx, const jsval aVal)

Nit: Please revert.

@@ +189,5 @@
>  
>      return NS_OK;
>    }
>  
> +  nsresult AppendArrayItem(JSContext* aCx, const jsval aVal, bool aFirst)

Nit: args on their own line.

@@ +194,5 @@
> +  {
> +    NS_ASSERTION(aFirst == mBuffer.IsEmpty(),
> +                 "Wrong aFirst or we didn't encode properly");
> +
> +    nsresult rv = EncodeJSVal(aCx, aVal, aFirst ? eMaxType : 0);

Wasn't there an eArray value? Do you want that instead?

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +1821,5 @@
>          else if (schemaVersion == MakeSchemaVersion(11, 0)) {
>            rv = UpgradeSchemaFrom11_0To12_0(connection);
>          }
> +        else if (schemaVersion == MakeSchemaVersion(12, 0)) {
> +          rv = connection->SetSchemaVersion(MakeSchemaVersion(13, 0));

Comment here why you're bumping the schema but not upgrading anything (backwards-incompatible change)
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-19 19:58:39 PST
Comment on attachment 583022 [details] [diff] [review]
Fixes review comments and the index.keyPath property

Review of attachment 583022 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/IDBIndex.cpp
@@ +382,5 @@
>    return mObjectStore->GetName(aStoreName);
>  }
>  
>  NS_IMETHODIMP
> +IDBIndex::GetKeyPath(JSContext* aCx, jsval* aVal)

Nit: Sorta dumb (I wish I could undo this...) but each param gets its own line.

@@ +387,5 @@
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>  
> +  if (UsesKeyPathArray()) {
> +    JSObject* array = JS_NewArrayObject(aCx, 0, nsnull);

Er, you should give the correct length here. You know what it is, right?

@@ +408,5 @@
> +
> +    *aVal = OBJECT_TO_JSVAL(array);
> +  }
> +  else {
> +    if (!xpc_qsStringToJsval(aCx, mKeyPath, aVal)) {

Nit: else if

::: dom/indexedDB/IDBObjectStore.cpp
@@ +577,5 @@
>        jsval key;
>        nsresult rv = GetJSValFromKeyPath(aCx, aObject, aKeyPathArray[i], key);
>        NS_ENSURE_SUCCESS(rv, rv);
>  
> +      if (NS_FAILED(value.AppendArrayItem(i == 0, aCx, key))) {

This looks wrong... What's going on here? Your first patch has cx going first, and there are no changes in this patch that seem relevant...
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-20 00:43:13 PST
> ::: dom/indexedDB/IDBObjectStore.cpp
> @@ +1660,5 @@
> > +      return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> > +    }
> > +
> > +    if (!length) {
> > +      return NS_ERROR_DOM_INVALID_ACCESS_ERR;
> 
> Wait, what? According to the spec I think you want SYNTAX_ERR?

Spec doesn't actually define that this should throw an exception at all, but I think that's a bug. Happy to change to syntax_err though to reduce number of errors people have to check for.

> ::: dom/indexedDB/OpenDatabaseHelper.cpp
> @@ +1821,5 @@
> >          else if (schemaVersion == MakeSchemaVersion(11, 0)) {
> >            rv = UpgradeSchemaFrom11_0To12_0(connection);
> >          }
> > +        else if (schemaVersion == MakeSchemaVersion(12, 0)) {
> > +          rv = connection->SetSchemaVersion(MakeSchemaVersion(13, 0));
> 
> Comment here why you're bumping the schema but not upgrading anything
> (backwards-incompatible change)

I might actually remove this if this lands at the same time as arrays-as-keys. Otherwise I'll add a comment.
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-20 01:47:05 PST
Created attachment 583098 [details] [diff] [review]
Add support on objectStores too

Jan correctly reminded me that this should work on objectstores too!
Comment 10 Jan Varga [:janv] 2011-12-20 02:50:38 PST
Comment on attachment 583098 [details] [diff] [review]
Add support on objectStores too

r=janv
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-20 03:16:54 PST
Checked in!

https://hg.mozilla.org/mozilla-central/rev/0ad9c268e932

Thanks everyone for late night reviews!
Comment 12 Scoobidiver (away) 2011-12-21 05:19:16 PST
It missed the Firefox 11 release for a few hours.
Comment 13 Scoobidiver (away) 2011-12-21 09:34:57 PST
I was wrong about the Firefox 11 target because this changeset is between:
* The latest changeset in Nightly 11.0a1/20111220: http://hg.mozilla.org/mozilla-central/rev/2afd7ae68e8b.
* The first changeset in 11.0a2/20111220: http://hg.mozilla.org/mozilla-central/rev/a8506ab2c654
Comment 14 Chris Mills (Mozilla, MDN editor) [:cmills] 2015-12-01 05:43:58 PST
I have updated the relevant pages with this detail

https://developer.mozilla.org/en-US/docs/Web/API/IDBDatabase/createObjectStore
https://developer.mozilla.org/en-US/docs/Web/API/IDBObjectStore/createIndex

And added a note to the relevant release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/11#IndexedDB

Note You need to log in before you can comment on or make changes to this bug.