IndexedDB: Allow passing an array as keypath

RESOLVED FIXED

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

({dev-doc-complete})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
Depends on: 692614
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
Assignee: nobody → jonas
Attachment #582968 - Flags: review?(Jan.Varga)

Comment 2

6 years ago
Comment on attachment 582968 [details] [diff] [review]
Patch to fix

we discussed all this stuff on IRC, Jonas addressed all my comments
r=janv
Attachment #582968 - Flags: review?(Jan.Varga) → review+
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.
Attachment #583008 - Flags: review?(Jan.Varga)
Created attachment 583022 [details] [diff] [review]
Fixes review comments and the index.keyPath property

This one works!
Attachment #583008 - Attachment is obsolete: true
Attachment #583008 - Flags: review?(Jan.Varga)
Attachment #583022 - Flags: review?(Jan.Varga)
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
Attachment #583022 - Flags: review?(bent.mozilla)
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)
Attachment #582968 - Flags: review+
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...
Attachment #583022 - Flags: review?(bent.mozilla) → review+
> ::: 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.
Created attachment 583098 [details] [diff] [review]
Add support on objectStores too

Jan correctly reminded me that this should work on objectstores too!
Attachment #583098 - Flags: review?(Jan.Varga)

Updated

6 years ago
Attachment #583022 - Flags: review?(Jan.Varga)

Comment 10

6 years ago
Comment on attachment 583098 [details] [diff] [review]
Add support on objectStores too

r=janv
Attachment #583098 - Flags: review?(Jan.Varga) → review+
Checked in!

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

Thanks everyone for late night reviews!
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
Target Milestone: --- → mozilla11

Comment 12

6 years ago
It missed the Firefox 11 release for a few hours.
Target Milestone: mozilla11 → mozilla12

Comment 13

6 years ago
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
Target Milestone: mozilla12 → mozilla11
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla11 → ---
Version: Trunk → unspecified
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.