DBAL-2925: Added functional test checking ability to change PK for po… #2929

SCIF · 2017-11-30T18:00:03Z

…stgres platform

simPod · 2017-12-27T17:42:17Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

@@ -3570,4 +3575,14 @@ public function getStringLiteralQuoteCharacter()
    {
        return "'";
    }
+


I think the phpdoc is redundant

Hi @simPod ,

Yeah, you are correct, it's totally useless and I will drop it soon.

PapsOu · 2018-01-12T15:09:34Z

@simPod Does this PR is ready to be merged ? I've got the same problem for PK under postgresql.

Majkl578 · 2018-01-12T15:13:35Z

@PapsOu Unfortunately nobody has simply got to review it yet.

Majkl578 · 2018-01-12T15:14:03Z

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

@@ -752,6 +752,11 @@ public function getDropForeignKeySQL($foreignKey, $table)
        return $this->getDropConstraintSQL($foreignKey, $table);
    }

+    protected function getDropPrimaryKeySQL(string $table) : string
+    {
+        return $this->getDropConstraintSQL("{$table}_pkey", $table);


Please use string concatenation here + ' instead of variable in string.

I found a couple of articles explaining that this is the way how the PK should be dropped on PostgreSQL but before seeing the explanation, this approach looks like using a "magic" suffix.

It would be nice if a link to documentation describing this approach was provided in the doc-block.

Majkl578 · 2018-01-12T15:14:34Z

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

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


Constant strings should be enclosed in '.

There should be a difference and not false

It's a very vague assertion. I'd omit it together with the previous one about the autoincrement and only focus on the end result where the new primary key is (id).

morozov · 2018-01-12T21:12:07Z

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

+    /**
+     * @group 2925
+     */
+    public function testAlterTableChangePrimaryKey() : void


Please move this test to SchemaManagerFunctionalTestCase. It should be checked and work the same for all platforms, not only for PostgreSQL.

morozov · 2018-01-12T21:14:28Z

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

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


There should be a difference and not false

It's a very vague assertion. I'd omit it together with the previous one about the autoincrement and only focus on the end result where the new primary key is (id).

morozov · 2018-01-12T21:16:40Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

@@ -2065,6 +2065,11 @@ protected function getPreAlterTableIndexForeignKeySQL(TableDiff $diff)
            $sql[] = $this->getDropIndexSQL($index, $tableName);
        }
        foreach ($diff->changedIndexes as $index) {
+            if ($index->isPrimary()) {
+                $sql[] = $this->getDropPrimaryKeySQL($tableName);


@Majkl578 is there a way to enforce a new line before flow control operators like return, continue, etc on the code sniffer level? It would be very nice to have.

No sniff I'm aware of, but shouldn't be hard to implement. Just needs to be compatible i.e. with single statements:

if (...) { continue; }

or comments:

foo(); // comment return 123;

Yeah. More generically, the first case is if the previous line has flow control statement which is a block opener, there shouldn't be an empty line. Like:

function x() { foreach (...) { foreach (...) { } } foreach (...) { } }

I've added new line before continue.

@carusogabriel do you want to file this as a requirement for coding standards (I see we have disabled issues for the repository). It'd be nice to have a reference point somewhere. Otherwise, I constantly forget where the discussion started.

/cc @deeky666

@morozov Require new lines before control structions you mean? :)

Yes. Before and after.

@morozov , Totally agree with this proposal, but code marked as still required your review :) Mark it properly please.

@SCIF it's not because of the formatting issue. I want to see if the behavior of dropping a non-existing PK on SQL Server is consistent with other platforms.

This thread is regarding cs only, the PK ones here

morozov · 2018-01-12T21:19:50Z

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

@@ -752,6 +752,11 @@ public function getDropForeignKeySQL($foreignKey, $table)
        return $this->getDropConstraintSQL($foreignKey, $table);
    }

