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

Jump to bottom

Make it possible to addSql() that is executed as a statement #1326

Open
wants to merge 1 commit into
base: 3.6.x
Choose a base branch
from
Open

Make it possible to addSql() that is executed as a statement #1326

Show file tree
Hide file tree
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/Doctrine/Migrations/AbstractMigration.php
View file
Open in desktop
Expand Up @@ -126,9 +126,10 @@ public function down(Schema $schema): void
protected function addSql(
string $sql,
array $params = [],
array $types = []
array $types = [],
bool $executeAsStatement = false
): void {
$this->plannedSql[] = new Query($sql, $params, $types);
$this->plannedSql[] = new Query($sql, $params, $types, $executeAsStatement);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think boolean flags are a code smell. Let's add a new method called addSqlStatement() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean, like a static constructor method ob Query?

Copy link
Member

Choose a reason for hiding this comment

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

No, I mean like a new method next to addSql, called addSqlStatement.

protected function addSqlStatement(
        string $sql,
        array $params = [],
        array $types = [],
): void {
    $this->plannedSql[] = new Query($sql, $params, $types, true);
}


/** @return Query[] */
Expand Down
16 changes: 12 additions & 4 deletions lib/Doctrine/Migrations/Query/Query.php
View file
Open in desktop
Expand Up @@ -21,19 +21,22 @@ final class Query
/** @var mixed[] */
private array $types;

private bool $executeAsStatement;

/**
* @param mixed[] $parameters
* @param mixed[] $types
*/
public function __construct(string $statement, array $parameters = [], array $types = [])
public function __construct(string $statement, array $parameters = [], array $types = [], bool $executeAsStatement = false)
{
if (count($types) > count($parameters)) {
throw InvalidArguments::wrongTypesArgumentCount($statement, count($parameters), count($types));
}

$this->statement = $statement;
$this->parameters = $parameters;
$this->types = $types;
$this->statement = $statement;
$this->parameters = $parameters;
$this->types = $types;
$this->executeAsStatement = $executeAsStatement;
}

public function __toString(): string
Expand All @@ -57,4 +60,9 @@ public function getTypes(): array
{
return $this->types;
}

public function getExecuteAsStatement(): bool
Copy link
Member

@greg0ire greg0ire Aug 16, 2023 •
edited

Choose a reason for hiding this comment

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

This implies that this is an SQL query, but for some reason we want to execute it as a statement. Apparently, it's the opposite: https://stackoverflow.com/questions/4735856/difference-between-a-statement-and-a-query-in-sql

I would create a parent class called Statement to model this better, and then use instanceof.

Copy link
Member

Choose a reason for hiding this comment

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

Well, the hard part being that even though queries are statements in the understanding of SQL in this SO answer, this is not entirely true for the DBAL API.
If you use executeStatement to execute a statement that returns a record set (i.e. to execute a query), some drivers will fail due to receiving a non expected record set. For instance, PDO only supports having 1 buffer at a time for record sets, so you must consume or close the result of a query before executing another query: #888 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Also, DBAL (and also the ORM) seem to use Query as the more generic name (see QueryBuilder as name)

Copy link
Member

Choose a reason for hiding this comment

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

Still, it feels weird to leak a DBAL implementation detail in the public API of this value object, or even for that value object to have knowledge about it.

According to the SO answer, queries are statements, and not the other way around. According to DBAL, queries and statements are different things (so would be best modeled as 2 classes extending a common one I suppose?).

Also, DBAL (and also the ORM) seem to use Query as the more generic name (see QueryBuilder as name)

I don't think Doctrine\ORM\Query and Doctrine\ORM\Query\AST\*Statement can really be compared if that is what you are referring to, and how the ORM treats one as more generic as the other.

As for QueryBuilder… I guess it's badly named (although it's a mistake I could have made myself before today)? Other parts of the DBAL don't seem to treat query as the more generic name (otherwise Doctrine\DBAL\Statement would not have that name). Anyway, if there is a naming/modeling mistake in other packages, I don't see a huge benefit in mimicking it for the sake of consistency. It's IMO fine for doctrine/migrations to have its own model of statements and queries, to fulfill its own purposes, regardless of whether it's using DBAL or another API.

I suggest we model the SQL standard instead of mimicking the DBAL API, that way the Query class is not modeled after a lower-level API, but against a higher-level, common abstraction that the DBAL also knows about: SQL.

