DBAL-50 - Explicitly add an index for each foreign key added to a Schema\Table...

DBAL-50 - Explicitly add an index for each foreign key added to a Schema\Table instance. Removed code to FixSchema based on foreign keys, since assumptions now dont apply anymore. Refactored algorithms to check for duplicate or potential overrule indexes into Schema\Index classes, added tests and unified Table API handling these conflicts

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

@@ -1823,14 +1823,6 @@ abstract class AbstractPlatform
         return true;
     }
 
-    /**
-     * @return bool
-     */
-    public function createsExplicitIndexForForeignKeys()
-    {
-        return false;
-    }
-
     /**
      * Whether the platform supports getting the affected rows of a recent
      * update/delete type query.

lib/Doctrine/DBAL/Platforms/MySqlPlatform.php

@@ -577,11 +577,6 @@ class MySqlPlatform extends AbstractPlatform
         return 'mysql';
     }
 
-    public function createsExplicitIndexForForeignKeys()
-    {
-        return true;
-    }
-
     public function getReadLockSQL()
     {
         return 'LOCK IN SHARE MODE';

lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php

@@ -770,7 +770,6 @@ abstract class AbstractSchemaManager
     public function createSchemaConfig()
     {
         $schemaConfig = new SchemaConfig();
-        $schemaConfig->setExplicitForeignKeyIndexes($this->_platform->createsExplicitIndexForForeignKeys());
         $schemaConfig->setMaxIdentifierLength($this->_platform->getMaxIdentifierLength());
 
         return $schemaConfig;

lib/Doctrine/DBAL/Schema/Comparator.php

@@ -57,10 +57,6 @@ class Comparator
      */
     public function compare(Schema $fromSchema, Schema $toSchema)
     {
-        if ($fromSchema->hasExplicitForeignKeyIndexes() && !$toSchema->hasExplicitForeignKeyIndexes()) {
-            $toSchema->visit(new \Doctrine\DBAL\Schema\Visitor\FixSchema(true));
-        }
-
         $diff = new SchemaDiff();
 
         $foreignKeysToTable = array();
@@ -363,31 +359,9 @@ class Comparator
      */
     public function diffIndex(Index $index1, Index $index2)
     {
-        if($index1->isPrimary() != $index2->isPrimary()) {
-            return true;
+        if ($index1->isFullfilledBy($index2) && $index2->isFullfilledBy($index1)) {
+            return false;
         }
-        if($index1->isUnique() != $index2->isUnique()) {
-            return true;
-        }
-
-        // Check for removed index fields in $index2
-        $index1Columns = $index1->getColumns();
-        for($i = 0; $i < count($index1Columns); $i++) {
-            $indexColumn = $index1Columns[$i];
-            if (!$index2->hasColumnAtPosition($indexColumn, $i)) {
-                return true;
-            }
-        }
-
-        // Check for new index fields in $index2
-        $index2Columns = $index2->getColumns();
-        for($i = 0; $i < count($index2Columns); $i++) {
-            $indexColumn = $index2Columns[$i];
-            if (!$index1->hasColumnAtPosition($indexColumn, $i)) {
-                return true;
-            }
-        }
-
-        return false;
+        return true;
     }
 }

lib/Doctrine/DBAL/Schema/Index.php

@@ -106,4 +106,73 @@ class Index extends AbstractAsset implements Constraint
         $indexColumns = \array_map('strtolower', $this->getColumns());
         return \array_search($columnName, $indexColumns) === $pos;
     }
+
+    /**
+     * Check if this index exactly spans the given column names in the correct order.
+     *
+     * @param array $columnNames
+     * @return boolean
+     */
+    public function spansColumns(array $columnNames)
+    {
+        $sameColumns = true;
+        for ($i = 0; $i < count($this->_columns); $i++) {
+            if (!isset($columnNames[$i]) || $this->_columns[$i] != $columnNames[$i]) {
+                $sameColumns = false;
+            }
+        }
+        return $sameColumns;
+    }
+
+    /**
+     * Check if the other index already fullfills all the indexing and constraint needs of the current one.
+     *
+     * @param Index $other
+     * @return bool
+     */
+    public function isFullfilledBy(Index $other)
+    {
+        // allow the other index to be equally large only. It being larger is an option
+        // but it creates a problem with scenarios of the kind PRIMARY KEY(foo,bar) UNIQUE(foo)
+        if (count($other->getColumns()) != count($this->getColumns())) {
+            return false;
+        }
+
+        // Check if columns are the same, and even in the same order
+        $sameColumns = $this->spansColumns($other->getColumns());
+
+        if ($sameColumns) {
+            if (!$this->isUnique() && !$this->isPrimary()) {
+                // this is a special case: If the current key is neither primary or unique, any uniqe or
+                // primary key will always have the same effect for the index and there cannot be any constraint
+                // overlaps. This means a primary or unique index can always fullfill the requirements of just an
+                // index that has no constraints.
+                return true;
+            } else if ($other->isPrimary() != $this->isPrimary()) {
+                return false;
+            } else if ($other->isUnique() != $this->isUnique()) {
+                return false;
+            }
+            return true;
+        }
+        return false;
+    }
+
+    /**
+     * Detect if the other index is a non-unique, non primary index that can be overwritten by this one.
+     *
+     * @param Index $other
+     * @return bool
+     */
+    public function overrules(Index $other)
+    {
+        if ($other->isPrimary() || $other->isUnique()) {
+            return false;
+        }
+
+        if ($this->spansColumns($other->getColumns()) && ($this->isPrimary() || $this->isUnique())) {
+            return true;
+        }
+        return false;
+    }
 }
\ No newline at end of file

lib/Doctrine/DBAL/Schema/Table.php

