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

fix(dbapi): autocommit enabling fails if no transactions begun #177

Merged
merged 35 commits into from Dec 30, 2020

Conversation

IlyaFaer
Copy link
Member

@IlyaFaer IlyaFaer commented Nov 30, 2020

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.

@IlyaFaer IlyaFaer added api: spanner type: bug labels Nov 30, 2020
@google-cla google-cla bot added the cla: yes label Nov 30, 2020
@IlyaFaer IlyaFaer added the kokoro:force-run label Nov 30, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Nov 30, 2020
@IlyaFaer IlyaFaer marked this pull request as ready for review Nov 30, 2020
@IlyaFaer IlyaFaer requested a review from as a code owner Nov 30, 2020
c24t
c24t approved these changes Dec 1, 2020
Copy link
Contributor

@c24t c24t left a 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.

@IlyaFaer
Copy link
Member Author

@IlyaFaer IlyaFaer commented Dec 1, 2020

@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.

@IlyaFaer IlyaFaer added the kokoro:force-run label Dec 3, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Dec 3, 2020
@IlyaFaer IlyaFaer added the kokoro:force-run label Dec 9, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Dec 9, 2020
@c24t c24t added the kokoro:force-run label Dec 10, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Dec 10, 2020
@c24t
Copy link
Contributor

@c24t c24t commented Dec 17, 2020

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.

larkee
larkee approved these changes Dec 17, 2020
Copy link
Contributor

@larkee larkee left a 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 👍

@c24t c24t added the kokoro:force-run label Dec 18, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Dec 18, 2020
@IlyaFaer IlyaFaer added the kokoro:force-run label Dec 23, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Dec 23, 2020
@larkee larkee added the kokoro:force-run label Dec 30, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Dec 30, 2020
@larkee larkee merged commit e981adb into googleapis:master Dec 30, 2020
9 checks passed
@IlyaFaer IlyaFaer deleted the autocommit_change branch Jan 1, 2021
gcf-merge-on-green bot pushed a commit that referenced this issue Jan 15, 2021
🤖 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants