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

Jump to bottom

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

Merged
merged 35 commits into from Dec 30, 2020
Merged

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

merged 35 commits into from Dec 30, 2020

Conversation

IlyaFaer
Copy link
Contributor

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 Issues related to the googleapis/python-spanner API. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Nov 30, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 30, 2020
@IlyaFaer IlyaFaer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 30, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 30, 2020
@IlyaFaer IlyaFaer marked this pull request as ready for review November 30, 2020 09:32
@IlyaFaer IlyaFaer requested a review from a team as a code owner November 30, 2020 09:32
c24t
c24t approved these changes Dec 1, 2020
View reviewed changes
Copy link
Contributor

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

@IlyaFaer IlyaFaer mentioned this pull request Dec 1, 2020
Closed
@IlyaFaer
Copy link
Contributor Author

IlyaFaer commented Dec 1, 2020 •
edited

@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 Add this label to force Kokoro to re-run the tests. label Dec 3, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 3, 2020
@IlyaFaer IlyaFaer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 9, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 9, 2020
@c24t c24t added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 10, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 10, 2020
@c24t
Copy link
Contributor

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
View reviewed changes
Copy link
Contributor

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

@c24t c24t added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 18, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 18, 2020
@IlyaFaer IlyaFaer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2020
@larkee larkee added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 30, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 30, 2020
@larkee larkee merged commit e981adb into googleapis:master Dec 30, 2020
@IlyaFaer IlyaFaer deleted the autocommit_change branch January 1, 2021 08:38
gcf-merge-on-green bot pushed a commit that referenced this pull request Jan 15, 2021
@release-please
chore: release 3.0.0 (#208)
58359f9 
🤖 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
Reviewers

@larkee larkee larkee approved these changes

@c24t c24t c24t approved these changes

Assignees
No one assigned
Labels
api: spanner Issues related to the googleapis/python-spanner API. cla: yes This human has signed the Contributor License Agreement. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@IlyaFaer @c24t @larkee @yoshi-kokoro @AVaksman