Merge pull request #16610 from noone-silent/T16604-catch-exception-ro…

…llback-and-re-throw

[#16604] - fix: correctly handle transaction rollbacks if exceptions are thrown

.gitignore

@@ -59,6 +59,10 @@ autom4te.cache/
 /vendor
 /ide/
 
+# Jetbrains IDE
+.idea/
+*.iml
+
 .zephir/
 .temp/
 

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
 

phalcon/Mvc/Model/Query.zep

@@ -752,7 +752,7 @@ class Query implements QueryInterface, InjectionAwareInterface
      */
     final protected function executeDelete(array intermediate, array bindParams, array bindTypes) -> <StatusInterface>
     {
-        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();
         }
 
         /**

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();
     }
 
     /**

tests/_data/fixtures/Migrations/RollbackTestMigration.php

@@ -0,0 +1,48 @@
+<?php
+
+/**
+ * This file is part of the Phalcon Framework.
+ *
+ * (c) Phalcon Team <team@phalcon.io>
+ *
+ * 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 [];
+    }
+}

tests/_data/fixtures/models/RbTestModel.php

@@ -0,0 +1,19 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Phalcon\Tests\Models;
+
+use Phalcon\Mvc\Model;
+
+class RbTestModel extends Model
+{
+    public $id;
+
+    public $name; // varchar 10
+
+    public function initialize()
+    {
+        $this->setSource('co_rb_test_model');
+    }
+}

tests/database/Mvc/Model/Query/RollbackOnExceptionCest.php

@@ -0,0 +1,124 @@
+<?php
+
+/**
+ * This file is part of the Phalcon Framework.
+ * (c) Phalcon Team <team@phalcon.io>
+ * 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 <lominum@protonmail.com>
+     * @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();
+        }
+    }
+}