-
Notifications
You must be signed in to change notification settings - Fork 93
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
fix(dbapi): autocommit enabling fails if no transactions begun #177
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to other reviewers, this addresses a bug in the django tests: https://github.com/googleapis/python-spanner-django/pull/559/checks?check_run_id=147277725, but the fix isn't specific to django.
@IlyaFaer LGTM, but please confirm that the django tests pass with this change, and that this is the right autocommit behavior according to the DBAPI spec and sqlite implementation before merging this one.
@c24t, this behavior is mimicked from the Java implementation, because in Python (and its DB APIs) we didn't find any strict directions of what should be done on enabling an autocommit mode. Right now, we're trying to commit the current transaction without checking its state, so errors are raised in several cases: when the transaction was normally commited, and we enabling an autocommit mode; when it was rolled back, and we're enabling autocommit mode; when the transaction wasn't even started. With this PR we'll first be checking if the transaction is in progress - in this only case it should be commited. |
FYI @larkee, @skuruppu this brings back the "dummy where clause" in https://github.com/googleapis/python-spanner/pull/177/files#diff-e942c1913bf8770f329585a42ab8ce8abe3d2109a0a77431ed7a8f337f467425R534. There's some more conversation on this in googleapis/python-spanner-django#516 (comment). I wanted to get your OK on this before merging. @IlyaFaer is working on some tests that touch this now, and found that inserting the dummy clause is a more difficult change in spanner-django than in the DB API layer here. Since spanner-django is the only consumer at the moment this wouldn't change anything in practice, but it is the wrong place for it in principle. If you're willing to approve it for now we'll merge and open a ticket to revert the change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're willing to approve it for now we'll merge and open a ticket to revert the change.
Approving under the mentioned condition 👍
…-spanner into autocommit_change
…-spanner into autocommit_change
🤖 I have created a release \*beep\* \*boop\* --- ## [3.0.0](https://github.com/googleapis/python-spanner/compare/v2.1.0...v3.0.0) (2021-01-15) ### ⚠ BREAKING CHANGES * convert operations pbs into Operation objects when listing operations (#186) ### Features * add support for instance labels ([#193](https://github.com/googleapis/python-spanner/issues/193)) ([ed462b5](https://github.com/googleapis/python-spanner/commit/ed462b567a1a33f9105ffb37ba1218f379603614)) * add support for ssl credentials; add throttled field to UpdateDatabaseDdlMetadata ([#161](https://github.com/googleapis/python-spanner/issues/161)) ([2faf01b](https://github.com/googleapis/python-spanner/commit/2faf01b135360586ef27c66976646593fd85fd1e)) * adding missing docstrings for functions & classes ([#188](https://github.com/googleapis/python-spanner/issues/188)) ([9788cf8](https://github.com/googleapis/python-spanner/commit/9788cf8678d882bd4ccf551f828050cbbb8c8f3a)) * autocommit sample ([#172](https://github.com/googleapis/python-spanner/issues/172)) ([4ef793c](https://github.com/googleapis/python-spanner/commit/4ef793c9cd5d6dec6e92faf159665e11d63762ad)) ### Bug Fixes * convert operations pbs into Operation objects when listing operations ([#186](https://github.com/googleapis/python-spanner/issues/186)) ([ed7152a](https://github.com/googleapis/python-spanner/commit/ed7152adc37290c63e59865265f36c593d9b8da3)) * Convert PBs in system test cleanup ([#199](https://github.com/googleapis/python-spanner/issues/199)) ([ede4343](https://github.com/googleapis/python-spanner/commit/ede4343e518780a4ab13ae83017480d7046464d6)) * **dbapi:** autocommit enabling fails if no transactions begun ([#177](https://github.com/googleapis/python-spanner/issues/177)) ([e981adb](https://github.com/googleapis/python-spanner/commit/e981adb3157bb06e4cb466ca81d74d85da976754)) * **dbapi:** executemany() hiding all the results except the last ([#181](https://github.com/googleapis/python-spanner/issues/181)) ([020dc17](https://github.com/googleapis/python-spanner/commit/020dc17c823dfb65bfaacace14d2c9f491c97e11)) * **dbapi:** Spanner protobuf changes causes KeyError's ([#206](https://github.com/googleapis/python-spanner/issues/206)) ([f1e21ed](https://github.com/googleapis/python-spanner/commit/f1e21edbf37aab93615fd415d61f829d2574916b)) * remove client side gRPC receive limits ([#192](https://github.com/googleapis/python-spanner/issues/192)) ([90effc4](https://github.com/googleapis/python-spanner/commit/90effc4d0f4780b7a7c466169f9fc1e45dab8e7f)) * Rename to fix "Mismatched region tag" check ([#201](https://github.com/googleapis/python-spanner/issues/201)) ([c000ec4](https://github.com/googleapis/python-spanner/commit/c000ec4d9b306baa0d5e9ed95f23c0273d9adf32)) ### Documentation * homogenize region tags ([#194](https://github.com/googleapis/python-spanner/issues/194)) ([1501022](https://github.com/googleapis/python-spanner/commit/1501022239dfa8c20290ca0e0cf6a36e9255732c)) * homogenize region tags pt 2 ([#202](https://github.com/googleapis/python-spanner/issues/202)) ([87789c9](https://github.com/googleapis/python-spanner/commit/87789c939990794bfd91f5300bedc449fd74bd7e)) * update CHANGELOG breaking change comment ([#180](https://github.com/googleapis/python-spanner/issues/180)) ([c7b3b9e](https://github.com/googleapis/python-spanner/commit/c7b3b9e4be29a199618be9d9ffa1d63a9d0f8de7)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
When enabling an
autocommit
mode, all the begun transactions must be commited, but while trying to commit a transaction we don't check if it is already committed/rollbacked.