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

Require precision and scale for decimal columns #5631

Merged
merged 1 commit into from
Sep 1, 2022
Merged

Require precision and scale for decimal columns #5631

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions UPGRADE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ awareness about deprecated code.

# Upgrade to 4.0

## BC BREAK: removed default precision and scale of decimal columns.

The DBAL no longer provides default values for precision and scale of decimal columns.

## BC BREAK: a non-empty WHERE clause is not enforced in data manipulation `Connection` methods.

The `Connection::update()` and `::delete()` methods no longer enforce a non-empty WHERE clause. If modification
Expand Down
18 changes: 18 additions & 0 deletions src/Exception/ColumnPrecisionRequired.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Exception;

use Doctrine\DBAL\Exception;

/**
* @psalm-immutable
*/
final class ColumnPrecisionRequired extends Exception
{
public static function new(): self
{
return new self('Column precision is not specified');
}
}
18 changes: 18 additions & 0 deletions src/Exception/ColumnScaleRequired.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Exception;

use Doctrine\DBAL\Exception;

/**
* @psalm-immutable
*/
final class ColumnScaleRequired extends Exception
{
public static function new(): self
{
return new self('Column scale is not specified');
}
}
38 changes: 10 additions & 28 deletions src/Platforms/AbstractPlatform.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
use Doctrine\DBAL\Events;
use Doctrine\DBAL\Exception;
use Doctrine\DBAL\Exception\ColumnLengthRequired;
use Doctrine\DBAL\Exception\ColumnPrecisionRequired;
use Doctrine\DBAL\Exception\ColumnScaleRequired;
use Doctrine\DBAL\LockMode;
use Doctrine\DBAL\Platforms\Exception\NoColumnsSpecifiedForTable;
use Doctrine\DBAL\Platforms\Exception\NotSupported;
Expand All @@ -36,7 +38,6 @@
use Doctrine\DBAL\Types;
use Doctrine\DBAL\Types\Exception\TypeNotFound;
use Doctrine\DBAL\Types\Type;
use Doctrine\Deprecations\Deprecation;
use InvalidArgumentException;
use UnexpectedValueException;