@@ -185,15 +185,8 @@ class Table extends AbstractAsset
     public function columnsAreIndexed(array $columnsNames)
     {
         foreach ($this->getIndexes() AS $index) {
-            $indexColumns = $index->getColumns();
-            $areIndexed = true;
-            for ($i = 0; $i < count($columnsNames); $i++) {
-                if ($columnsNames[$i] != $indexColumns[$i]) {
-                    $areIndexed = false;
-                }
-            }
-
-            if ($areIndexed) {
+            /* @var $index Index */
+            if ($index->spansColumns($columnsNames)) {
                 return true;
             }
         }
@@ -386,31 +379,37 @@ class Table extends AbstractAsset
     /**
      * Add index to table
      * 
-     * @param Index $index
+     * @param Index $indexCandidate
      * @return Table
      */
-    protected function _addIndex(Index $index)
+    protected function _addIndex(Index $indexCandidate)
     {
         // check for duplicates
-        $c = new Comparator();
         foreach ($this->_indexes AS $existingIndex) {
-            if ($c->diffIndex($index, $existingIndex) == false) {
+            if ($indexCandidate->isFullfilledBy($existingIndex)) {
                 return $this;
             }
         }
 
-        $indexName = $index->getName();
+        $indexName = $indexCandidate->getName();
         $indexName = strtolower($indexName);
 
-        if (isset($this->_indexes[$indexName]) || ($this->_primaryKeyName != false && $index->isPrimary())) {
+        if (isset($this->_indexes[$indexName]) || ($this->_primaryKeyName != false && $indexCandidate->isPrimary())) {
             throw SchemaException::indexAlreadyExists($indexName, $this->_name);
         }
 
-        if ($index->isPrimary()) {
+        // remove overruled indexes
+        foreach ($this->_indexes AS $idxKey => $existingIndex) {
+            if ($indexCandidate->overrules($existingIndex)) {
+                unset($this->_indexes[$idxKey]);
+            }
+        }
+
+        if ($indexCandidate->isPrimary()) {
             $this->_primaryKeyName = $indexName;
         }
 
-        $this->_indexes[$indexName] = $index;
+        $this->_indexes[$indexName] = $indexCandidate;
         return $this;
     }
 
@@ -431,6 +430,10 @@ class Table extends AbstractAsset
         $name = strtolower($name);
 
         $this->_fkConstraints[$name] = $constraint;
+        // add an explicit index on the foreign key columns. If there is already an index that fullfils this requirements drop the request.
+        // In the case of __construct calling this method during hydration from schema-details all the explicitly added indexes
+        // lead to duplicates. This creates compuation overhead in this case, however no duplicate indexes are ever added (based on columns).
+        $this->addIndex($constraint->getColumns());
     }
 
     /**

lib/Doctrine/DBAL/Schema/Visitor/FixSchema.php

-<?php
-
-namespace Doctrine\DBAL\Schema\Visitor;
-
-use Doctrine\DBAL\Platforms\AbstractPlatform,
-    Doctrine\DBAL\Schema\Table,
-    Doctrine\DBAL\Schema\Schema,
-    Doctrine\DBAL\Schema\Column,
-    Doctrine\DBAL\Schema\ForeignKeyConstraint,
-    Doctrine\DBAL\Schema\Constraint,
-    Doctrine\DBAL\Schema\Sequence,
-    Doctrine\DBAL\Schema\Index;
-
-class FixSchema implements Visitor
-{
-    /**
-     * @var bool
-     */
-    private $_addExplicitIndexForForeignKey = null;
-
-    public function __construct($addExplicitIndexForForeignKey)
-    {
-        $this->_addExplicitIndexForForeignKey = $addExplicitIndexForForeignKey;
-    }
-
-    /**
-     * @param Schema $schema
-     */
-    public function acceptSchema(Schema $schema)
-    {
-        
-    }
-
-    /**
-     * @param Table $table
-     */
-    public function acceptTable(Table $table)
-    {
-
-    }
-
-    /**
-     * @param Column $column
-     */
-    public function acceptColumn(Table $table, Column $column)
-    {
-        
-    }
-
-    /**
-     * @param Table $localTable
-     * @param ForeignKeyConstraint $fkConstraint
-     */
-    public function acceptForeignKey(Table $localTable, ForeignKeyConstraint $fkConstraint)
-    {
-        if ($this->_addExplicitIndexForForeignKey) {
-            $columns = $fkConstraint->getColumns();
-            if ($localTable->columnsAreIndexed($columns)) {
-                return;
-            }
-
-            $localTable->addIndex($columns);
-        }
-    }
-
-    /**
-     * @param Table $table
-     * @param Index $index
-     */
-    public function acceptIndex(Table $table, Index $index)
-    {
-
-    }
-
-    /**
-     * @param Sequence $sequence
-     */
-    public function acceptSequence(Sequence $sequence)
-    {
-
-    }
-}
\ No newline at end of file

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

@@ -222,7 +222,8 @@ class SchemaManagerFunctionalTestCase extends \Doctrine\Tests\DbalFunctionalTest
 
         $this->_sm->dropAndCreateTable($tableA);
 
-        $fkConstraints = $this->_sm->listTableForeignKeys('test_create_fk');
+        $fkTable = $this->_sm->listTableDetails('test_create_fk');
+        $fkConstraints = $fkTable->getForeignKeys();
         $this->assertEquals(1, count($fkConstraints), "Table 'test_create_fk1' has to have one foreign key.");
 
         $fkConstraint = current($fkConstraints);
@@ -230,6 +231,7 @@ class SchemaManagerFunctionalTestCase extends \Doctrine\Tests\DbalFunctionalTest
         $this->assertEquals('test_foreign',             strtolower($fkConstraint->getForeignTableName()));
         $this->assertEquals(array('foreign_key_test'),  array_map('strtolower', $fkConstraint->getColumns()));
         $this->assertEquals(array('id'),                array_map('strtolower', $fkConstraint->getForeignColumns()));
+        $this->assertTrue($fkTable->columnsAreIndexed($fkConstraint->getColumns()));
     }
 
     public function testListForeignKeys()

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

@@ -40,4 +40,45 @@ class IndexTest extends \PHPUnit_Framework_TestCase
         $this->assertTrue($idx->isUnique());
         $this->assertFalse($idx->isPrimary());
     }
+
+    /**
+     * @group DBAL-50
+     */
+    public function testFullfilledByUnique()
+    {
+        $idx1 = $this->createIndex(true, false);
+        $idx2 = $this->createIndex(true, false);
+        $idx3 = $this->createIndex();
+
+        $this->assertTrue($idx1->isFullfilledBy($idx2));
+        $this->assertFalse($idx1->isFullfilledBy($idx3));
+    }
+
+    /**
+     * @group DBAL-50
+     */
+    public function testFullfilledByPrimary()
+    {
+        $idx1 = $this->createIndex(true, true);
+        $idx2 = $this->createIndex(true, true);
+        $idx3 = $this->createIndex(true, false);
+
+        $this->assertTrue($idx1->isFullfilledBy($idx2));
+        $this->assertFalse($idx1->isFullfilledBy($idx3));
+    }
+
+    /**
+     * @group DBAL-50
+     */
+    public function testFullfilledByIndex()
+    {
+        $idx1 = $this->createIndex();
+        $idx2 = $this->createIndex();
+        $pri = $this->createIndex(true, true);
+        $uniq = $this->createIndex(true);
+
+        $this->assertTrue($idx1->isFullfilledBy($idx2));
+        $this->assertTrue($idx1->isFullfilledBy($pri));
+        $this->assertTrue($idx1->isFullfilledBy($uniq));
+    }
 }
