From b93222875907f2c269c16ab9d65100494a29cfc9 Mon Sep 17 00:00:00 2001 From: myrmidex Date: Sat, 27 Jun 2026 17:33:25 +0200 Subject: [PATCH] 57 - Add partial-update PATCH to the Bucket API with priority-uniqueness validation --- src/Entity/Bucket.php | 10 +- src/Repository/BucketRepository.php | 20 ++ src/Validator/UniquePriorityPerScenario.php | 18 ++ .../UniquePriorityPerScenarioValidator.php | 44 ++++ tests/Functional/BucketApiTest.php | 243 ++++++++++++++++++ tests/Functional/BucketOwnerApiTest.php | 88 +++++++ .../UniquePriorityPerScenarioTest.php | 170 ++++++++++++ ...UniquePriorityPerScenarioValidatorTest.php | 117 +++++++++ 8 files changed, 709 insertions(+), 1 deletion(-) create mode 100644 src/Validator/UniquePriorityPerScenario.php create mode 100644 src/Validator/UniquePriorityPerScenarioValidator.php create mode 100644 tests/Validator/UniquePriorityPerScenarioTest.php create mode 100644 tests/Validator/UniquePriorityPerScenarioValidatorTest.php diff --git a/src/Entity/Bucket.php b/src/Entity/Bucket.php index 5329b2b..641b1b5 100644 --- a/src/Entity/Bucket.php +++ b/src/Entity/Bucket.php @@ -5,6 +5,7 @@ namespace App\Entity; use ApiPlatform\Metadata\ApiResource; use ApiPlatform\Metadata\Get; use ApiPlatform\Metadata\GetCollection; +use ApiPlatform\Metadata\Patch; use ApiPlatform\Metadata\Post; use App\Enum\BucketAllocationType; use App\Enum\BucketType; @@ -12,6 +13,7 @@ use App\Repository\BucketRepository; use App\Validator\BufferOnlyForFixedLimit; use App\Validator\CompatibleAllocationType; use App\Validator\SingleOverflowPerScenario; +use App\Validator\UniquePriorityPerScenario; use Doctrine\ORM\Mapping as ORM; use Symfony\Component\Uid\UuidV7; use Symfony\Component\Validator\Constraints as Assert; @@ -21,9 +23,10 @@ use Symfony\Component\Validator\Constraints as Assert; #[ORM\UniqueConstraint(name: 'unique_scenario_priority', columns: ['scenario_id', 'priority'])] #[CompatibleAllocationType] #[SingleOverflowPerScenario] +#[UniquePriorityPerScenario] #[BufferOnlyForFixedLimit] #[ApiResource( - operations: [new GetCollection(), new Get(), new Post()], + operations: [new GetCollection(), new Get(), new Post(), new Patch()], security: "is_granted('ROLE_USER')", )] class Bucket @@ -180,4 +183,9 @@ class Bucket return $this; } + + public function hasPriority(): bool + { + return isset($this->priority); + } } diff --git a/src/Repository/BucketRepository.php b/src/Repository/BucketRepository.php index 8acf35b..c433b7e 100644 --- a/src/Repository/BucketRepository.php +++ b/src/Repository/BucketRepository.php @@ -48,4 +48,24 @@ class BucketRepository extends ServiceEntityRepository ->getQuery() ->execute(); } + + public function countBucketsWithPriorityForScenario( + Scenario $scenario, + int $priority, + ?UuidV7 $excludeId = null, + ): int { + $qb = $this->createQueryBuilder('b') + ->select('COUNT(b.id)') + ->andWhere('b.scenario = :scenario') + ->andWhere('b.priority = :priority') + ->setParameter('scenario', $scenario) + ->setParameter('priority', $priority); + + if (null !== $excludeId) { + $qb->andWhere('b.id != :excludeId') + ->setParameter('excludeId', $excludeId, 'uuid'); + } + + return (int) $qb->getQuery()->getSingleScalarResult(); + } } diff --git a/src/Validator/UniquePriorityPerScenario.php b/src/Validator/UniquePriorityPerScenario.php new file mode 100644 index 0000000..5422d1e --- /dev/null +++ b/src/Validator/UniquePriorityPerScenario.php @@ -0,0 +1,18 @@ +hasPriority()) { + return; + } + + $existing = $this->bucketRepository->countBucketsWithPriorityForScenario( + $value->getScenario(), + $value->getPriority(), + $value->getId(), + ); + + if ($existing > 0) { + $this->context->buildViolation($constraint->message) + ->atPath('priority') + ->setCode(UniquePriorityPerScenario::PRIORITY_NOT_UNIQUE_ERROR) + ->addViolation(); + } + } +} diff --git a/tests/Functional/BucketApiTest.php b/tests/Functional/BucketApiTest.php index 33897f5..d1266be 100644 --- a/tests/Functional/BucketApiTest.php +++ b/tests/Functional/BucketApiTest.php @@ -445,6 +445,249 @@ final class BucketApiTest extends WebTestCase ); } + public function testPatchUpdatesAnOwnedBucketPartially(): void + { + $scenario = $this->persistScenario('Household Budget'); + $bucket = $this->persistBucket($scenario, 'Rent', 1); + + $this->login(); + + $this->client->request( + 'PATCH', + '/api/buckets/'.$bucket->getId(), + server: [ + 'CONTENT_TYPE' => 'application/merge-patch+json', + 'HTTP_ACCEPT' => 'application/ld+json', + ], + content: json_encode(['name' => 'Rent (updated)']), + ); + + $response = $this->client->getResponse(); + + self::assertSame( + 200, + $response->getStatusCode(), + 'PATCH /api/buckets/{id} with a valid merge-patch body for an owned bucket must return 200.', + ); + + $this->em->clear(); + + $persisted = $this->em->getRepository(Bucket::class)->find($bucket->getId()); + + self::assertNotNull($persisted, 'The patched bucket must still exist.'); + self::assertSame( + 'Rent (updated)', + $persisted->getName(), + 'The patched field (name) must be updated.', + ); + self::assertSame( + 1, + $persisted->getPriority(), + 'A field NOT included in the PATCH body (priority) must remain unchanged — proves a partial merge, not a full replace.', + ); + self::assertSame( + BucketType::NEED, + $persisted->getType(), + 'A field NOT included in the PATCH body (type) must remain unchanged — proves a partial merge, not a full replace.', + ); + } + + public function testPatchRequiresMergePatchContentType(): void + { + $scenario = $this->persistScenario('Household Budget'); + $bucket = $this->persistBucket($scenario, 'Rent', 1); + + $this->login(); + + $this->client->request( + 'PATCH', + '/api/buckets/'.$bucket->getId(), + server: [ + 'CONTENT_TYPE' => 'application/ld+json', + 'HTTP_ACCEPT' => 'application/ld+json', + ], + content: json_encode(['name' => 'Rent (updated)']), + ); + + self::assertSame( + 415, + $this->client->getResponse()->getStatusCode(), + 'PATCH /api/buckets/{id} with the wrong content type (application/ld+json instead of application/merge-patch+json) must be rejected with 415.', + ); + + $this->em->clear(); + + $persisted = $this->em->getRepository(Bucket::class)->find($bucket->getId()); + + self::assertNotNull($persisted); + self::assertSame( + 'Rent', + $persisted->getName(), + 'A rejected PATCH (wrong content type) must not mutate the bucket.', + ); + } + + public function testPatchIsRejectedWhenUnauthenticated(): void + { + $scenario = $this->persistScenario('Household Budget'); + $bucket = $this->persistBucket($scenario, 'Rent', 1); + + $this->client->request( + 'PATCH', + '/api/buckets/'.$bucket->getId(), + server: [ + 'CONTENT_TYPE' => 'application/merge-patch+json', + 'HTTP_ACCEPT' => 'application/ld+json', + ], + content: json_encode(['name' => 'Rent (updated)']), + ); + + self::assertSame( + 401, + $this->client->getResponse()->getStatusCode(), + 'PATCH /api/buckets/{id} must require authentication.', + ); + } + + public function testPatchIntroducingSecondOverflowIsRejectedAsUnprocessable(): void + { + $scenario = $this->persistScenario('Household Budget'); + + $existingOverflow = new Bucket(); + $existingOverflow->setScenario($scenario); + $existingOverflow->setName('Overflow'); + $existingOverflow->setPriority(1); + $existingOverflow->setType(BucketType::OVERFLOW); + $existingOverflow->setAllocationType(BucketAllocationType::UNLIMITED); + + $this->em->persist($existingOverflow); + $this->em->flush(); + + $secondBucket = $this->persistBucket($scenario, 'Wants', 2); + + $this->login(); + + $this->client->request( + 'PATCH', + '/api/buckets/'.$secondBucket->getId(), + server: [ + 'CONTENT_TYPE' => 'application/merge-patch+json', + 'HTTP_ACCEPT' => 'application/ld+json', + ], + content: json_encode([ + 'type' => BucketType::OVERFLOW->value, + 'allocationType' => BucketAllocationType::UNLIMITED->value, + ]), + ); + + $response = $this->client->getResponse(); + + self::assertSame( + 422, + $response->getStatusCode(), + 'PATCH /api/buckets/{id} flipping a second bucket to overflow on a scenario that already has one must be rejected by the SingleOverflowPerScenario validator (422, not 500).', + ); + + $this->assertHasViolationAt('type', $response); + } + + public function testPatchSettingBufferOnNonFixedLimitIsRejectedAsUnprocessable(): void + { + $scenario = $this->persistScenario('Household Budget'); + + $bucket = new Bucket(); + $bucket->setScenario($scenario); + $bucket->setName('Wants'); + $bucket->setPriority(1); + $bucket->setType(BucketType::WANT); + $bucket->setAllocationType(BucketAllocationType::PERCENTAGE); + $bucket->setAllocationValue(2000); + + $this->em->persist($bucket); + $this->em->flush(); + + $this->login(); + + $this->client->request( + 'PATCH', + '/api/buckets/'.$bucket->getId(), + server: [ + 'CONTENT_TYPE' => 'application/merge-patch+json', + 'HTTP_ACCEPT' => 'application/ld+json', + ], + content: json_encode([ + 'bufferMultiplier' => '1.50', + ]), + ); + + $response = $this->client->getResponse(); + + self::assertSame( + 422, + $response->getStatusCode(), + 'PATCH /api/buckets/{id} setting a non-zero buffer on a bucket whose allocation type is not fixed-limit must be rejected by the BufferOnlyForFixedLimit validator (422, not 500).', + ); + + $this->assertHasViolationAt('bufferMultiplier', $response); + } + + public function testPatchToIncompatibleAllocationTypeIsRejectedAsUnprocessable(): void + { + $scenario = $this->persistScenario('Household Budget'); + $bucket = $this->persistBucket($scenario, 'Rent', 1); + + $this->login(); + + $this->client->request( + 'PATCH', + '/api/buckets/'.$bucket->getId(), + server: [ + 'CONTENT_TYPE' => 'application/merge-patch+json', + 'HTTP_ACCEPT' => 'application/ld+json', + ], + content: json_encode([ + 'allocationType' => BucketAllocationType::UNLIMITED->value, + ]), + ); + + $response = $this->client->getResponse(); + + self::assertSame( + 422, + $response->getStatusCode(), + 'PATCH /api/buckets/{id} setting allocationType to a value incompatible with the bucket type (NEED can only be fixed_limit/percentage) must be rejected by the CompatibleAllocationType validator (422, not 500).', + ); + + $this->assertHasViolationAt('allocationType', $response); + } + + public function testPatchCollidingOnPriorityIsRejected(): void + { + $scenario = $this->persistScenario('Household Budget'); + $this->persistBucket($scenario, 'Rent', 1); + $groceries = $this->persistBucket($scenario, 'Groceries', 2); + + $this->login(); + + $this->client->request( + 'PATCH', + '/api/buckets/'.$groceries->getId(), + server: [ + 'CONTENT_TYPE' => 'application/merge-patch+json', + 'HTTP_ACCEPT' => 'application/ld+json', + ], + content: json_encode([ + 'priority' => 1, + ]), + ); + + self::assertSame( + 422, + $this->client->getResponse()->getStatusCode(), + 'PATCH /api/buckets/{id} colliding on priority with another bucket in the same scenario must be rejected as a validation error (422), not surfaced as an unhandled server error.', + ); + } + public function testPostWithNonJsonContentTypeIsRejected(): void { $this->login(); diff --git a/tests/Functional/BucketOwnerApiTest.php b/tests/Functional/BucketOwnerApiTest.php index 25b4e5e..7e8c1ba 100644 --- a/tests/Functional/BucketOwnerApiTest.php +++ b/tests/Functional/BucketOwnerApiTest.php @@ -279,4 +279,92 @@ final class BucketOwnerApiTest extends WebTestCase 'GET /api/buckets/{id} without authentication must be rejected by the security layer.', ); } + + public function testPatchOnABucketOwnedBySomeoneElseReturnsNotFound(): void + { + $othersScenario = $this->persistScenarioFor($this->otherUser, 'Someone Else\'s Fund'); + $othersBucket = $this->persistBucket($othersScenario, 'Their Rent', 1); + + $this->login(); + + $this->client->request( + 'PATCH', + '/api/buckets/'.$othersBucket->getId(), + server: [ + 'CONTENT_TYPE' => 'application/merge-patch+json', + 'HTTP_ACCEPT' => 'application/ld+json', + ], + content: json_encode(['name' => 'Sneaky Rename']), + ); + + self::assertSame( + 404, + $this->client->getResponse()->getStatusCode(), + 'PATCH /api/buckets/{id} for a bucket whose scenario is owned by another user must return 404, not 403 — ' + .'no existence oracle for buckets you do not own, scoped transitively via scenario.owner.', + ); + + $this->em->clear(); + + $persisted = $this->em->getRepository(Bucket::class)->find($othersBucket->getId()); + + self::assertNotNull($persisted, 'The other user\'s bucket must still exist.'); + self::assertSame( + 'Their Rent', + $persisted->getName(), + 'A rejected PATCH against a non-owned bucket must never mutate it — no mutation must leak through ' + .'the 404 item-resolution failure.', + ); + } + + /** + * Mirrors the class-level docblock on {@see testPostUnderAForeignScenarioCurrentlyReturnsBadRequestNotFound()}: + * this pins OBSERVED behaviour for an attempted cross-tenant re-parent via PATCH, not an assumed contract. + * + * The caller PATCHes their OWN bucket but supplies a `scenario` IRI pointing at a scenario owned by + * `otherUser`. `ScenarioOwnerExtension` (#50 Story 2) already makes that IRI unresolvable to a non-owner, + * so API Platform's IRI denormalizer fails to resolve `scenario` while building the merge-patched object — + * the same `AbstractItemNormalizer::getResourceFromIri` path that produces 400 on the analogous foreign-scenario + * POST. Empirically this also resolves to 400 here. Pinning it as a coherent rejection: the bucket is + * confirmed NOT re-parented below. + */ + public function testPatchReparentingIntoForeignScenarioIsRejected(): void + { + $myScenario = $this->persistScenarioFor($this->caller, 'My Fund'); + $myBucket = $this->persistBucket($myScenario, 'Groceries', 1); + + $othersScenario = $this->persistScenarioFor($this->otherUser, 'Someone Else\'s Fund'); + + $this->login(); + + $this->client->request( + 'PATCH', + '/api/buckets/'.$myBucket->getId(), + server: [ + 'CONTENT_TYPE' => 'application/merge-patch+json', + 'HTTP_ACCEPT' => 'application/ld+json', + ], + content: json_encode(['scenario' => '/api/scenarios/'.$othersScenario->getId()]), + ); + + self::assertSame( + 400, + $this->client->getResponse()->getStatusCode(), + 'PATCH /api/buckets/{id} attempting to re-parent the caller\'s own bucket into a scenario owned by ' + .'another user currently resolves to 400 (unresolvable IRI denormalization failure), mirroring the ' + .'foreign-scenario POST behaviour — pinning the observed rejection. See docblock above this test.', + ); + + $this->em->clear(); + + $persisted = $this->em->getRepository(Bucket::class)->find($myBucket->getId()); + + self::assertNotNull($persisted, 'The caller\'s bucket must still exist.'); + self::assertSame( + $myScenario->getId()->toRfc4122(), + $persisted->getScenario()->getId()->toRfc4122(), + 'A rejected re-parenting PATCH must never actually move the bucket into the foreign scenario — ' + .'the bucket must remain under its original scenario.', + ); + } } diff --git a/tests/Validator/UniquePriorityPerScenarioTest.php b/tests/Validator/UniquePriorityPerScenarioTest.php new file mode 100644 index 0000000..fbf3eeb --- /dev/null +++ b/tests/Validator/UniquePriorityPerScenarioTest.php @@ -0,0 +1,170 @@ +em = self::getContainer()->get(EntityManagerInterface::class); + $this->validator = self::getContainer()->get(ValidatorInterface::class); + } + + public function testBucketCollidingOnPriorityWithAnotherBucketInTheSameScenarioRaisesAViolation(): void + { + $scenario = new Scenario(); + $scenario->setName('Household Budget'); + $scenario->setOwner($this->persistOwner()); + $this->em->persist($scenario); + $this->em->flush(); + + $existing = $this->makeBucket($scenario, 1); + $this->em->persist($existing); + $this->em->flush(); + + $colliding = $this->makeBucket($scenario, 1); + + $violations = $this->validator->validate($colliding); + + $matching = $this->violationsAt($violations, 'priority'); + + self::assertNotCount( + 0, + $matching, + 'A bucket whose priority collides with another bucket in the same scenario must raise a violation on priority. ' + .'The existing bucket is persisted-but-uncommitted in this test\'s DAMA transaction — ' + .'the validator\'s repository query must still see it within the same transaction.', + ); + } + + public function testTheBucketThatAlreadyHoldsThatPriorityValidatesWithNoUniquenessViolationAgainstItself(): void + { + $scenario = new Scenario(); + $scenario->setName('Household Budget'); + $scenario->setOwner($this->persistOwner()); + $this->em->persist($scenario); + $this->em->flush(); + + $bucket = $this->makeBucket($scenario, 1); + $this->em->persist($bucket); + $this->em->flush(); + + $violations = $this->validator->validate($bucket); + + $matching = $this->violationsAt($violations, 'priority'); + + self::assertCount( + 0, + $matching, + 'The bucket that already holds this priority must not raise this constraint against itself ' + .'(exclude-self-by-id must be working) — this is the PATCH-without-changing-priority flow.', + ); + } + + public function testBucketWithAPriorityUniqueWithinItsScenarioIsValid(): void + { + $scenario = new Scenario(); + $scenario->setName('Household Budget'); + $scenario->setOwner($this->persistOwner()); + $this->em->persist($scenario); + $this->em->flush(); + + $existing = $this->makeBucket($scenario, 1); + $this->em->persist($existing); + $this->em->flush(); + + $unique = $this->makeBucket($scenario, 2); + + $violations = $this->validator->validate($unique); + + $matching = $this->violationsAt($violations, 'priority'); + + self::assertCount( + 0, + $matching, + 'A brand-new, unpersisted bucket with a priority not used elsewhere in its scenario must not raise this constraint.', + ); + } + + public function testSamePriorityValueInADifferentScenarioDoesNotRaiseAViolation(): void + { + $scenarioOne = new Scenario(); + $scenarioOne->setName('Household Budget'); + $scenarioOne->setOwner($this->persistOwner()); + $this->em->persist($scenarioOne); + + $scenarioTwo = new Scenario(); + $scenarioTwo->setName('Side Project Budget'); + $scenarioTwo->setOwner($this->persistOwner()); + $this->em->persist($scenarioTwo); + $this->em->flush(); + + $existing = $this->makeBucket($scenarioOne, 1); + $this->em->persist($existing); + $this->em->flush(); + + $sameValueDifferentScenario = $this->makeBucket($scenarioTwo, 1); + + $violations = $this->validator->validate($sameValueDifferentScenario); + + $matching = $this->violationsAt($violations, 'priority'); + + self::assertCount( + 0, + $matching, + 'Priority uniqueness is scoped per-scenario, matching the unique_scenario_priority DB constraint columns ' + .'(scenario_id, priority) — the same priority value in a different scenario must not raise a violation.', + ); + } + + private function makeBucket(Scenario $scenario, int $priority): Bucket + { + $bucket = new Bucket(); + $bucket->setScenario($scenario); + $bucket->setType(BucketType::NEED); + $bucket->setName('Bucket '.$priority); + $bucket->setPriority($priority); + $bucket->setSortOrder($priority); + $bucket->setAllocationType(BucketAllocationType::FIXED_LIMIT); + $bucket->setAllocationValue(50000); + $bucket->setStartingAmount(0); + $bucket->setBufferMultiplier('0.00'); + + return $bucket; + } + + /** + * @return list<\Symfony\Component\Validator\ConstraintViolationInterface> + */ + private function violationsAt(ConstraintViolationListInterface $violations, string $propertyPath): array + { + $matching = []; + + foreach ($violations as $violation) { + if ($violation->getPropertyPath() === $propertyPath + && $violation->getConstraint() instanceof UniquePriorityPerScenario + ) { + $matching[] = $violation; + } + } + + return $matching; + } +} diff --git a/tests/Validator/UniquePriorityPerScenarioValidatorTest.php b/tests/Validator/UniquePriorityPerScenarioValidatorTest.php new file mode 100644 index 0000000..cd59e47 --- /dev/null +++ b/tests/Validator/UniquePriorityPerScenarioValidatorTest.php @@ -0,0 +1,117 @@ + + */ +final class UniquePriorityPerScenarioValidatorTest extends ConstraintValidatorTestCase +{ + private BucketRepository&Stub $bucketRepository; + + protected function createValidator(): ConstraintValidatorInterface + { + $this->bucketRepository = $this->createStub(BucketRepository::class); + + return new UniquePriorityPerScenarioValidator($this->bucketRepository); + } + + public function testThrowsWhenGivenTheWrongConstraintType(): void + { + $bucket = $this->makeBucket(1); + + $this->expectException(UnexpectedTypeException::class); + + $this->validator->validate($bucket, new SingleOverflowPerScenario()); + } + + public function testNonBucketValueIsIgnored(): void + { + $this->validator->validate('not a bucket', new UniquePriorityPerScenario()); + + $this->assertNoViolation(); + } + + public function testBucketIsValidWhenRepositoryReportsNoOtherBucketWithThatPriority(): void + { + $this->bucketRepository + ->method('countBucketsWithPriorityForScenario') + ->willReturn(0); + + $bucket = $this->makeBucket(1); + + $this->validator->validate($bucket, new UniquePriorityPerScenario()); + + $this->assertNoViolation(); + } + + public function testBucketIsInvalidWhenRepositoryReportsAnExistingBucketWithThatPriority(): void + { + $this->bucketRepository + ->method('countBucketsWithPriorityForScenario') + ->willReturn(1); + + $bucket = $this->makeBucket(1); + + $this->validator->validate($bucket, new UniquePriorityPerScenario()); + + $this->buildViolation('This priority is already used by another bucket in this scenario.') + ->atPath('property.path.priority') + ->setCode(UniquePriorityPerScenario::PRIORITY_NOT_UNIQUE_ERROR) + ->assertRaised(); + } + + public function testBucketWithUninitializedPriorityIsIgnoredWithoutQueryingTheRepository(): void + { + $this->bucketRepository + ->method('countBucketsWithPriorityForScenario') + ->willThrowException(new \LogicException( + 'countBucketsWithPriorityForScenario must not be called when priority is uninitialized.' + )); + + $bucket = $this->makeBucketWithoutPriority(); + + $this->validator->validate($bucket, new UniquePriorityPerScenario()); + + $this->assertNoViolation(); + } + + private function makeBucket(int $priority): Bucket + { + $bucket = $this->makeBucketWithoutPriority(); + $bucket->setPriority($priority); + + return $bucket; + } + + private function makeBucketWithoutPriority(): Bucket + { + $scenario = new Scenario(); + $scenario->setName('Household Budget'); + + $bucket = new Bucket(); + $bucket->setScenario($scenario); + $bucket->setType(BucketType::NEED); + $bucket->setName('Test Bucket'); + $bucket->setSortOrder(0); + $bucket->setAllocationType(BucketAllocationType::FIXED_LIMIT); + $bucket->setAllocationValue(50000); + $bucket->setStartingAmount(0); + $bucket->setBufferMultiplier('0.00'); + + return $bucket; + } +}