Don't Set a Default Value on PostgreSQL SERIAL Fields #2907

chrisguitarguy · 2017-11-07T16:52:14Z

Specifically when 'notnull' => false is in the column options.

Right now DBAL adds a DEFAULT NULL when that happens. [BIG]SERIAL fields already have a default value and adding the DEFAULT NULL produces an error.

Fixes #2906

Not sure if the functional tests here are necessary, but it seemed like a good idea.

chrisguitarguy · 2017-11-09T14:03:25Z

The scrutenizer failure here seems unrelated, just picked up because I changed a file with a pre-existing issue: https://scrutinizer-ci.com/g/doctrine/dbal/inspections/6801d978-162b-436c-a2e2-c1315ea461c5/issues/files/tests/Doctrine/Tests/DBAL/Functional/Schema/PostgreSqlSchemaManagerTest.php?status=new&orderField=path&order=asc&honorSelectedPaths=0

lcobucci

Nice job, great commit organisation too.

I got some suggestions, questions, and nitpicking - obviously.

Can you please make sure that the code you're adding follows our standards? phpcs is already configured - with phpcbf - and can be helpful (I've tried to make it mandatory and simplify the lives of new contributors on #2849 but @Ocramius said NOPE 😂).

lcobucci · 2017-11-12T10:26:59Z

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

