Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define db's associated upgrade tx to align with impls/tests #201

Merged
merged 3 commits into from
May 12, 2017

Conversation

inexorabletash
Copy link
Member

@inexorabletash inexorabletash commented May 11, 2017

Implementations and tests have create/deleteObjectStore throw "TransactionInactiveError" if the transaction has aborted but the abort event has not yet fired. The spec disagreed, or at least was imprecise about the definition of "finished".

To resolve this, databases are given an associated upgrade tx which is nulled out when the event fires, rather than relying on "finished". This simplifies text elsewhere too.

@brettz9 - can you review this change? This should address the outstanding issue you noted over in web-platform-tests/wpt#5612

Tracking bug for this is #192

@inexorabletash inexorabletash changed the title Define db's associated upgrade tx to align with impls/tests. Resolves #192 Define db's associated upgrade tx to align with impls/tests May 11, 2017
@inexorabletash
Copy link
Member Author

Testing for this is already covered in WPT:

IndexedDB/idbdatabase-createObjectStore-exception-order.htm
IndexedDB/idbdatabase-deleteObjectStore-exception-order.htm

@brettz9
Copy link
Contributor

brettz9 commented May 12, 2017

While I'm still checking (looks good so far), I did have the following concern/question... It is admittedly a concern about a missing test rather than spec issue, but I also wanted to confirm what the testing behavior would be if it existed...

idbtransaction-objectStore-finished.html, and idbobjectstore-index-finished.html cover the expected InvalidStateError throwing immediately after an abort (and suggest by their test names that "finished" at least definitively occurs within abort). However, I don't see any tests for an abort throwing immediately after another abort (as would be consistent with this understanding (of one subset) of "finished" since abort is to throw upon a transaction being "finished"). (FWIW, besides my manual checking, my regex to check the files was abort\([\s\S]*abort\(.)

Should there be such a test which checks for InvalidStateError immediately after a prior abort?

@brettz9
Copy link
Contributor

brettz9 commented May 12, 2017

Besides my somewhat-related question about the expected behavior of multiple aborts, the PR looks good.

@inexorabletash
Copy link
Member Author

I don't see any tests for an abort throwing immediately after another abort (as would be consistent with this understanding (of one subset) of "finished" since abort is to throw upon a transaction being "finished").

Yes, it does look we're missing a test for that case.

@inexorabletash inexorabletash merged commit ee40bb3 into master May 12, 2017
@inexorabletash inexorabletash deleted the assoc-upgrade-tx branch May 12, 2017 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants