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: fixed an error when rolling back a transaction that did not execute begin #166

Merged
merged 4 commits into from
Jan 16, 2024

Conversation

halnique
Copy link
Contributor

@halnique halnique commented Jan 12, 2024

Fix: #165

Several features and tests were broken due to the google/cloud-spanner v1.68.0 release.

  • An exception is thrown if rollback is performed without any query call in the transaction.
  • Tests that depend on the query execution order called from the cursor() method fail.

Checklist

  • CHANGELOG

Reference

@halnique halnique changed the title fix: fixed an error when rolling back a transaction that did not exec… fix: fixed an error when rolling back a transaction that did not execute begin Jan 15, 2024
@halnique halnique marked this pull request as ready for review January 15, 2024 06:47
@halnique halnique self-assigned this Jan 15, 2024
if ($passes === 0) {
$this->deleteSession($conn);
$passes++;
}

$cursor = $conn->cursor('SELECT 12345');
Copy link
Contributor Author

@halnique halnique Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The processing order of the cursor() method of the Result class has changed.
Before the change, a generator was generated and validated when the rows() method was called, but after the change, a generator is generated and validated when the Result class is initialized.
In other words, the timing at which the query specified in the cursor() method is executed has changed from when the Generator::current() method is called to when the Connection::cursor() method is called.

ref: https://github.com/googleapis/google-cloud-php/pull/6708/files#diff-1c94ccf8ee4a0849e0cd857c1fb93ea32906963063a3652d9e3263412cb9267e

Before the change, the test code would fail due to this effect.

1) Colopl\Spanner\Tests\SessionNotFoundTest::test_session_not_found_on_cursor
Transaction should be called twice
Failed asserting that 3 matches expected 2.

/project/tests/SessionNotFoundTest.php:174

The reason why $passes is 3 is because it is handled as follows.

1.$cursor = $conn->cursor('SELECT 12345'); succeeds and executeStreamingSql is sent
2.$this->deleteSession($conn); is called
3.$passes++; value is 1
4.$this->assertEquals([12345], $cursor->current()); succeeds but is not expected
5.$passes++; value is 2
6. attempts to commit the transaction but fails because the session has been deleted and tries to retry handling NotFoundException
7. $cursor = $conn->cursor('SELECT 12345'); succeeds and executeStreamingSql is sent
8. $this->assertEquals([12345], $cursor->current()); succeeds
9. $passes++; value is 3
10. commit the transaction

@@ -168,7 +168,7 @@ protected function performRollBack($toLevel)

if ($this->currentTransaction !== null) {
try {
if ($this->currentTransaction->state() === Transaction::STATE_ACTIVE) {
if ($this->currentTransaction->state() === Transaction::STATE_ACTIVE && $this->currentTransaction->id() !== null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When rolling back a transaction, an exception is now thrown if the target transactionId is not specified.

ref: https://github.com/googleapis/google-cloud-php/pull/6708/files#diff-25ee9adab31aa54b734a2bc5f254b0e922b13e1c0ad27f1fff60a9a534552a03
L192-L194

This caused the following tests to fail.

1) Colopl\Spanner\Tests\ConnectionTest::test_connection_transaction_reset_on_exceptions
InvalidArgumentException: Rollback failed: Transaction not initiated.

/project/vendor/google/cloud-spanner/src/Operation.php:193
/project/vendor/google/cloud-spanner/src/Transaction.php:572
/project/src/Concerns/ManagesTransactions.php:172
/project/vendor/laravel/framework/src/Illuminate/Database/Concerns/ManagesTransactions.php:264
/project/src/Concerns/ManagesTransactions.php:73
/project/vendor/google/cloud-spanner/src/Database.php:957
/project/vendor/google/cloud-core/src/Retry.php:80
/project/vendor/google/cloud-spanner/src/Database.php:975
/project/src/Concerns/ManagesTransactions.php:76
/project/src/Connection.php:525
/project/src/Concerns/ManagesTransactions.php:81
/project/tests/ConnectionTest.php:541

Originally, when a transaction was called without specifying any options, Operation::beginTransaction() was called internally to set the transactionId.
With this change, the default options specified have been changed and Operation::beginTransaction() is no longer called internally.

ref: https://github.com/googleapis/google-cloud-php/pull/6708/files#diff-25ee9adab31aa54b734a2bc5f254b0e922b13e1c0ad27f1fff60a9a534552a03
L495-L497

As a result, InvalidArgumentException('Rollback failed: Transaction not initiated.') exception is thrown when rollback is done without calling any query in Database::runTransaction().

@halnique halnique merged commit dd06a26 into master Jan 16, 2024
1 check passed
@halnique halnique deleted the fix/rollback-fail-tranasction-not-initiated branch January 16, 2024 07:21
taka-oyama pushed a commit that referenced this pull request Jan 23, 2024
…ute begin (#166)

* fix: fixed an error when rolling back a transaction that did not execute begin

* test: fix cursor rollback

* chore: update CHANGELOG

* chore: fill release date
# Conflicts:
#	CHANGELOG.md
dkkoma added a commit that referenced this pull request Nov 19, 2024
dkkoma added a commit that referenced this pull request Nov 20, 2024
* fix: backport #150

* fix: backport #166

* fix: phpstan error

* fix: remove broken test

* fix: docs

* fix: github workflow

* fix release date
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests are failing
4 participants