+     */
+    public function getDefaultValueDeclarationSQL($field)
+    {
+        if (self::isSerialField($field) && empty($field['notnull'])) {


This condition will result to true when $field['notnull'] doesn't exist, is this the expected behaviour? I'd suggest to be more explicit about that, like $field['notnull'] ?? false === false.

I know that we use empty() on the parent method, but we didn't have these features back then.

Is there any reason to leave the empty($field['notnull']) out of the method?

Also do we have unit tests covering all the paths?

Is there any reason to leave the empty($field['notnull']) out of the method?

Yep. nonnull doesn't have anything to do with being a SERIAL field. This is perfectly okay: whatever SERIAL NOT NULL. So if someone wants to make sure the field is notnull they are free to do so. so it make more sense to me to check for a serial field as well as check notnull in one condition here. Could move it all into a isSerialFieldAndNotNull method, though.

This condition will result to true when $field['notnull'] doesn't exist, is this the expected behaviour?

$field['notnull'] missing is the same as $field['notnull'] = false yeah? I think you're right though, should be more explicit.

Also do we have unit tests covering all the paths?

I need to add some for whatever SERIAL NOT NULL now that I think about it.

Okay, coming back to this notnull thing. I'm not actually it sure it should get checked at all. Any default value for a serial field is invalid -- they already have defaults built in. Maybe getDefaultValueDeclarationSQL should just return its empty string for any SERIAL field?

lcobucci · 2017-11-12T10:33:31Z

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

+        return parent::getDefaultValueDeclarationSQL($field);
+    }
+
+    private static function isSerialField($field)


Missing type declarations (parameters and return)

Also, any reason to make this static?

Missing type declarations (parameters and return)

Left out the parameter type declaration because the method from which this is called doesn't have one either (getDefaultValueDeclarationSQL). $field theoretically should be array? Happy to add the type declaration for $field, but that was the reason I didn't.

Definitely should have the return type!

Also, any reason to make this static?

Habit of my, sorry. I tend to make "pure" utility functions static -- since nothing from the platform's instance state is required, I made it static. Want me to make it non-static?

I got your point. I don't see much difference in this case, since the caller is already in the scope of the instance. I'd keep it non-static here but was just curious 😄

lcobucci · 2017-11-12T10:34:25Z

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

+
+    private static function isSerialField($field)
+    {
+        return !empty($field['autoincrement']) && isset($field['type']) && (


Same comments about being explicit here (with empty())

lcobucci · 2017-11-12T10:34:57Z

tests/Doctrine/Tests/DBAL/Functional/Schema/PostgreSqlSchemaManagerTest.php

+
+    /**
+     * @dataProvider serialTypes
+     * @group https://github.com/doctrine/dbal/issues/2906


Please use only 2906 as group

lcobucci · 2017-11-12T10:35:07Z

tests/Doctrine/Tests/DBAL/Platforms/AbstractPostgreSqlPlatformTestCase.php

+
+    /**
+     * @dataProvider serialTypes
+     * @group https://github.com/doctrine/dbal/issues/2906


Please use only 2906 as group

lcobucci · 2017-11-12T10:43:48Z

tests/Doctrine/Tests/DBAL/Functional/Schema/PostgreSqlSchemaManagerTest.php

+     * @dataProvider serialTypes
+     * @group https://github.com/doctrine/dbal/issues/2906
+     */
+    public function testAutoIncrementCreatesSerialDataTypesWithoutADefaultValueWheNotNullIsFalse($type)


public function testAutoIncrementCreatesSerialDataTypesWithoutADefaultValueWheNotNullIsFalse(string $type) : void

lcobucci · 2017-11-12T10:44:35Z

tests/Doctrine/Tests/DBAL/Functional/Schema/PostgreSqlSchemaManagerTest.php

+
+        $columns = $this->_sm->listTableColumns($tableName);
+
+        $this->assertNull($columns['id']->getDefault());


Please use self::assert* instead

lcobucci · 2017-11-12T10:45:00Z

tests/Doctrine/Tests/DBAL/Platforms/AbstractPostgreSqlPlatformTestCase.php

@@ -144,6 +144,30 @@ public function testGenerateTableWithAutoincrement()
        self::assertEquals(array('CREATE TABLE autoinc_table (id SERIAL NOT NULL)'), $this->_platform->getCreateTableSQL($table));
    }

+    public static function serialTypes()


Add : array as return type

lcobucci · 2017-11-12T10:45:26Z

tests/Doctrine/Tests/DBAL/Platforms/AbstractPostgreSqlPlatformTestCase.php

+     * @dataProvider serialTypes
+     * @group https://github.com/doctrine/dbal/issues/2906
+     */
+    public function testGenerateTableWithAutoincrementAndNotNullFalseDoesNotSetADefaultValueOnSerialFields($type, $definition)


Please add type declarations here

lcobucci · 2017-11-12T10:45:46Z

tests/Doctrine/Tests/DBAL/Platforms/AbstractPostgreSqlPlatformTestCase.php

+
+        $sql = $this->_platform->getCreateTableSQL($table);
+
+        self::assertEquals(array("CREATE TABLE autoinc_table_notnull (id $definition)"), $sql);


Please use short-array notation

chrisguitarguy · 2017-11-12T17:41:14Z

phpcs is already configured - with phpcbf - and can be helpful

I did the best I could with this. Found some violations in the tests with aligned equals signs. Running either tool on the whole codebase produces a lot of violations, even running it on the files I edited were 40 or 50 fixes per file.

I'm not actually it sure it should get checked at all. Any default value for a serial field is invalid -- they already have defaults built in.

I did make the change so notnull is no longer checked. I'm not sure if serial fields should error if a default is set? Or just silently ignore it as it does with the latest changes. Doesn't look like the default value SQL stuff is allowed to throw based on its docblocks.

morozov · 2017-11-12T17:56:16Z

Time for a shameless self-promotion:

pull-request doctrine dbal 2907 --standard=vendor/doctrine/coding-standard/lib/Doctrine

FILE: tests/Doctrine/Tests/DBAL/Platforms/AbstractPostgreSqlPlatformTestCase.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 159 | WARNING | Line exceeds 120 characters; contains 147 characters
 175 | WARNING | Line exceeds 120 characters; contains 149 characters
--------------------------------------------------------------------------------

@chrisguitarguy, see morozov/diff-sniffer-pull-request and morozov/diff-sniffer-pre-commit.

lcobucci

Great job @chrisguitarguy, just one more thing (apart from what @morozov said).

lcobucci · 2017-11-14T10:33:58Z

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

+        return parent::getDefaultValueDeclarationSQL($field);
+    }
+
+    private static function isSerialField($field) : bool


Regarding the type, we just don't have the type declaration on getDefaultValueDeclarationSQL for historical reasons. Let's add it here too

chrisguitarguy · 2017-11-17T14:50:13Z

Thanks, @morozov, those tools are great!

Updated with the latest requested changes and fixed the PHP CS warnings, @lcobucci.

Postgres has a `SERIAL` data type to allow shorthand for `nextval('sequence_name')`, but DDL like this: CREATE TABLE example (id SERIAL) Cannot be generated in the DBAL schema APIs.

Serial fields already have a default value. By opting out of `notnull` serial fields will just set the next value in the sequence.

Backporting doctrine#2907

lcobucci · 2017-11-19T00:17:54Z

@chrisguitarguy merged and backported to 2.6, thanks for your contribution!

chrisguitarguy · 2017-11-19T03:28:54Z

My pleasure, thanks for the support @lcobucci!

lcobucci added this to the 2.7 milestone Nov 12, 2017

lcobucci suggested changes Nov 12, 2017

View reviewed changes

lcobucci suggested changes Nov 14, 2017

View reviewed changes

chrisguitarguy added 2 commits November 19, 2017 00:59

Add Tests Cases to Demonstrate doctrine#2906

186188b

Postgres has a `SERIAL` data type to allow shorthand for `nextval('sequence_name')`, but DDL like this: CREATE TABLE example (id SERIAL) Cannot be generated in the DBAL schema APIs.

Don't Set a Default Value on Serial Fields When notnull is Set

15c2aee

Serial fields already have a default value. By opting out of `notnull` serial fields will just set the next value in the sequence.

lcobucci force-pushed the pg_serial_defaults branch from 0a19f37 to 15c2aee Compare November 19, 2017 00:00

lcobucci modified the milestones: 2.7, 2.6.3 Nov 19, 2017

lcobucci added Bug Platforms PostgreSQL Type Mapping labels Nov 19, 2017

lcobucci approved these changes Nov 19, 2017

View reviewed changes

lcobucci merged commit 51bf0ae into doctrine:master Nov 19, 2017

lcobucci self-assigned this Nov 19, 2017

lcobucci added a commit to lcobucci/dbal that referenced this pull request Nov 19, 2017

Merge branch 'pg_serial_defaults' into 2.6

fe82415

Backporting doctrine#2907

chrisguitarguy deleted the pg_serial_defaults branch August 4, 2022 15:54

github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't Set a Default Value on PostgreSQL SERIAL Fields #2907

Don't Set a Default Value on PostgreSQL SERIAL Fields #2907

chrisguitarguy commented Nov 7, 2017

chrisguitarguy commented Nov 9, 2017

lcobucci left a comment

lcobucci Nov 12, 2017

lcobucci Nov 12, 2017

lcobucci Nov 12, 2017

lcobucci Nov 12, 2017

chrisguitarguy Nov 12, 2017 •

edited

chrisguitarguy Nov 12, 2017 •

edited

lcobucci Nov 12, 2017

lcobucci Nov 12, 2017

chrisguitarguy Nov 12, 2017

lcobucci Nov 14, 2017

lcobucci Nov 12, 2017

lcobucci Nov 12, 2017

lcobucci Nov 12, 2017

lcobucci Nov 12, 2017

lcobucci Nov 12, 2017

lcobucci Nov 12, 2017

lcobucci Nov 12, 2017

lcobucci Nov 12, 2017

chrisguitarguy commented Nov 12, 2017 •

edited

morozov commented Nov 12, 2017

lcobucci left a comment

lcobucci Nov 14, 2017

chrisguitarguy commented Nov 17, 2017

lcobucci commented Nov 19, 2017

chrisguitarguy commented Nov 19, 2017


		$columns = $this->_sm->listTableColumns($tableName);

		$this->assertNull($columns['id']->getDefault());


		$sql = $this->_platform->getCreateTableSQL($table);

		self::assertEquals(array("CREATE TABLE autoinc_table_notnull (id $definition)"), $sql);

Don't Set a Default Value on PostgreSQL SERIAL Fields #2907

Don't Set a Default Value on PostgreSQL SERIAL Fields #2907

Conversation

chrisguitarguy commented Nov 7, 2017

chrisguitarguy commented Nov 9, 2017

lcobucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisguitarguy Nov 12, 2017 • edited

Choose a reason for hiding this comment

chrisguitarguy Nov 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisguitarguy commented Nov 12, 2017 • edited

morozov commented Nov 12, 2017

lcobucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisguitarguy commented Nov 17, 2017

lcobucci commented Nov 19, 2017

chrisguitarguy commented Nov 19, 2017

chrisguitarguy Nov 12, 2017 •

edited

chrisguitarguy Nov 12, 2017 •

edited

chrisguitarguy commented Nov 12, 2017 •

edited