Possible [Bug] when handling transactions

[dijitaltrix]

I've just started with Atlas so forgive me if I'm missing something but I have a problem with transactions, I don't think they are working as intended.

I have a simple example, an Invoice with many child Items which I want to save inside a transaction. This is happening when I'm creating dummy invoices in a loop, the data is generated with Faker, the first create fails as expected, no items and no invoice but the subsequent invoices are created when the invoice items are not.

$invoice = $invoice_repository->create($data);

Here is some example input:

Array
(
    [customer_id] => 20
    [date] => 2017-12-23
    [reference] => 47213
    [items] => Array
        (
            [0] => Array
                (
                    [description] => Non ex officiis et incidunt dolore repellendus a.
                    [quantity] => 5
                    [price] => 1808.49
                )

            [1] => Array
                (
                    [description] => Occaecati dolorem ratione suscipit eum minima porro dolore quidem.
                    [quantity] => 9
                    [price] => 1380.57
                )

            [2] => Array
                (
                    [description] => Non animi numquam quae maiores.
                    [quantity] => 3
                    [price] => 959.32
                )
        )
)

This data is passed to the create function in InvoiceRepository.php, this code works fine but may not be optimal as I couldn't find a way to get persist() to save the invoice items directly from the array, they had to be converted to a recordSet:

// InvoiceRepository.php

    public function create(Array $data=[])
    {
        // begin transaction
        $transaction = $this->atlas->newTransaction();
        
        // create invoice from data, stripping out related child ['items'] from $data
        $invoice = $this->atlas->newRecord(InvoiceMapper::class, array_diff_key($data, ['items'=>0]));

        // create invoice items as record set
        $invoice->items = $this->atlas->newRecordSet(ItemMapper::class);
    
        // append item children from $data['items'] to $invoice->items
        foreach ($data['items'] as $item_data) {
            $invoice->items[] = $this->atlas->newRecord(ItemMapper::class, $item_data);
        }

        // store invoice and items
        $transaction->persist($invoice);
        
        // execute transaction and handle success/failure
        if ($transaction->exec()) {
            
            echo "OK";
        
        } else {

            // get the exception that was thrown in the transaction
            $e = $transaction->getException();

            // get the work element that threw the exception
            $work = $transaction->getFailure();

            // some output
            echo "The Transaction failed: ";
            echo $work->getLabel() . ' threw ' . $e->getMessage();
            
        }
        
    }

However once I added an event which threw an Exception I noticed the 'invoice' was created even though the 'invoice items' weren't created and the Exception was raised showing the "The Transaction failed" message.

// ItemMapperEvents.php

    public function beforeInsert(MapperInterface $mapper, RecordInterface $record)
    {
        throw new Exception("");
    }

Unless I have the code in the InvoiceRepository create function wrong I'd expect both the 'invoice' and 'invoice items' not to be created.

Here's my versions:

atlas/cli 1.0.1
atlas/orm 2.1.0
aura/cli 2.2.0
aura/sql 2.5.1
aura/sqlquery 2.7.1
aura/sqlschema 2.0.3 

[pmjones]

Hi @dijitaltrix --

I am unable to reproduce the error you describe. I tried doing so in the issue-86 branch, via this commit: 0c7cd7e

Take a look at that test, and let me know how to make it match your case more closely.

[dijitaltrix]

Hi @pmjones,

Thanks for taking a look, it may be the issue is with MariaDB or the PDO driver.

I've run the tests from Issue-86 and can confirm they test ok against an sqlite memory database, I've not modified the test you supplied as the issue only seems to occur using MySQL and though I modified the phpunit tests in orm/tests they threw a few errors due to backticks and I wasn't confident of the results.

I've created a repository here https://github.com/dijitaltrix/Issue-86 with the minimum code required to show the issue. It follows the examples in the docs and the code in Issue86 very closely.

You'll see two files mysql_create.php and sqlite_create.php they are identical apart from the new AtlasContainer() statement.

The Sqlite code runs as excepted, it creates the parent and relations as it should, and when an Exception is thrown no records are created.

With MySQL the same code gives this error:

The Transaction failed:
persist Test\Parent\ParentRecord via Test\Parent\ParentMapper threw exception Expected 1 row affected, actual 0.

[sunkan]

Hi @dijitaltrix

I looked at your tests and the error is because you are trying to use time() on an mysql date type.

And that don't work so I change to date('Y-m-d') in you test data after that I get the correct exception.

A better question might be why PDO don't report the error from mysql

[dijitaltrix]

Hi @sunkan
Thanks for taking a look, I should have spotted that! As you say the example does run fine when given the correct data! I have updated the example as I'm still seeing the issue, please would you check again.

[dijitaltrix]

Hi @pmjones
I've updated my example, now it's looping through a data array and creating records off that. The issue only occurs if a subsequent transaction fails.

There's four test files now.. I assumed the issue was only occurring in MySQL but it also occurs in Sqlite, they're almost identical as before so take your pick!

I think the issue may be to do with the response from an Aura SQL call $this->pdo->inTransaction() it seems to return 1 on the first transaction but not on subsequent transactions.

[sunkan]

The problem is in Atlas\Orm\Table\ConnectionManager::getConnection

It checks if the connection already exists and just returns it.

It should also check inTransaction is true like this

if (isset($this->conn[$type][$tableClass])) {
    if ($this->inTransaction() && ! $this->conn[$type][$tableClass]->inTransaction()) {
        $this->conn[$type][$tableClass]->beginTransaction();
    }
    return $this->conn[$type][$tableClass];
}

I will put together a pull request in a an hour need to do some actual work now :)

@pmjones

[pmjones]

@sunkan I look forward to it, and thanks!

[dijitaltrix]

@sunkan commit #87 has fixed the issue for me, thank you!

[pmjones]

@dijitaltrix I have merged the fix from @sunkan into the 2.x branch, with a refactoring of the logic therein. Try it out and let me know if it still works for you; if it does, I'll make a release.

Thanks, everyone, for your work here.

[dijitaltrix]

@pmjones, @sunkan Thanks, I've tested the fix with my example code and it's working as expected, I assumed it was an issue with MySQL hence I created the example and didn't modify your unit tests. I'll write a test and create a pull request.

[pmjones]

Closing this as "bug has been fixed" and will wait for separate PR from @dijitaltrix for a test. Thanks everyone!