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

[BUG]: Phalcon\Mvc\Model\Query does not handle internal transaction on PDOException #16604

Closed
poz opened this issue Jun 4, 2024 · 3 comments
Closed
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium

Comments

@poz
Copy link

poz commented Jun 4, 2024

Describe the bug
When a PDOException is throw by Phalcon\Mvc\Model\Query::executeUpdate(), the internal transaction did not rollback correctly and mess up the subsequent queries.

To Reproduce
Steps to reproduce the behavior:

  1. Create & populate a table with about 5 row of data (see example script below).
  2. Use $this->modelsManager->executeQuery to update for each single row.
  3. Force the 3rd update to hit a PDOException by updating a string value that is longer than varchar(10) (mysql strict mode)
  4. Catch and handle the exeception above. Continue updating remaining rows

Result:

  • Row 1-2 is updated correctly
  • Row 3 is not updated
  • Row 4-5 is not updated

Provide minimal script to reproduce the issue

<?php

use Phalcon\Mvc\Model;
use Phalcon\Di\FactoryDefault;
use Phalcon\DI\Injectable;
use Phalcon\Db\Adapter\Pdo\MySQL;

/*
Setup Model
set sql_mode=STRICT_ALL_TABLES;

CREATE TABLE `test_table`  (
  `id` INT UNSIGNED NOT NULL AUTO_INCREMENT,
  `name` VARCHAR(10) NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=INNODB;
*/
class TestModel extends Model
{
    public $id;
    public $name; // varchar 10

    public function initialize() {
        $this->setSource('test_table');
    }
}

// Setup db
$options = [
    'host'     => 'mysql',
    'dbname'   => 'test-phalcon',
    'port'     => 3306,
    'username' => 'user',
    'password' => 'password',
];
$connection = new MySQL($options);


// Create a DI
$di = new FactoryDefault();
$di->setShared('db', $connection);

// Main testing code
class TestComponent extends Injectable
{
    public function runTest() {
                
        // Reset / Repopulate 5 rows of data
        $this->modelsManager->executeQuery('DELETE FROM TestModel');
        $this->modelsManager->executeQuery('INSERT INTO TestModel (id) VALUES (1)');
        $this->modelsManager->executeQuery('INSERT INTO TestModel (id) VALUES (2)');
        $this->modelsManager->executeQuery('INSERT INTO TestModel (id) VALUES (3)');
        $this->modelsManager->executeQuery('INSERT INTO TestModel (id) VALUES (4)');
        $this->modelsManager->executeQuery('INSERT INTO TestModel (id) VALUES (5)');

        $this->update(1, 'test 1 OK');
        $this->update(2, 'test 2 OK');
        $this->update(3, 'test 3 DATA TRUNCATED ERROR');
        $this->update(4, 'test 4 OK');
        $this->update(5, 'test 5 OK');
    }
    
    public function update($id, $name) {
        try {
            $query = 'UPDATE TestModel SET name = :name: WHERE id = :id:';
            $result = $this->modelsManager->executeQuery($query, ['id' => $id, 'name' => $name]);
            echo "Update test $id OK" . PHP_EOL;
        } catch (Exception $ex) {
            // just print and ignore the exception here
            echo "Update test $id " . get_class($ex) . ':' . $ex->getMessage() . PHP_EOL;
        }
    }
}

$test = new TestComponent();
$test->runTest();

Expected behavior

  • Row 1-2 should update correctly
  • Row 3 should fail and exception are handled in try/catch
  • Row 4-5 should update correctly
Update test 1 OK
Update test 2 OK
Update test 3 PDOException:SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'name' at row 1
Update test 4 OK
Update test 5 OK

Details

  • Phalcon version: 5.6.2
  • PHP Version: 8.2.19
  • Operating System: WSL2 Ubuntu 20
  • Installation type: installing via package manager
  • Zephir version (if any):
  • Server: Nginx However the test above is ran in cli directly
  • MySQL 8.0.26, with set sql_mode=STRICT_ALL_TABLES;

Additional context
I did some investigation to the source and notice that in Phalcon\Mvc\Model\Query , it does not catch any exception.

https://github.com/phalcon/cphalcon/blob/master/phalcon/Mvc/Model/Query.zep#L1485-L1515

I'm not familiar with zephir I've assumed that the exception is not handled properly there. It seems to leave the function instantly without calling connection->rollback when exception happens.

However if I manually called $this->db->rollback() right after catching the exception, the result is working as intended. I don't think this is a good workaround in case exception is happened elsewhere though.

Below are the (simplified) mysql general log, which also confirms my suspicion.


