Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

fixed bug on schema comparator, prevent multiple rename candidates for a single original field #213

Merged
merged 7 commits into from

6 participants

@leedavis81

Starting with the following Entity

/**
 * @Table(name="user")
 * @Entity
 */
class User
{
    /**
     * @var integer $id
     * @Column(name="id", type="integer", length=4)
     * @Id
     * @GeneratedValue(strategy="IDENTITY")
     */
    private $id;

    /**
     * @var \DateTime $date_alerted_email
     * @Column(name="date_alerted", type="datetime", nullable=true)
     */
    private $date_alerted;
}

Upon making the following alterations (remove date_alerted, and add two additional columns of the same type but different names):

/**
 * @Table(name="user")
 * @Entity
 */
class User
{
    /**
     * @var integer $id
     * @Column(name="id", type="integer", length=4)
     * @Id
     * @GeneratedValue(strategy="IDENTITY")
     */
    private $id;

    /**
     * @var \DateTime $date_alerted_email
     * @Column(name="date_alerted_email", type="datetime", nullable=true)
     */
    private $date_alerted_email;

    /**
     * @var \DateTime $date_alerted_js
     * @Column(name="date_alerted_js", type="datetime", nullable=true)
     */
    private $date_alerted_js;
}

The doctrine cli schema tool used to run the update (dump sql) produces the following result:

ALTER TABLE user CHANGE date_alerted date_alerted_js DATETIME DEFAULT NULL

Expected result:

ALTER TABLE message ADD date_alerted_js DATETIME DEFAULT NULL, CHANGE date_alerted date_alerted_email DATETIME DEFAULT NULL

What went wrong

Upon running diffTable($from, $to) in \Doctrine\DBAL\Schema\Comparator.php line 69 both new columns are added to the "addedColumns" array, and the one removed column is correctly present in the removedColumns array.

The first iteration on detectColumnRenamings (line 272) puts the two NEW columns up as rename candidates as expected, however they share the original field name.

When iterating over the rename candidates no further checks are done and these entries are added to the renamedColumns array. The last overwrites the original as they share the same array key and the original gets ignored.

My change checks the renamedColumns array for the existence of the old column name. It only becomes a rename candidate if the original field hasn't already used previously. Otherwise it remains in the addedColumns array.

In the example above these changes will produce 1 change column and 1 add column as expected.

@doctrinebot
Collaborator

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DBAL-361

lib/Doctrine/DBAL/Schema/Comparator.php
@@ -282,10 +282,13 @@ private function detectColumnRenamings(TableDiff $tableDifferences)
list($removedColumn, $addedColumn) = $candidateColumns[0];
$removedColumnName = strtolower($removedColumn->getName());
$addedColumnName = strtolower($addedColumn->getName());
-
- $tableDifferences->renamedColumns[$removedColumnName] = $addedColumn;
- unset($tableDifferences->addedColumns[$addedColumnName]);
- unset($tableDifferences->removedColumns[$removedColumnName]);
+
+ if (!isset($tableDifferences->renamedColumns[$removedColumnName]))
@guilhermeblanco Owner

Missing spaces around !.
Open curly braces should be at the same line of conditional expression if (...) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/DBAL/Schema/Comparator.php
@@ -282,10 +282,13 @@ private function detectColumnRenamings(TableDiff $tableDifferences)
list($removedColumn, $addedColumn) = $candidateColumns[0];
$removedColumnName = strtolower($removedColumn->getName());
$addedColumnName = strtolower($addedColumn->getName());
-
- $tableDifferences->renamedColumns[$removedColumnName] = $addedColumn;
- unset($tableDifferences->addedColumns[$addedColumnName]);
- unset($tableDifferences->removedColumns[$removedColumnName]);
+
+ if (!isset($tableDifferences->renamedColumns[$removedColumnName]))
+ {
+ $tableDifferences->renamedColumns[$removedColumnName] = $addedColumn;
+ unset($tableDifferences->addedColumns[$addedColumnName]);
@guilhermeblanco Owner

Missing line break

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Mezzle

Great writeup :)

Are there any tests you can write to stop this bug regressing?

@leedavis81

I'll try and sort some out as soon as I get the time.

///
Struggling to get PHPUnit set up and running with DBAL, seem to be getting nothing back from CLI.
Is there a specific version of PHPUnit you'd advise me to use? Currently have 3.6.10 installed.

@stof

@leedavis81 the test should be easy to write as you already have the needed case. You simply need to write the Schema objet for both entities above and pass them to the comparator.

@leedavis81

As I'm unable to setup PHPUnit to work on DBAL i've no idea if the test i've written is suitable. Does anyone have any advice on my previous comment? I don't seem to be getting any output when running phpunit.

@stof

