diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 5c70faa..a7adce9 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -31,7 +31,7 @@ jobs: - name: Upgrade analysis tools to latest if: ${{ matrix.dependency-versions == 'lowest' }} - run: composer update phpstan/phpstan rector/rector phpunit/phpunit squizlabs/php_codesniffer --with-all-dependencies --no-interaction + run: composer update phpstan/phpstan phpstan/phpstan-phpunit shipmonk/phpstan-rules rector/rector assoconnect/php-quality-config phpunit/phpunit squizlabs/php_codesniffer --with-all-dependencies --no-interaction - run: vendor/bin/phpcs if: ${{ failure() || success() }} diff --git a/src/Validator/Constraints/EntityValidator.php b/src/Validator/Constraints/EntityValidator.php index 86bbff1..6671b46 100644 --- a/src/Validator/Constraints/EntityValidator.php +++ b/src/Validator/Constraints/EntityValidator.php @@ -9,6 +9,7 @@ use Doctrine\ORM\EntityManagerInterface; use Symfony\Component\PropertyAccess\Exception\UnexpectedTypeException; use Symfony\Component\PropertyAccess\PropertyAccess; +use Symfony\Component\PropertyAccess\PropertyAccessorInterface; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\Constraints\All; use Symfony\Component\Validator\Constraints\NotNull; @@ -22,6 +23,11 @@ class EntityValidator extends ConstraintValidator private EntityManagerInterface $em; /** @var FieldConstraintsSetProviderInterface[] */ private iterable $fieldConstraintsSetFactories; + private ?PropertyAccessorInterface $propertyAccessor = null; + /** @var array>> */ + private array $constraintsCache = []; + /** @var array> */ + private array $onlyValidateOnUpdateCache = []; /** * @param FieldConstraintsSetProviderInterface[] $fieldConstraintsSetFactories @@ -48,7 +54,7 @@ public function validate(mixed $entity, Constraint $constraint): void $metadata = $this->em->getClassMetadata($class); $fields = array_keys(self::reflectionPropertiesToArray($metadata->getReflectionProperties())); $validator = $this->context->getValidator()->inContext($this->context); - $propertyAccessor = PropertyAccess::createPropertyAccessor(); + $propertyAccessor = $this->propertyAccessor ??= PropertyAccess::createPropertyAccessor(); foreach ($fields as $field) { if (!$this->checkIfFieldNeedsToBeValidated($entity, $field)) { @@ -89,9 +95,20 @@ public function getConstraintsForType(array $fieldMapping): array } /** + * Constraints only depend on the Doctrine mapping, so they are memoized per class and field. + * Sharing the constraint instances is safe: constraints are immutable value objects. + * * @return array */ public function getConstraints(string $class, string $field): array + { + return $this->constraintsCache[$class][$field] ??= $this->buildConstraints($class, $field); + } + + /** + * @return array + */ + private function buildConstraints(string $class, string $field): array { $metadata = $this->em->getClassMetadata($class); @@ -148,15 +165,32 @@ private static function reflectionPropertiesToArray(iterable $properties): array private function checkIfFieldNeedsToBeValidated(object $entity, string $field): bool { - $reflectionClass = new \ReflectionClass($entity::class); - $fieldAttributes = $this->getFieldAttributes($reflectionClass, $field); + $class = $entity::class; + $hasOnlyValidateOnUpdate = $this->onlyValidateOnUpdateCache[$class][$field] + ??= $this->hasOnlyValidateOnUpdateAttribute($class, $field); + + if ($hasOnlyValidateOnUpdate) { + return in_array($field, array_keys($this->em->getUnitOfWork()->getEntityChangeSet($entity)), true); + } + return true; + } + + /** + * Whether the field carries the OnlyValidateOnUpdate attribute only depends on the class + * definition, so it is memoized per class and field to avoid repeated reflection. + * + * @param class-string $class + */ + private function hasOnlyValidateOnUpdateAttribute(string $class, string $field): bool + { + $fieldAttributes = $this->getFieldAttributes(new \ReflectionClass($class), $field); foreach ($fieldAttributes as $attribute) { if (OnlyValidateOnUpdate::class === $attribute->getName()) { - return in_array($field, array_keys($this->em->getUnitOfWork()->getEntityChangeSet($entity)), true); + return true; } } - return true; + return false; } /** diff --git a/tests/Validator/Constraints/EntityValidatorTest.php b/tests/Validator/Constraints/EntityValidatorTest.php index 1150226..c5cd1af 100644 --- a/tests/Validator/Constraints/EntityValidatorTest.php +++ b/tests/Validator/Constraints/EntityValidatorTest.php @@ -9,6 +9,7 @@ use AssoConnect\ValidatorBundle\Test\Functional\App\Entity\MyEntityParent; use AssoConnect\ValidatorBundle\Validator\Constraints\Entity; use AssoConnect\ValidatorBundle\Validator\Constraints\EntityValidator; +use AssoConnect\ValidatorBundle\Validator\Constraints\OnlyValidateOnUpdate; use AssoConnect\ValidatorBundle\Validator\Constraints\Phone; use AssoConnect\ValidatorBundle\Validator\ConstraintsSetProvider\Field\PhoneProvider; use Doctrine\ORM\EntityManagerInterface; @@ -18,6 +19,8 @@ use Doctrine\ORM\Mapping\ManyToManyOwningSideMapping; use Doctrine\ORM\Mapping\ManyToOneAssociationMapping; use Doctrine\ORM\Mapping\OneToManyAssociationMapping; +use Doctrine\ORM\UnitOfWork; +use PHPUnit\Framework\MockObject\MockObject; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\Constraints\All; use Symfony\Component\Validator\Constraints\NotNull; @@ -30,7 +33,7 @@ */ class EntityValidatorTest extends ConstraintValidatorTestCase { - protected EntityManagerInterface $em; + protected EntityManagerInterface&MockObject $em; /** @var ClassMetadata */ protected ClassMetadata $classMetadata; @@ -94,6 +97,14 @@ public function testGetConstraintsForNotNullableField(): void ); } + public function testGetConstraintsIsMemoizedPerClassAndField(): void + { + $constraints = $this->validator->getConstraints('class', 'notnullable'); + + self::assertSame($constraints, $this->validator->getConstraints('class', 'notnullable')); + self::assertNotSame($constraints, $this->validator->getConstraints('class', 'nullable')); + } + public function testGetConstraintsForEmbeddable(): void { self::assertArrayContainsSameObjects( @@ -164,6 +175,172 @@ public function __construct() $this->validator->validate($entity, new Entity()); } + public function testValidateAppliesTheSameConstraintsToEveryInstanceOfAClass(): void + { + $createEntity = static fn(): object => new class { + public readonly ?string $nullable; + + public function __construct() + { + $this->nullable = '+33611223344'; + } + }; + $first = $createEntity(); + $second = $createEntity(); + $this->classMetadata->reflFields = [ + 'nullable' => new \ReflectionProperty($first, 'nullable'), + ]; + + $this->expectValidateValueAt(0, 'nullable', '+33611223344', [new Phone()]); + $this->expectValidateValueAt(1, 'nullable', '+33611223344', [new Phone()]); + + $this->validator->validate($first, new Entity()); + $this->validator->validate($second, new Entity()); + + self::assertSame( + $this->validator->getConstraints($first::class, 'nullable'), + $this->validator->getConstraints($second::class, 'nullable') + ); + } + + public function testValidateSkipsOnlyValidateOnUpdateFieldsOnInsert(): void + { + $entity = new class { + #[OnlyValidateOnUpdate] + public readonly ?string $nullable; + + public function __construct() + { + $this->nullable = '+33611223344'; + } + }; + $this->classMetadata->reflFields = [ + 'nullable' => new \ReflectionProperty($entity, 'nullable'), + ]; + $this->mockEntityChangeSet([]); + + $this->expectNoValidate(); + + $this->validator->validate($entity, new Entity()); + + self::assertNoViolation(); + } + + public function testValidateChecksOnlyValidateOnUpdateFieldsPartOfTheChangeSet(): void + { + $entity = new class { + #[OnlyValidateOnUpdate] + public readonly ?string $nullable; + + public function __construct() + { + $this->nullable = '+33611223344'; + } + }; + $this->classMetadata->reflFields = [ + 'nullable' => new \ReflectionProperty($entity, 'nullable'), + ]; + $this->mockEntityChangeSet(['nullable' => [null, '+33611223344']]); + + $this->expectValidateValueAt(0, 'nullable', '+33611223344', [new Phone()]); + + $this->validator->validate($entity, new Entity()); + } + + /** + * @param array $changeSet + */ + private function mockEntityChangeSet(array $changeSet): void + { + $unitOfWork = self::createStub(UnitOfWork::class); + $unitOfWork->method('getEntityChangeSet')->willReturn($changeSet); + $this->em->method('getUnitOfWork')->willReturn($unitOfWork); + } + + public function testGetConstraintsForNotNullableFieldWithOrm3MappingObject(): void + { + self::skipUnlessOrm3(); + + $metadata = new ClassMetadata(MyEntityParent::class); + $metadata->fieldMappings = [ + 'notnullable' => FieldMapping::fromMappingArray([ + 'type' => 'phone', + 'fieldName' => 'notnullable', + 'columnName' => 'notnullable', + 'nullable' => false, + ]), + ]; + + self::assertArrayContainsSameObjects( + $this->createValidatorForMetadata($metadata)->getConstraints('class', 'notnullable'), + [new NotNull(), new Phone()] + ); + } + + public function testGetConstraintsForRelationsWithOrm3MappingObjects(): void + { + self::skipUnlessOrm3(); + + $metadata = new ClassMetadata(MyEntityParent::class); + $metadata->associationMappings = [ + 'notowning' => OneToManyAssociationMapping::fromMappingArray([ + 'fieldName' => 'notowning', + 'sourceEntity' => MyEntityParent::class, + 'targetEntity' => MyEntityParent::class, + 'mappedBy' => 'parent', + ]), + 'owningToOne' => ManyToOneAssociationMapping::fromMappingArray([ + 'fieldName' => 'owningToOne', + 'sourceEntity' => MyEntityParent::class, + 'targetEntity' => MyEntityParent::class, + ]), + 'owningToOneNotNull' => ManyToOneAssociationMapping::fromMappingArray([ + 'fieldName' => 'owningToOneNotNull', + 'sourceEntity' => MyEntityParent::class, + 'targetEntity' => MyEntityParent::class, + 'joinColumns' => [['name' => 'parent_id', 'referencedColumnName' => 'id', 'nullable' => false]], + ]), + 'owningToMany' => ManyToManyOwningSideMapping::fromMappingArray([ + 'fieldName' => 'owningToMany', + 'sourceEntity' => MyEntityParent::class, + 'targetEntity' => MyEntityParent::class, + ]), + ]; + $validator = $this->createValidatorForMetadata($metadata); + + self::assertEmpty($validator->getConstraints('class', 'notowning')); + self::assertArrayContainsSameObjects( + $validator->getConstraints('class', 'owningToOne'), + [new Type(MyEntityParent::class)] + ); + self::assertArrayContainsSameObjects( + $validator->getConstraints('class', 'owningToOneNotNull'), + [new Type(MyEntityParent::class), new NotNull()] + ); + self::assertArrayContainsSameObjects( + $validator->getConstraints('class', 'owningToMany'), + [new All(constraints: [new Type(MyEntityParent::class)])] + ); + } + + private static function skipUnlessOrm3(): void + { + if (!class_exists(FieldMapping::class)) { + self::markTestSkipped('Requires the doctrine/orm 3 mapping objects'); + } + } + + /** + * @param ClassMetadata $metadata + */ + private function createValidatorForMetadata(ClassMetadata $metadata): EntityValidator + { + $em = $this->createMock(EntityManagerInterface::class); + $em->method('getClassMetadata')->willReturn($metadata); + + return new EntityValidator($em, [new PhoneProvider()]); + } + public static function providerInvalidValues(): iterable { return [];