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

Adds metadata field type validation against Entity property type #10946

Merged
merged 1 commit into from
Sep 23, 2023
Merged
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
107 changes: 107 additions & 0 deletions lib/Doctrine/ORM/Tools/SchemaValidator.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,73 @@

namespace Doctrine\ORM\Tools;

use Doctrine\DBAL\Types\AsciiStringType;
use Doctrine\DBAL\Types\BigIntType;
use Doctrine\DBAL\Types\BooleanType;
use Doctrine\DBAL\Types\DecimalType;
use Doctrine\DBAL\Types\FloatType;
use Doctrine\DBAL\Types\GuidType;
use Doctrine\DBAL\Types\IntegerType;
use Doctrine\DBAL\Types\JsonType;
use Doctrine\DBAL\Types\SimpleArrayType;
use Doctrine\DBAL\Types\SmallIntType;
use Doctrine\DBAL\Types\StringType;
use Doctrine\DBAL\Types\TextType;
use Doctrine\DBAL\Types\Type;
use Doctrine\Deprecations\Deprecation;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\Mapping\ClassMetadataInfo;
use ReflectionNamedType;

use function array_diff;
use function array_filter;
use function array_key_exists;
use function array_map;
use function array_push;
use function array_search;
use function array_values;
use function assert;
use function class_exists;
use function class_parents;
use function count;
use function get_class;
use function implode;
use function in_array;
use function sprintf;

use const PHP_VERSION_ID;

/**
* Performs strict validation of the mapping schema
*
* @link www.doctrine-project.com
*
* @psalm-import-type FieldMapping from ClassMetadata
*/
class SchemaValidator
{
/** @var EntityManagerInterface */
private $em;

/**
* It maps built-in Doctrine types to PHP types
*/
private const BUILTIN_TYPES_MAP = [
AsciiStringType::class => 'string',
BigIntType::class => 'string',
BooleanType::class => 'bool',
DecimalType::class => 'string',
FloatType::class => 'float',
GuidType::class => 'string',
IntegerType::class => 'int',
JsonType::class => 'array',
SimpleArrayType::class => 'array',
SmallIntType::class => 'int',
StringType::class => 'string',
TextType::class => 'string',
];
Copy link
Member Author

Choose a reason for hiding this comment

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

IMO we should get rid of this harcoded map when DBAL v4 is out and switch to reflection on the DBAL type instead. The signatures will be typed.

Copy link
Member

Choose a reason for hiding this comment

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

This list is intentional though: instanceof is NOT sufficient, because subclassing allows for types to be customized in userland.

If doctrine/dbal:^4 has final on all types, then we can use instanceof

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I understand what you mean now: convertToPhpType(): T, so we look at T?

Copy link
Member Author

@greg0ire greg0ire Sep 15, 2023

Choose a reason for hiding this comment

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

Yes. It could go in a property that is wider than T, or even narrower but that's kind of dangerous I'd say. At the very least, the intersection of both types must not be empty.


public function __construct(EntityManagerInterface $em)
{
$this->em = $em;
Expand Down Expand Up @@ -92,6 +132,11 @@
}
}

// PHP 7.4 introduces the ability to type properties, so we can't validate them in previous versions
if (PHP_VERSION_ID >= 70400) {
array_push($ce, ...$this->validatePropertiesTypes($class));
}

if ($class->isEmbeddedClass && count($class->associationMappings) > 0) {
$ce[] = "Embeddable '" . $class->name . "' does not support associations";

Expand Down Expand Up @@ -304,4 +349,66 @@

return $schemaTool->getUpdateSchemaSql($allMetadata, true);
}

/** @return list<string> containing the found issues */
private function validatePropertiesTypes(ClassMetadataInfo $class): array
Copy link
Member Author

Choose a reason for hiding this comment

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

