755514 - Cursor.update support for keypath arrays

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

We added the support already, just needs some tests.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ok, I'm convinced that I don't actually understand how this is supposed to work.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: 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.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/6b50a5c705c2