\ No newline at end of file

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

@@ -167,39 +167,6 @@ class SchemaTest extends \PHPUnit_Framework_TestCase
         $schema = new Schema(array(), array($sequence, $sequence));
     }
 
-    public function testFixSchema_AddExplicitIndexForForeignKey()
-    {
-        $schema = new Schema();
-        $tableA = $schema->createTable('foo');
-        $tableA->addColumn('id', 'integer');
-
-        $tableB = $schema->createTable('bar');
-        $tableB->addColumn('id', 'integer');
-        $tableB->addColumn('foo_id', 'integer');
-        $tableB->addForeignKeyConstraint($tableA, array('foo_id'), array('id'));
-
-        $this->assertEquals(0, count($tableB->getIndexes()));
-
-        $schema->visit(new \Doctrine\DBAL\Schema\Visitor\FixSchema(true));
-
-        $this->assertEquals(1, count($tableB->getIndexes()));
-        $indexes = $tableB->getIndexes();
-        $index = current($indexes);
-        $this->assertTrue($index->hasColumnAtPosition('foo_id', 0));
-    }
-
-    public function testConfigHasExplicitForeignKeyIndex()
-    {
-        $schemaConfig = new \Doctrine\DBAL\Schema\SchemaConfig();
-        $schemaConfig->setExplicitForeignKeyIndexes(false);
-
-        $schema = new Schema(array(), array(), $schemaConfig);
-        $this->assertFalse($schema->hasExplicitForeignKeyIndexes());
-
-        $schemaConfig->setExplicitForeignKeyIndexes(true);
-        $this->assertTrue($schema->hasExplicitForeignKeyIndexes());
-    }
-
     public function testConfigMaxIdentifierLength()
     {
         $schemaConfig = new \Doctrine\DBAL\Schema\SchemaConfig();

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

@@ -112,17 +112,21 @@ class TableTest extends \PHPUnit_Framework_TestCase
         $columns = array(new Column("foo", $type), new Column("bar", $type), new Column("baz", $type));
         $table = new Table("foo", $columns);
         
-        $table->addIndex(array("foo", "bar", "baz"));
-        $table->addUniqueIndex(array("foo", "bar", "baz"));
+        $table->addIndex(array("foo", "bar"));
+        $table->addUniqueIndex(array("bar", "baz"));
 
-        $this->assertTrue($table->hasIndex("foo_foo_bar_baz_idx"));
-        $this->assertTrue($table->hasIndex("foo_foo_bar_baz_uniq"));
+        $this->assertTrue($table->hasIndex("foo_foo_bar_idx"));
+        $this->assertTrue($table->hasIndex("foo_bar_baz_uniq"));
     }
 
     public function testIndexCaseInsensitive()
     {
         $type = \Doctrine\DBAL\Types\Type::getType('integer');
-        $columns = array(new Column("foo", $type), new Column("bar", $type), new Column("baz", $type));
+        $columns = array(
+            new Column("foo", $type),
+            new Column("bar", $type),
+            new Column("baz", $type)
+        );
         $table = new Table("foo", $columns);
 
         $table->addIndex(array("foo", "bar", "baz"), "Foo_Idx");
@@ -135,20 +139,23 @@ class TableTest extends \PHPUnit_Framework_TestCase
     public function testAddIndexes()
     {
         $type = \Doctrine\DBAL\Types\Type::getType('integer');
-        $columns = array(new Column("foo", $type));
+        $columns = array(
+            new Column("foo", $type),
+            new Column("bar", $type),
+        );
         $indexes = array(
             new Index("the_primary", array("foo"), true, true),
-            new Index("foo_idx", array("foo"), false, false),
+            new Index("bar_idx", array("bar"), false, false),
         );
         $table = new Table("foo", $columns, $indexes, array());
 
         $this->assertTrue($table->hasIndex("the_primary"));
-        $this->assertTrue($table->hasIndex("foo_idx"));
+        $this->assertTrue($table->hasIndex("bar_idx"));
         $this->assertFalse($table->hasIndex("some_idx"));
 
         $this->assertType('Doctrine\DBAL\Schema\Index', $table->getPrimaryKey());
         $this->assertType('Doctrine\DBAL\Schema\Index', $table->getIndex('the_primary'));
-        $this->assertType('Doctrine\DBAL\Schema\Index', $table->getIndex('foo_idx'));
+        $this->assertType('Doctrine\DBAL\Schema\Index', $table->getIndex('bar_idx'));
     }
 
     public function testGetUnknownIndexThrowsException()
@@ -345,4 +352,52 @@ class TableTest extends \PHPUnit_Framework_TestCase
 
         $this->assertTrue($table->hasIndex('foo_bar_baz_idx'));
     }
