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

Jump to bottom

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

Closed
wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
de8a64f
DBAL-2925: Added functional test checking ability to change PK for po…
SCIF Nov 30, 2017
881188a
DBAL-2925: added dropPrimaryKeySQL() method to platform. Used to fix …
SCIF Dec 5, 2017
f976fb3
Excessive PHPDoc removed
SCIF Dec 29, 2017
ba9a0c1
Some codereview changes
SCIF Jan 16, 2018
c7a088b
PK tests changed
SCIF Mar 2, 2018
2e29acd
Fixes of primary key management for MySQL
SCIF Mar 29, 2018
bb17141
MsSQL fixes and CS fix
SCIF Mar 30, 2018
49992d5
CS fixes
SCIF Mar 30, 2018
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
11 changes: 11 additions & 0 deletions lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2078,6 +2078,12 @@ protected function getPreAlterTableIndexForeignKeySQL(TableDiff $diff)
$sql[] = $this->getDropIndexSQL($index, $tableName);
}
foreach ($diff->changedIndexes as $index) {
if ($index->isPrimary()) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

$sql[] = $this->getDropPrimaryKeySQL($tableName);
Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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;

Copy link
Member

@morozov morozov Jan 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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 (...) {
    }
}

Copy link
Author

Choose a reason for hiding this comment

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

I've added new line before continue.

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Before and after.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@morozov morozov Mar 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Author

Choose a reason for hiding this comment

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

This thread is regarding cs only, the PK ones here


continue;
}

$sql[] = $this->getDropIndexSQL($index, $tableName);
}

Expand Down Expand Up @@ -3605,4 +3611,9 @@ protected function getLikeWildcardCharacters() : string
{
return '%_';
}

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

@morozov morozov Mar 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@SCIF SCIF Apr 1, 2018 •
edited by morozov
Loading

Choose a reason for hiding this comment

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

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().

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@SCIF SCIF Apr 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}
}
8 changes: 0 additions & 8 deletions lib/Doctrine/DBAL/Platforms/DrizzlePlatform.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,14 +442,6 @@ public function getDropIndexSQL($index, $table=null)
return 'DROP INDEX ' . $indexName . ' ON ' . $table;
}

/**
* {@inheritDoc}
*/
protected function getDropPrimaryKeySQL($table)
{
return 'ALTER TABLE ' . $table . ' DROP PRIMARY KEY';
}

/**
* {@inheritDoc}
*/
Expand Down
15 changes: 5 additions & 10 deletions lib/Doctrine/DBAL/Platforms/MySqlPlatform.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,11 @@ public function getAlterTableSQL(TableDiff $diff)
$oldColumnName = new Identifier($oldColumnName);
$columnArray = $column->toArray();
$columnArray['comment'] = $this->getColumnComment($column);

if ($column->getAutoincrement()) {
$queryParts[] = 'ADD INDEX (' . $column->getName() . ')';
}

$queryParts[] = 'CHANGE ' . $oldColumnName->getQuotedName($this) . ' '
. $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnArray);
}
Expand Down Expand Up @@ -975,16 +980,6 @@ public function getDropIndexSQL($index, $table=null)
return 'DROP INDEX ' . $indexName . ' ON ' . $table;
}

/**
* @param string $table
*
* @return string
*/
protected function getDropPrimaryKeySQL($table)
{
return 'ALTER TABLE ' . $table . ' DROP PRIMARY KEY';
}

/**
* {@inheritDoc}
*/
Expand Down
5 changes: 5 additions & 0 deletions lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
* {@inheritDoc}
*/
Expand Down
16 changes: 16 additions & 0 deletions lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1619,4 +1619,20 @@ private function generateIdentifierName($identifier)

return strtoupper(dechex(crc32($identifier->getName())));
}

protected function getDropPrimaryKeySQL(string $table) : string
{
return <<<EOT
DECLARE @table NVARCHAR(512), @sql NVARCHAR(MAX), @tableid NVARCHAR(512);
SELECT @table = N'{$table}';
SELECT @tableid = OBJECT_ID(@table);
SELECT @sql = 'ALTER TABLE ' + @table + ' DROP CONSTRAINT ' + name + ';'
FROM sys.key_constraints
WHERE [type] = 'PK'
AND [parent_object_id] = @tableid;

EXEC sp_executeSQL @sql;
EOT;
}

}
26 changes: 26 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1394,4 +1394,30 @@ public function testCreateAndListSequences() : void
self::assertEquals($sequence2AllocationSize, $actualSequence2->getAllocationSize());
self::assertEquals($sequence2InitialValue, $actualSequence2->getInitialValue());
}

/**
* @group 2925
*/
public function testAlterTableChangePrimaryKey() : void
{
$tableFrom = new Table('primary_key_id');
$column = $tableFrom->addColumn('user_id', 'integer');
$column->setAutoincrement(true);
$tableFrom->setPrimaryKey(['user_id']);
$this->_sm->dropAndCreateTable($tableFrom);

$tableFrom = $this->_sm->listTableDetails('primary_key_id');
self::assertEquals(['USER_ID'], \array_map('strtoupper', $tableFrom->getPrimaryKey()->getColumns()));

$tableTo = new Table('primary_key_id');
$column = $tableTo->addColumn('id', 'integer');
$column->setAutoincrement(true);
$tableTo->setPrimaryKey(['id']);

$c = new Comparator();
$diff = $c->diffTable($tableFrom, $tableTo);
$this->_sm->alterTable($diff);
$tableFinal = $this->_sm->listTableDetails('primary_key_id');
self::assertEquals(['ID'], \array_map('strtoupper', $tableFinal->getPrimaryKey()->getColumns()));
}
}