Skip to content

Loading…

The schema tool now doesn't add a foreign constraint when subclassess of... #440

Closed
wants to merge 2 commits into from

3 participants

@sroddy

... a STI use the same field to map relations with entities of different classes

It seems that there are no particular side-effects in mapping different relationship using the same table column name in different subclasses of a STI hierarchy.
Eg. Tag superclass of BookTag (id, object_id, comment) and MovieTag (id, object_id, comment), so having two attributes $book and $movie mapped to the same object_id field with @JoinColumn annotation

I wasn't sure about this so I tested to see if there were errors. And I was surprised that everything runs without any particular problem.
Moreover this is a really interesting (wanted? unwanted?) feature, very useful if you have huge tables that need to be joined with other table using the discriminator and object_id columns, or if you have some kind of logging class and you need to keep the table efficient and small. The only problem is that the schema generator always inserts a foreign column constraint (probably the last specified) when generating these kind of Entities.

This PR ensures that the foreign key is not added if there are at least two different classes mapped on the same field of a db table.

@stof
Doctrine member

This needs tests

Stefano Rodr... added some commits
Stefano Rodriguez adedd failing test for PR #440 0fa3bff
Stefano Rodriguez The schema tool now doesn't add a foreign constraint when subclassess…
… of a STI use the same field to map relations with entities of different classes
daaa87b
@sroddy

@stof the PR should be ready now.

@sroddy

@beberlei @guilhermeblanco what do you think about this?

@beberlei beberlei added a commit that referenced this pull request
Stefano Rodriguez adedd failing test for PR #440 b1c69eb
@beberlei
Doctrine member

Merged into master.

@beberlei beberlei closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 8, 2012
  1. adedd failing test for PR #440

    Stefano Rodriguez committed
  2. The schema tool now doesn't add a foreign constraint when subclassess…

    Stefano Rodriguez committed
    … of a STI use the same field to map relations with entities of different classes
