diff --git a/.gitignore b/.gitignore index 70283d979ea..2014b3cbf03 100644 --- a/.gitignore +++ b/.gitignore @@ -59,6 +59,10 @@ autom4te.cache/ /vendor /ide/ +# Jetbrains IDE +.idea/ +*.iml + .zephir/ .temp/ diff --git a/CHANGELOG-5.0.md b/CHANGELOG-5.0.md index 051245658f6..0e741aef880 100644 --- a/CHANGELOG-5.0.md +++ b/CHANGELOG-5.0.md @@ -27,6 +27,7 @@ ### Fixed - Fixed `Phalcon\Support\Helper\PascalCase` causing memory leak by anonymous function [#16593](https://github.com/phalcon/cphalcon/issues/16593) +- Fixed `Phalcon\Mvc\Model\Query` to rollback failed transactions and re-throw exception for data consistency [#16604](https://github.com/phalcon/cphalcon/issues/16604) ### Removed diff --git a/phalcon/Mvc/Model/Query.zep b/phalcon/Mvc/Model/Query.zep index 9085f5cb52f..1094894bd87 100644 --- a/phalcon/Mvc/Model/Query.zep +++ b/phalcon/Mvc/Model/Query.zep @@ -752,7 +752,7 @@ class Query implements QueryInterface, InjectionAwareInterface */ final protected function executeDelete(array intermediate, array bindParams, array bindTypes) -> { - var models, modelName, model, records, connection, record; + var models, modelName, model, records, connection, record, exception; let models = intermediate["models"]; @@ -802,21 +802,27 @@ class Query implements QueryInterface, InjectionAwareInterface records->rewind(); while records->valid() { - let record = records->current(); + try { + let record = records->current(); - /** - * We delete every record found - */ - if !record->delete() { /** - * Rollback the transaction + * We delete every record found */ + if !record->delete() { + /** + * Rollback the transaction + */ + connection->rollback(); + + return new Status(false, record); + } + + records->next(); + } catch \PDOException, exception { connection->rollback(); - return new Status(false, record); + throw exception; } - - records->next(); } /** @@ -1356,7 +1362,8 @@ class Query implements QueryInterface, InjectionAwareInterface { var models, modelName, model, connection, dialect, fields, values, updateValues, fieldName, value, selectBindParams, selectBindTypes, - number, field, records, exprValue, updateValue, wildcard, record; + number, field, records, exprValue, updateValue, wildcard, record, + exception; let models = intermediate["models"]; @@ -1486,25 +1493,30 @@ class Query implements QueryInterface, InjectionAwareInterface records->rewind(); - //for record in iterator(records) { while records->valid() { - let record = records->current(); + try { + let record = records->current(); - record->assign(updateValues); + record->assign(updateValues); - /** - * We apply the executed values to every record found - */ - if !record->update() { /** - * Rollback the transaction on failure + * We apply the executed values to every record found */ + if !record->update() { + /** + * Rollback the transaction on failure + */ + connection->rollback(); + + return new Status(false, record); + } + + records->next(); + } catch \PDOException, exception { connection->rollback(); - return new Status(false, record); + throw exception; } - - records->next(); } /** diff --git a/tests/_data/fixtures/Migrations/AbstractMigration.php b/tests/_data/fixtures/Migrations/AbstractMigration.php index 943e9bfbea0..6a9308cb6fb 100644 --- a/tests/_data/fixtures/Migrations/AbstractMigration.php +++ b/tests/_data/fixtures/Migrations/AbstractMigration.php @@ -38,11 +38,11 @@ abstract class AbstractMigration * * @param PDO|null $connection */ - final public function __construct(PDO $connection = null) + final public function __construct(PDO $connection = null, bool $withClear = true) { $this->connection = $connection; - $this->clear(); + $withClear && $this->clear(); } /** diff --git a/tests/_data/fixtures/Migrations/RollbackTestMigration.php b/tests/_data/fixtures/Migrations/RollbackTestMigration.php new file mode 100644 index 00000000000..47644593636 --- /dev/null +++ b/tests/_data/fixtures/Migrations/RollbackTestMigration.php @@ -0,0 +1,48 @@ + + * + * For the full copyright and license information, please view the + * LICENSE.txt file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Phalcon\Tests\Fixtures\Migrations; + +/** + * Class InvoicesMigration + */ +class RollbackTestMigration extends AbstractMigration +{ + protected $table = 'co_rb_test_model'; + + protected function getSqlMysql(): array + { + return [ + 'DROP TABLE IF EXISTS co_rb_test_model;', + 'CREATE TABLE co_rb_test_model (id SMALLINT, name VARCHAR(10) NOT NULL);', + ]; + } + + protected function getSqlSqlite(): array + { + return []; + } + + protected function getSqlPgsql(): array + { + return [ + 'DROP TABLE IF EXISTS co_rb_test_model;', + 'CREATE TABLE co_rb_test_model (id SMALLINT, name VARCHAR(10) NOT NULL);', + ]; + } + + protected function getSqlSqlsrv(): array + { + return []; + } +} diff --git a/tests/_data/fixtures/models/RbTestModel.php b/tests/_data/fixtures/models/RbTestModel.php new file mode 100644 index 00000000000..bedd49ea643 --- /dev/null +++ b/tests/_data/fixtures/models/RbTestModel.php @@ -0,0 +1,19 @@ +setSource('co_rb_test_model'); + } +} diff --git a/tests/database/Mvc/Model/Query/RollbackOnExceptionCest.php b/tests/database/Mvc/Model/Query/RollbackOnExceptionCest.php new file mode 100644 index 00000000000..3f1658ffb39 --- /dev/null +++ b/tests/database/Mvc/Model/Query/RollbackOnExceptionCest.php @@ -0,0 +1,124 @@ + + * For the full copyright and license information, please view the + * LICENSE.txt file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Phalcon\Tests\Database\Mvc\Model\Query; + +use Codeception\Util\Debug; +use DatabaseTester; +use Phalcon\Mvc\Model\Manager; +use Phalcon\Storage\Exception; +use Phalcon\Tests\Fixtures\Migrations\RollbackTestMigration; +use Phalcon\Tests\Fixtures\Traits\DiTrait; +use Phalcon\Tests\Fixtures\Traits\RecordsTrait; +use PDOException; + +class RollbackOnExceptionCest +{ + use DiTrait; + use RecordsTrait; + + /** + * @var RollbackTestMigration + */ + private $migration; + + /** + * Executed before each test + * + * @param DatabaseTester $I + * + * @return void + */ + public function _before(DatabaseTester $I): void + { + try { + $this->setNewFactoryDefault(); + } catch (Exception $e) { + $I->fail($e->getMessage()); + } + + $this->setDatabase($I); + + $this->migration = new RollbackTestMigration($I->getConnection(), false); + } + + /** + * Tests Phalcon\Mvc\Model\Query :: mvcModelQueryRollbackOnException() - Issue 16604 + * + * @param DatabaseTester $I + * + * @author noone-silent + * @since 2024-06-10 + * @issue 16604 + * @group mysql + * @group pgsql + */ + public function mvcModelQueryRollbackOnException(DatabaseTester $I) + { + $I->wantToTest('Mvc\Model\Query :: mvcModelQueryRollbackOnException() - #16604'); + + $this->migration->create(); + $this->migration->clear(); + + $I->dontSeeInDatabase($this->migration->getTable(), ['name' => 'abc']); + $I->dontSeeInDatabase($this->migration->getTable(), ['name' => 'test 4 OK']); + $I->dontSeeInDatabase($this->migration->getTable(), ['name' => 'test 5 OK']); + + /** @var Manager|null $modelsManager */ + $modelsManager = $this->container->get('modelsManager'); + if ($modelsManager instanceof Manager === false) { + throw new \RuntimeException('Manager not set in di'); + } + + $modelsManager->executeQuery('DELETE FROM \\Phalcon\\Tests\\Models\\RbTestModel'); + $modelsManager->executeQuery('INSERT INTO \\Phalcon\\Tests\\Models\\RbTestModel (id, name) VALUES (1, "abc")'); + $modelsManager->executeQuery('INSERT INTO \\Phalcon\\Tests\\Models\\RbTestModel (id, name) VALUES (2, "abc")'); + $modelsManager->executeQuery('INSERT INTO \\Phalcon\\Tests\\Models\\RbTestModel (id, name) VALUES (3, "abc")'); + $modelsManager->executeQuery('INSERT INTO \\Phalcon\\Tests\\Models\\RbTestModel (id, name) VALUES (4, "abc")'); + $modelsManager->executeQuery('INSERT INTO \\Phalcon\\Tests\\Models\\RbTestModel (id, name) VALUES (5, "abc")'); + + $messages = []; + + $messages[] = $this->update(1, 'test 1 OK', $modelsManager); + $messages[] = $this->update(2, 'test 2 OK', $modelsManager); + $messages[] = $this->update(3, 'test 3 DATA TRUNCATED ERROR', $modelsManager); + $messages[] = $this->update(4, 'test 4 OK', $modelsManager); + $messages[] = $this->update(5, 'test 5 OK', $modelsManager); + + Debug::debug($messages); + + $I->assertEquals('Update test 1 OK', $messages[0]); + $I->assertEquals('Update test 2 OK', $messages[1]); + $I->assertNotEquals('Update test 3 OK', $messages[2]); + $I->assertStringContainsString('PDOException', $messages[2]); + $I->assertEquals('Update test 4 OK', $messages[3]); + $I->assertEquals('Update test 5 OK', $messages[4]); + + $I->seeInDatabase($this->migration->getTable(), ['name' => 'test 1 OK']); + $I->seeInDatabase($this->migration->getTable(), ['name' => 'test 2 OK']); + $I->seeInDatabase($this->migration->getTable(), ['id' => 3, 'name' => 'abc']); + $I->seeInDatabase($this->migration->getTable(), ['name' => 'test 4 OK']); + $I->seeInDatabase($this->migration->getTable(), ['name' => 'test 5 OK']); + } + + private function update(int $id, string $name, Manager $modelsManager): string + { + try { + $query = 'UPDATE \\Phalcon\\Tests\\Models\\RbTestModel SET name = :name: WHERE id = :id:'; + $modelsManager->executeQuery($query, ['id' => $id, 'name' => $name]); + return "Update $name"; + } catch (PDOException $exc) { + return $exc::class . ' ' . $exc->getMessage(); + } catch (\Throwable $exc) { + return get_class($exc) . ' ' . $exc->getMessage(); + } + } +}