We have no choice but to use ClassMetadataInfo here for now because of the calling code, and that should be fine, although I considered using if (PHP_VERSION_ID >= 70400 && $class instanceof ClassMetadata) {… that would be a bit weird.

Copy link
Member

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 understand this comment. Is this because of a possible different Persistence version and validateClass(ClassMetadataInfo $class)?

Copy link
Member Author

@greg0ire greg0ire Sep 18, 2023

Choose a reason for hiding this comment

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

I'm saying that in the unique call site of this method, we have no guarantee that we have a CM and not just a CMI (so yes, it does have to do with validateClass(ClassMetadataInfo $class). This explains why we use this type declaration here.

{
return array_values(
array_filter(
array_map(
/** @param FieldMapping $fieldMapping */
function (array $fieldMapping) use ($class): ?string {
$fieldName = $fieldMapping['fieldName'];
assert(isset($class->reflFields[$fieldName]));
$propertyType = $class->reflFields[$fieldName]->getType();

// If the field type is not a built-in type, we cannot check it
if (! Type::hasType($fieldMapping['type'])) {
return null;

Check warning on line 367 in lib/Doctrine/ORM/Tools/SchemaValidator.php

View check run for this annotation

Codecov / codecov/patch

lib/Doctrine/ORM/Tools/SchemaValidator.php#L367

Added line #L367 was not covered by tests
}

// If the property type is not a named type, we cannot check it
if (! ($propertyType instanceof ReflectionNamedType)) {
return null;
}

$metadataFieldType = $this->findBuiltInType(Type::getType($fieldMapping['type']));

//If the metadata field type is not a mapped built-in type, we cannot check it
if ($metadataFieldType === null) {
return null;

Check warning on line 379 in lib/Doctrine/ORM/Tools/SchemaValidator.php

View check run for this annotation

Codecov / codecov/patch

lib/Doctrine/ORM/Tools/SchemaValidator.php#L379

Added line #L379 was not covered by tests
}

$propertyType = $propertyType->getName();

// If the property type is the same as the metadata field type, we are ok
if ($propertyType === $metadataFieldType) {
return null;

Check warning on line 386 in lib/Doctrine/ORM/Tools/SchemaValidator.php

View check run for this annotation

Codecov / codecov/patch

lib/Doctrine/ORM/Tools/SchemaValidator.php#L386

Added line #L386 was not covered by tests
}

return sprintf(
"The field '%s#%s' has the property type '%s' that differs from the metadata field type '%s' returned by the '%s' DBAL type.",
$class->name,
$fieldName,
$propertyType,
$metadataFieldType,
$fieldMapping['type']
);
},
$class->fieldMappings
)
)
);
}

/**
* The exact DBAL type must be used (no subclasses), since consumers of doctrine/orm may have their own
* customization around field types.
*/
private function findBuiltInType(Type $type): ?string
{
$typeName = get_class($type);

return self::BUILTIN_TYPES_MAP[$typeName] ?? null;
}
}
53 changes: 53 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/Ticket/GH10661/GH10661Test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?php

declare(strict_types=1);

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

use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Tools\SchemaValidator;
use Doctrine\Tests\OrmTestCase;

/**
* @requires PHP >= 7.4
*/
final class GH10661Test extends OrmTestCase
{
/** @var EntityManagerInterface */
private $em;

/** @var SchemaValidator */
private $validator;

protected function setUp(): void
{
$this->em = $this->getTestEntityManager();
$this->validator = new SchemaValidator($this->em);
}

public function testMetadataFieldTypeNotCoherentWithEntityPropertyType(): void
{
$class = $this->em->getClassMetadata(InvalidEntity::class);
$ce = $this->validator->validateClass($class);

self::assertEquals(
["The field 'Doctrine\Tests\ORM\Functional\Ticket\GH10661\InvalidEntity#property1' has the property type 'float' that differs from the metadata field type 'string' returned by the 'decimal' DBAL type."],
Copy link
Member Author

@greg0ire greg0ire Sep 15, 2023

Choose a reason for hiding this comment

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

I added the "returned by the 'decimal' DBAL type" part, so that people understand where we get the second type from.

$ce
);
}

public function testMetadataFieldTypeNotCoherentWithEntityPropertyTypeWithInheritance(): void
{
$class = $this->em->getClassMetadata(InvalidChildEntity::class);
$ce = $this->validator->validateClass($class);

self::assertEquals(
[
"The field 'Doctrine\Tests\ORM\Functional\Ticket\GH10661\InvalidChildEntity#property1' has the property type 'float' that differs from the metadata field type 'string' returned by the 'decimal' DBAL type.",
"The field 'Doctrine\Tests\ORM\Functional\Ticket\GH10661\InvalidChildEntity#property2' has the property type 'int' that differs from the metadata field type 'string' returned by the 'string' DBAL type.",
"The field 'Doctrine\Tests\ORM\Functional\Ticket\GH10661\InvalidChildEntity#anotherProperty' has the property type 'string' that differs from the metadata field type 'bool' returned by the 'boolean' DBAL type.",
],
$ce
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

declare(strict_types=1);

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

use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;

/** @Entity */
class InvalidChildEntity extends InvalidEntity
{
/** @Column(type="string") */
protected int $property2;

/**
* @Column(type="boolean")
*/
private string $anotherProperty;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

declare(strict_types=1);

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

use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\Id;

/** @Entity */
class InvalidEntity
{
/**
* @var int
* @Id
* @Column
*/
protected $key;

/**
* @Column(type="decimal")
*/
protected float $property1;
Copy link
Member Author

Choose a reason for hiding this comment

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

I switched to decimal vs float here because I think it is a more common mistake, and it was the one I had in mind so I wanted to see how it looked for that case.

}