Closed Bug 1357636 Opened 3 years ago Closed Last year
Error: Data provided to an operation does not meet requirements" is way too vague
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.
Is there other error type defined in http://w3c.github.io/IndexedDB/#exceptions more descriptive that we could consider?
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"
It would be nice to report this spec issue in https://github.com/w3c/IndexedDB/issues/ instead.
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."
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.
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.
Attachment #8999894 - Flags: review?(jvarga)
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 ?
Andrew, see comment 7.
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 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.
Attachment #8999894 - Flags: review?(jvarga) → review+
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).
Looks good. Thanks. Let me run IndexedDB tests locally, we might not need to push this to the try server.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1cee27f6b233 Improve IndexedDB error message on deleting an undefined object; r=janv
Attachment #9000359 - Attachment is obsolete: true
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?).
I landed the patch already, see comment 13.
:janv Nice, I didn't realize that message meant the patch had landed.
Sure, thank you for the patch, nice work!
Assignee: nobody → dpino
Status: NEW → ASSIGNED
You need to log in before you can comment on or make changes to this bug.