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

Detect invalid indexBy configuration in the SchemaValidator #11610

Draft
wants to merge 4 commits into
base: 3.2.x
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/Tools/SchemaValidator.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
use ReflectionEnum;
use ReflectionNamedType;

use function array_column;
use function array_diff;
use function array_filter;
use function array_key_exists;
Expand Down Expand Up @@ -276,6 +277,22 @@
}
}
}

if ($assoc->isIndexed()) {
$joinColumns = array_column($targetMetadata->getAssociationMappings(), 'joinColumns');
$joinColumns = array_column($joinColumns, 0);
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it take all join columns into account instead ?

Copy link
Author

Choose a reason for hiding this comment

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

I guess it should. I didn't realise/think there could be multiple join columns. Thanks.

$joinColumns = array_column($joinColumns, 'name');

Comment on lines +284 to +285
Copy link
Member

Choose a reason for hiding this comment

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

I suggest using a foreach loop instead of several successive array_columns, allowing to loop once

Copy link
Author

@janklan janklan Sep 25, 2024

Choose a reason for hiding this comment

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

I'm not sure I follow the allowing to loop once: $targetMetadata->getAssociationMappings() returns a multi-dimensional array. I'll have to have [probably three] nested foreach loops to gather all the data anyway.

$allAvailableIndexNames = [
...$joinColumns,
...$targetMetadata->getColumnNames(),
];

if (! in_array($assoc->indexBy(), $allAvailableIndexNames, true)) {

Check failure on line 291 in src/Tools/SchemaValidator.php

View workflow job for this annotation

GitHub Actions / Static Analysis with Psalm (default)

PossiblyUndefinedMethod

src/Tools/SchemaValidator.php:291:40: PossiblyUndefinedMethod: Method (Doctrine\ORM\Mapping\InverseSideMapping&Doctrine\ORM\Mapping\OneToOneAssociationMapping&Doctrine\ORM\Mapping\ManyToOneAssociationMapping)::indexBy does not exist (see https://psalm.dev/108)

Check failure on line 291 in src/Tools/SchemaValidator.php

View workflow job for this annotation

GitHub Actions / Static Analysis with Psalm (3.8.2)

PossiblyUndefinedMethod

src/Tools/SchemaValidator.php:291:40: PossiblyUndefinedMethod: Method (Doctrine\ORM\Mapping\InverseSideMapping&Doctrine\ORM\Mapping\OneToOneAssociationMapping&Doctrine\ORM\Mapping\ManyToOneAssociationMapping)::indexBy does not exist (see https://psalm.dev/108)
$ce[] = 'The association ' . $class->name . '#' . $fieldName . ' is indexed by a field ' .
$assoc->indexBy() . ' on ' . $targetMetadata->name . ', but the field doesn\'t exist.';

Check failure on line 293 in src/Tools/SchemaValidator.php

View workflow job for this annotation

GitHub Actions / Static Analysis with Psalm (default)

PossiblyUndefinedMethod

src/Tools/SchemaValidator.php:293:33: PossiblyUndefinedMethod: Method (Doctrine\ORM\Mapping\InverseSideMapping&Doctrine\ORM\Mapping\OneToOneAssociationMapping&Doctrine\ORM\Mapping\ManyToOneAssociationMapping)::indexBy does not exist (see https://psalm.dev/108)

Check failure on line 293 in src/Tools/SchemaValidator.php

View workflow job for this annotation

GitHub Actions / Static Analysis with Psalm (3.8.2)

PossiblyUndefinedMethod

src/Tools/SchemaValidator.php:293:33: PossiblyUndefinedMethod: Method (Doctrine\ORM\Mapping\InverseSideMapping&Doctrine\ORM\Mapping\OneToOneAssociationMapping&Doctrine\ORM\Mapping\ManyToOneAssociationMapping)::indexBy does not exist (see https://psalm.dev/108)
}
}
}

if (
Expand Down
13 changes: 13 additions & 0 deletions tests/Tests/ORM/Functional/SchemaValidatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use Doctrine\Tests\DbalTypes\CustomIdObjectType;
use Doctrine\Tests\DbalTypes\NegativeToPositiveType;
use Doctrine\Tests\DbalTypes\UpperCaseStringType;
use Doctrine\Tests\ORM\Functional\Ticket\GH11608\GH11608Test;
use Doctrine\Tests\OrmFunctionalTestCase;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\Attributes\Group;
Expand Down Expand Up @@ -52,6 +53,18 @@ public static function dataValidateModelSets(): array
$modelSets = [];

foreach (array_keys(self::$modelSets) as $modelSet) {

/**
* GH11608 Tests whether the Schema validator picks up on invalid mapping. The entities are intentionally
* invalid, and so for the purpose of this test case, those entities should be ignored.
Copy link
Member

Choose a reason for hiding this comment

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

Then I'd avoid creating a model set entirely. There are plenty of tests that don't.

*
* @see GH11608Test
* @see https://github.com/doctrine/orm/issues/11608
*/
if ($modelSet === GH11608Test::class) {
continue;
}

$modelSets[$modelSet] = [$modelSet];
}

Expand Down
44 changes: 44 additions & 0 deletions tests/Tests/ORM/Functional/Ticket/GH11608/ConnectingEntity.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional\Ticket\GH11608;

use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Doctrine\ORM\Mapping\JoinColumn;
use Doctrine\ORM\Mapping\ManyToOne;

#[Entity]
class ConnectingEntity
{
#[Id]
#[Column(type: 'integer')]
#[GeneratedValue]
public int $id;

#[ManyToOne(inversedBy: 'validRightSideEntities')]
#[JoinColumn(nullable: false)]
public LeftSideEntity $validLeftSideMappedByAssociation;

#[ManyToOne(inversedBy: 'invalidRightSideEntities')]
#[JoinColumn(nullable: false)]
public LeftSideEntity $invalidLeftSideMappedByAssociation;

#[ManyToOne(inversedBy: 'validConnections')]
#[JoinColumn(nullable: false)]
public LeftSideEntity $validLeftSideMappedByColumn;

#[ManyToOne(inversedBy: 'invalidConnections')]
#[JoinColumn(nullable: false)]
public LeftSideEntity $invalidLeftSideMappedByColumn;

#[ManyToOne(inversedBy: 'leftSideEntities')]
#[JoinColumn(name: 'right_side_id', nullable: false)]
public RightSideEntity $rightSide;

#[Column(type: 'text', nullable: false)]
public string $arbitraryValue;
}
52 changes: 52 additions & 0 deletions tests/Tests/ORM/Functional/Ticket/GH11608/GH11608Test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional\Ticket\GH11608;

use Doctrine\ORM\Mapping\UnderscoreNamingStrategy;
use Doctrine\ORM\Tools\SchemaValidator;
use Doctrine\Tests\OrmFunctionalTestCase;
use Generator;
use PHPUnit\Framework\Attributes\DataProvider;

use function var_dump;

Check failure on line 13 in tests/Tests/ORM/Functional/Ticket/GH11608/GH11608Test.php

View workflow job for this annotation

GitHub Actions / coding-standards / Coding Standards (8.3)

Type var_dump is not used in this file.

class GH11608Test extends OrmFunctionalTestCase
{
protected function setUp(): void
{
parent::setUp();

$this->useModelSet(self::class);
$this->_em->getConfiguration()->setNamingStrategy(new UnderscoreNamingStrategy());
}

public function testOneToManyIndexByErrorDetected(): void
{
$validator = new SchemaValidator($this->_em);

$errors = $validator->validateClass($this->_em->getClassMetadata(LeftSideEntity::class));

self::assertCount(2, $errors);

self::assertStringContainsStringIgnoringCase('invalidRightSideEntities is indexed by a field right_side_id_that_doesnt_exist', $errors[0]);
self::assertStringContainsStringIgnoringCase('invalidConnections is indexed by a field arbitrary_value_that_doesnt_exist', $errors[1]);
}

#[DataProvider('provideValidEntitiesCauseNoErrors')]
public function testValidEntitiesCauseNoErrors(string $className): void
{
$validator = new SchemaValidator($this->_em);

$errors = $validator->validateClass($this->_em->getClassMetadata($className));

self::assertEmpty($errors);
}

public static function provideValidEntitiesCauseNoErrors(): Generator
{
yield [ConnectingEntity::class];
yield [RightSideEntity::class];
}
}
40 changes: 40 additions & 0 deletions tests/Tests/ORM/Functional/Ticket/GH11608/LeftSideEntity.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional\Ticket\GH11608;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Doctrine\ORM\Mapping\OneToMany;

#[Entity]
class LeftSideEntity
{
#[Id]
#[Column(type: 'integer')]
#[GeneratedValue]
public int $id;

#[OneToMany(targetEntity: ConnectingEntity::class, mappedBy: 'validLeftSideMappedByAssociation', indexBy: 'right_side_id')]
public Collection $validRightSideEntities;

#[OneToMany(targetEntity: ConnectingEntity::class, mappedBy: 'invalidLeftSideMappedByAssociation', indexBy: 'right_side_id_that_doesnt_exist')]
public Collection $invalidRightSideEntities;

#[OneToMany(targetEntity: ConnectingEntity::class, mappedBy: 'validLeftSideMappedByColumn', indexBy: 'arbitrary_value')]
public Collection $validConnections;

#[OneToMany(targetEntity: ConnectingEntity::class, mappedBy: 'invalidLeftSideMappedByColumn', indexBy: 'arbitrary_value_that_doesnt_exist')]
public Collection $invalidConnections;

public function __construct()
{
$this->validRightSideEntities = new ArrayCollection();
$this->invalidRightSideEntities = new ArrayCollection();
}
}
30 changes: 30 additions & 0 deletions tests/Tests/ORM/Functional/Ticket/GH11608/RightSideEntity.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional\Ticket\GH11608;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Doctrine\ORM\Mapping\OneToMany;

#[Entity]
class RightSideEntity
{
#[Id]
#[Column(type: 'integer')]
#[GeneratedValue]
public int $id;

#[OneToMany(targetEntity: ConnectingEntity::class, mappedBy: 'rightSide', indexBy: 'id')]
public Collection $leftSideEntities;

public function __construct()
{
$this->leftSideEntities = new ArrayCollection();
}
}
6 changes: 6 additions & 0 deletions tests/Tests/OrmFunctionalTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@
use Doctrine\Tests\Models\ValueConversionType\OwningOneToOneEntity;
use Doctrine\Tests\Models\VersionedManyToOne\Article;
use Doctrine\Tests\Models\VersionedManyToOne\Category;
use Doctrine\Tests\ORM\Functional\Ticket\GH11608;
use Exception;
use PHPUnit\Framework\AssertionFailedError;
use PHPUnit\Framework\Constraint\Count;
Expand Down Expand Up @@ -483,6 +484,11 @@ abstract class OrmFunctionalTestCase extends OrmTestCase
Models\Issue9300\Issue9300Child::class,
Models\Issue9300\Issue9300Parent::class,
],
GH11608\GH11608Test::class => [
GH11608\LeftSideEntity::class,
GH11608\RightSideEntity::class,
GH11608\ConnectingEntity::class,
],
];

/** @param class-string ...$models */
Expand Down
Loading