SELECT IF(COUNT(*) > 0, 1, 0) FROM `INFORMATION_SCHEMA`.`TABLES` WHERE `TABLE_NAME` = 'test_table' AND `TABLE_SCHEMA` = DATABASE()
DESCRIBE `test_table`
SELECT `test_table`.`id`, `test_table`.`name` FROM `test_table`
START TRANSACTION
DELETE FROM `test_table` WHERE `id` = 1
DELETE FROM `test_table` WHERE `id` = 2
DELETE FROM `test_table` WHERE `id` = 3
DELETE FROM `test_table` WHERE `id` = 4
DELETE FROM `test_table` WHERE `id` = 5
COMMIT
SELECT COUNT(*) "rowcount" FROM `test_table` WHERE `id` = 1
SELECT COUNT(*) "rowcount" FROM `test_table` WHERE `id` = 1
INSERT INTO `test_table` (`name`, `id`) VALUES ('test 1', 1)
SELECT COUNT(*) "rowcount" FROM `test_table` WHERE `id` = 2
SELECT COUNT(*) "rowcount" FROM `test_table` WHERE `id` = 2
INSERT INTO `test_table` (`name`, `id`) VALUES ('test 2', 2)
SELECT COUNT(*) "rowcount" FROM `test_table` WHERE `id` = 3
SELECT COUNT(*) "rowcount" FROM `test_table` WHERE `id` = 3
INSERT INTO `test_table` (`name`, `id`) VALUES ('test 3', 3)
SELECT COUNT(*) "rowcount" FROM `test_table` WHERE `id` = 4
SELECT COUNT(*) "rowcount" FROM `test_table` WHERE `id` = 4
INSERT INTO `test_table` (`name`, `id`) VALUES ('test 4', 4)
SELECT COUNT(*) "rowcount" FROM `test_table` WHERE `id` = 5
SELECT COUNT(*) "rowcount" FROM `test_table` WHERE `id` = 5
INSERT INTO `test_table` (`name`, `id`) VALUES ('test 5', 5)
SELECT `test_table`.`id`, `test_table`.`name` FROM `test_table` WHERE `test_table`.`id` = '1'
START TRANSACTION
UPDATE `test_table` SET `name` = 'test 1 OK' WHERE `id` = 1
COMMIT
SELECT `test_table`.`id`, `test_table`.`name` FROM `test_table` WHERE `test_table`.`id` = '2'
START TRANSACTION
UPDATE `test_table` SET `name` = 'test 2 OK' WHERE `id` = 2
COMMIT
SELECT `test_table`.`id`, `test_table`.`name` FROM `test_table` WHERE `test_table`.`id` = '3'
START TRANSACTION
UPDATE `test_table` SET `name` = 'test 3 DATA TRUNCATED ERROR' WHERE `id` = 3
SELECT `test_table`.`id`, `test_table`.`name` FROM `test_table` WHERE `test_table`.`id` = '4'
UPDATE `test_table` SET `name` = 'test 4 OK' WHERE `id` = 4
SELECT `test_table`.`id`, `test_table`.`name` FROM `test_table` WHERE `test_table`.`id` = '5'
UPDATE `test_table` SET `name` = 'test 5 OK' WHERE `id` = 5
ROLLBACK

Noticed how after 3rd row update, the was no more COMMIT because pdo adapater handles transaction level internally and it stays on level 2 until the end of script

@poz poz added bug A bug report status: unverified Unverified labels Jun 4, 2024
@poz poz changed the title [BUG]: Phalcon\Mvc\Model\Query does not handle internal transaction on PDO exception [BUG]: Phalcon\Mvc\Model\Query does not handle internal transaction on PDOException Jun 4, 2024
@poz
Copy link
Author

poz commented Jun 4, 2024

Additional findings:

This also happen to DELETE query. INSERT query are not affected because executeInsert() does not call connection->begin()

@noone-silent
Copy link
Contributor

I look into this

noone-silent added a commit to noone-silent/cphalcon that referenced this issue Jun 10, 2024
noone-silent added a commit to noone-silent/cphalcon that referenced this issue Jun 10, 2024
noone-silent added a commit to noone-silent/cphalcon that referenced this issue Jun 10, 2024
…xception-rollback-and-re-throw

# Conflicts:
#	composer.json
#	tests/cli/Cli/Console/HandleCest.php
niden added a commit that referenced this issue Jun 16, 2024
…llback-and-re-throw

[#16604] - fix: correctly handle transaction rollbacks if exceptions are thrown
@niden niden added status: medium Medium 5.0 The issues we want to solve in the 5.0 release and removed status: unverified Unverified labels Jun 16, 2024
@niden niden added this to Phalcon v5 Jun 16, 2024
@niden niden moved this to Implemented in Phalcon v5 Jun 16, 2024
@niden
Copy link
Member

niden commented Jun 16, 2024

Resolved in #16610

Thank you to @noone-silent and thank you @poz for reporting

@niden niden closed this as completed Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium
Projects
Status: Implemented
Development

No branches or pull requests

3 participants