Closed Bug 1357636 Opened 3 years ago Closed Last year

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: mstange, Assigned: dpino)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 2 obsolete files)

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?
Flags: needinfo?(bugmail)
Flags: needinfo?(btseng)
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"
Flags: needinfo?(btseng)
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."
Flags: needinfo?(bugmail)
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.
Blocks: 1358950
Priority: -- → P3
Whiteboard: [good first bug]
Keywords: good-first-bug
Whiteboard: [good first bug]
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.
Flags: needinfo?(bugmail)
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.
Flags: needinfo?(bugmail)
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).
Attachment #8999894 - Attachment is obsolete: true
Attachment #9000359 - Flags: review?(jvarga)
Looks good. Thanks.
Let me run IndexedDB tests locally, we might not need to push this to the try server.
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cee27f6b233
Improve IndexedDB error message on deleting an undefined object; r=janv
Attachment #9000359 - Flags: review?(jvarga) → review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/1cee27f6b233
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.