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.

Sign up for GitHub

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

Add test case for successful transactional DDL #4544

Closed
wants to merge 1 commit into from
Closed

Add test case for successful transactional DDL #4544

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/ConnectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
use Doctrine\DBAL\DriverManager;
use Doctrine\DBAL\ParameterType;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Schema\Column;
use Doctrine\DBAL\Schema\Table;
use Doctrine\DBAL\Types\Type;
use Doctrine\DBAL\Types\Types;
use Doctrine\Tests\DbalFunctionalTestCase;
use Error;
Expand Down Expand Up @@ -283,6 +286,18 @@ public function testTransactionalReturnValue(): void
self::assertEquals(42, $res);
}

public function testTransactionalDDL(): void
{
$this->connection->transactional(static function (Connection $conn): void {
$table = new Table('foo', [new Column('bar', Type::getType('string'))]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is incomplete. It performs a single atomic statement that doesn't need to be wrapped in a transaction. Please modify by adding another always failing statement (e.g. throw new Exception() after the foreach). Then define the expected behavior.

Copy link
Member Author

@ostrolucky ostrolucky Mar 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not testing failure here, but success. MySQL commits operation successfully and despite that, DBAL throws error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The successfull scenario doesn't need a transaction.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Successful test scenario needs transaction if it exists to assert no exception has been thrown during successful transaction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DDL statements are not transactional in MySQL and Oracle. No matter how much one wants them to be. If there are two DDLs in a transaction, and the second one fails, the first one will get committed no matter what API is used. I don't really understand this test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's understandable that if there are two DDLs in a transaction, and the second one fails, the first one will get commited in MySQL. And in that case nobody would blame DBAL that it cannot do a rollback. But again, this test isn't about failure but success. If all operations in transaction() block succeed, there is no reason for DBAL to throw exception.

You are saying this doesn't need to be running in transaction for MySQL... But library using DBAL doesn't know what kind of statements user passes in, it will just always use transactions, because there is no point not to. I also need to remind this is not a MySQL test. This test is being executed for all RDBMses. So it does make sense for it to be always running in transaction, because transactional DDL is supported in some of them. Similar rational applies for using this in libraries.

Now, I was going for minimal test scenario, but if you seriously need hardcoded test case where transaction looks more useful even on MySQL, I could just change the queries in transaction to do some insert and then (successful) DDL. In such case when first (and only) DDL fails, changes would be rollbacked even in MySQL, so you would have to stop suggesting using transaction there doesn't make sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But again, this test isn't about failure but success.

This is fundamentally wrong. There's no point in running a transaction if you don't care about error handling. The whole point of a transaction is error handling (well, and isolation).

If all operations in transaction() block succeed, there is no reason for DBAL to throw exception.

The method assumes that it controls the transaction flow but it fails to. Hence the exception.

foreach ($conn->getDatabasePlatform()->getCreateTableSQL($table) as $sql) {
$conn->executeStatement($sql);
}
});

self::assertTrue($this->connection->getSchemaManager()->tablesExist('foo'));
}

/**
* Tests that the quote function accepts DBAL and PDO types.
*/
Expand Down