Fix changing column from int to bigint with autoincrement #2916

icewind1991 · 2017-11-15T15:53:11Z

Currently it tries to alter the column type to BIGSERIAL which is not a valid type to be used in an ALTER TABLE query.

Also it currently drops the default which breaks the auto increment.

cc @nickvergessen

lcobucci

Please rebase your branch to sync to the latest changes of master

lcobucci · 2017-11-19T12:54:08Z

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

                $sql[] = 'ALTER TABLE ' . $diff->getName($this)->getQuotedName($this) . ' ' . $query;
            }

-            if ($columnDiff->hasChanged('default') || $columnDiff->hasChanged('type')) {
+            if ($columnDiff->fromColumn) {
+                $oldTypeIsNumeric = $columnDiff->fromColumn->getType()->getName() === Type::INTEGER || $columnDiff->fromColumn->getType()->getName() === Type::BIGINT;


Please don't rely on Type::getName(), use instance of instead.

lcobucci · 2017-11-19T12:54:13Z

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

-            if ($columnDiff->hasChanged('default') || $columnDiff->hasChanged('type')) {
+            if ($columnDiff->fromColumn) {
+                $oldTypeIsNumeric = $columnDiff->fromColumn->getType()->getName() === Type::INTEGER || $columnDiff->fromColumn->getType()->getName() === Type::BIGINT;
+                $newTypeIsNumeric = $column->getType()->getName() === Type::INTEGER || $column->getType()->getName() === Type::BIGINT;


Please don't rely on Type::getName(), use instance of instead.

lcobucci · 2017-11-19T12:55:45Z

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

@@ -419,6 +419,29 @@ public function testListNegativeColumnDefaultValue()
        self::assertEquals(-1.1, $columns['col_decimal']->getDefault());
        self::assertEquals('(-1)', $columns['col_string']->getDefault());
    }
+
+    public function testAlterTableAutoIncrementBigInt()


: void missing

Please add @group 2916 to a docblock here

lcobucci · 2017-11-19T12:55:53Z

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

+
+    public function testAlterTableAutoIncrementBigInt()
+    {
+        $tableFrom = new \Doctrine\DBAL\Schema\Table('autoinc_table_bitgint');


Please simplify the FQCN

lcobucci · 2017-11-19T12:56:00Z

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

+        $tableFrom = $this->_sm->listTableDetails('autoinc_table_bitgint');
+        self::assertTrue($tableFrom->getColumn('id')->getAutoincrement());
+
+        $tableTo = new \Doctrine\DBAL\Schema\Table('autoinc_table_bitgint');


Please simplify the FQCN

lcobucci · 2017-11-19T12:56:04Z

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

+        $column = $tableTo->addColumn('id', 'bigint');
+        $column->setAutoincrement(true);
+
+        $c = new \Doctrine\DBAL\Schema\Comparator();


Please simplify the FQCN

lcobucci · 2017-11-19T12:56:19Z

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

+        $c = new \Doctrine\DBAL\Schema\Comparator();
+        $diff = $c->diffTable($tableFrom, $tableTo);
+        self::assertInstanceOf('Doctrine\DBAL\Schema\TableDiff', $diff, "There should be a difference and not false being returned from the table comparison");
+        self::assertEquals(array("ALTER TABLE autoinc_table_bitgint ALTER id TYPE BIGINT"), $this->_conn->getDatabasePlatform()->getAlterTableSQL($diff));


assertSame() is more appropriated here

Please also use short-array syntax

icewind1991 · 2017-11-20T13:12:07Z

@lcobucci all done

lcobucci

Nice job, some more nitpicks 😜

~~Just to understand, is this needed only for PGSQL 10+ compatibility? I mean, I didn't see any issue reporting this before.~~
~~This question is just to categorise this PR correctly.~~

I've already checked, this operation breaks badly on PGSQL 9.x too

lcobucci · 2017-11-20T13:37:45Z

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

+
+        $c = new Comparator();
+        $diff = $c->diffTable($tableFrom, $tableTo);
+        self::assertInstanceOf('Doctrine\DBAL\Schema\TableDiff', $diff, "There should be a difference and not false being returned from the table comparison");


Please use TableDiff::class instead

lcobucci · 2017-11-20T13:43:05Z

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

                $sql[] = 'ALTER TABLE ' . $diff->getName($this)->getQuotedName($this) . ' ' . $query;
            }

-            if ($columnDiff->hasChanged('default') || $columnDiff->hasChanged('type')) {
+            if ($columnDiff->fromColumn) {


What do you think about extracting this if block to a private method? I think that something like this would improve the readability a bit:

private function hasDefaultTypeChanged(ColumnDiff $columnDiff, Column $newColumn) : bool { $defaultTypeChanged = $columnDiff->hasChanged('type'); if ( ! $columnDiff->fromColumn || ! $defaultTypeChanged) { return $defaultTypeChanged; } $oldTypeIsNumeric = $this->isNumericType($columnDiff->fromColumn->getType()); $newTypeIsNumeric = $this->isNumericType($newColumn->getType()); return ! ($oldTypeIsNumeric && $newTypeIsNumeric && $newColumn->getDefault() === null) }

We might improve things there (specially naming wise), but I think it makes things easier and more understandable 😄

lcobucci · 2017-11-20T13:50:45Z

One last request, could add some info on the commit msg on why this is needed? BTW thanks for ensuring that the commit history is organised =)

lcobucci · 2017-11-20T13:56:07Z

Ahhh I forgot one more thing, can you please add a test for the reverse operation too? bigint -> int

icewind1991 · 2017-11-20T15:23:04Z

done

- SERIAL and BIGSERIAL are not true types and can't be used in ALTER TABLE expressions - When changing between int and bigint auto increment fields the default should no be dropped to preserve the link between the column and the sequence.

Backporting: #2916 (review)

lcobucci · 2017-11-20T21:59:13Z

@icewind1991 🚢 and backported to 2.6.x (with some minor improvements). Thanks for your contribution!

icewind1991 · 2017-11-21T15:06:01Z

Any chance of getting things backported to 2.5.x? We're stuck with supporting php 5.6/7.0 for the foreseeable future

This was referenced Nov 17, 2017

add postgresql 10 compatibility to dbal nextcloud/3rdparty#70

Merged

add postgresql 10 compatibility to dbal nextcloud/server#7210

Merged

lcobucci suggested changes Nov 19, 2017

View reviewed changes

lcobucci suggested changes Nov 20, 2017

View reviewed changes

lcobucci added Bug PostgreSQL Schema Comparison labels Nov 20, 2017

lcobucci added this to the 2.6.4 milestone Nov 20, 2017

lcobucci approved these changes Nov 20, 2017

View reviewed changes

lcobucci merged commit 0e8ad2c into doctrine:master Nov 20, 2017

lcobucci added a commit that referenced this pull request Nov 20, 2017

Merge branch 'postgres-int-to-bigint' into 2.6

8302030

Backporting: #2916 (review)

icewind1991 deleted the postgres-int-to-bigint branch November 21, 2017 13:43

VicDeo mentioned this pull request Apr 14, 2018

Bump doctrine/dbal from 2.6.3 to 2.7.1 owncloud/core#31059

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix changing column from int to bigint with autoincrement #2916

Fix changing column from int to bigint with autoincrement #2916

icewind1991 commented Nov 15, 2017

lcobucci left a comment

lcobucci Nov 19, 2017

lcobucci Nov 19, 2017

lcobucci Nov 19, 2017

lcobucci Nov 19, 2017

lcobucci Nov 19, 2017

lcobucci Nov 19, 2017

lcobucci Nov 19, 2017

lcobucci Nov 19, 2017

lcobucci Nov 19, 2017

icewind1991 commented Nov 20, 2017

lcobucci left a comment •

edited

Loading

lcobucci Nov 20, 2017

lcobucci Nov 20, 2017 •

edited

Loading

lcobucci commented Nov 20, 2017

lcobucci commented Nov 20, 2017

icewind1991 commented Nov 20, 2017

lcobucci commented Nov 20, 2017

icewind1991 commented Nov 21, 2017

Fix changing column from int to bigint with autoincrement #2916

Fix changing column from int to bigint with autoincrement #2916

Conversation

icewind1991 commented Nov 15, 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

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

icewind1991 commented Nov 20, 2017

lcobucci left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

lcobucci commented Nov 20, 2017

lcobucci commented Nov 20, 2017

icewind1991 commented Nov 20, 2017

lcobucci commented Nov 20, 2017

icewind1991 commented Nov 21, 2017

lcobucci left a comment •

edited

Loading

lcobucci Nov 20, 2017 •

edited

Loading