Bug 17681 - [IndexedDB] Operations that raise multiple exceptions types should define order
[IndexedDB] Operations that raise multiple exceptions types should define order
Status: RESOLVED LATER
Product: WebAppsWG
Classification: Unclassified
Component: Indexed Database API
unspecified
All All
: P2 minor
: ---
Assigned To: This bug has no owner yet - up for the taking
public-webapps-bugzilla
Version 2
: futureConsideration
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-03 21:00 UTC by Joshua Bell
Modified: 2013-04-26 17:52 UTC (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-07-03 21:00:08 UTC
(This has been discussed before, but I didn't see a tracking bug.)

Example: The description of IDBDatabase.transaction() says:

If this method is called on IDBDatabase object for which a "versionchange" transaction is still running, a InvalidStateError exception must be thrown....  Likewise, if this method is called on a IDBDatabase instance where the closePending flag is set, a InvalidStateError exception must be thrown.

Followed by the table:

InvalidStateError	 The close() method has been called on this IDBDatabase instance.
NotFoundError	One of the names provided in the storeNames argument doesn't exist in this database.
TypeError	The value for the mode parameter is invalid.
InvalidAccessError	The function was called with an empty list of store names

In the case where multiple conditions apply that would result in an exception being thrown, the order is undefined. For example:

db.transaction("no-such-store", "invalid-mode"); // TypeError or NotFoundError?
db.close() && db.transaction([], "invalid-mode"); // InvalidStateError, InvalidAccessError or TypeError?

The specification should enumerate the conditions in a well defined order so that different implementations will behave consistently.
Comment 1 Joshua Bell 2012-07-03 21:01:48 UTC
For posterity (and possible test cases), that should be:

db.close(); db.transaction([], "invalid-mode");
Comment 2 Israel Hilerio [MSFT] 2012-08-02 23:55:33 UTC
TypeError exceptions should be considered WebIDL type conversion errors. Therefore, we should always assume they are fired first before DOM exceptions.  The only possible caveat is where we return TypeError for passing zero on the IDBCursor.advance method.  For this method, we can specify that TypeError should be thrown first.
Comment 3 Joshua Bell 2012-08-03 01:03:59 UTC
(In reply to comment #2)
> TypeError exceptions should be considered WebIDL type conversion errors.
> Therefore, we should always assume they are fired first before DOM exceptions. 

Agreed - that matches the Chromium/WebKit implementation as well. Otherwise, we're flexible on ordering, including...

> The only possible caveat is where we return TypeError for passing zero on the
> IDBCursor.advance method.  For this method, we can specify that TypeError
> should be thrown first.

If it helps I can document Chromium's current exception priority, but as I said we're flexible.
Comment 4 Eliot Graff 2013-02-07 21:32:37 UTC
Made the following additions to method descriptions:

IDBDatabase.transaction() & IDBDatabaseSync.transaction()
If the value for the mode parameter is invalid, this method MUST throw a JavaScript TypeError exception.

IDBObjectStore.openCursor() & IDBObjectStoreSync.openCursor()
If the value for the direction parameter is invalid, this method MUST throw a JavaScript TypeError exception.

IDBIndex.openCursor() & IDBIndexSync.openCursor()
If the value for the direction parameter is invalid, this method MUST throw a JavaScript TypeError exception.

IDBIndex.openKeyCursor() & IDBIndexSync.openKeyCursor()
If the value for the direction parameter is invalid, this method MUST throw a JavaScript TypeError exception.

IDBCursor.advance() & IDBCursorSync.advance()
Nothing added. This already existed:
If the value for count is 0 (zero) or a negative number, this method MUST throw a JavaScript TypeError exception.

IDBFactory.open() & IDBFactorySync.open()
If the value of version is 0 (zero) or a negative number, this method MUST throw a JavaScript TypeError exception.
Comment 5 Joshua Bell 2013-02-07 22:23:07 UTC
I'm not sure we'd consider this fully resolved yet. But the spec is still ambiguous about the order for non-TypeError exceptions. This impacts testability if not true interoperability.

I'll wait for other implementers to weigh in. I'm still happy to document Chromium's order, and it's still easy to change it if consensus goes a different way.
Comment 6 Israel Hilerio [MSFT] 2013-02-08 08:09:58 UTC
Based on our experience, developers normally don't special case different behaviors based on different exception types.  The different exception types are useful when debugging but not during production when deciding what to do with the transaction lifetime. Because of this, I don't expect us to have interop issues if we don't specify the ordering of the exceptions.
Comment 7 Jonas Sicking 2013-03-16 11:25:57 UTC
Reopening. This bug was specifically about specifying order between all exception types. I agree that generally it doesn't matter which exception is thrown, but if we don't think that should be defined, then we should mark this bug WONTFIX rather than FIXED.

I do think it would be good to define all exception orders though.

One way to do it would be to have a global list that defines some sort of priority based on exception type. So if both a TransactionInactiveError and a ReadOnlyError test is failing, we always throw ReadOnlyError. And if ReadOnlyError and DataError both are possible, then we throw DataError.

Or something like that.
Comment 8 Joshua Bell 2013-03-22 19:09:18 UTC
(In reply to comment #7)
> I do think it would be good to define all exception orders though.

I still agree, and again Chrome is willing and able to change orders to match other implementations.

> One way to do it would be to have a global list that defines some sort of
> priority based on exception type. So if both a TransactionInactiveError and
> a ReadOnlyError test is failing, we always throw ReadOnlyError. And if
> ReadOnlyError and DataError both are possible, then we throw DataError.
> 
> Or something like that.

I wrote up a script to test this. As it stands now, there no overall ordering in Chrome's implementation. A > B means A is thrown instead of B if both error conditions are true:

IDBDatabase.createObjectStore(): InvalidStateError > ConstraintError > SyntaxError > InvalidAccessError
IDBDatabase.deleteObjectStore(): InvalidStateError > NotFoundError
IDBDatabase.transaction(): InvalidStateError > NotFoundError > InvalidAccessError*
IDBObjectStore.add(): InvalidStateError > TransactionInactiveError > ReadOnlyError > DataError > DataCloneError
IDBObjectStore.clear(): InvalidStateError > TransactionInactiveError > ReadOnlyError
IDBObjectStore.count(): DataError > InvalidStateError > TransactionInactiveError
IDBObjectStore.createIndex(): InvalidStateError > TransactionInactiveError > SyntaxError > ConstraintError > InvalidAccessError
IDBObjectStore.delete(): DataError > InvalidStateError > TransactionInactiveError > ReadOnlyError
IDBObjectStore.deleteIndex(): InvalidStateError > TransactionInactiveError > NotFoundError
IDBObjectStore.get(): DataError > InvalidStateError > TransactionInactiveError
IDBObjectStore.index(): InvalidStateError > TransactionInactiveError
IDBObjectStore.openCursor(): DataError > InvalidStateError > TransactionInactiveError
IDBObjectStore.put(): InvalidStateError > TransactionInactiveError > ReadOnlyError > DataError > DataCloneError
IDBIndex.count(): DataError > InvalidStateError > TransactionInactiveError
IDBIndex.get(): DataError > InvalidStateError > TransactionInactiveError
IDBIndex.getKey(): DataError > InvalidStateError > TransactionInactiveError
IDBIndex.openCursor(): DataError > InvalidStateError > TransactionInactiveError
IDBIndex.openKeyCursor(): DataError > InvalidStateError > TransactionInactiveError
IDBCursor.advance(): InvalidStateError > TransactionInactiveError
IDBCursor.continue(): DataError > TransactionInactiveError > InvalidStateError
IDBCursor.delete(): TransactionInactiveError > ReadOnlyError > InvalidStateError
IDBCursor.update(): InvalidStateError > TransactionInactiveError > ReadOnlyError > DataError > DataCloneError
IDBTransaction.objectStore(): InvalidStateError > NotFoundError

* NOTE: Can't cause both NotFoundError and InvalidAccessError at the same time in IDBDatabase.transaction()

The script is a bit mind-numbing to read but I can clean it up and share it if there's interest.

From quick code inspection, the ordering anomalies (e.g. sometimes DataError > InvalidStateError, sometimes DataError < InvalidStateError) would be easy to fix. There's not much logic behind the above ordering, and I'd love an excuse to clean it up.
Comment 9 Jonas Sicking 2013-04-01 04:24:45 UTC
I don't particularly care which order we define for the various functions, as long as it is defined.

I guess there's some value in having a consistent ordering. For example it seems surprising that the orders between .add and .delete are so different:

IDBObjectStore.add(): InvalidStateError > TransactionInactiveError > ReadOnlyError > DataError > DataCloneError
IDBObjectStore.delete(): DataError > InvalidStateError > TransactionInactiveError > ReadOnlyError

If the transaction-related errors are thrown after the data-related errors then it might make it easier to first process the incoming data and then call more generic functions that schedule a request against a transaction. That's not something that made sense in the current gecko implementation, but I could see that being the case elsewhere.
Comment 10 Joshua Bell 2013-04-01 19:41:25 UTC
(In reply to comment #9)
> I guess there's some value in having a consistent ordering. For example it
> seems surprising that the orders between .add and .delete are so different:
> 
> IDBObjectStore.add(): InvalidStateError > TransactionInactiveError >
> ReadOnlyError > DataError > DataCloneError
> IDBObjectStore.delete(): DataError > InvalidStateError >
> TransactionInactiveError > ReadOnlyError

In Chrome's impl, some of that is completely arbitrary (ordering of tests in the function), some of it is due to calling overloads (e.g. the overload that takes keys checks to see if the key is valid first then defers to the range overload; the range overload checks transaction/object state first...)

> If the transaction-related errors are thrown after the data-related errors
> then it might make it easier to first process the incoming data and then
> call more generic functions that schedule a request against a transaction.
> That's not something that made sense in the current gecko implementation,
> but I could see that being the case elsewhere.

Agreed. Of course, I'd also be fine with the reverse - check that the overall state is good-to-go, then look at the specific data.

I'll try and run my scripts against IE; if it's ordering is sensible perhaps we can follow what it does since the other impl's can rev more quickly.
Comment 11 Israel Hilerio [MSFT] 2013-04-11 23:25:43 UTC
(In reply to comment #7)
> Reopening. This bug was specifically about specifying order between all
> exception types. I agree that generally it doesn't matter which exception is
> thrown, but if we don't think that should be defined, then we should mark
> this bug WONTFIX rather than FIXED.

This isn't worth the implementation cost to define an order and have us all maintain it. We agree with Jonas that it generally doesn't matter which exception is thrown.  We propose a WONTFIX for this bug.
Comment 12 Joshua Bell 2013-04-18 21:51:22 UTC
I have gone in and removed the "legacy" exception tables, replacing them with prose. In nearly all cases this was simply mechanically turning this:

 <dl>
   <dt>FooError
   <dd>Condition
 </dl>

Into this:

 <p>
   If condition, throw a DOMException of type FooError.
 </p>

https://dvcs.w3.org/hg/IndexedDB/rev/7648f0c71309

This does NOT define an ordering, i.e. the methods are not stated algorithmically, so there should be no normative change, and it means there's no state change for this issue.

I also didn't re-order the exceptions to be first, so some of the methods read like:

| Overview of method.
|
| If condition1 throw exception1.
| 
| Start of method description.
|
| If condition2 throw exception2.
|
| More of description.
|
| If condition3 throw exception3.

This funky ordering isn't new, but may be more obvious now. I can do a second pass to move all the exception conditions to the start of each method.

I also standardized (for ease of later cleanup) on the grotesque wording:

  If <condition>, 
  the implementation MUST throw a DOMException of type FooError.

Per http://dom.spec.whatwg.org/#exception-domexception this could be more succinctly stated as simply:

  If <condition>, throw a "FooError".

... but that wording clashes with the non-algorithmic description of the methods, and implies an ordering.
Comment 13 Eliot Graff 2013-04-25 22:19:30 UTC
Moving for consideration after V1.
Comment 14 Joshua Bell 2013-04-26 17:52:04 UTC
I've done a little more tidying around the exception clauses but they still are not written in any sort of strict normative order.

Per discussion at the F2F http://www.w3.org/wiki/Webapps/April2013Meeting defining a strict ordering in the standard is deferred to a future version of the spec.