Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for duplicating parent relation columns to child tables in JTI #100

Open
wants to merge 12 commits into
base: 4.x
Choose a base branch
from
41 changes: 40 additions & 1 deletion src/Configurator.php
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,20 @@ public function initFields(EntitySchema $entity, \ReflectionClass $class, string
}

$field = $this->initField($property->getName(), $column, $class, $columnPrefix);
$field->setEntityClass($property->getDeclaringClass()->getName());
$field->setEntityClass($this->findOwningEntity($class, $property->getDeclaringClass())->getName());
$entity->getFields()->set($property->getName(), $field);
}
}

public function initRelations(EntitySchema $entity, \ReflectionClass $class): void
{
foreach ($class->getProperties() as $property) {
// ignore properties declared by parent entities
// otherwise all the relation columns declared in parent would be duplicated across all child tables in JTI
if ($this->findOwningEntity($class, $property->getDeclaringClass())->getName() !== $class->getName()) {
continue;
}
Comment on lines +125 to +127
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added tests for this case and there is an issue when an entity extended from another entity without STI or JTI.

In thos case all the relations of the parent entity have to be cloned into the child class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is TableInheritance generator. It might be better to clean unnecessary relations there in cases STI or JTI

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review.

You are right, it is quite a nasty use case though. I will take a look at it in the evening,
It makes sense to clear the entity in the inheritance generator, I will try to move the logic there.

The column ownership use cases are alright?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, columns look great. Thank you.


$metadata = $this->getPropertyMetadata($property, RelationAnnotation\RelationInterface::class);

foreach ($metadata as $meta) {
Expand Down Expand Up @@ -413,4 +419,37 @@ private function isOnInsertGeneratedField(Field $field): bool
default => $field->isPrimary()
};
}

/**
* Function to find an owning entity class in the inheritance hierarchy.
*
* Entity classes may extend a base class and this function is needed route the properties from declaring class to the entity class.
* The function stops only when the declaring class is truly found, it does not naively stop on first entity.
* This behaviour makes it also functional in cases of Joined Table Inheritance on theoretically any number of nesting levels.
*/
private function findOwningEntity(\ReflectionClass $currentClass, \ReflectionClass $declaringClass): \ReflectionClass
{
// latest found entityClass before declaringClass
$latestEntityClass = $currentClass;

do {
// we found declaringClass in the hierarchy
// in most cases the execution will stop here in first loop
if ($currentClass->getName() === $declaringClass->getName()) {
return $latestEntityClass;
}

$currentClass = $currentClass->getParentClass();

// not possible to happen for logical reasons, but defensively check anyway
if (!$currentClass instanceof \ReflectionClass) {
return $latestEntityClass;
}

// if a currentClass in hierarchy is an entity on its own, the property belongs to that entity
if (\count($currentClass->getAttributes(Entity::class)) > 0) {
$latestEntityClass = $currentClass;
}
} while (true); // the inheritance hierarchy cannot be infinite
}
}
6 changes: 5 additions & 1 deletion src/TableInheritance.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,11 @@ public function run(Registry $registry): Registry

// All child should be presented in a schema as separated entity
// Every child will be handled according its table inheritance type
\assert($child->getRole() !== null && $entity !== null && isset($parent));
// todo should $parent be not null?
// \assert(isset($parent));

\assert($child->getRole() !== null && $entity !== null);

if (!$registry->hasEntity($child->getRole())) {
$registry->register($child);

Expand Down
9 changes: 9 additions & 0 deletions tests/Annotated/Fixtures/Fixtures16/Executive.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Cycle\Annotated\Annotation\Column;
use Cycle\Annotated\Annotation\Entity;
use Cycle\Annotated\Annotation\Inheritance\JoinedTable as InheritanceJoinedTable;
use Cycle\Annotated\Annotation\Relation\HasOne;

/**
* @Entity
Expand All @@ -21,4 +22,12 @@ class Executive extends ExecutiveProxy
/** @Column(type="int") */
#[Column(type: 'int')]
public int $bonus;

/** @Column(type="int", nullable=true, typecast="int") */
#[Column(type: 'int', nullable: true, typecast: 'int')]
public ?int $added_tool_id;

/** @HasOne(target=Tool::class, innerKey="added_tool_id", outerKey="added_tool_id", nullable=true) */
#[HasOne(target: Tool::class, innerKey: 'added_tool_id', outerKey: 'added_tool_id', nullable: true)]
public Tool $addedTool;
}
18 changes: 18 additions & 0 deletions tests/Annotated/Fixtures/Fixtures16/Executive2.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php

declare(strict_types=1);

namespace Cycle\Annotated\Tests\Fixtures\Fixtures16;

use Cycle\Annotated\Annotation\Entity;
use Cycle\Annotated\Annotation\Inheritance\JoinedTable as InheritanceJoinedTable;