View
50 lib/Doctrine/ORM/Tools/SchemaTool.php
@@ -40,6 +40,7 @@
* @author Jonathan Wage <jonwage@gmail.com>
* @author Roman Borschel <roman@code-factory.org>
* @author Benjamin Eberlei <kontakt@beberlei.de>
+ * @author Stefano Rodriguez <stefano.rodriguez@fubles.com>
*/
class SchemaTool
{
@@ -140,6 +141,9 @@ public function getSchemaFromMetadata(array $classes)
$metadataSchemaConfig->setExplicitForeignKeyIndexes(false);
$schema = new Schema(array(), array(), $metadataSchemaConfig);
+ $addedFks = array();
+ $blacklistedFks = array();
+
foreach ($classes as $class) {
if ($this->processingNotRequired($class, $processedClasses)) {
continue;
@@ -150,7 +154,7 @@ public function getSchemaFromMetadata(array $classes)
if ($class->isInheritanceTypeSingleTable()) {
$columns = $this->_gatherColumns($class, $table);
- $this->_gatherRelationsSql($class, $table, $schema);
+ $this->_gatherRelationsSql($class, $table, $schema, $addedFks, $blacklistedFks);
// Add the discriminator column
$this->addDiscriminatorColumnDefinition($class, $table);
@@ -164,7 +168,7 @@ public function getSchemaFromMetadata(array $classes)
foreach ($class->subClasses as $subClassName) {
$subClass = $this->em->getClassMetadata($subClassName);
$this->_gatherColumns($subClass, $table);
- $this->_gatherRelationsSql($subClass, $table, $schema);
+ $this->_gatherRelationsSql($subClass, $table, $schema, $addedFks, $blacklistedFks);
$processedClasses[$subClassName] = true;
}
} else if ($class->isInheritanceTypeJoined()) {
@@ -181,7 +185,7 @@ public function getSchemaFromMetadata(array $classes)
}
}
- $this->_gatherRelationsSql($class, $table, $schema);
+ $this->_gatherRelationsSql($class, $table, $schema, $addedFks, $blacklistedFks);
// Add the discriminator column only to the root table
if ($class->name == $class->rootEntityName) {
@@ -210,7 +214,7 @@ public function getSchemaFromMetadata(array $classes)
throw ORMException::notSupported();
} else {
$this->_gatherColumns($class, $table);
- $this->_gatherRelationsSql($class, $table, $schema);
+ $this->_gatherRelationsSql($class, $table, $schema, $addedFks, $blacklistedFks);
}
$pkColumns = array();
@@ -415,9 +419,11 @@ private function _gatherColumn($class, array $mapping, $table)
* @param ClassMetadata $class
* @param \Doctrine\DBAL\Schema\Table $table
* @param \Doctrine\DBAL\Schema\Schema $schema
+ * @param array $addedFks
+ * @param array $blacklistedFks
* @return void
*/
- private function _gatherRelationsSql($class, $table, $schema)
+ private function _gatherRelationsSql($class, $table, $schema, &$addedFks, &$blacklistedFks)
{
foreach ($class->associationMappings as $fieldName => $mapping) {
if (isset($mapping['inherited'])) {
@@ -429,7 +435,7 @@ private function _gatherRelationsSql($class, $table, $schema)
if ($mapping['type'] & ClassMetadata::TO_ONE && $mapping['isOwningSide']) {
$primaryKeyColumns = $uniqueConstraints = array(); // PK is unnecessary for this relation-type
- $this->_gatherRelationJoinColumns($mapping['joinColumns'], $table, $foreignClass, $mapping, $primaryKeyColumns, $uniqueConstraints);
+ $this->_gatherRelationJoinColumns($mapping['joinColumns'], $table, $foreignClass, $mapping, $primaryKeyColumns, $uniqueConstraints, $addedFks, $blacklistedFks);
foreach($uniqueConstraints as $indexName => $unique) {
$table->addUniqueIndex($unique['columns'], is_numeric($indexName) ? null : $indexName);
@@ -446,10 +452,10 @@ private function _gatherRelationsSql($class, $table, $schema)
$primaryKeyColumns = $uniqueConstraints = array();
// Build first FK constraint (relation table => source table)
- $this->_gatherRelationJoinColumns($joinTable['joinColumns'], $theJoinTable, $class, $mapping, $primaryKeyColumns, $uniqueConstraints);
+ $this->_gatherRelationJoinColumns($joinTable['joinColumns'], $theJoinTable, $class, $mapping, $primaryKeyColumns, $uniqueConstraints, $addedFks, $blacklistedFks);
// Build second FK constraint (relation table => target table)
- $this->_gatherRelationJoinColumns($joinTable['inverseJoinColumns'], $theJoinTable, $foreignClass, $mapping, $primaryKeyColumns, $uniqueConstraints);
+ $this->_gatherRelationJoinColumns($joinTable['inverseJoinColumns'], $theJoinTable, $foreignClass, $mapping, $primaryKeyColumns, $uniqueConstraints, $addedFks, $blacklistedFks);
$theJoinTable->setPrimaryKey($primaryKeyColumns);
@@ -503,8 +509,10 @@ private function getDefiningClass($class, $referencedColumnName)
* @param array $mapping
* @param array $primaryKeyColumns
* @param array $uniqueConstraints
+ * @param array $addedFks
+ * @param array $blacklistedFks
*/
- private function _gatherRelationJoinColumns($joinColumns, $theJoinTable, $class, $mapping, &$primaryKeyColumns, &$uniqueConstraints)
+ private function _gatherRelationJoinColumns($joinColumns, $theJoinTable, $class, $mapping, &$primaryKeyColumns, &$uniqueConstraints, &$addedFks, &$blacklistedFks)
{
$localColumns = array();
$foreignColumns = array();
@@ -565,9 +573,27 @@ private function _gatherRelationJoinColumns($joinColumns, $theJoinTable, $class,
}
}
- $theJoinTable->addUnnamedForeignKeyConstraint(
- $foreignTableName, $localColumns, $foreignColumns, $fkOptions
- );
+ $compositeName = $theJoinTable->getName().'.'.implode('', $localColumns);
+ if (isset($addedFks[$compositeName])
+ && ($foreignTableName != $addedFks[$compositeName]['foreignTableName']
+ || 0 < count(array_diff($foreignColumns, $addedFks[$compositeName]['foreignColumns'])))
+ ) {
+ foreach ($theJoinTable->getForeignKeys() as $fkName => $key) {
+ if (0 === count(array_diff($key->getLocalColumns(), $localColumns))
+ && (($key->getForeignTableName() != $foreignTableName)
+ || 0 < count(array_diff($key->getForeignColumns(), $foreignColumns)))
+ ) {
+ $theJoinTable->removeForeignKey($fkName);
+ break;
+ }
+ }
+ $blacklistedFks[$compositeName] = true;
+ } elseif (!isset($blacklistedFks[$compositeName])) {
+ $addedFks[$compositeName] = array('foreignTableName' => $foreignTableName, 'foreignColumns' => $foreignColumns);
+ $theJoinTable->addUnnamedForeignKeyConstraint(
+ $foreignTableName, $localColumns, $foreignColumns, $fkOptions
+ );
+ }
}
/**
View
22 tests/Doctrine/Tests/Models/SingleTableInheritanceType/Structure.php
@@ -0,0 +1,22 @@
+<?php
+
+namespace Doctrine\Tests\Models\SingleTableInheritanceType;
+
+/**
+ * @Table(name="structures")
+ * @Entity
+ */
+class Structure
+{
+ /**
+ * @Id
+ * @Column(type="integer")
+ * @GeneratedValue(strategy="AUTO")
+ */
+ protected $id;
+
+ /**
+ * @Column(type="string", length=32, nullable=true)
+ */
+ protected $name;
+}
View
124 tests/Doctrine/Tests/Models/SingleTableInheritanceType/User.php
@@ -0,0 +1,124 @@
+<?php
+
+namespace Doctrine\Tests\Models\SingleTableInheritanceType;
+
+use Doctrine\Common\Collections\ArrayCollection;
+
+/**
+ * @Table(name="users")
+ * @Entity
+ */
+class User
+{
+ /**
+ * @Id
+ * @Column(type="integer")
+ * @GeneratedValue(strategy="AUTO")
+ */
+ protected $id;
+
+ /**
+ * @Column(type="string", length=32, nullable=true)
+ */
+ protected $name;
+
+ /**
+ * @var ArrayCollection $followedUsers
+ * @OneToMany(targetEntity="UserFollowedUser", mappedBy="user", cascade={"persist"}, orphanRemoval=true)
+ */
+ protected $followedUsers;
+
+ /**
+ * @var ArrayCollection $followedStructures
+ * @OneToMany(targetEntity="UserFollowedStructure", mappedBy="user", cascade={"persist"}, orphanRemoval=true)
+ */
+ protected $followedStructures;
+
+ public function __construct()
+ {
+ $this->followedUsers = new ArrayCollection();
+ $this->followedStructures = new ArrayCollection();
+ }
+
+ /*
+ * Remove followers
+ *
+ * @param UserFollowedUser $followers
+ */
+ private function removeFollower(UserFollowedUser $followers)
+ {
+ $this->followers->removeElement($followers);
+ }
+
+ /**
+ * Add followedUsers
+ *
+ * @param UserFollowedUser $followedUsers
+ * @return User
+ */
+ public function addFollowedUser(UserFollowedUser $followedUsers)
+ {
+ $this->followedUsers[] = $followedUsers;
+
+ return $this;
+ }
+
+ /**
+ * Remove followedUsers
+ *
+ * @param UserFollowedUser $followedUsers
+ * @return User
+ */
+ public function removeFollowedUser(UserFollowedUser $followedUsers)
+ {
+ $this->followedUsers->removeElement($followedUsers);
+
+ return $this;
+ }
+
+ /**
+ * Get followedUsers
+ *
+ * @return Doctrine\Common\Collections\Collection
+ */
+ public function getFollowedUsers()
+ {
+ return $this->followedUsers;
+ }
+
+ /**
+ * Add followedStructures
+ *
+ * @param UserFollowedStructure $followedStructures
+ * @return User
+ */
+ public function addFollowedStructure(UserFollowedStructure $followedStructures)
+ {
+ $this->followedStructures[] = $followedStructures;
+
+ return $this;
+ }
+
+ /**
+ * Remove followedStructures
+ *
+ * @param UserFollowedStructure $followedStructures
+ * @return User
+ */
+ public function removeFollowedStructure(UserFollowedStructure $followedStructures)
+ {
+ $this->followedStructures->removeElement($followedStructures);
+
+ return $this;
+ }
+
+ /**
+ * Get followedStructures
+ *
+ * @return Doctrine\Common\Collections\Collection
+ */
+ public function getFollowedStructures()
+ {
+ return $this->followedStructures;
+ }
+}
View
32 tests/Doctrine/Tests/Models/SingleTableInheritanceType/UserFollowedObject.php
@@ -0,0 +1,32 @@
+<?php
+
+namespace Doctrine\Tests\Models\SingleTableInheritanceType;
+
+/**
+ * @Entity
+ * @Table(name="users_followed_objects")
+ * @InheritanceType("SINGLE_TABLE")
+ * @DiscriminatorColumn(name="object_type", type="smallint")
+ * @DiscriminatorMap({4 = "UserFollowedUser", 3 = "UserFollowedStructure"})
+ */
+abstract class UserFollowedObject
+{
+ /**
+ * @var integer $id
+ *
+ * @Column(name="id", type="integer")
+ * @Id
+ * @GeneratedValue(strategy="AUTO")
+ */
+ protected $id;
+
+ /**
+ * Get id
+ *
+ * @return integer
+ */
+ public function getId()
+ {
+ return $this->id;
+ }
+}
View
54 tests/Doctrine/Tests/Models/SingleTableInheritanceType/UserFollowedStructure.php
@@ -0,0 +1,54 @@
+<?php
+
+namespace Doctrine\Tests\Models\SingleTableInheritanceType;
+
+/**
+ * @Entity
+ */
+class UserFollowedStructure extends UserFollowedObject
+{
+ /**
+ * @ManyToOne(targetEntity="User", inversedBy="followedStructures")
+ * @JoinColumn(name="user_id", referencedColumnName="id", nullable=false)
+ * @var User $user
+ */
+ protected $user;
+
+ /**
+ * @ManyToOne(targetEntity="Structure")
+ * @JoinColumn(name="object_id", referencedColumnName="id", nullable=false)
+ * @var Structure $followedStructure
+ */
+ private $followedStructure;
+
+ /**
+ * Construct a UserFollowedStructure entity
+ *
+ * @param User $user
+ * @param Structure $followedStructure
+ */
+ public function __construct(User $user, Structure $followedStructure)
+ {
+ $this->user = $user;
+ $this->followedStructure = $followedStructure;
+ }
+
+ /**
+ *
+ * @return User
+ */
+ public function getUser()
+ {
+ return $this->user;
+ }
+
+ /**
+ * Gets followed structure
+ *
+ * @return Structure
+ */
+ public function getFollowedStructure()
+ {
+ return $this->followedStructure;
+ }
+}
View
55 tests/Doctrine/Tests/Models/SingleTableInheritanceType/UserFollowedUser.php
@@ -0,0 +1,55 @@
+<?php
+
+namespace Doctrine\Tests\Models\SingleTableInheritanceType;
+
+/**
+ * @Entity
+ */
+class UserFollowedUser extends UserFollowedObject
+{
+ /**
+ * @ManyToOne(targetEntity="User", inversedBy="followedUsers")
+ * @JoinColumn(name="user_id", referencedColumnName="id", nullable=false)
+ * @var User $user
+ */
+ protected $user;
+
+ /**
+ * @ManyToOne(targetEntity="User")
+ * @JoinColumn(name="object_id", referencedColumnName="id", nullable=false)
+ * @var User $user
+ */
+ private $followedUser;
+
+ /**
+ * Construct a UserFollowedUser entity
+ *
+ * @param User $user
+ * @param User $followedUser
+ * @param bool $giveAgency
+ */
+ public function __construct(User $user, User $followedUser)
+ {
+ $this->user = $user;
+ $this->followedUser = $followedUser;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getUser()
+ {
+ return $this->user;
+ }
+
+ /**
+ * Gets followed user
+ *
+ * @return User
+ */
+ public function getFollowedUser()
+ {
+ return $this->followedUser;
+ }
+
+}
View
32 tests/Doctrine/Tests/ORM/Tools/SchemaToolTest.php
@@ -32,6 +32,38 @@ public function testAddUniqueIndexForUniqueFieldAnnocation()
$this->assertTrue($schema->getTable('cms_users')->columnsAreIndexed(array('username')), "username column should be indexed.");
}
+ public function testForeignKeyOnSTIWithMultipleMapping()
+ {
+ $em = $this->_getTestEntityManager();
+ $schemaTool = new SchemaTool($em);
+
+ $classes = array(
+ $em->getClassMetadata('Doctrine\Tests\Models\SingleTableInheritanceType\User'),
+ $em->getClassMetadata('Doctrine\Tests\Models\SingleTableInheritanceType\Structure'),
+ $em->getClassMetadata('Doctrine\Tests\Models\SingleTableInheritanceType\UserFollowedObject'),
+ $em->getClassMetadata('Doctrine\Tests\Models\SingleTableInheritanceType\UserFollowedStructure'),
+ $em->getClassMetadata('Doctrine\Tests\Models\SingleTableInheritanceType\UserFollowedUser')
+ );
+
+ $schema = $schemaTool->getSchemaFromMetadata($classes);
+ $this->assertTrue($schema->hasTable('users_followed_objects'), "Table users_followed_objects should exist.");
+
+ /* @var $table \Doctrine\DBAL\Schema\Table */
+ $table = ($schema->getTable('users_followed_objects'));
+ $this->assertTrue($table->columnsAreIndexed(array('object_id')));
+ $this->assertTrue($table->columnsAreIndexed(array('user_id')));
+ $foreignKeys = $table->getForeignKeys();
+ $this->assertCount(1, $foreignKeys, 'user_id column has to have FK, but not object_id');
+
+ /* @var $fk \Doctrine\DBAL\Schema\ForeignKeyConstraint */
+ $fk = reset($foreignKeys);
+ $this->assertEquals('users', $fk->getForeignTableName());
+
+ $localColumns = $fk->getLocalColumns();
+ $this->assertContains('user_id', $localColumns);
+ $this->assertCount(1, $localColumns);
+ }
+
public function testAnnotationOptionsAttribute()
{
$em = $this->_getTestEntityManager();
Something went wrong with that request. Please try again.