-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix a condition that would not allow diffing of foreign keys for Sqlite #4012
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.
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
Changes from all commits
dd6aaea
43dd601
c4b463e
714f4c5
2341e1c
80aa6c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,161 @@ | ||
| <?php | ||
|
|
||
| namespace Doctrine\Tests\DBAL\Schema; | ||
|
|
||
| use Doctrine\DBAL\DBALException; | ||
| use Doctrine\DBAL\Platforms\AbstractPlatform; | ||
| use Doctrine\DBAL\Schema\ForeignKeyConstraint; | ||
| use Doctrine\DBAL\Schema\SchemaDiff; | ||
| use Doctrine\DBAL\Schema\Table; | ||
| use PHPUnit\Framework\MockObject\MockObject; | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| class SqliteSchemaDiffTest extends TestCase | ||
| { | ||
| /** | ||
| * @return AbstractPlatform|MockObject | ||
| */ | ||
| private function getGenericPlatform() | ||
| { | ||
| $platform = $this->createMock(AbstractPlatform::class); | ||
|
|
||
| $platform->expects($this->exactly(1)) | ||
| ->method('supportsSchemas') | ||
| ->will($this->returnValue(false)); | ||
|
|
||
| $platform->expects($this->exactly(1)) | ||
| ->method('supportsSequences') | ||
| ->will($this->returnValue(false)); | ||
|
|
||
| $platform->expects($this->exactly(0)) | ||
| ->method('supportsForeignKeyConstraints') | ||
| ->will($this->returnValue(true)); | ||
|
|
||
| $platform->expects($this->exactly(1)) | ||
| ->method('getCreateTableSql') | ||
| ->with($this->isInstanceOf(Table::class)) | ||
| ->will($this->returnValue(['create_table'])); | ||
|
|
||
| return $platform; | ||
| } | ||
|
|
||
| /** | ||
| * @param bool $safe Whether the method should output only safe SQL code or not | ||
| * | ||
| * @return AbstractPlatform|MockObject | ||
| */ | ||
| private function getForeignKeyConstraintsOnlyPlatform(bool $safe = true) | ||
| { | ||
| $platform = $this->getGenericPlatform(); | ||
|
|
||
| $platform->expects($this->any()) | ||
| ->method('supportsCreateDropForeignKeyConstraints') | ||
| ->will($this->returnValue(false)); | ||
|
|
||
| $platform->expects($this->exactly(0)) | ||
| ->method('getCreateForeignKeySQL') | ||
| ->with($this->isInstanceOf(ForeignKeyConstraint::class)) | ||
| ->will($this->throwException( | ||
| new DBALException('This platform does not support alter foreign key, the table must be fully recreated using getAlterTableSQL.') | ||
| )); | ||
|
|
||
| if (! $safe) { | ||
| $platform->expects($this->exactly(0)) | ||
| ->method('getDropForeignKeySql') | ||
| ->with( | ||
| $this->isInstanceOf(ForeignKeyConstraint::class), | ||
| $this->isInstanceOf(Table::class) | ||
| ) | ||
| ->will($this->throwException( | ||
| new DBALException('This platform does not support alter foreign key, the table must be fully recreated using getAlterTableSQL.') | ||
| )); | ||
| } | ||
|
|
||
| return $platform; | ||
| } | ||
|
|
||
| /** | ||
| * @param bool $safe Whether the method should output only safe SQL code or not | ||
| * | ||
| * @return AbstractPlatform|MockObject | ||
| */ | ||
| private function getCreateDropForeignKeyConstraintsPlatform(bool $safe = true) | ||
| { | ||
| $platform = $this->getGenericPlatform(); | ||
|
|
||
| $platform->expects($this->any()) | ||
| ->method('supportsCreateDropForeignKeyConstraints') | ||
| ->will($this->returnValue(true)); | ||
|
|
||
| $platform->expects($this->exactly(1)) | ||
| ->method('getCreateForeignKeySQL') | ||
| ->with($this->isInstanceOf(ForeignKeyConstraint::class)) | ||
| ->will($this->returnValue('create_foreign_key')); | ||
|
|
||
| if (! $safe) { | ||
| $platform->expects($this->exactly(1)) | ||
| ->method('getDropForeignKeySql') | ||
| ->with( | ||
| $this->isInstanceOf(ForeignKeyConstraint::class), | ||
| $this->isInstanceOf(Table::class) | ||
| ) | ||
| ->will($this->returnValue('drop_orphan_fk')); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a lot of mocking… why not use the real object? A cross-platform functional test should make all this a bit shorter.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thoughts exactly. All of your above remarks apply. I just copied the SchemaDiffTest and kept the same coding style. |
||
|
|
||
| return $platform; | ||
| } | ||
|
|
||
| /** | ||
| * @return mixed[] | ||
| */ | ||
| public function provider() : array | ||
| { | ||
| $diff = $this->createSchemaDiff(); | ||
|
|
||
| return [ | ||
| 'supportsForeignKeyConstraintsOnly' => [ | ||
| ['create_table'], | ||
| $diff->toSql($this->getForeignKeyConstraintsOnlyPlatform(false)), | ||
| ], | ||
| 'supportsForeignKeyConstraintsOnlySaveSql' => [ | ||
| ['create_table'], | ||
| $diff->toSaveSql($this->getForeignKeyConstraintsOnlyPlatform()), | ||
| ], | ||
| 'supportsCreateDropForeignKeyConstraints' => [ | ||
| ['drop_orphan_fk', 'create_table', 'create_foreign_key'], | ||
| $diff->toSql($this->getCreateDropForeignKeyConstraintsPlatform(false)), | ||
| ], | ||
| 'supportsCreateDropForeignKeyConstraintsSaveSql' => [ | ||
| ['create_table', 'create_foreign_key'], | ||
| $diff->toSaveSql($this->getCreateDropForeignKeyConstraintsPlatform()), | ||
| ], | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * @param string[] $expectedActions An array of actions to be taken on the database | ||
| * @param string[] $sql An array of actions gnerated by the SchemaDiff | ||
| * | ||
| * @dataProvider provider | ||
| */ | ||
| public function testSchemaDiff(array $expectedActions, array $sql) : void | ||
| { | ||
| self::assertEquals($expectedActions, $sql); | ||
| } | ||
|
|
||
| public function createSchemaDiff() : SchemaDiff | ||
| { | ||
| $diff = new SchemaDiff(); | ||
|
|
||
| $diff->newTables['foo_table'] = new Table('foo_table'); | ||
| $diff->newTables['foo_table']->addColumn('foreign_id', 'integer'); | ||
| $diff->newTables['foo_table']->addForeignKeyConstraint('foreign_table', ['foreign_id'], ['id']); | ||
|
|
||
| $fk = new ForeignKeyConstraint(['id'], 'foreign_table', ['id']); | ||
| $fk->setLocalTable(new Table('local_table')); | ||
|
|
||
| $diff->orphanedForeignKeys[] = $fk; | ||
|
|
||
| return $diff; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the tests specific to Sqlite or could they apply to other platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. They are testing a specific use case that I encountered with SQLite, it might apply to any platform where
supportsForeignKeyConstraints()istrueandsupportsCreateDropForeignKeyConstraints()isfalse.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the number of things mocked, I can barely understand what is being tested here. Would it make sense to have an integration test instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so. I just copied the code that was in the SchemaDiffTest. Maybe a more generic integration test could cover both cases.