/**
* @Entity
* @InheritanceJoinedTable(outerKey="foo_id")
*/
#[Entity]
#[InheritanceJoinedTable(outerKey: 'foo_id')]
class Executive2 extends Executive
{
}
5 changes: 2 additions & 3 deletions tests/Annotated/Fixtures/Fixtures16/ExecutiveProxy.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,11 @@
/**
* This proxy class doesn't have an {@see Entity} annotation (attribute) declaration,
* and it shouldn't be presented in Schema.
* Note: this behavior might be improved. There will be added support for
* annotated base class columns without Entity annotation declaration.
* But all the classes that extend this class should contain all the fields from this class.
*/
class ExecutiveProxy extends Employee
{
/** @Column(type="string") */
/** @Column(type="string", name="proxy") */
#[Column(type: 'string', name: 'proxy')]
public ?string $proxyFieldWithAnnotation = null;

Expand Down
9 changes: 9 additions & 0 deletions tests/Annotated/Fixtures/Fixtures16/Person.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Cycle\Annotated\Annotation\Column;
use Cycle\Annotated\Annotation\Entity;
use Cycle\Annotated\Annotation\Inheritance\DiscriminatorColumn;
use Cycle\Annotated\Annotation\Relation\HasOne;

/**
* @Entity
Expand All @@ -28,6 +29,14 @@ class Person
#[Column(type: 'string')]
public string $type;

/** @Column(type="int", nullable=true, typecast="int") */
#[Column(type: 'int', nullable: true, typecast: 'int')]
public ?int $tool_id;

/** @HasOne(target=Tool::class, innerKey="id", outerKey="tool_id", nullable=true) */
#[HasOne(target: Tool::class, innerKey: 'id', outerKey: 'tool_id', nullable: true)]
public Tool $tool;

public function getFooId(): int
{
return $this->foo_id;
Expand Down
22 changes: 22 additions & 0 deletions tests/Annotated/Fixtures/Fixtures16/Tool.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

declare(strict_types=1);

namespace Cycle\Annotated\Tests\Fixtures\Fixtures16;

use Cycle\Annotated\Annotation\Column;
use Cycle\Annotated\Annotation\Entity;
use Cycle\Annotated\Annotation\Inheritance\DiscriminatorColumn;

/**
* @Entity
* @DiscriminatorColumn(name="type")
*/
#[Entity]
#[DiscriminatorColumn(name: 'type')]
class Tool
{
/** @Column(type="primary", name="id") */
#[Column(type: 'primary', name: 'id')]
public int $tool_id;
}
6 changes: 3 additions & 3 deletions tests/Annotated/Functional/Driver/Common/BaseTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -115,14 +115,14 @@ public function getDriver(): Driver

public static function singularReadersProvider(): \Traversable
{
yield ['Annotation reader' => new AnnotationReader()];
yield ['Attribute reader' => new AttributeReader()];
yield 'Annotation reader' => [new AnnotationReader()];
yield 'Attribute reader' => [new AttributeReader()];
}

public static function allReadersProvider(): \Traversable
{
yield from static::singularReadersProvider();
yield ['Selective reader' => new SelectiveReader([new AttributeReader(), new AnnotationReader()])];
yield 'Selective reader' => [new SelectiveReader([new AttributeReader(), new AnnotationReader()])];
}

protected function getDatabase(): Database
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public function testTableInheritance(ReaderInterface $reader): void
$this->assertSame('secret', $loadedExecutive->hidden);
$this->assertSame(15000, $loadedExecutive->bonus);
$this->assertSame('executive', $loadedExecutive->getType());
$this->assertNull($loadedExecutive->proxyFieldWithAnnotation);
$this->assertSame('value', $loadedExecutive->proxyFieldWithAnnotation);
}

#[DataProvider('allReadersProvider')]
Expand Down
33 changes: 29 additions & 4 deletions tests/Annotated/Functional/Driver/Common/InheritanceTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Cycle\Annotated\Tests\Fixtures\Fixtures16\Employee;
use Cycle\Annotated\Tests\Fixtures\Fixtures16\Executive;
use Cycle\Annotated\Tests\Fixtures\Fixtures16\Person;
use Cycle\Annotated\Tests\Fixtures\Fixtures16\Tool;
use Cycle\ORM\SchemaInterface;
use Cycle\Schema\Compiler;
use Cycle\Schema\Generator\GenerateRelations;
Expand Down Expand Up @@ -69,6 +70,9 @@ public function testTableInheritance(ReaderInterface $reader): void
// Ceo - Single table inheritance {value: ceo}
// Beaver - Separate table

// Tool
$this->assertArrayHasKey('tool', $schema);

// Person
$this->assertCount(3, $schema['person'][SchemaInterface::CHILDREN]);
$this->assertEquals([
Expand All @@ -86,52 +90,72 @@ public function testTableInheritance(ReaderInterface $reader): void
// 'bonus' => 'bonus', // JTI
'preferences' => 'preferences',
'stocks' => 'stocks',
'tool_id' => 'tool_id',
// 'teethAmount' => 'teeth_amount', // Not child
], $schema['person'][SchemaInterface::COLUMNS]);
$this->assertEmpty($schema['person'][SchemaInterface::PARENT] ?? null);
$this->assertEmpty($schema['person'][SchemaInterface::PARENT_KEY] ?? null);
$this->assertSame('people', $schema['person'][SchemaInterface::TABLE]);
$this->assertCount(1, $schema['person'][SchemaInterface::RELATIONS]);

// Employee
$this->assertArrayHasKey('employee', $schema);
$this->assertCount(1, $schema['employee']);
$this->assertSame(Employee::class, $schema['employee'][SchemaInterface::ENTITY]);
$this->assertNull($schema['employee'][SchemaInterface::TABLE] ?? null);
$this->assertCount(0, $schema['employee'][SchemaInterface::RELATIONS] ?? []);

// Customer
$this->assertArrayHasKey('customer', $schema);
$this->assertCount(1, $schema['customer']);
$this->assertSame(Customer::class, $schema['customer'][SchemaInterface::ENTITY]);
$this->assertNull($schema['customer'][SchemaInterface::TABLE] ?? null);
$this->assertCount(0, $schema['customer'][SchemaInterface::RELATIONS] ?? []);

// Executive
$this->assertSame('employee', $schema['executive'][SchemaInterface::PARENT]);
$this->assertSame('foo_id', $schema['executive'][SchemaInterface::PARENT_KEY]);
$this->assertSame('executives', $schema['executive'][SchemaInterface::TABLE]);
$this->assertSame(
['bonus' => 'bonus', 'foo_id' => 'id', 'hidden' => 'hidden'],
$schema['executive'][SchemaInterface::COLUMNS]
$this->assertEquals(
[
'bonus' => 'bonus',
'proxyFieldWithAnnotation' => 'proxy',
'foo_id' => 'id',
'hidden' => 'hidden',
'added_tool_id' => 'added_tool_id',
],
$schema['executive'][SchemaInterface::COLUMNS],
);
$this->assertCount(1, $schema['executive'][SchemaInterface::RELATIONS]);

// Executive2
$this->assertSame('executive', $schema['executive2'][SchemaInterface::PARENT]);
$this->assertSame('foo_id', $schema['executive2'][SchemaInterface::PARENT_KEY]);
$this->assertEquals(['foo_id' => 'id'], $schema['executive2'][SchemaInterface::COLUMNS]);
$this->assertCount(0, $schema['executive2'][SchemaInterface::RELATIONS]);

// Ceo
$this->assertArrayHasKey('ceo', $schema);
$this->assertCount(1, $schema['ceo']);
$this->assertSame(Ceo::class, $schema['ceo'][SchemaInterface::ENTITY]);
$this->assertNull($schema['ceo'][SchemaInterface::TABLE] ?? null);
$this->assertCount(0, $schema['ceo'][SchemaInterface::RELATIONS] ?? []);

// Beaver
$this->assertEmpty($schema['beaver'][SchemaInterface::DISCRIMINATOR] ?? null);
$this->assertEmpty($schema['beaver'][SchemaInterface::PARENT] ?? null);
$this->assertEmpty($schema['beaver'][SchemaInterface::PARENT_KEY] ?? null);
$this->assertEmpty($schema['beaver'][SchemaInterface::CHILDREN] ?? null);
$this->assertSame('beavers', $schema['beaver'][SchemaInterface::TABLE]);
$this->assertSame([
$this->assertEquals([
'teethAmount' => 'teeth_amount',
'foo_id' => 'id',
'name' => 'name',
'type' => 'type',
'hidden' => 'hidden',
'tool_id' => 'tool_id',
], $schema['beaver'][SchemaInterface::COLUMNS]);
$this->assertCount(1, $schema['beaver'][SchemaInterface::RELATIONS] ?? []);
}

public function testTableInheritanceWithIncorrectClassesOrder(): void
Expand All @@ -145,6 +169,7 @@ public function testTableInheritanceWithIncorrectClassesOrder(): void
new \ReflectionClass(Employee::class),
new \ReflectionClass(Executive::class),
new \ReflectionClass(Person::class),
new \ReflectionClass(Tool::class),
]);

$schema = (new Compiler())->compile($r, [
Expand Down
2 changes: 1 addition & 1 deletion tests/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ services:
ACCEPT_EULA: "Y"

mysql_latest:
image: mysql:latest
image: mysql:8.0
restart: always
command: --default-authentication-plugin=mysql_native_password
ports:
Expand Down
Loading