From 062bf053d8a673f05fc9ff732c6cffde0906d6d5 Mon Sep 17 00:00:00 2001 From: 66Ton99 <66ton99@gmail.com> Date: Fri, 28 Aug 2026 09:40:23 +0300 Subject: [PATCH] fix: stop reporting an edited record as its own duplicate A form that edits a record submits the value that record already holds, so the lookup found the record itself and the browser reported "already used" for every edit of an entity carrying a UniqueEntity constraint. The submit was blocked client side even though Symfony's own validator, which skips the object it is validating, would have accepted it. The browser already sends the identifier of the object the form is bound to as "entityId", and the documentation already told custom controllers to use it. The default controller now does the same: a value is free when every record holding it is the one that identifier names. That identifier is the one part of the lookup the request decides, so a caller can ask for an answer that ignores one record. It changes an answer, never data, and the server side validator is unaffected; the route stays the existence oracle documented in 3_9. --- AGENTS.md | 6 ++ Tests/Controller/AjaxControllerTest.php | 112 ++++++++++++++++++++++++ src/Controller/AjaxController.php | 109 ++++++++++++++++++++++- src/Resources/doc/3_9.md | 15 +++- 4 files changed, 238 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 34f42a0d..321fefd5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -75,3 +75,9 @@ Last verified in the Nix shell on PHP 8.5.6 / Node 24.16.0: the class validation metadata, so the endpoint refuses it. Declare `UniqueEntity` on the entity class, or point the bundle at a custom controller. +- The lookup skips the record the form is editing, named by the `entityId` the + browser sends, so an edit is not reported as a duplicate of itself. That + identifier is the one thing the request does decide, which lets a caller ask + for an answer that ignores one record. It only changes an answer, never data, + and the server side validator is unaffected. Documented in + `src/Resources/doc/3_9.md`. diff --git a/Tests/Controller/AjaxControllerTest.php b/Tests/Controller/AjaxControllerTest.php index b818f06d..b5833c53 100644 --- a/Tests/Controller/AjaxControllerTest.php +++ b/Tests/Controller/AjaxControllerTest.php @@ -429,6 +429,82 @@ private function createRegistry(ObjectRepository $repository) return $registry; } + /** + * A form that edits a record submits the value that record already holds, + * so the record matches itself. Symfony's own validator skips the object it + * validates; this endpoint is told which record that is by "entityId" + */ + public function testTheEditedRecordIsNotADuplicateOfItself() + { + $controller = new AjaxController( + $this->createRegistry(new IdentifiedRepository()), + $this->createMetadataFactory() + ); + + $response = $controller->checkUniqueEntityAction(new Request(array(), array( + 'entityName' => InMemoryEntity::class, + 'repositoryMethod' => 'findBy', + 'entityId' => '15', + 'data' => array('email' => 'existing_email'), + ))); + + $this->assertTrue(json_decode($response->getContent()), 'The record being edited is not its own duplicate'); + } + + public function testAValueHeldByAnotherRecordIsADuplicate() + { + $controller = new AjaxController( + $this->createRegistry(new IdentifiedRepository()), + $this->createMetadataFactory() + ); + + $response = $controller->checkUniqueEntityAction(new Request(array(), array( + 'entityName' => InMemoryEntity::class, + 'repositoryMethod' => 'findBy', + 'entityId' => '16', + 'data' => array('email' => 'existing_email'), + ))); + + $this->assertFalse(json_decode($response->getContent()), 'Another record holding the value is a duplicate'); + } + + public function testAnExistingValueIsADuplicateWhenNoRecordIsBeingEdited() + { + $controller = new AjaxController( + $this->createRegistry(new IdentifiedRepository()), + $this->createMetadataFactory() + ); + + $response = $controller->checkUniqueEntityAction(new Request(array(), array( + 'entityName' => InMemoryEntity::class, + 'repositoryMethod' => 'findBy', + 'data' => array('email' => 'existing_email'), + ))); + + $this->assertFalse(json_decode($response->getContent()), 'A create form has no record to skip'); + } + + /** + * Nothing can be said about a match whose identifier cannot be read, so it + * is never taken for the record being edited + */ + public function testAMatchWithoutAnIdentifierIsADuplicate() + { + $controller = new AjaxController( + $this->createRegistry(new InMemoryRepository()), + $this->createMetadataFactory() + ); + + $response = $controller->checkUniqueEntityAction(new Request(array(), array( + 'entityName' => InMemoryEntity::class, + 'repositoryMethod' => 'findBy', + 'entityId' => '15', + 'data' => array('email' => 'existing_email'), + ))); + + $this->assertFalse(json_decode($response->getContent()), 'A match with no identifier is a duplicate'); + } + /** * The constraints the fixture application declared * @@ -541,6 +617,42 @@ public function getClassName(): string } } +class IdentifiedRepository extends InMemoryRepository +{ + public function findBy(array $criteria, ?array $orderBy = null, ?int $limit = null, ?int $offset = null): array + { + if (isset($criteria['email']) && 'existing_email' === $criteria['email']) { + return array(new IdentifiedEntity(15)); + } + + return array(); + } +} + +class IdentifiedEntity +{ + /** + * @var int + */ + private $id; + + /** + * @param int $id + */ + public function __construct($id) + { + $this->id = $id; + } + + /** + * @return int + */ + public function getId() + { + return $this->id; + } +} + class MagicRepository extends InMemoryRepository { public static function shared(): array diff --git a/src/Controller/AjaxController.php b/src/Controller/AjaxController.php index 907279fb..f13c48b0 100644 --- a/src/Controller/AjaxController.php +++ b/src/Controller/AjaxController.php @@ -128,9 +128,114 @@ public function checkUniqueEntityAction(Request $request) )); } - $entity = $repository->{$constraint->repositoryMethod}($values); + $matches = $this->toList($repository->{$constraint->repositoryMethod}($values)); - return new JsonResponse(empty($entity)); + if (!$matches) { + return new JsonResponse(true); + } + + return new JsonResponse($this->holdsOnlyTheEditedRecord($matches, $entityName, $data)); + } + + /** + * The repository method a UniqueEntity constraint declares answers with a + * list, but a custom one may answer with a single entity or with nothing + * + * @param mixed $result + * + * @return array + */ + private function toList($result) + { + if (null === $result) { + return array(); + } + + if (is_array($result)) { + return $result; + } + + if ($result instanceof \Traversable) { + return iterator_to_array($result); + } + + return array($result); + } + + /** + * Whether every record holding the value is the record the form is editing + * + * A form that edits a record submits the value that record already holds, + * so the record matches itself. Symfony's own validator knows the object it + * is validating and skips it; here the identifier of that object travels + * with the request, which means a caller can ask for an answer that ignores + * one record of its choosing. The answer is a hint either way - the + * validator refuses the submit itself, whatever this route said - and the + * route stays the existence oracle documented in 3_9. + * + * @param array $matches + * @param string $entityName + * @param array $data + * + * @return bool + */ + private function holdsOnlyTheEditedRecord(array $matches, $entityName, array $data) + { + if (!isset($data['entityId']) || !is_scalar($data['entityId']) || '' === $data['entityId']) { + return false; + } + + foreach ($matches as $match) { + $identifier = $this->identifierOf($match, $entityName); + + if (null === $identifier || (string)$identifier !== (string)$data['entityId']) { + return false; + } + } + + return true; + } + + /** + * The single identifier value of a record, or null when it has none the + * identifier in the request could be compared with: a composite key, or an + * object no manager and no getId() can answer for + * + * @param mixed $entity + * @param string $entityName + * + * @return mixed|null + */ + private function identifierOf($entity, $entityName) + { + if (!is_object($entity)) { + return null; + } + + $values = array(); + + try { + $manager = $this->doctrine->getManagerForClass($entityName); + if ($manager) { + $values = $manager->getClassMetadata($entityName)->getIdentifierValues($entity); + } + } catch (\Throwable $e) { + $values = array(); + } + + if (1 === count($values)) { + $value = reset($values); + + return is_scalar($value) ? $value : null; + } + + // A composite key cannot be matched against the single value the + // browser sends, so the record is never taken for the edited one + if ($values) { + return null; + } + + return method_exists($entity, 'getId') && is_scalar($entity->getId()) ? $entity->getId() : null; } /** diff --git a/src/Resources/doc/3_9.md b/src/Resources/doc/3_9.md index 6fc53dd2..f25008c7 100644 --- a/src/Resources/doc/3_9.md +++ b/src/Resources/doc/3_9.md @@ -19,6 +19,17 @@ the constraint, never from the request: - criteria values — must be scalar or `null`. An array value is refused, so a request cannot widen the lookup into an extra condition. +A form that edits a record submits the value that record already holds, so the +record matches itself. The controller answers that the value is free when every +record holding it is the one named by `entityId`, the way Symfony's own +validator skips the object it is validating. Without that, a `UniqueEntity` +constraint would report every edit of an existing record as a duplicate. + +`entityId` comes from the request, so a caller can ask for an answer that +ignores one record of its choosing. Nothing is created or changed by the answer, +and the server side validator still refuses a submit that is really a duplicate; +the route remains the existence check described below either way. + Two things this does not do, because only your application can: - For the field combinations you did declare unique, the route is still a public @@ -94,7 +105,7 @@ $data = [ ``` When the form is bound to an object that has a `getId()` method, `entityId` -contains the current object id. Custom uniqueness controllers can use it to -exclude the edited entity from their lookup. +contains the current object id. The default controller uses it to exclude the +edited record from its lookup, and a custom controller should do the same. Return `true` when the value is unique and `false` when it is already used.