Expand Down Expand Up @@ -1575,40 +1576,21 @@ public function getColumnDeclarationSQL(string $name, array $column): string
* Returns the SQL snippet that declares a floating point column of arbitrary precision.
*
* @param mixed[] $column
*
* @throws ColumnPrecisionRequired
* @throws ColumnScaleRequired
*/
public function getDecimalTypeDeclarationSQL(array $column): string
{
if (empty($column['precision'])) {
if (! isset($column['precision'])) {
Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/5637',
'Relying on the default decimal column precision is deprecated'
. ', specify the precision explicitly.',
);
}

$precision = 10;
} else {
$precision = $column['precision'];
if (! isset($column['precision'])) {
throw ColumnPrecisionRequired::new();
Copy link
Member

Choose a reason for hiding this comment

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

Is this typically going to be caught and improved with additional context elsewhere, or should we add additional context based on information contained in $column?

Copy link
Member

Choose a reason for hiding this comment

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

I got mislead by the variable name (which should probably be $columnOptions, I don't think there is actually going to be anything helpful in there. The concern still stands though: are these exceptions going to be handled at a level where the column's fully qualified name is available?

Copy link
Member Author

@morozov morozov Sep 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I understand the problem. We have the same problem in other places, e.g.

dbal/src/Platforms/AbstractPlatform.php

Lines 267 to 271 in 08630c6

protected function getVarcharTypeDeclarationSQLSnippet(?int $length): string
{
if ($length === null) {
throw ColumnLengthRequired::new($this, 'VARCHAR');
}

And it's even more acute there since the column name is not available in the scope from where the exception is thrown.

I cannot think of a simple solution at this point. I don't want to introduce a dependency on $column['name'] in the code being added just to use it as a parameter for the exception because the exception is unrelated to the name.

A proper solution might be to improve the exception hierarchy:

  1. All these three exceptions should have a common type (e.g. InvalidColumnType).
  2. The code that calls the functions that may throw such exceptions should have the column name in its scope and should catch these exceptions and throw a more user-friendly exception (e.g. InvalidColumnDefinition).

}

if (empty($column['scale'])) {
if (! isset($column['scale'])) {
Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/5637',
'Relying on the default decimal column scale is deprecated'
. ', specify the scale explicitly.',
);
}

$scale = 0;
} else {
$scale = $column['scale'];
if (! isset($column['scale'])) {
throw ColumnScaleRequired::new();
}

return 'NUMERIC(' . $precision . ', ' . $scale . ')';
return 'NUMERIC(' . $column['precision'] . ', ' . $column['scale'] . ')';
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/Schema/Comparator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ public function diffColumn(Column $column1, Column $column2): array
$changedProperties[] = 'fixed';
}
} elseif ($properties1['type'] instanceof Types\DecimalType) {
if (($properties1['precision'] ?? 10) !== ($properties2['precision'] ?? 10)) {
if ($properties1['precision'] !== $properties2['precision']) {
$changedProperties[] = 'precision';
}

Expand Down
12 changes: 10 additions & 2 deletions tests/Functional/Schema/MySQLSchemaManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,16 @@ public function testListDecimalTypeColumns(): void
$tableName = 'test_list_decimal_columns';
$table = new Table($tableName);

$table->addColumn('col', 'decimal');
$table->addColumn('col_unsigned', 'decimal', ['unsigned' => true]);
$table->addColumn('col', 'decimal', [
'precision' => 10,
'scale' => 6,
]);

$table->addColumn('col_unsigned', 'decimal', [
'precision' => 10,
'scale' => 6,
'unsigned' => true,
]);

$this->dropAndCreateTable($table);

Expand Down
6 changes: 5 additions & 1 deletion tests/Functional/Schema/PostgreSQLSchemaManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,11 @@ public function testListNegativeColumnDefaultValue(): void
$table->addColumn('col_integer', 'integer', ['default' => -1]);
$table->addColumn('col_bigint', 'bigint', ['default' => -1]);
$table->addColumn('col_float', 'float', ['default' => -1.1]);
$table->addColumn('col_decimal', 'decimal', ['default' => -1.1]);
$table->addColumn('col_decimal', 'decimal', [
'precision' => 2,
'scale' => 1,
'default' => -1.1,
]);
$table->addColumn('col_string', 'string', ['default' => '(-1)']);

$this->dropAndCreateTable($table);
Expand Down
4 changes: 2 additions & 2 deletions tests/Functional/Schema/SQLServerSchemaManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@ public function testDropColumnConstraints(): void
{
$table = new Table('sqlsrv_drop_column');
$table->addColumn('id', 'integer');
$table->addColumn('todrop', 'decimal', ['default' => 10.2]);
$table->addColumn('todrop', 'integer', ['default' => 10]);

$this->schemaManager->createTable($table);

$diff = new TableDiff(
'sqlsrv_drop_column',
[],
[],
['todrop' => new Column('todrop', Type::getType('decimal'))],
['todrop' => new Column('todrop', Type::getType('integer'))],
);

$this->schemaManager->alterTable($diff);
Expand Down
11 changes: 3 additions & 8 deletions tests/Platforms/AbstractMySQLPlatformTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,14 +688,9 @@ protected function getGeneratesAlterTableRenameIndexUsedByForeignKeySQL(): array
*/
public static function getGeneratesDecimalTypeDeclarationSQL(): iterable
{
return [
[[], 'NUMERIC(10, 0)'],
[['unsigned' => true], 'NUMERIC(10, 0) UNSIGNED'],
[['unsigned' => false], 'NUMERIC(10, 0)'],
[['precision' => 5], 'NUMERIC(5, 0)'],
[['scale' => 5], 'NUMERIC(10, 5)'],
[['precision' => 8, 'scale' => 2], 'NUMERIC(8, 2)'],
];
yield [['precision' => 10, 'scale' => 8, 'unsigned' => true], 'NUMERIC(10, 8) UNSIGNED'];

yield from parent::getGeneratesDecimalTypeDeclarationSQL();
}

/**
Expand Down
26 changes: 17 additions & 9 deletions tests/Platforms/AbstractPlatformTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
use Doctrine\Common\EventManager;
use Doctrine\DBAL\Events;
use Doctrine\DBAL\Exception;
use Doctrine\DBAL\Exception\ColumnPrecisionRequired;
use Doctrine\DBAL\Exception\ColumnScaleRequired;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Schema\Column;
use Doctrine\DBAL\Schema\ColumnDiff;
Expand Down Expand Up @@ -739,6 +741,18 @@ public function getExpectedVariableLengthBinaryTypeDeclarationSQLWithLength(): s
return 'VARBINARY(16)';
}

public function testGetDecimalTypeDeclarationSQLNoPrecision(): void
{
$this->expectException(ColumnPrecisionRequired::class);
$this->platform->getDecimalTypeDeclarationSQL(['scale' => 2]);
}

public function testGetDecimalTypeDeclarationSQLNoScale(): void
{
$this->expectException(ColumnScaleRequired::class);
$this->platform->getDecimalTypeDeclarationSQL(['precision' => 10]);
}

public function testReturnsJsonTypeDeclarationSQL(): void
{
$column = [
Expand Down Expand Up @@ -1207,17 +1221,11 @@ public function testGeneratesDecimalTypeDeclarationSQL(array $column, string $ex
self::assertSame($expectedSql, $this->platform->getDecimalTypeDeclarationSQL($column));
}

/** @return mixed[][] */
/** @return iterable<array{array<string,mixed>,string}> */
public static function getGeneratesDecimalTypeDeclarationSQL(): iterable
{
return [
[[], 'NUMERIC(10, 0)'],
[['unsigned' => true], 'NUMERIC(10, 0)'],
[['unsigned' => false], 'NUMERIC(10, 0)'],
[['precision' => 5], 'NUMERIC(5, 0)'],
[['scale' => 5], 'NUMERIC(10, 5)'],
[['precision' => 8, 'scale' => 2], 'NUMERIC(8, 2)'],
];
yield [['precision' => 10, 'scale' => 0], 'NUMERIC(10, 0)'];
yield [['precision' => 8, 'scale' => 2], 'NUMERIC(8, 2)'];
}

/**
Expand Down