fix(dbapi): autocommit enabling fails if no transactions begun - #177
larkee merged 35 commits into
Conversation
Sorry, something went wrong.
c24t
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
larkee
left a comment
There was a problem hiding this comment.
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 👍
Sorry, something went wrong.
…-spanner into autocommit_change
🤖 I have created a release \*beep\* \*boop\* --- ## [3.0.0](https://www.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://www.github.com/googleapis/python-spanner/issues/193)) ([ed462b5](https://www.github.com/googleapis/python-spanner/commit/ed462b567a1a33f9105ffb37ba1218f379603614)) * add support for ssl credentials; add throttled field to UpdateDatabaseDdlMetadata ([#161](https://www.github.com/googleapis/python-spanner/issues/161)) ([2faf01b](https://www.github.com/googleapis/python-spanner/commit/2faf01b135360586ef27c66976646593fd85fd1e)) * adding missing docstrings for functions & classes ([#188](https://www.github.com/googleapis/python-spanner/issues/188)) ([9788cf8](https://www.github.com/googleapis/python-spanner/commit/9788cf8678d882bd4ccf551f828050cbbb8c8f3a)) * autocommit sample ([#172](https://www.github.com/googleapis/python-spanner/issues/172)) ([4ef793c](https://www.github.com/googleapis/python-spanner/commit/4ef793c9cd5d6dec6e92faf159665e11d63762ad)) ### Bug Fixes * convert operations pbs into Operation objects when listing operations ([#186](https://www.github.com/googleapis/python-spanner/issues/186)) ([ed7152a](https://www.github.com/googleapis/python-spanner/commit/ed7152adc37290c63e59865265f36c593d9b8da3)) * Convert PBs in system test cleanup ([#199](https://www.github.com/googleapis/python-spanner/issues/199)) ([ede4343](https://www.github.com/googleapis/python-spanner/commit/ede4343e518780a4ab13ae83017480d7046464d6)) * **dbapi:** autocommit enabling fails if no transactions begun ([#177](https://www.github.com/googleapis/python-spanner/issues/177)) ([e981adb](https://www.github.com/googleapis/python-spanner/commit/e981adb3157bb06e4cb466ca81d74d85da976754)) * **dbapi:** executemany() hiding all the results except the last ([#181](https://www.github.com/googleapis/python-spanner/issues/181)) ([020dc17](https://www.github.com/googleapis/python-spanner/commit/020dc17c823dfb65bfaacace14d2c9f491c97e11)) * **dbapi:** Spanner protobuf changes causes KeyError's ([#206](https://www.github.com/googleapis/python-spanner/issues/206)) ([f1e21ed](https://www.github.com/googleapis/python-spanner/commit/f1e21edbf37aab93615fd415d61f829d2574916b)) * remove client side gRPC receive limits ([#192](https://www.github.com/googleapis/python-spanner/issues/192)) ([90effc4](https://www.github.com/googleapis/python-spanner/commit/90effc4d0f4780b7a7c466169f9fc1e45dab8e7f)) * Rename to fix "Mismatched region tag" check ([#201](https://www.github.com/googleapis/python-spanner/issues/201)) ([c000ec4](https://www.github.com/googleapis/python-spanner/commit/c000ec4d9b306baa0d5e9ed95f23c0273d9adf32)) ### Documentation * homogenize region tags ([#194](https://www.github.com/googleapis/python-spanner/issues/194)) ([1501022](https://www.github.com/googleapis/python-spanner/commit/1501022239dfa8c20290ca0e0cf6a36e9255732c)) * homogenize region tags pt 2 ([#202](https://www.github.com/googleapis/python-spanner/issues/202)) ([87789c9](https://www.github.com/googleapis/python-spanner/commit/87789c939990794bfd91f5300bedc449fd74bd7e)) * update CHANGELOG breaking change comment ([#180](https://www.github.com/googleapis/python-spanner/issues/180)) ([c7b3b9e](https://www.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.