1357636 - "DataError: Data provided to an operation does not meet requirements" is way too vague

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect, P3)

Description

[Markus Stange [:mstange]]

In this case I was calling objectStore.delete(undefined), but I didn't realize that I was passing undefined. It would be nice to have a message like "undefined is not a valid key or key range in IDBObjectStore.delete" somewhere.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Is there other error type defined in http://w3c.github.io/IndexedDB/#exceptions more descriptive that we could consider?

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

The DataError is the correct type we have to throw if the key value is undefined according to the spec. :|
http://w3c.github.io/IndexedDB/#convert-a-value-to-a-key-range
"Data provided to an operation does not meet requirements"

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

It would be nice to report this spec issue in 
https://github.com/w3c/IndexedDB/issues/ instead.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

I'm not sure this necessarily needs to be a spec issue.  The choice of error string is our own.  We currently define a single DataError string for IDB at http://searchfox.org/mozilla-central/source/dom/base/domerr.msg#62 but we could define additional developer-friendly error strings that are still reported as DataErrors.  For example: "You provided an `undefined` or `null` key range value in a case where this does not default to creating an unbounded key range. This is probably a bug in your code."

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Oh, you are right! The error code is not the only thing we can use to report the error.
You reminded me something we've done earlier to explain why Add/Put is failed when the object size is too big:
http://searchfox.org/mozilla-central/source/dom/indexedDB/IDBObjectStore.cpp#1453-1457

ErrorResult::ThrowDOMException(rv, const nsACString& message = EmptyCString()) allows us to add optional explanation for this.

Sorry for misleading.

Comment 6

[Diego Pino]

Attachment: 0001-Improve-IndexDB-error-message-on-deleting-an-undefin.patch

I'm attaching a patch that changes the error message when there's an invalid key (null or undefined), as commented in the bug description.

Comment 7

[Jan Varga [:janv]]

Comment on attachment 8999894 [details] [diff] [review]
0001-Improve-IndexDB-error-message-on-deleting-an-undefin.patch

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

Thank you for the patch. It looks good, but there are other places where a key is used as an argument.
For example index.get() or cursor.continue() or indexedDB.cmp().
Maybe we should just extend the existing description for NS_ERROR_DOM_INDEXEDDB_DATA_ERR.
I went through all the places in the code where we return NS_ERROR_DOM_INDEXEDDB_DATA_ERR and it seems to be all related to keys.
Andrew, what do you think ?

Comment 8

[Jan Varga [:janv]]

Andrew, see comment 7.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

The current error is definitely excessively vague.  NS_ERROR_DOM_INDEXEDDB_DATA_ERR is used in a variety of places that cover more than passing undefined, for example, invalid dates.  Since comment 0 was partially around the error message misdirecting the reader, I think I'd suggest we:
- Land the patch as-is.
- Perhaps convert some other call sites to the new, better error when it's exactly right as a follow-up.
- Maybe add a few more error codes to cover other cases such as when there was a complication in key extraction/traversal.

Comment 10

[Jan Varga [:janv]]

Comment on attachment 8999894 [details] [diff] [review]
0001-Improve-IndexDB-error-message-on-deleting-an-undefin.patch

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

Ok, but I think at least IDBIndex::GetInternal should be changed too.

Comment 11

[Diego Pino]

Attachment: 0001-Improve-IndexDB-error-message-on-deleting-an-undefin.patch

I amended the previous patch to replace error message in IDBIndex::GetInternal too.

:janv could you help me push the patch to the try server? (this is my first patch).

Comment 12

[Jan Varga [:janv]]

Looks good. Thanks.
Let me run IndexedDB tests locally, we might not need to push this to the try server.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cee27f6b233
Improve IndexedDB error message on deleting an undefined object; r=janv

Comment 14

[Diego Pino]

Attachment: 0001-Bug-1357636-Improve-IndexDB-error-message-when-key-i.patch

Keywords: checkin-needed

Comment 15

[Diego Pino]

I updated the commit message. Previously it said something like "improve error message when deleting object", but actually the error message applies now to more contexts than deleting an object.

OTOH, I cannot edit the bug to add the keyword "checkin-needed". I think it's because I haven't been assigned to the bug (maybe?).

Comment 16

[Jan Varga [:janv]]

I landed the patch already, see comment 13.

Comment 17

[Diego Pino]

:janv Nice, I didn't realize that message meant the patch had landed.

Comment 18

[Jan Varga [:janv]]

Sure, thank you for the patch, nice work!

Comment 19

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/1cee27f6b233