Memoize constraint resolution and attribute lookups in EntityValidator

.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() }}

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<string, array<string, array<Constraint>>> */
+    private array $constraintsCache = [];
+    /** @var array<string, array<string, bool>> */
+    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<Constraint>
      */
     public function getConstraints(string $class, string $field): array
+    {
+        return $this->constraintsCache[$class][$field] ??= $this->buildConstraints($class, $field);
+    }
+
+    /**
+     * @return array<Constraint>
+     */
+    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;
     }
 
     /**

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<MyEntityParent> */
     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<string, array{mixed, mixed}> $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<MyEntityParent> $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 [];