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

[flea89]

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

[daniel-sanche]

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

[artoale]

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]

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]

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