+
+    /**
+     * @group DBAL-50
+     */
+    public function testAddIndexTwice_IgnoreSecond()
+    {
+        $table = new Table("foo.bar");
+        $table->addColumn('baz', 'integer', array());
+        $table->addIndex(array('baz'));
+        $table->addIndex(array('baz'));
+
+        $this->assertEquals(1, count($table->getIndexes()));
+    }
+
+    /**
+     * @group DBAL-50
+     */
+    public function testAddForeignKeyIndexImplicitly()
+    {
+        $table = new Table("foo");
+        $table->addColumn("id", 'integer');
+
+        $foreignTable = new Table("bar");
+        $foreignTable->addColumn("id", 'integer');
+
+        $table->addForeignKeyConstraint($foreignTable, array("id"), array("id"), array("foo" => "bar"));
+
+        $this->assertEquals(1, count($table->getIndexes()));
+        $this->assertTrue($table->hasIndex("foo_id_idx"));
+        $this->assertEquals(array('id'), $table->getIndex('foo_id_idx')->getColumns());
+    }
+
+    /**
+     * @group DBAL-50
+     */
+    public function testOverruleIndex()
+    {
+        $table = new Table("bar");
+        $table->addColumn('baz', 'integer', array());
+        $table->addIndex(array('baz'));
+        $this->assertEquals(1, count($table->getIndexes()));
+        $this->assertTrue($table->hasIndex('bar_baz_idx'));
+
+        $table->addUniqueIndex(array('baz'));
+        $this->assertEquals(1, count($table->getIndexes()));
+        $this->assertFalse($table->hasIndex('bar_baz_idx'));
+        $this->assertTrue($table->hasIndex('bar_baz_uniq'));
+    }
 }
\ No newline at end of file