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.

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

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

Merged
merged 4 commits into from
Jan 16, 2024
+8 −3
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
047d889
fix: fixed an error when rolling back a transaction that did not exec…
halnique Jan 12, 2024
beef732
test: fix cursor rollback
halnique Jan 15, 2024
9613ab6
chore: update CHANGELOG
halnique Jan 15, 2024
feff339
chore: fill release date
halnique Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ Changed
Fixed
- `Schema\Grammar::compileAdd()` `Schema\Grammar::compileChange()` `Schema\Grammar::compileChange()` now create separate statements (#159)

# v6.1.2 (2024-01-16)

Fixed
- Fixed an error when rolling back a transaction that did not execute begin (#166)

# v6.1.1 (2023-12-11)

Fixed
Expand Down
2 changes: 1 addition & 1 deletion src/Concerns/ManagesTransactions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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().

$this->currentTransaction->rollBack();
}
} finally {
Expand Down
4 changes: 2 additions & 2 deletions tests/SessionNotFoundTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,13 +160,13 @@ public function test_session_not_found_on_cursor(): void
$passes = 0;

$conn->transaction(function () use ($conn, &$passes) {
$cursor = $conn->cursor('SELECT 12345');

if ($passes === 0) {
$this->deleteSession($conn);
$passes++;
}

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

@halnique halnique Jan 15, 2024 •
edited
Loading

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


$this->assertEquals([12345], $cursor->current());

$passes++;
Expand Down