Skip to content

Commit

Permalink
Fix association handling when there is a MappedSuperclass in the midd…
Browse files Browse the repository at this point in the history
…le of an inheritance hierarchy

This fixes two closely related bugs.

1. When inheriting a to-one association from a mapped superclass, update the `sourceEntity` class name to the current class only when the association is actually _declared_ in the mapped superclass.
2. Reject association types that are not allowed on mapped superclasses only when they are actually _declared_ in a mapped superclass, not when inherited from parent classes.

Currently, when a many-to-one association is inherited from a `MappedSuperclass`, mapping information will be updated so that the association has the current (inheriting) class as the source entity.

https://github.com/doctrine/orm/blob/2138cc93834cfae9cd3f86c991fa051a3129b693/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php#L384-L393

This was added in 7dc8ef1 for [DDC-671](doctrine#5181).

The reason for this is that a mapped superclass is not an entity itself and has no table.

So, in the database, associations can only be from the inheriting entities' tables towards the referred-to target. This is also the reason for the limitation that only to-one associations may be added in mapped superclasses, since for those the database foreign key can be placed on the table(s) of the inheriting entities (and there may be more than one child class).

Neither the decision to update the `sourceEntity` nor the validation check should be based on `$parent->isMappedSuperclass`.

This only works in the simple case where the class hierarchy is `Mapped Superclass → Entity`.

The check is wrong when we have an inheritance hierarchy set up and the class hierarchy is `Base Entity → Mapped Superclass → Child Entity`.

Bug 1: The association should keep the root entity as the source. After all, in a JTI, the root table will contain the foreign key, and we need to base joins on that table when traversing `FROM LeafClass l JOIN l.target`.

Bug 2: Do not reject the to-many association declared in the base class. It is ok to have the reverse (owning) side point back to the base entity, as it would be if there were no mapped superclasses at all. The mapped superclass does not declare, add or otherwise interfere with the to-many association at all.

Base the decision to change the `sourceEntity` on `$mapping['inherited']` being set. This field points to the topmost _parent entity_ class in the ancestry tree where the relationship mapping appears for the first time.

When it is not set, the current class is the first _entity_ class in the hierarchy with that association. Since we are inheriting the relation, it must have been added in a mapped superclass above, but was not yet present in the nearest parent entity class.

In that case, it may only be a to-one association and the source entity needs to be updated.

(See doctrine#10396 for a clarification of the semantics of `inherited`.)

Here is a simplified example of the class hierarchy.

See the two tests added for more details – one is for checking the correct usage of a to-one association against/with the base class in JTI. The other is to test that a to-many association on the base class is not rejected.

I am sure that there are other tests that (still) cover the update of `sourceEntity` is happening.

```php
/**
 * @entity
 */
class AssociationTarget
{
    /**
     * @column(type="integer")
     * @id
     * @GeneratedValue
     */
    public $id;
}

/**
 * @entity
 * @InheritanceType("JOINED")
 * @DiscriminatorColumn(name="discriminator", type="string")
 * @DiscriminatorMap({"1" = "BaseClass", "2" = "LeafClass"})
 */
class BaseClass
{
    /**
     * @column(type="integer")
     * @id
     * @GeneratedValue
     */
    public $id;

    /**
     * @manytoone(targetEntity="AssociationTarget")
     */
    public $target;
}

/**
 * @MappedSuperclass
 */
class MediumSuperclass extends BaseClass
{
}

/**
 * @entity
 */
class LeafClass extends MediumSuperclass
{
}
```

When querying `FROM LeafClass l`, it should be possible to `JOIN l.target`. This currently leads to an SQL error because the SQL join will be made via `LeafClass.target_id` instead of `BaseClass.target_id`. `LeafClass` is considered the `sourceEntity` for the association – which is wrong–, and so the foreign key field is expected to be in the `LeafClass` table (using JTI here).

Fixes doctrine#5998, fixes doctrine#7825.

I have removed the abstract entity class, since it is not relevant for the issue and took the discussion off course. Also, the discriminator map now contains all classes.

Added the second variant of the bug, namely that a to-many association would wrongly be rejected in the same situation.
  • Loading branch information
mpdude committed Jan 17, 2023
1 parent d68baef commit ed87b18
Show file tree
Hide file tree
Showing 4 changed files with 477 additions and 9 deletions.
23 changes: 14 additions & 9 deletions lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -384,15 +384,6 @@ private function addInheritedFields(ClassMetadata $subClass, ClassMetadata $pare
private function addInheritedRelations(ClassMetadata $subClass, ClassMetadata $parentClass): void
{
foreach ($parentClass->associationMappings as $field => $mapping) {
if ($parentClass->isMappedSuperclass) {
if ($mapping['type'] & ClassMetadata::TO_MANY && ! $mapping['isOwningSide']) {
throw MappingException::illegalToManyAssociationOnMappedSuperclass($parentClass->name, $field);
}

$mapping['sourceEntity'] = $subClass->name;
}

//$subclassMapping = $mapping;
if (! isset($mapping['inherited']) && ! $parentClass->isMappedSuperclass) {
$mapping['inherited'] = $parentClass->name;
}
Expand All @@ -401,6 +392,20 @@ private function addInheritedRelations(ClassMetadata $subClass, ClassMetadata $p
$mapping['declared'] = $parentClass->name;
}

// When the class inheriting the relation ($subClass) is the first entity class since the
// relation has been defined in a mapped superclass (or in a chain
// of mapped superclasses) above, then declare this current entity class as the source of
// the relationship.
// According to the definitions given in https://github.com/doctrine/orm/pull/10396/,
// this is the case <=> ! isset($mapping['inherited']).
if (! isset($mapping['inherited'])) {
if ($mapping['type'] & ClassMetadata::TO_MANY && ! $mapping['isOwningSide']) {
throw MappingException::illegalToManyAssociationOnMappedSuperclass($parentClass->name, $field);
}

$mapping['sourceEntity'] = $subClass->name;
}

$subClass->addInheritedAssociationMapping($mapping);
}
}
Expand Down
255 changes: 255 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/Ticket/GH5998Test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,255 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional\Ticket;

use Doctrine\DBAL\LockMode;
use Doctrine\ORM\Mapping as ORM;
use Doctrine\Tests\OrmFunctionalTestCase;

/**
* @group GH-5998
*/
class GH5998Test extends OrmFunctionalTestCase
{
protected function setUp(): void
{
parent::setUp();

$this->_schemaTool->createSchema([
$this->_em->getClassMetadata(GH5998JTI::class),
$this->_em->getClassMetadata(GH5998JTIChild::class),
$this->_em->getClassMetadata(GH5998STI::class),
$this->_em->getClassMetadata(GH5998Basic::class),
$this->_em->getClassMetadata(GH5998Related::class),
]);
}

/**
* Verifies that MappedSuperclasses work within an inheritance hierarchy.
*/
public function testIssue(): void
{
// Test JTI
$this->classTests(GH5998JTIChild::class);
// Test STI
$this->classTests(GH5998STIChild::class);
// Test Basic
$this->classTests(GH5998Basic::class);
}

private function classTests($className): void
{
// Test insert
$child = new $className('Sam', 0, 1);
$child->rel = new GH5998Related();
$this->_em->persist($child);
$this->_em->persist($child->rel);
$this->_em->flush();
$this->_em->clear();

// Test find by rel
$child = $this->_em->getRepository($className)->findOneBy(['rel' => $child->rel]);
self::assertNotNull($child);
$this->_em->clear();

// Test query by id with fetch join
$child = $this->_em->createQuery('SELECT t, r FROM ' . $className . ' t JOIN t.rel r WHERE t.id = 1')->getOneOrNullResult();
self::assertNotNull($child);

// Test lock and update
$this->_em->transactional(static function ($em) use ($child): void {
$em->lock($child, LockMode::NONE);
$child->firstName = 'Bob';
$child->status = 0;
});
$this->_em->clear();
$child = $this->_em->getRepository($className)->find(1);
self::assertEquals($child->firstName, 'Bob');
self::assertEquals($child->status, 0);

// Test delete
$this->_em->remove($child);
$this->_em->flush();
$child = $this->_em->getRepository($className)->find(1);
self::assertNull($child);
}
}

/**
* @ORM\MappedSuperclass
*/
class GH5998Common
{
/**
* @ORM\Id
* @ORM\Column(type="integer")
* @ORM\GeneratedValue
*
* @var int
*/
public $id;
/**
* @ORM\ManyToOne(targetEntity=GH5998Related::class)
* @ORM\JoinColumn(name="related_id", referencedColumnName="id")
*
* @var GH5998Related
*/
public $rel;
/**
* @ORM\Version
* @ORM\Column(type="integer")
*
* @var int
*/
public $version;

/** @var mixed */
public $other;
}

/**
* @ORM\Entity
* @ORM\InheritanceType("JOINED")
* @ORM\DiscriminatorMap({"child" = GH5998JTIChild::class})
*/
abstract class GH5998JTI extends GH5998Common
{
/**
* @ORM\Column(type="string", length=255)
*
* @var string
*/
public $firstName;
}

/**
* @ORM\MappedSuperclass
*/
class GH5998JTICommon extends GH5998JTI
{
/**
* @ORM\Column(type="integer")
*
* @var int
*/
public $status;
}

/**
* @ORM\Entity
*/
class GH5998JTIChild extends GH5998JTICommon
{
/**
* @ORM\Column(type="integer")
*
* @var int
*/
public $type;

public function __construct(string $firstName, int $type, int $status)
{
$this->firstName = $firstName;
$this->type = $type;
$this->status = $status;
}
}

/**
* @ORM\Entity
* @ORM\InheritanceType("SINGLE_TABLE")
* @ORM\DiscriminatorMap({"child" = GH5998STIChild::class})
*/
abstract class GH5998STI extends GH5998Common
{
/**
* @ORM\Column(type="string", length=255)
*
* @var string
*/
public $firstName;
}

/**
* @ORM\MappedSuperclass
*/
class GH5998STICommon extends GH5998STI
{
/**
* @ORM\Column(type="integer")
*
* @var int
*/
public $status;
}

/**
* @ORM\Entity
*/
class GH5998STIChild extends GH5998STICommon
{
/**
* @ORM\Column(type="integer")
*
* @var int
*/
public $type;

public function __construct(string $firstName, int $type, int $status)
{
$this->firstName = $firstName;
$this->type = $type;
$this->status = $status;
}
}

/**
* @ORM\Entity
*/
class GH5998Basic extends GH5998Common
{
/**
* @ORM\Column(type="string", length=255)
*
* @var string
*/
public $firstName;

/**
* @ORM\Column(type="integer")
*
* @var int
*/
public $status;

/**
* @ORM\Column(type="integer")
*
* @var int
*/
public $type;

public function __construct(string $firstName, int $type, int $status)
{
$this->firstName = $firstName;
$this->type = $type;
$this->status = $status;
}
}

/**
* @ORM\Entity()
*/
class GH5998Related
{
/**
* @ORM\Id
* @ORM\Column(type="integer")
* @ORM\GeneratedValue
*
* @var int
*/
public $id;
}
Loading

0 comments on commit ed87b18

Please sign in to comment.