fixes incorrect database migration for quoted columns #3395

svycka · 2018-12-07T07:33:10Z

Q	A
Type	bug
BC Break	no
Fixed issues	#3387

Summary

I think this should accept as the same column if one is quoted and other is not

…nother is not I am not sure this is correct just testing if other tests will not fail

svycka · 2018-12-07T08:15:17Z

lib/Doctrine/DBAL/Schema/ForeignKeyConstraint.php

            foreach ($this->_localColumnNames as $localColumn) {
-                if (strtolower($indexColumn) === strtolower($localColumn->getName())) {
+                if (strtolower($indexColumn) === strtolower($localColumn->trimQuotes($localColumn->getName()))) {


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

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

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?

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

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.

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

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

Ocramius · 2018-12-07T11:18:29Z

tests/Doctrine/Tests/DBAL/Schema/ForeignKeyConstraintTest.php

@@ -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']);


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.

Ocramius · 2018-12-07T11:19:04Z

lib/Doctrine/DBAL/Schema/ForeignKeyConstraint.php

            foreach ($this->_localColumnNames as $localColumn) {
-                if (strtolower($indexColumn) === strtolower($localColumn->getName())) {
+                if (strtolower($indexColumn) === strtolower($localColumn->trimQuotes($localColumn->getName()))) {


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

svycka · 2018-12-10T07:55:55Z

@deeky666 would be nice if you could review as this is your code :) implemented with #756

morozov · 2018-12-10T16:52:38Z

lib/Doctrine/DBAL/Schema/ForeignKeyConstraint.php

            foreach ($this->_localColumnNames as $localColumn) {
-                if (strtolower($indexColumn) === strtolower($localColumn->getName())) {
+                if (strtolower($indexColumn) === strtolower($localColumn->trimQuotes($localColumn->getName()))) {


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.

svycka · 2019-05-17T07:19:59Z

@Ocramius I have created a new test, is it ok now?

Ocramius · 2019-05-17T13:47:16Z

tests/Doctrine/Tests/DBAL/Schema/ForeignKeyConstraintTest.php

+     *
+     * @dataProvider getIntersectsIndexColumnsData
+     */
+    public function testIntersectsIndexEscapedColumns(array $indexColumns, $expectedResult)


What's $expectedResult? Needs type documentation here

I guess I could add bool type hint here. Is that would be ok? or I have to update doc block?

Adding a type declaration would be best (instead of docblock)

Ocramius · 2019-05-17T13:48:05Z

tests/Doctrine/Tests/DBAL/Schema/ForeignKeyConstraintTest.php

-        $index->expects($this->once())
-            ->method('getColumns')
-            ->will($this->returnValue($indexColumns));
+        $index = new Index('INDEX_NAME', $indexColumns);


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

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

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

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

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

…t I decided go with actual implementation

svycka · 2019-05-20T09:01:06Z

lib/Doctrine/DBAL/Schema/ForeignKeyConstraint.php

@@ -366,7 +366,7 @@ public function intersectsIndexColumns(Index $index)
    {
        foreach ($index->getUnquotedColumns() as $indexColumn) {
            foreach ($this->_localColumnNames as $localColumn) {
-                if (strtolower($indexColumn) === strtolower($localColumn->trimQuotes($localColumn->getName()))) {


it seems this was not required testIntersectsIndexEscapedColumns should cover this

morozov · 2020-02-26T14:15:35Z

Given that this is a cross-platform change, we need a new functional test that reproduces the problem. Just a unit test that covers some implementation details doesn't tell anyone anything.

svycka · 2020-02-27T07:18:20Z

Given that this is a cross-platform change, we need a new functional test that reproduces the problem. Just a unit test that covers some implementation details doesn't tell anyone anything.

maybe you are right. But I think this is just a bug I don't know database with could say they are different columns just because it is quoted for example column1 and `column1` do you know databases with allows to have those booths? So I think if we check if it exists we should check both at least in this case when we actually mean to check if the column exists.

And about a test, I am not sure how to test this it does not treat it as the same column and you say it is not a bug as I understand. You can see an example here #3387 but I don't know how to write a test for this. Maybe someone could help or write a test for this. Or at least some kind of similar example I could write a test if this is a bug.

morozov · 2020-02-28T18:52:59Z

Given that this is a cross-platform change, we need a new functional test that reproduces the problem. Just a unit test that covers some implementation details doesn't tell anyone anything.

maybe you are right.

I usually am.

But I think this is just a bug I don't know database with could say they are different columns just because it is quoted for example column1 and `column1` do you know databases with allows to have those booths?

No.

And about a test, I am not sure how to test this it does not treat it as the same column and you say it is not a bug as I understand.

I’m not saying it's not a bug. I’m saying that the unit test doesn't represent the bug.

You can see an example here #3387 but I don't know how to write a test for this. Maybe someone could help or write a test for this. Or at least some kind of similar example I could write a test if this is a bug.

As for the high-level “what the expected test should look like”, please look at DateExpressionTest.php and DefaultExpressionTest.php. The test should deploy the initial schema to a real DB, make the needed changes to it and assert that the changes have been applied correctly.

svycka added 2 commits December 7, 2018 09:30

fixes doctrine#3387 does not intersect columns if one is quoted and a…

b3096b2

…nother is not I am not sure this is correct just testing if other tests will not fail

updated tests to test agains queted

8401951

svycka mentioned this pull request Dec 7, 2018

Generates invalid up() migration #3387

Open

svycka commented Dec 7, 2018

View reviewed changes

Ocramius suggested changes Dec 7, 2018

View reviewed changes

Ocramius added Bug Schema BC Break Quoting Schema Comparison Schema Introspection labels Dec 7, 2018

Ocramius requested review from morozov and deeky666 December 7, 2018 11:19

Ocramius added the Schema Management label Dec 7, 2018

Extracted into separate test

77f349a

morozov requested changes Dec 10, 2018

View reviewed changes

svycka changed the title ~~does't intersect columns if one is quoted and another is not~~ fixes incorrect database migration for quoted columns Mar 25, 2019

Ocramius suggested changes May 17, 2019

View reviewed changes

Ocramius reviewed May 17, 2019

View reviewed changes

svycka added 2 commits May 20, 2019 10:28

removed improvement from not related test :)

f58443d

I still have to change this test and keeping mock makes harder to tes…

806ec1e

…t I decided go with actual implementation

svycka commented May 20, 2019

View reviewed changes

Merge branch 'master' into patch-2

6d592fd

svycka requested review from Ocramius and morozov February 26, 2020 09:09

morozov removed their request for review October 23, 2020 16:43

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

morozov closed this Oct 26, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes incorrect database migration for quoted columns #3395

fixes incorrect database migration for quoted columns #3395

svycka commented Dec 7, 2018 •

edited

Loading

svycka Dec 7, 2018 •

edited

Loading

Ocramius Dec 7, 2018

morozov Dec 7, 2018

svycka Dec 10, 2018

morozov Dec 10, 2018

SenseException Dec 10, 2018

svycka Dec 11, 2018

Ocramius Dec 7, 2018

Ocramius Dec 7, 2018

svycka commented Dec 10, 2018

morozov Dec 10, 2018

svycka commented May 17, 2019

Ocramius May 17, 2019

svycka May 17, 2019

Ocramius May 17, 2019

Ocramius May 17, 2019 •

edited

Loading

svycka May 17, 2019

Ocramius May 17, 2019

svycka May 20, 2019

svycka May 20, 2019

svycka May 20, 2019

morozov commented Feb 26, 2020

svycka commented Feb 27, 2020

morozov commented Feb 28, 2020

fixes incorrect database migration for quoted columns #3395

fixes incorrect database migration for quoted columns #3395

Conversation

svycka commented Dec 7, 2018 • edited Loading

Summary

svycka Dec 7, 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

Choose a reason for hiding this comment

svycka commented Dec 10, 2018

Choose a reason for hiding this comment

svycka commented May 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius May 17, 2019 • 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 commented Feb 26, 2020

svycka commented Feb 27, 2020

morozov commented Feb 28, 2020

svycka commented Dec 7, 2018 •

edited

Loading

svycka Dec 7, 2018 •

edited

Loading

Ocramius May 17, 2019 •

edited

Loading