Closed Bug 694138 Opened 13 years ago Closed 12 years ago

IndexedDB: Allow passing an array as keypath

Categories

(Core :: Storage: IndexedDB, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sicking, Assigned: sicking)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 1 obsolete file)

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.
Attached patch Patch to fixSplinter Review
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 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+
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)
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.
Jan correctly reminded me that this should work on objectstores too!
Attachment #583098 - Flags: review?(Jan.Varga)
Attachment #583022 - Flags: review?(Jan.Varga)
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
It missed the Firefox 11 release for a few hours.
Target Milestone: mozilla11 → mozilla12
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: