Skip to content

Commit

Permalink
fix #6198: stop considering generated column definition as being its …
Browse files Browse the repository at this point in the history
…default value

The `pg_get_expr(adbin, adrelid)` expression in Postgresql's internal table `pg_attrdef` is used to know a column definition

for most column, if this returns something, it means this column's default value. So DBAL has been considering has ALWAYS meaning it contains its default.

However for generated columns, it contains the value definition and not its default value, so we change the selectTableColumns' query to correctly set the 'default' column to `null` for generated columns

It will help setting correctly the column's attributes which in turn will help generate correct schema diff.
  • Loading branch information
allan-simon committed Nov 21, 2023
1 parent 45941c6 commit bfb754f
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 6 deletions.
5 changes: 5 additions & 0 deletions src/Driver/AbstractPostgreSQLDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use Doctrine\DBAL\Exception;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Platforms\PostgreSQL100Platform;
use Doctrine\DBAL\Platforms\PostgreSQL120Platform;
use Doctrine\DBAL\Platforms\PostgreSQL94Platform;
use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
use Doctrine\DBAL\Schema\PostgreSQLSchemaManager;
Expand Down Expand Up @@ -40,6 +41,10 @@ public function createDatabasePlatformForVersion($version)
$patchVersion = $versionParts['patch'] ?? 0;
$version = $majorVersion . '.' . $minorVersion . '.' . $patchVersion;

if (version_compare($version, '12.0', '>=')) {
return new PostgreSQL120Platform();
}

if (version_compare($version, '10.0', '>=')) {
return new PostgreSQL100Platform();
}
Expand Down
30 changes: 30 additions & 0 deletions src/Platforms/PostgreSQL120Platform.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Platforms;

/**
* Provides the behavior, features and SQL dialect of the PostgreSQL 12.0 database platform.
*/
class PostgreSQL120Platform extends PostgreSQL100Platform
{
public function getDefaultColumnValueSQLSnippet(): string
{
// in case of GENERATED ALWAYS AS ( foobar) STORED column (added in PostgreSQL 12.0)
// postgreSQL's pg_get_expr(adbin, adrelid) will return the 'foobar' part
// which is not the 'default' value of the column but its 'definition'
// so in that case we force it to NULL as DBAL will use that column only for the
// 'default' value
return <<<'SQL'
SELECT
CASE
WHEN a.attgenerated = 's' THEN NULL
ELSE pg_get_expr(adbin, adrelid)
END
FROM pg_attrdef
WHERE c.oid = pg_attrdef.adrelid
AND pg_attrdef.adnum=a.attnum
SQL;
}
}
13 changes: 13 additions & 0 deletions src/Platforms/PostgreSQLPlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -1186,6 +1186,19 @@ public function getTruncateTableSQL($tableName, $cascade = false)
return $sql;
}

/**
* Get the snippet used to retrieve the default value for a given column
*/
public function getDefaultColumnValueSQLSnippet(): string
{
return <<<'SQL'
SELECT pg_get_expr(adbin, adrelid)
FROM pg_attrdef
WHERE c.oid = pg_attrdef.adrelid
AND pg_attrdef.adnum=a.attnum
SQL;
}

/**
* {@inheritDoc}
*/
Expand Down
10 changes: 4 additions & 6 deletions src/Schema/PostgreSQLSchemaManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,9 @@ protected function selectTableColumns(string $databaseName, ?string $tableName =
$sql .= ' c.relname AS table_name, n.nspname AS schema_name,';
}

$sql .= <<<'SQL'
$fragmentDefaultValueSQL = $this->_platform->getDefaultColumnValueSQLSnippet();

Check warning on line 619 in src/Schema/PostgreSQLSchemaManager.php

View check run for this annotation

Codecov / codecov/patch

src/Schema/PostgreSQLSchemaManager.php#L619

Added line #L619 was not covered by tests

$sql .= <<<SQL

Check warning on line 621 in src/Schema/PostgreSQLSchemaManager.php

View check run for this annotation

Codecov / codecov/patch

src/Schema/PostgreSQLSchemaManager.php#L621

Added line #L621 was not covered by tests
a.attnum,
quote_ident(a.attname) AS field,
t.typname AS type,
Expand All @@ -632,11 +634,7 @@ protected function selectTableColumns(string $databaseName, ?string $tableName =
AND pg_index.indkey[0] = a.attnum
AND pg_index.indisprimary = 't'
) AS pri,
(SELECT pg_get_expr(adbin, adrelid)
FROM pg_attrdef
WHERE c.oid = pg_attrdef.adrelid
AND pg_attrdef.adnum=a.attnum
) AS default,
($fragmentDefaultValueSQL) AS default,

Check warning on line 637 in src/Schema/PostgreSQLSchemaManager.php

View check run for this annotation

Codecov / codecov/patch

src/Schema/PostgreSQLSchemaManager.php#L637

Added line #L637 was not covered by tests
(SELECT pg_description.description
FROM pg_description WHERE pg_description.objoid = c.oid AND a.attnum = pg_description.objsubid
) AS comment
Expand Down
2 changes: 2 additions & 0 deletions tests/Driver/VersionAwarePlatformDriverTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Doctrine\DBAL\Platforms\MySQL80Platform;
use Doctrine\DBAL\Platforms\MySQLPlatform;
use Doctrine\DBAL\Platforms\PostgreSQL100Platform;
use Doctrine\DBAL\Platforms\PostgreSQL120Platform;
use Doctrine\DBAL\Platforms\PostgreSQL94Platform;
use Doctrine\DBAL\VersionAwarePlatformDriver;
use Doctrine\Deprecations\PHPUnit\VerifyDeprecations;
Expand Down Expand Up @@ -109,6 +110,7 @@ public static function postgreSQLVersionProvider(): array
['9.4.0', PostgreSQL94Platform::class],
['9.4.1', PostgreSQL94Platform::class],
['10', PostgreSQL100Platform::class],
['12', PostgreSQL120Platform::class],
];
}

Expand Down
31 changes: 31 additions & 0 deletions tests/Functional/Schema/PostgreSQLSchemaManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use Doctrine\DBAL\Driver\ServerInfoAwareConnection;
use Doctrine\DBAL\Exception\DatabaseObjectNotFoundException;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Platforms\PostgreSQL120Platform;
use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
use Doctrine\DBAL\Schema\AbstractSchemaManager;
use Doctrine\DBAL\Schema\Comparator;
Expand Down Expand Up @@ -320,6 +321,36 @@ public function testBooleanDefault(callable $comparatorFactory): void
self::assertFalse($diff);
}

/**
* @param callable(AbstractSchemaManager):Comparator $comparatorFactory
*
* @dataProvider \Doctrine\DBAL\Tests\Functional\Schema\ComparatorTestUtils::comparatorProvider
*/
public function testGeneratedColumn(callable $comparatorFactory): void
{
$wrappedConnection = $this->connection->getWrappedConnection();
assert($wrappedConnection instanceof ServerInfoAwareConnection);
if (! $this->connection->getDatabasePlatform() instanceof PostgreSQL120Platform) {
self::markTestSkipped('Generated columns are not supported in Postgres 11 and earlier');
}

$table = new Table('ddc6198_generated_always_as');
$table->addColumn('id', Types::INTEGER);
$table->addColumn(
'idIsOdd',
Types::BOOLEAN,
['columnDefinition' => 'boolean GENERATED ALWAYS AS (id % 2 = 1) STORED', 'notNull' => false],
);

$this->dropAndCreateTable($table);

$databaseTable = $this->schemaManager->introspectTable($table->getName());

$diff = $comparatorFactory($this->schemaManager)->diffTable($table, $databaseTable);

self::assertFalse($diff);
}

/**
* PostgreSQL stores BINARY columns as BLOB
*/
Expand Down

0 comments on commit bfb754f

Please sign in to comment.