Skip to content

Transactions - Bug when using transaction APIs explicitly (not through the context manager) #447

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.

Sign up for GitHub

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
Closed
flea89 opened this issue Jun 12, 2023 · 4 comments · May be fixed by #578
Closed

Transactions - Bug when using transaction APIs explicitly (not through the context manager) #447

flea89 opened this issue Jun 12, 2023 · 4 comments · May be fixed by #578
Assignees
@daniel-sanche
Labels
api: datastore Issues related to the googleapis/python-datastore API. priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@flea89
Copy link

flea89 commented Jun 12, 2023

TL;DR : when using trx.begin() and trx.commit() the transactions don't behave as expected.

Please refer to this merge request to see the bug highlighted in the test.

From a quick look at the code, the problem seems to be the transaction is not added to the client._batch_stack when the APIs are called explicitly.
_push_batch an _pop_batch are called only through the context manager __enter__ and __exit__.

Because of this
client.current_batch and current_transaction

Environment details

  • OS type and version: MacOs Ventura 13.1
  • Python version: 3.8.16
  • pip version: 22.0.4
  • google-cloud-datastore version: 2.15.2

Steps to reproduce

  1. Create a transaction client.transaction()
  2. Start a transaction txn.begin()
  3. Get an object entity_in_txn = client.get(key)
  4. Update and put the same entity outside of the transaction.
  5. Update the entity and put the entity within the transaction
  6. Commit the transaction txn.commit()

The transaction should raise a Conflit but it doesn't.
(When doing the same with the context manager the Exeption is raised, see here

Code example

See here

Stack trace

# example
N/A
@product-auto-label product-auto-label bot added the api: datastore Issues related to the googleapis/python-datastore API. label Jun 12, 2023
@flea89 flea89 changed the title Transactions - Bug when using transaction APIs explicitly (not the context manager) Transactions - Bug when using transaction APIs explicitly (not through the context manager) Jun 20, 2023
@daniel-sanche daniel-sanche added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p3 Desirable enhancement or fix. May not be included in next release. labels Jan 11, 2024
@daniel-sanche
Copy link
Contributor

I believe currently, transactions are designed to only work as part of a context manager

That said, I think it would be a good idea to re-structure it so that it can be used without a context manager, as in your example

@daniel-sanche daniel-sanche added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Jul 2, 2024
@artoale
Copy link

artoale commented Jul 3, 2024 •
edited
Loading

I believe currently, transactions are designed to only work as part of a context manager

That said, I think it would be a good idea to re-structure it so that it can be used without a context manager, as in your example

The official documentation states:

This method is called automatically when entering a with statement, however it can be called explicitly if you don't want to use a context manager.

I'd suggest either changing the behaviour (which should be a matter of moving the logic from __enter__ to begin or update the docs to reflect that the method is unsupported. Current behaviour fails in fairly hard-to-identify ways

@daniel-sanche daniel-sanche added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Jul 3, 2024
@daniel-sanche
Copy link
Contributor

Ok, good catch. I'll mark this as a bug again then. We should make sure to add tests for this use case as well

@daniel-sanche daniel-sanche mentioned this issue Dec 14, 2024
Open
@daniel-sanche
Copy link
Contributor

After looking closer at the code, it's clear the intention is that only context managers are automatically associated with the client object. Changing the library so that any begin call triggers a client._push_batch would have implications for existing code, and would result in a breaking change

If calling managing the lifecycle manually, you can still read data without conflict using client.get(key, transaction=txn). So explicit usage without context managers works, it's just a bit more work to manage everything yourself

I opened #578 to update the docstrings to hopefully make things more clear, and add the new test that avoids context managers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@daniel-sanche daniel-sanche

Labels
api: datastore Issues related to the googleapis/python-datastore API. priority: p3 Desirable enhancement or fix. May not be included in next release. 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 a pull request may close this issue.

chore(docs): update docstrings for batches without context managers googleapis/python-datastore
3 participants
@artoale @flea89 @daniel-sanche