{
return $this->executeAsStatement;
}
}
9 changes: 7 additions & 2 deletions lib/Doctrine/Migrations/Version/DbalExecutor.php
View file
Open in desktop
Expand Up @@ -290,8 +290,13 @@ private function executeResult(MigratorConfiguration $configuration): void
$this->outputSqlQuery($query, $configuration);

$stopwatchEvent = $this->stopwatch->start('query');
// executeQuery() must be used here because $query might return a result set, for instance REPAIR does
$this->connection->executeQuery($query->getStatement(), $query->getParameters(), $query->getTypes());

if ($query->getExecuteAsStatement()) {
$this->connection->executeStatement($query->getStatement(), $query->getParameters(), $query->getTypes());
} else {
$this->connection->executeQuery($query->getStatement(), $query->getParameters(), $query->getTypes());
}

$stopwatchEvent->stop();

if (! $configuration->getTimeAllQueries()) {
Expand Down
29 changes: 28 additions & 1 deletion tests/Doctrine/Migrations/Tests/Version/ExecutorTest.php
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@
use Doctrine\Migrations\Query\Query;
use Doctrine\Migrations\Tests\TestLogger;
use Doctrine\Migrations\Tests\Version\Fixture\EmptyTestMigration;
use Doctrine\Migrations\Tests\Version\Fixture\TestMigrationWithStatement;
use Doctrine\Migrations\Tests\Version\Fixture\VersionExecutorTestMigration;
use Doctrine\Migrations\Version\DbalExecutor;
use Doctrine\Migrations\Version\Direction;
Expand Down Expand Up @@ -94,6 +95,22 @@ public function testExecuteWithNoQueries(): void
], $this->logger->logs);
}

public function testExecuteWithStatement(): void
{
$migratorConfiguration = new MigratorConfiguration();

$migration = new TestMigrationWithStatement($this->connection, $this->logger);
$version = new Version('xx');
$plan = new MigrationPlan($version, $migration, Direction::UP);

$this->connection
->expects(self::once())
->method('executeStatement')
->with('CREATE TRIGGER', [], []);

$this->versionExecutor->execute($plan, $migratorConfiguration);
}

public function testExecuteUp(): void
{
$this->metadataStorage
Expand Down Expand Up @@ -214,6 +231,7 @@ public function testExecuteDryRun(): void
self::assertSame(Direction::UP, $result->getDirection());

yield new Query('INSERT INTO doctrine_migration_versions (version, executed_at, execution_time) VALUE (' . $result->getVersion() . ', NOW(), 0)');
yield new Query('CREATE TRIGGER something...', executeAsStatement: true);
});

$this->connection
Expand All @@ -224,6 +242,10 @@ public function testExecuteDryRun(): void
->expects(self::never())
->method('executeUpdate');

$this->connection
->expects(self::never())
->method('executeStatement');

$migratorConfiguration = (new MigratorConfiguration())
->setDryRun(true)
->setTimeAllQueries(true);
Expand All @@ -237,7 +259,7 @@ public function testExecuteDryRun(): void

$queries = $result->getSql();

self::assertCount(3, $queries);
self::assertCount(4, $queries);
self::assertSame('SELECT 1', $queries[0]->getStatement());
self::assertSame([1], $queries[0]->getParameters());
self::assertSame([3], $queries[0]->getTypes());
Expand All @@ -250,6 +272,11 @@ public function testExecuteDryRun(): void
self::assertSame([], $queries[2]->getParameters());
self::assertSame([], $queries[2]->getTypes());

self::assertSame('CREATE TRIGGER something...', $queries[3]->getStatement());
self::assertSame([], $queries[3]->getParameters());
self::assertSame([], $queries[3]->getTypes());
self::assertTrue($queries[3]->getExecuteAsStatement());

self::assertNotNull($result->getTime());
self::assertSame(State::NONE, $result->getState());
self::assertTrue($this->migration->preUpExecuted);
Expand Down
16 changes: 16 additions & 0 deletions tests/Doctrine/Migrations/Tests/Version/Fixture/TestMigrationWithStatement.php
View file
Open in desktop
@@ -0,0 +1,16 @@
<?php

declare(strict_types=1);

namespace Doctrine\Migrations\Tests\Version\Fixture;

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;

class TestMigrationWithStatement extends AbstractMigration
{
public function up(Schema $schema): void
{
$this->addSql('CREATE TRIGGER', executeAsStatement: true);
}
}