Cursor.update support for keypath arrays

RESOLVED FIXED in mozilla16

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: khuey, Assigned: sicking)

Tracking

unspecified
mozilla16
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
Created attachment 637190 [details] [diff] [review]
Patch

We added the support already, just needs some tests.
Attachment #637190 - Flags: review?(bent.mozilla)
Comment on attachment 637190 [details] [diff] [review]
Patch

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

::: dom/indexedDB/test/unit/test_complex_keyPaths.js
@@ +175,5 @@
> +    let cursor = e.target.result;
> +    if (cursor) {
> +      request = cursor.update(info.value);
> +      request.onerror = errorHandler;
> +      request.onsuccess = grabEventAndContinueHandler;

This won't actually test what we want to test. In the object store used here we don't use inline keys at all. So you'll want to move this code up to the "Test creating object stores and inserting data" part of the test.

Also, you only need to put the test in the part of the code run when we have a "key" property. That way you won't need the if(cursor) tests.
Attachment #637190 - Flags: review?(bent.mozilla) → review-
Created attachment 637720 [details] [diff] [review]
Patch
Assignee: nobody → khuey
Attachment #637190 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #637720 - Flags: review?(jonas)
Comment on attachment 637720 [details] [diff] [review]
Patch

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

Err.. info.key is always true there, otherwise we would have called 'continue' earlier. I'll write something up.
Attachment #637720 - Flags: review?(jonas) → review-
Ok, I'm convinced that I don't actually understand how this is supposed to work.
Assignee: khuey → jonas
Created attachment 637766 [details] [diff] [review]
Tests + fix

Basically you were doing the right thing, except that you had inverted the initial if-branch-check, causing none of your code to actually run. You were also using an index cursor for some reason even though that wasn't needed.


This patch both adds tests which makes sure that calling

cursor.update(<existing value>)

always works (which is what the previous versions of the patch was trying to do I believe).

It also tests that

cursor.update(<value with key changed>)

always throws.

This actually surfaced a case where we were doing the wrong thing. Calling cursor.update(<existing value>) on an object store with an empty keyPath throws. The fix is to remove a bogus check.
Attachment #637720 - Attachment is obsolete: true
Attachment #637766 - Flags: review?(khuey)
Attachment #637766 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/6b50a5c705c2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.