+    protected function getDropPrimaryKeySQL(string $table) : string
+    {
+        return $this->getDropConstraintSQL("{$table}_pkey", $table);


I found a couple of articles explaining that this is the way how the PK should be dropped on PostgreSQL but before seeing the explanation, this approach looks like using a "magic" suffix.

It would be nice if a link to documentation describing this approach was provided in the doc-block.

SCIF · 2018-02-20T00:11:51Z

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

+        $this->_sm->dropAndCreateTable($tableFrom);
+
+        $tableFrom = $this->_sm->listTableDetails('primary_key_userid');
+        self::assertTrue($tableFrom->getColumn('user_id')->getAutoincrement());


I can see that this test is failed on Oracle. Should I skip this assert for Oracle or does whole test make sense for Oracle?

@SCIF if the test is about being able to change PK, why does it use/verify autoincrement? Please try to not use it and use getPrimaryKey() instead.

…stgres platform

…primary key management bug

SCIF · 2018-03-30T12:35:06Z

Hi @Majkl578 , @morozov ,

I've fixed MsSQL and MySQL issues discovered after moving test to common Schema testcase.

morozov

@SCIF thanks for the cross-platform test. As you can see, it's already bringing value :-)

Please see my comments and address the code style issues.

morozov · 2018-03-30T17:21:51Z

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php

+
+    protected function getDropPrimaryKeySQL(string $table) : string
+    {
+        return "


Could you use heredoc syntax instead?

morozov · 2018-03-30T17:26:36Z

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php

+SELECT @sql = 'ALTER TABLE ' + @table + ' DROP CONSTRAINT '  + name + ';'
+    FROM sys.key_constraints
+    WHERE [type] = 'PK'
+    AND [parent_object_id] = OBJECT_ID(@table);


Would it make sense to move the object ID outside of the @sql so that the function is not invoked for every single PK in sys.key_constraints?

How will this query behave if the table doesn't have a PK? I'd expect sp_executeSQL be invoked with an empty argument and throwing some sort of exception.

Emm. If you will try to drop PK for any other platforms on table not containing it, exception will be thrown as well, so I don't see any inconsistency here.

Is it the same type of exception?

Hmm, different one, you are right. Just syntax error vs can't drop unexisted PK. What's your proposal? I don't have windows pc and that is my very first mssql experience in life :)

You don't need a Windows PC. The SQL Server extension can be compiled for PHP on Linux, and the server itself can be running in a Docker container (microsoft/mssql-server-linux).

I don't have a proposal currently. My only concern is this code may execute knowingly syntactically invalid SQL. Instead of doing the in the dynamic SQL way, can we just rely on some conventional PK name in SQL Server (like DROP CONSTRAINT PK_$table; from here)?

Hold on. getDropPrimaryKeySQL() is not even part of the abstract platform API. Why are we using it in the abstract platform code? From what I see, most of the platforms are capable of identifying PK by name including SQL Server. Why should we take special care about the PK on SQL Server if it can be just dropped by name same as other constraints?

We can't rely on any name conventions/best practices. As far as I investigated, the name builds according to schema PK_{$tableName}_{$someHash} and can't be predicted.

morozov · 2018-03-30T17:40:14Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

@@ -2078,6 +2078,12 @@ protected function getPreAlterTableIndexForeignKeySQL(TableDiff $diff)
            $sql[] = $this->getDropIndexSQL($index, $tableName);
        }
        foreach ($diff->changedIndexes as $index) {
+            if ($index->isPrimary()) {


The Scrutinizer failure reveals this as a decent candidate for refactoring in the future. Instead of the platform knowing how to drop a given index based on its properties (primary, non-primary and so on), the index should tell the platform how to drop itself (sort of a Visitor pattern):

interface Index { function drop(Platform $platform); } interface Platform { function dropIndex(string $name); function dropPrimaryKey(string $tableName); }

This is outside of scope, just noting down.

/cc @Ocramius @deeky666

I was thinking about it but that will lead to creation of platform specific indices. Quite heavy refactoring will be required, I worry. I'm afraid I'm not able to do it in reasonable time.

Not really fan of the suggested visitor approach. Index would then need to know how to represent itself on all existing platforms and even those not provided by DBAL out-of-box.

@SCIF no need to worry about it right now. It's just for the record.

Not really fan of the suggested visitor approach. Index would then need to know how to represent itself on all existing platforms and even those not provided by DBAL out-of-box.

Not exactly. They will need to be able to identify themselves as a type while the platform will know how to handle certain types. Same as we do with column types.

morozov · 2018-03-31T01:26:18Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

+
+    protected function getDropPrimaryKeySQL(string $table) : string
+    {
+        return 'ALTER TABLE ' . $table . ' DROP PRIMARY KEY';


I see, getDropPrimaryKeySQL() is being introduced here for an abstract platform but it's only needed for MySQL and has MySQL-specific syntax. This is a no-go.

I believe, the original issue (#2925) is filed for MySQL where DROP INDEX PRIMARY ON $table is not supported. Instead of introducing this method, MySqlPlatform::getDropIndexSQL() should be updated to produce correct SQL for the PK.

The issue originally discovered during work with Postgres. Initially I tried to modify getDropIndexSQL() method but it can't be done for postgres at least, so separate method for primary keys only is the best solution.

PK is not just an index, but it's a constraint in some RDBMS. Handling could be done by changing getDropIndexSQL() signature but I believe it's not the best option.

so separate method for primary keys only is the best solution

It's fine to have it for Postgres and MySQL which seem unable to drop the PK by name but it shouldn't be part of the abstract platform API. Those platforms which can drop PK by its name shouldn't have any changes.

I really don't understand what you propose. Do you mean define getDropPrimaryKeySQL as:

protected function getDropPrimaryKeySQL(string $table) : string { $this->getDropIndexSQL('primary', $table); }

?
I can't get rid of getDropPrimaryKeySQL in favour getDropIndexSQL() because $index defined as Index|string. So, BC break required, if you would like to use only getDropIndexSQL().

Then, in the case when $index is a string, you get its object representation and see if it's primary or not.

You also can fix it only for your use case when the $index comes as an object from the schema manager. If there's another case when dropping the PK by name is required, it can be solved separately.

@morozov

Then, in the case when $index is a string, you get its object representation and see if it's primary or not.

How I can do that? I don't know a way to do it. If you mean to find a workaround for particular getAlterTableSQL() usage — yes, I can fix it in another way, but it's bloody stupid idea to find a big issue and solve only one of use-cases rather than fix it in principal. I've spent a lot of time on supporting this PR and would like to solve a root of evil, rather than just superficial solution of the problem.

How I can do that? I don't know a way to do it.

Neither do I. That's why I'm saying it's fine if you solve only the case where the passed index is an object.

I've spent a lot of time on supporting this PR and would like to solve a root of evil, rather than just superficial solution of the problem.

The root can could be only solved by reworking the entire index-related API (I recall there was a discussion or at least mention of that /cc @deeky666). This API would account for all differences between indices, constraints, primary keys, foreign keys, etc. in different platforms and would provide some unified higher level API.

It's impossible to solve it correctly in terms of the current API without introducing more controversial methods (I'm talking about getDropPrimaryKeySQL()). You can either solve your case in 2.x or postpone it until 3.x and solve properly.

Right now, Index class contain primary property so, really not necessary to be revised because reflect it's purpose pretty fine. The problem is that index can be supplied by name, which would be my proposal to drop in 3.x.
@morozov , thanks for your input. I will refactor this PR just to fix comparing issue soon.

SCIF · 2018-03-31T02:44:24Z

Travis fails on getting https://dev.mysql.com/get/mysql-apt-config_0.8.9-1_all.deb which can be downloaded for me.

Majkl578 · 2018-03-31T02:54:14Z

Travis issue reported in travis-ci/travis-ci#9421.

SCIF mentioned this pull request Nov 30, 2017

Incorrect handling changed primary key #2925

Open

SCIF force-pushed the bugfix/DBAL-2925 branch 4 times, most recently from 7d1bdee to 6333a15 Compare December 5, 2017 10:34

simPod reviewed Dec 27, 2017

View reviewed changes

simPod mentioned this pull request Dec 27, 2017

ERROR: index "primary" does not exist doctrine/orm#6271

Closed

SCIF force-pushed the bugfix/DBAL-2925 branch from 86ea186 to 6147eb5 Compare December 29, 2017 06:21

Majkl578 reviewed Jan 12, 2018

View reviewed changes

Majkl578 requested a review from morozov January 12, 2018 15:15

morozov requested changes Jan 12, 2018

View reviewed changes

SCIF force-pushed the bugfix/DBAL-2925 branch from 6147eb5 to 1a0614b Compare January 16, 2018 10:56

SCIF commented Feb 20, 2018

View reviewed changes

SCIF added 4 commits March 3, 2018 14:29

DBAL-2925: Added functional test checking ability to change PK for po…

de8a64f

…stgres platform

DBAL-2925: added dropPrimaryKeySQL() method to platform. Used to fix …

881188a

…primary key management bug

Excessive PHPDoc removed

f976fb3

Some codereview changes

ba9a0c1

SCIF force-pushed the bugfix/DBAL-2925 branch 3 times, most recently from 842c6ac to 5e886df Compare March 3, 2018 08:59

PK tests changed

c7a088b

SCIF force-pushed the bugfix/DBAL-2925 branch from 5e886df to c7a088b Compare March 3, 2018 22:51

SCIF added 2 commits March 30, 2018 11:20

Fixes of primary key management for MySQL

2e29acd

MsSQL fixes and CS fix

bb17141

SCIF force-pushed the bugfix/DBAL-2925 branch from e9b62f7 to bb17141 Compare March 30, 2018 12:15

morozov reviewed Mar 30, 2018

View reviewed changes

CS fixes

49992d5

morozov reviewed Mar 31, 2018

View reviewed changes

morozov mentioned this pull request Apr 3, 2018

DBAL-837: Cannot drop index needed in a foreign key constraint #2072

Closed

SCIF mentioned this pull request Jun 15, 2018

DBAL-2925: fixed issue changing PK for PostgreSQL10 #3184

Closed

goetas mentioned this pull request Apr 9, 2020

Detect index rename/change columns for primary keys as remove/create #3936

Closed

Base automatically changed from master to 4.0.x January 22, 2021 07:43

morozov closed this Oct 26, 2021

github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 2022

DBAL-2925: Added functional test checking ability to change PK for po… #2929

DBAL-2925: Added functional test checking ability to change PK for po… #2929

Conversation

SCIF commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PapsOu commented Jan 12, 2018

Majkl578 commented Jan 12, 2018 • edited Loading

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

morozov Jan 12, 2018 • edited Loading

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

morozov Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SCIF Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SCIF commented Mar 30, 2018

morozov 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

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

morozov Mar 31, 2018 • edited Loading

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

SCIF Apr 1, 2018 • edited by morozov Loading

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

SCIF Apr 8, 2018 • edited Loading

Choose a reason for hiding this comment

SCIF commented Mar 31, 2018

Majkl578 commented Mar 31, 2018

Majkl578 commented Jan 12, 2018 •

edited

Loading

morozov Jan 12, 2018 •

edited

Loading

morozov Mar 30, 2018 •

edited

Loading

SCIF Feb 20, 2018 •

edited

Loading

morozov Mar 31, 2018 •

edited

Loading

SCIF Apr 1, 2018 •

edited by morozov

Loading

SCIF Apr 8, 2018 •

edited

Loading