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

fixes incorrect database migration for quoted columns #3395

Closed
wants to merge 6 commits into from
Closed

fixes incorrect database migration for quoted columns #3395

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b3096b2
fixes #3387 does not intersect columns if one is quoted and another i…
svycka Dec 7, 2018
8401951
updated tests to test agains queted
svycka Dec 7, 2018
77f349a
Extracted into separate test
svycka Dec 10, 2018
f58443d
removed improvement from not related test :)
svycka May 20, 2019
806ec1e
I still have to change this test and keeping mock makes harder to tes…
svycka May 20, 2019
6d592fd
Merge branch 'master' into patch-2
svycka Nov 4, 2019
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
4 changes: 2 additions & 2 deletions lib/Doctrine/DBAL/Schema/ForeignKeyConstraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,9 +364,9 @@ private function onEvent($event)
*/
public function intersectsIndexColumns(Index $index)
{
foreach ($index->getColumns() as $indexColumn) {
foreach ($index->getUnquotedColumns() as $indexColumn) {
foreach ($this->_localColumnNames as $localColumn) {
if (strtolower($indexColumn) === strtolower($localColumn->getName())) {
if (strtolower($indexColumn) === strtolower($localColumn->trimQuotes($localColumn->getName()))) {
Copy link
Contributor Author

@svycka svycka Dec 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure that I have to trim quotes for $localColumn->getName() this should already be unquoted?

Copy link
Member

Choose a reason for hiding this comment

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

Can't really help with that due to my unfamiliarity with this code area. @deeky666 @morozov can you check this?

Copy link
Member

Choose a reason for hiding this comment

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

I'm unfamiliar with this either but it feels bad. An unquoted column name is the value itself, a quoted column name is a SQL expression representing this value. Why would we want to mix them and compare by trimming quotes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morozov maybe I did not understand you. I need them to ignore escaping because of #3387 while debugging I noticed that for up and down migrations it compares escaped or unescaped values and generates or not gerenates migration. This place checks if the column name is included in the foreign constraint and it should not matter if it is escaped or not should say it is in both cases. I just not sure about this $localColumn->trimQuotes($localColumn->getName()).

Also I am not sure this will not break other parts but tests looks green so maybe ok?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a valid case for DBAL. Right now there's no difference between quoted and unquoted identifiers in the DBAL type system but it doesn't mean it should handle them transparently. Most likely, there's a lot of other cases where mixing quoted and unquoted identifiers will cause unexpected behavior.

Instead of changing the comparison logic in DBAL, I'd try to fix the inconsistency on the calling side.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a valid case where a database allows reserved words when they are quoted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SenseException this is the case where I spotted this with migration for this did not work becouse "`group`" is escaped with `

     * @ORM\ManyToOne(targetEntity="Group")
     * @ORM\JoinColumn(name="`group`", referencedColumnName="id")

@morozov I can find only one place where this method is called https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php#L829
so you suggest create one more method for same problem and don't use old one(witch I think is buggy)?

anyway, feel free to close this if you think this is a bad fix for #3387 and create better one. But this worked for my use case.

return true;
}
}
Expand Down
11 changes: 4 additions & 7 deletions tests/Doctrine/Tests/DBAL/Schema/ForeignKeyConstraintTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,9 @@ class ForeignKeyConstraintTest extends TestCase
*/
public function testIntersectsIndexColumns(array $indexColumns, $expectedResult)
{
$foreignKey = new ForeignKeyConstraint(['foo', 'bar'], 'foreign_table', ['fk_foo', 'fk_bar']);
$foreignKey = new ForeignKeyConstraint(['foo', '`bar`'], 'foreign_table', ['fk_foo', 'fk_bar']);
Copy link
Member

Choose a reason for hiding this comment

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

Please design a new test: don't modify existing ones.

If an existing test is being modified out of necessity, we will likely hit a BC break scenario.


$index = $this->getMockBuilder(Index::class)
->disableOriginalConstructor()
->getMock();
$index->expects($this->once())
->method('getColumns')
->will($this->returnValue($indexColumns));
$index = new Index('INDEX_NAME', $indexColumns);
Copy link
Member

@Ocramius Ocramius May 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm vary of test changes here: what was broken that required getting rid of the mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing, just thought maybe better to use an actual object instead of mock no?

Copy link
Member

Choose a reason for hiding this comment

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

If the previous code works without changes, let's avoid changing it at all. Let's put test refactoring in separate patches, to avoid hiding potential BC breaks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ocramius as you can see tests fail and now I remember why. Because I use getUnquotedColumns instead of getColumns and I added some columns with quotes into data provider. And if I mock here can't test quoted names. I mean this:

$index->expects($this->once())
            ->method('getUnquotedColumns')
            ->will($this->returnValue($indexColumns));

should always return unquoted names and I can't add quoted names in data provider but maybe I don't have to?
I still think actual implementation would be better here and anyway I have to change mocked method anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ocramius if you want I can still leave this mock but will have to change mocked method and will also have to duplicate this test and test data to actually test quoted names.


self::assertSame($expectedResult, $foreignKey->intersectsIndexColumns($index));
}
Expand All @@ -37,6 +32,8 @@ public function getIntersectsIndexColumnsData()
[['baz'], false],
[['baz', 'bloo'], false],

[['`foo`'], true],
[['`bar`'], true],
[['foo'], true],
[['bar'], true],

Expand Down