57 - Add partial-update PATCH to the Bucket API with priority-uniqueness validation

This commit is contained in:
myrmidex 2026-06-27 17:33:25 +02:00
parent e563816cb3
commit b932228759
8 changed files with 709 additions and 1 deletions

View file

@ -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);
}
}

View file

@ -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();
}
}

View file

@ -0,0 +1,18 @@
<?php
namespace App\Validator;
use Symfony\Component\Validator\Constraint;
#[\Attribute(\Attribute::TARGET_CLASS)]
final class UniquePriorityPerScenario extends Constraint
{
public string $message = 'This priority is already used by another bucket in this scenario.';
public const string PRIORITY_NOT_UNIQUE_ERROR = 'PRIORITY_NOT_UNIQUE_ERROR';
public function getTargets(): string
{
return self::CLASS_CONSTRAINT;
}
}

View file

@ -0,0 +1,44 @@
<?php
namespace App\Validator;
use App\Entity\Bucket;
use App\Repository\BucketRepository;
use Symfony\Component\Validator\Constraint;
use Symfony\Component\Validator\ConstraintValidator;
use Symfony\Component\Validator\Exception\UnexpectedTypeException;
final class UniquePriorityPerScenarioValidator extends ConstraintValidator
{
public function __construct(private readonly BucketRepository $bucketRepository)
{
}
public function validate(mixed $value, Constraint $constraint): void
{
if (!$constraint instanceof UniquePriorityPerScenario) {
throw new UnexpectedTypeException($constraint, UniquePriorityPerScenario::class);
}
if (!$value instanceof Bucket) {
return;
}
if (!$value->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();
}
}
}

View file

@ -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();

View file

@ -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.',
);
}
}

View file

@ -0,0 +1,170 @@
<?php
namespace App\Tests\Validator;
use App\Entity\Bucket;
use App\Entity\Scenario;
use App\Enum\BucketAllocationType;
use App\Enum\BucketType;
use App\Tests\Concerns\CreatesUsers;
use App\Validator\UniquePriorityPerScenario;
use Doctrine\ORM\EntityManagerInterface;
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
use Symfony\Component\Validator\ConstraintViolationListInterface;
use Symfony\Component\Validator\Validator\ValidatorInterface;
final class UniquePriorityPerScenarioTest extends KernelTestCase
{
use CreatesUsers;
private EntityManagerInterface $em;
private ValidatorInterface $validator;
protected function setUp(): void
{
self::bootKernel();
$this->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;
}
}

View file

@ -0,0 +1,117 @@
<?php
namespace App\Tests\Validator;
use App\Entity\Bucket;
use App\Entity\Scenario;
use App\Enum\BucketAllocationType;
use App\Enum\BucketType;
use App\Repository\BucketRepository;
use App\Validator\SingleOverflowPerScenario;
use App\Validator\UniquePriorityPerScenario;
use App\Validator\UniquePriorityPerScenarioValidator;
use PHPUnit\Framework\MockObject\Stub;
use Symfony\Component\Validator\ConstraintValidatorInterface;
use Symfony\Component\Validator\Exception\UnexpectedTypeException;
use Symfony\Component\Validator\Test\ConstraintValidatorTestCase;
/**
* @extends ConstraintValidatorTestCase<UniquePriorityPerScenarioValidator>
*/
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;
}
}