@leedavis81 The DBAL testsuite is still setup with submodules. Run git submodule update --init

tests/Doctrine/Tests/DBAL/Schema/ComparatorTest.php
@@ -210,6 +210,26 @@ public function testCompareChangedColumns_ChangeCustomSchemaOption()
$this->assertEquals(array(), $c->diffColumn($column1, $column1));
}
+ public function testCompareChangeColumns_MultipleNewColumnsRename()
+ {
+ $tableA = new Table("foo");
+ $tableA->addColumn('datefield1', 'datetime');
+
+ $tableB = new Table("foo");
+ $tableB->addColumn('new_datefield1', 'datetime');
+ $tableB->addColumn('new_datefield2', 'datetime');
+
+ $c = new Comparator();
+ $tableDiff = $c->diffTable($tableA, $tableB);
+
+ $this->assertEquals(1, count($tableDiff->renamedColumns), "we should have one rename datefield1 => new_datefield1.");
@stof
stof added a note

you should use assertCount

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@leedavis81

Thanks stof, I got the suite running and updated the test as requested.

I had some trouble coming up with a suitable name for the test method. I settled with testCompareChangeColumns_MultipleNewColumnsRename() but may not be appropriate. Happy for any suggestions.

lib/Doctrine/DBAL/Schema/Comparator.php
@@ -282,10 +282,12 @@ private function detectColumnRenamings(TableDiff $tableDifferences)
list($removedColumn, $addedColumn) = $candidateColumns[0];
$removedColumnName = strtolower($removedColumn->getName());
$addedColumnName = strtolower($addedColumn->getName());
-
- $tableDifferences->renamedColumns[$removedColumnName] = $addedColumn;
- unset($tableDifferences->addedColumns[$addedColumnName]);
- unset($tableDifferences->removedColumns[$removedColumnName]);
+
@stof
stof added a note

Please don't add trailing whitespaces

I've removed the trailing whitespace, is this note redundant?

@stof
stof added a note

It is the old note, which has not been hidden by github.

but you should have kept the empty line before the if, just making it really empty

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@leedavis81

Is there anything else I need to do to get this to be merged? It's quite a frustrating issue that causes me to run an update twice to ensure changes to the DB have been correctly applied.

This bug also affects the migrations tool.

Pull request has been created #213

@beberlei beberlei merged commit d0e6092 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
8 lib/Doctrine/DBAL/Schema/Comparator.php
@@ -283,9 +283,11 @@ private function detectColumnRenamings(TableDiff $tableDifferences)
$removedColumnName = strtolower($removedColumn->getName());
$addedColumnName = strtolower($addedColumn->getName());
- $tableDifferences->renamedColumns[$removedColumnName] = $addedColumn;
- unset($tableDifferences->addedColumns[$addedColumnName]);
- unset($tableDifferences->removedColumns[$removedColumnName]);
+ if ( ! isset($tableDifferences->renamedColumns[$removedColumnName])) {
+ $tableDifferences->renamedColumns[$removedColumnName] = $addedColumn;
+ unset($tableDifferences->addedColumns[$addedColumnName]);
+ unset($tableDifferences->removedColumns[$removedColumnName]);
+ }
}
}
}
View
20 tests/Doctrine/Tests/DBAL/Schema/ComparatorTest.php
@@ -210,6 +210,26 @@ public function testCompareChangedColumns_ChangeCustomSchemaOption()
$this->assertEquals(array(), $c->diffColumn($column1, $column1));
}
+ public function testCompareChangeColumns_MultipleNewColumnsRename()
+ {
+ $tableA = new Table("foo");
+ $tableA->addColumn('datefield1', 'datetime');
+
+ $tableB = new Table("foo");
+ $tableB->addColumn('new_datefield1', 'datetime');
+ $tableB->addColumn('new_datefield2', 'datetime');
+
+ $c = new Comparator();
+ $tableDiff = $c->diffTable($tableA, $tableB);
+
+ $this->assertCount(1, $tableDiff->renamedColumns, "we should have one rename datefield1 => new_datefield1.");
+ $this->assertArrayHasKey('datefield1', $tableDiff->renamedColumns, "'datefield1' should be set to be renamed to new_datefield1");
+ $this->assertCount(1, $tableDiff->addedColumns, "'new_datefield2' should be added");
+ $this->assertArrayHasKey('new_datefield2', $tableDiff->addedColumns, "'new_datefield2' should be added, not created through renaming!");
+ $this->assertCount(0, $tableDiff->removedColumns, "Nothing should be removed.");
+ $this->assertCount(0, $tableDiff->changedColumns, "Nothing should be changed as all fields old & new have diff names.");
+ }
+
public function testCompareRemovedIndex()
{
$schema1 = new Schema( array(
Something went wrong with that request. Please try again.