From 0a667231a075ffc599f7e428c775322159cf90b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Thu, 23 Feb 2023 16:17:54 +0100 Subject: [PATCH 1/2] Wrap every SQLite query in a transaction --- tests/WP_SQLite_Translator_Tests.php | 65 ++++++++++++- .../sqlite/class-wp-sqlite-translator.php | 93 +++++++++++++------ 2 files changed, 127 insertions(+), 31 deletions(-) diff --git a/tests/WP_SQLite_Translator_Tests.php b/tests/WP_SQLite_Translator_Tests.php index 9702f0d6..906c7ab0 100644 --- a/tests/WP_SQLite_Translator_Tests.php +++ b/tests/WP_SQLite_Translator_Tests.php @@ -851,21 +851,80 @@ public function testStartTransactionCommand() { $this->assertCount( 0, $this->engine->get_query_results() ); } - public function testNestedTransactionHasNoEffect() { + public function testNestedTransactionWork() { $this->engine->query( 'BEGIN' ); $this->engine->query( "INSERT INTO _options (option_name) VALUES ('first');" ); $this->engine->query( 'START TRANSACTION' ); $this->engine->query( "INSERT INTO _options (option_name) VALUES ('second');" ); + $this->engine->query( 'START TRANSACTION' ); + $this->engine->query( "INSERT INTO _options (option_name) VALUES ('third');" ); + $this->engine->query( 'SELECT * FROM _options;' ); + $this->assertCount( 3, $this->engine->get_query_results() ); + + $this->engine->query( 'ROLLBACK' ); $this->engine->query( 'SELECT * FROM _options;' ); $this->assertCount( 2, $this->engine->get_query_results() ); $this->engine->query( 'ROLLBACK' ); $this->engine->query( 'SELECT * FROM _options;' ); - $this->assertCount( 0, $this->engine->get_query_results() ); + $this->assertCount( 1, $this->engine->get_query_results() ); $this->engine->query( 'COMMIT' ); $this->engine->query( 'SELECT * FROM _options;' ); - $this->assertCount( 0, $this->engine->get_query_results() ); + $this->assertCount( 1, $this->engine->get_query_results() ); + } + + public function testNestedTransactionWorkComplexModify() { + $this->engine->query( 'BEGIN' ); + // Create a complex ALTER Table query where the first + // column is added successfully, but the second fails. + // Behind the scenes, this single MySQL query is split + // into multiple SQLite queries – some of them will + // succeed, some will fail. + $success = $this->engine->query( " + ALTER TABLE _options + ADD COLUMN test varchar(20), + ADD COLUMN test varchar(20) + " ); + $this->assertFalse($success); + // Commit the transaction. + $this->engine->query( 'COMMIT' ); + + // Confirm the entire query failed atomically and no column was + // added to the table. + $this->engine->query( 'DESCRIBE _options;' ); + $fields = $this->engine->get_query_results(); + + $this->assertEquals( + $fields, + array( + (object) array( + 'Field' => 'ID', + 'Type' => 'integer', + 'Null' => 'NO', + 'Key' => 'PRI', + 'Default' => null, + 'Extra' => '', + ), + (object) array( + 'Field' => 'option_name', + 'Type' => 'text', + 'Null' => 'NO', + 'Key' => '', + 'Default' => '', + 'Extra' => '', + ), + (object) array( + 'Field' => 'option_value', + 'Type' => 'text', + 'Null' => 'NO', + 'Key' => '', + 'Default' => '', + 'Extra' => '', + ) + ) + ); + } public function testCount() { diff --git a/wp-includes/sqlite/class-wp-sqlite-translator.php b/wp-includes/sqlite/class-wp-sqlite-translator.php index 13727e5a..65b67296 100644 --- a/wp-includes/sqlite/class-wp-sqlite-translator.php +++ b/wp-includes/sqlite/class-wp-sqlite-translator.php @@ -289,12 +289,12 @@ class WP_SQLite_Translator { private $return_value; /** - * Variable to check if there is an active transaction. + * Variable to keep track of nested transactions level. * - * @var boolean + * @var number * @access protected */ - protected $has_active_transaction = false; + protected $transaction_level = 0; /** * Constructor. @@ -341,7 +341,7 @@ public function __construct( $pdo = null ) { return false; } } - + new WP_SQLite_PDO_User_Defined_Functions( $pdo ); // MySQL data comes across stringified by default. @@ -523,19 +523,22 @@ private function prepare_directory() { */ public function query( $statement, $mode = PDO::FETCH_OBJ, ...$fetch_mode_args ) { // phpcs:ignore WordPress.DB.RestrictedClasses $this->flush(); + if ( + preg_match( '/^\s*START TRANSACTION/i', $statement ) + || preg_match( '/^\s*BEGIN/i', $statement ) + ) { + return $this->beginTransaction(); + } + if ( preg_match( '/^\s*COMMIT/i', $statement ) ) { + return $this->commit(); + } + if ( preg_match( '/^\s*ROLLBACK/i', $statement ) ) { + return $this->rollBack(); + } + try { - if ( - preg_match( '/^START TRANSACTION/i', $statement ) - || preg_match( '/^BEGIN/i', $statement ) - ) { - return $this->beginTransaction(); - } - if ( preg_match( '/^COMMIT/i', $statement ) ) { - return $this->commit(); - } - if ( preg_match( '/^ROLLBACK/i', $statement ) ) { - return $this->rollBack(); - } + // Perform all the queries in a nested transaction. + $this->beginTransaction(); do { $error = null; @@ -596,6 +599,18 @@ public function query( $statement, $mode = PDO::FETCH_OBJ, ...$fetch_mode_args ) } } + // Commit the nested transaction. + $this->commit(); + } catch ( Exception $err ) { + // Rollback the nested transaction. + $this->rollBack(); + if ( defined( 'PDO_DEBUG' ) && PDO_DEBUG === true ) { + throw $err; + } + return $this->handle_error( $err ); + } + + try { if ( $translation->calc_found_rows ) { $this->found_rows_result = $translation->calc_found_rows; } @@ -631,6 +646,8 @@ public function query( $statement, $mode = PDO::FETCH_OBJ, ...$fetch_mode_args ) return $this->return_value; } catch ( Exception $err ) { + // Rollback the nested transaction. + $this->rollBack(); if ( defined( 'PDO_DEBUG' ) && PDO_DEBUG === true ) { throw $err; } @@ -2999,11 +3016,19 @@ private function flush() { * @return boolean */ public function beginTransaction() { - if ( $this->has_active_transaction ) { - return false; + $success = false; + try { + if (0 === $this->transaction_level) { + $success = $this->pdo->beginTransaction(); + } else { + $success = $this->pdo->query('SAVEPOINT LEVEL' . $this->transaction_level); + } + } finally { + if ($success) { + ++$this->transaction_level; + } } - $this->has_active_transaction = $this->pdo->beginTransaction(); - return $this->has_active_transaction; + return $success; } /** @@ -3011,12 +3036,18 @@ public function beginTransaction() { * * @see PDO::commit() * - * @return void + * @return boolean True on success, false on failure. */ public function commit() { - if ( $this->has_active_transaction ) { - $this->pdo->commit(); - $this->has_active_transaction = false; + if ( $this->transaction_level === 0 ) { + return false; + } + + --$this->transaction_level; + if ( 0 === $this->transaction_level ) { + return $this->pdo->commit(); + } else { + return (bool) $this->pdo->query( 'RELEASE SAVEPOINT LEVEL' . $this->transaction_level ); } } @@ -3025,12 +3056,18 @@ public function commit() { * * @see PDO::rollBack() * - * @return void + * @return boolean True on success, false on failure. */ public function rollBack() { - if ( $this->has_active_transaction ) { - $this->pdo->rollBack(); - $this->has_active_transaction = false; + if ( $this->transaction_level === 0 ) { + return false; + } + + --$this->transaction_level; + if ( 0 === $this->transaction_level ) { + return $this->pdo->rollBack(); + } else { + return (bool) $this->pdo->query( 'ROLLBACK TO SAVEPOINT LEVEL' . $this->transaction_level ); } } } From eee4638136031b799cd4627814b2baf2d5b1713f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Thu, 23 Feb 2023 16:25:38 +0100 Subject: [PATCH 2/2] Don't split the query() method into two independent try/catch blocks --- wp-includes/sqlite/class-wp-sqlite-translator.php | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/wp-includes/sqlite/class-wp-sqlite-translator.php b/wp-includes/sqlite/class-wp-sqlite-translator.php index 65b67296..085d7402 100644 --- a/wp-includes/sqlite/class-wp-sqlite-translator.php +++ b/wp-includes/sqlite/class-wp-sqlite-translator.php @@ -599,18 +599,6 @@ public function query( $statement, $mode = PDO::FETCH_OBJ, ...$fetch_mode_args ) } } - // Commit the nested transaction. - $this->commit(); - } catch ( Exception $err ) { - // Rollback the nested transaction. - $this->rollBack(); - if ( defined( 'PDO_DEBUG' ) && PDO_DEBUG === true ) { - throw $err; - } - return $this->handle_error( $err ); - } - - try { if ( $translation->calc_found_rows ) { $this->found_rows_result = $translation->calc_found_rows; } @@ -644,6 +632,8 @@ public function query( $statement, $mode = PDO::FETCH_OBJ, ...$fetch_mode_args ) break; } + // Commit the nested transaction. + $this->commit(); return $this->return_value; } catch ( Exception $err ) { // Rollback the nested transaction.