From 20f951c017b77f5dfad8c830c6b14aa0a44c38e7 Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 3 Sep 2026 00:16:43 +0200 Subject: [PATCH] fix: a partly stale batch delete is not a success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit deleteByIdBatch() in AuthToken, Client, Category and CustomFieldDefinition refused only when the delete affected zero rows; the other nine services compare the count against count($ids). So a selection of five items of which one had already been deleted removed four and reported all five as removed — the 'update that matched nothing, reported as saved' shape, asked as the wrong question rather than not asked at all. It was already known. Three of the four unit tests stubbed one affected row for five ids and passed, because 1 !== 0, and the integration test for the fourth was called deleteMultiplePartialMatchIsNotDetected, with a docblock pinning 'the current (weaker-than-Tag) behaviour in place'. Tag, which it names as the stronger sibling, has always compared against count($ids). That test now asserts the refusal and is renamed, and the other three services get the deleteMultiplePartialFailure test Tag already had. CustomFieldDefinition's check stays inside its transactionAware() closure, so the refusal takes the partial delete with it; the other three behave exactly as Tag and User already do. --- CLAUDE.md | 9 +++++ src/Application/Auth/Services/AuthToken.php | 7 +++- .../Category/Services/Category.php | 7 +++- src/Application/Client/Services/Client.php | 7 +++- .../Services/CustomFieldDefinition.php | 5 ++- .../Controllers/AuthToken/AuthTokenTest.php | 31 ++++++++++++++ .../Web/Controllers/Category/CategoryTest.php | 32 +++++++++++++++ .../In/Web/Controllers/Client/ClientTest.php | 32 +++++++++++++++ .../CustomField/CustomFieldTest.php | 21 +++++----- .../Auth/Services/AuthTokenTest.php | 28 ++++++++++++- .../Category/Services/CategoryTest.php | 28 ++++++++++++- .../Client/Services/ClientTest.php | 28 ++++++++++++- .../Services/CustomFieldDefinitionTest.php | 40 +++++++++++++++++++ 13 files changed, 257 insertions(+), 18 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e1351eaca..da8956f1d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -553,6 +553,15 @@ mistaken for a missing row. Without that attribute this check would turn every u "not found", which is worth measuring through the application's own PDO options rather than the `mariadb` client, since the client does not set it and answers 0 for the same statement. +The same count, asked as the wrong question. `deleteByIdBatch()` has the identical shape and a +second way to get it wrong: `AuthToken`, `Client`, `Category` and `CustomFieldDefinition` refused +only when the delete affected **zero** rows, where the other nine compare it against +`count($ids)` — so a selection of five of which one was already gone came back as five removed. +Three of their unit tests stubbed "1 affected" for five ids and passed, and an integration test was +named `deleteMultiplePartialMatchIsNotDetected`, pinning the gap as known rather than fixing it. +**A test that records a weaker behaviour with a name saying so is a defect somebody chose to +describe** — read it as a lead, not as a decision, and check what its siblings do. + **A static factory a subclass cannot use.** `SPException` offers `error()`, `info()`, `critical()`, `warning()` and `from()`, each doing `new static($message, …)`. Four subclasses fix their own message and take `int $type` first instead — `AccountPermissionException`, `UnauthorizedActionException`, diff --git a/src/Application/Auth/Services/AuthToken.php b/src/Application/Auth/Services/AuthToken.php index d90c1a541..d0d01967c 100644 --- a/src/Application/Auth/Services/AuthToken.php +++ b/src/Application/Auth/Services/AuthToken.php @@ -155,7 +155,12 @@ public function delete(int $id): void */ public function deleteByIdBatch(array $ids): void { - if ($this->authTokenRepository->deleteByIdBatch($ids)->getAffectedNumRows() === 0) { + // A batch that removed fewer rows than it was given ids has not done what it was asked, + // and answering the caller with success reports the removal of something another session + // had already deleted — or of an id that never existed — as done. `=== 0` only refuses + // when *nothing* matched, so five ids of which one was stale came back as five removed. + // This is the comparison the other nine services make. + if ($this->authTokenRepository->deleteByIdBatch($ids)->getAffectedNumRows() !== count($ids)) { throw new ServiceException(__u('Error while removing the tokens'), SPException::WARNING); } } diff --git a/src/Application/Category/Services/Category.php b/src/Application/Category/Services/Category.php index 4594d8891..ee2c3fc98 100644 --- a/src/Application/Category/Services/Category.php +++ b/src/Application/Category/Services/Category.php @@ -132,7 +132,12 @@ public function delete(int $id): CategoryService */ public function deleteByIdBatch(array $ids): void { - if ($this->categoryRepository->deleteByIdBatch($ids)->getAffectedNumRows() === 0) { + // A batch that removed fewer rows than it was given ids has not done what it was asked, + // and answering the caller with success reports the removal of something another session + // had already deleted — or of an id that never existed — as done. `=== 0` only refuses + // when *nothing* matched, so five ids of which one was stale came back as five removed. + // This is the comparison the other nine services make. + if ($this->categoryRepository->deleteByIdBatch($ids)->getAffectedNumRows() !== count($ids)) { throw new ServiceException(__u('Error while deleting categories'), SPException::WARNING); } } diff --git a/src/Application/Client/Services/Client.php b/src/Application/Client/Services/Client.php index 9d7580ded..3a0c42b68 100644 --- a/src/Application/Client/Services/Client.php +++ b/src/Application/Client/Services/Client.php @@ -136,7 +136,12 @@ public function delete(int $id): ClientService */ public function deleteByIdBatch(array $ids): void { - if ($this->clientRepository->deleteByIdBatch($ids)->getAffectedNumRows() === 0) { + // A batch that removed fewer rows than it was given ids has not done what it was asked, + // and answering the caller with success reports the removal of something another session + // had already deleted — or of an id that never existed — as done. `=== 0` only refuses + // when *nothing* matched, so five ids of which one was stale came back as five removed. + // This is the comparison the other nine services make. + if ($this->clientRepository->deleteByIdBatch($ids)->getAffectedNumRows() !== count($ids)) { throw new ServiceException( __u('Error while deleting the clients'), SPException::WARNING diff --git a/src/Application/CustomField/Services/CustomFieldDefinition.php b/src/Application/CustomField/Services/CustomFieldDefinition.php index 945faafc9..6af1b659c 100644 --- a/src/Application/CustomField/Services/CustomFieldDefinition.php +++ b/src/Application/CustomField/Services/CustomFieldDefinition.php @@ -80,7 +80,10 @@ public function deleteByIdBatch(array $ids): void function () use ($ids) { $affectedNumRows = $this->customFieldDefinitionRepository->deleteByIdBatch($ids)->getAffectedNumRows(); - if ($affectedNumRows === 0) { + // Fewer rows than ids means at least one was already gone, and reporting that as + // a full removal is the defect the other nine services refuse. Inside the + // transaction, so the refusal takes the partial delete with it. + if ($affectedNumRows !== count($ids)) { throw ServiceException::warning(__u('Error while deleting the fields')); } }, diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AuthToken/AuthTokenTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AuthToken/AuthTokenTest.php index 7ec14c313..260453f56 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AuthToken/AuthTokenTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/AuthToken/AuthTokenTest.php @@ -75,6 +75,10 @@ public function create() #[Test] public function deleteMultiple() { + // The batch delete asserts that as many rows were affected as ids were sent. + $this->databaseQueryResolver = function (QueryData $queryData): QueryResult { + return new QueryResult([], 3, 0); + }; $container = $this->buildContainer( IntegrationTestCase::buildRequest( 'get', @@ -88,6 +92,33 @@ public function deleteMultiple() $this->expectOutputString('{"status":"OK","description":"Authorizations deleted","data":null}'); } + /** + * If the DELETE affects fewer rows than ids were sent — one of them no longer existed, or + * another session removed it first — the batch is reported as a failure rather than as a + * partial success. This service used to refuse only when *nothing* matched, so a selection of + * which one item was already gone came back as the whole selection removed. + * + * @throws ContainerExceptionInterface + * @throws Exception + * @throws NotFoundExceptionInterface + */ + #[Test] + public function deleteMultiplePartialFailure() + { + $this->databaseQueryResolver = function (QueryData $queryData): QueryResult { + // Only 1 of the 2 requested ids was actually removed. + return new QueryResult([], 1, 0); + }; + + $container = $this->buildContainer( + IntegrationTestCase::buildRequest('get', 'index.php', ['r' => 'authToken/delete', 'items' => [100, 200]]) + ); + + IntegrationTestCase::runApp($container); + + $this->expectOutputString('{"status":"ERROR","description":"Error while removing the tokens","data":null}'); + } + /** * @throws ContainerExceptionInterface * @throws Exception diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Category/CategoryTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Category/CategoryTest.php index 2abdefb48..ec9f8b924 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Category/CategoryTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Category/CategoryTest.php @@ -33,6 +33,7 @@ use Psr\Container\NotFoundExceptionInterface; use SP\Domain\Category\Models\Category; use SP\Domain\Common\Dtos\QueryResult; +use SP\Infrastructure\Database\QueryData; use SP\Tests\Support\BodyChecker; use SP\Tests\Support\Generators\CategoryGenerator; use SP\Tests\Support\IntegrationTestCase; @@ -68,6 +69,10 @@ public function create() #[Test] public function deleteMultiple() { + // The batch delete asserts that as many rows were affected as ids were sent. + $this->databaseQueryResolver = function (QueryData $queryData): QueryResult { + return new QueryResult([], 3, 0); + }; $container = $this->buildContainer( IntegrationTestCase::buildRequest('get', 'index.php', ['r' => 'category/delete', 'items' => [100, 200, 300]] ) @@ -78,6 +83,33 @@ public function deleteMultiple() $this->expectOutputString('{"status":"OK","description":"Categories deleted","data":null}'); } + /** + * If the DELETE affects fewer rows than ids were sent — one of them no longer existed, or + * another session removed it first — the batch is reported as a failure rather than as a + * partial success. This service used to refuse only when *nothing* matched, so a selection of + * which one item was already gone came back as the whole selection removed. + * + * @throws ContainerExceptionInterface + * @throws Exception + * @throws NotFoundExceptionInterface + */ + #[Test] + public function deleteMultiplePartialFailure() + { + $this->databaseQueryResolver = function (QueryData $queryData): QueryResult { + // Only 1 of the 2 requested ids was actually removed. + return new QueryResult([], 1, 0); + }; + + $container = $this->buildContainer( + IntegrationTestCase::buildRequest('get', 'index.php', ['r' => 'category/delete', 'items' => [100, 200]]) + ); + + IntegrationTestCase::runApp($container); + + $this->expectOutputString('{"status":"ERROR","description":"Error while deleting categories","data":null}'); + } + /** * @throws ContainerExceptionInterface * @throws Exception diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Client/ClientTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Client/ClientTest.php index 01de10f12..e6a875eb8 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Client/ClientTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Client/ClientTest.php @@ -33,6 +33,7 @@ use Psr\Container\NotFoundExceptionInterface; use SP\Domain\Client\Models\Client; use SP\Domain\Common\Dtos\QueryResult; +use SP\Infrastructure\Database\QueryData; use SP\Tests\Support\BodyChecker; use SP\Tests\Support\Generators\ClientGenerator; use SP\Tests\Support\IntegrationTestCase; @@ -68,6 +69,10 @@ public function create() #[Test] public function deleteMultiple() { + // The batch delete asserts that as many rows were affected as ids were sent. + $this->databaseQueryResolver = function (QueryData $queryData): QueryResult { + return new QueryResult([], 3, 0); + }; $container = $this->buildContainer( IntegrationTestCase::buildRequest('get', 'index.php', ['r' => 'client/delete', 'items' => [100, 200, 300]]) ); @@ -77,6 +82,33 @@ public function deleteMultiple() $this->expectOutputString('{"status":"OK","description":"Clients deleted","data":null}'); } + /** + * If the DELETE affects fewer rows than ids were sent — one of them no longer existed, or + * another session removed it first — the batch is reported as a failure rather than as a + * partial success. This service used to refuse only when *nothing* matched, so a selection of + * which one item was already gone came back as the whole selection removed. + * + * @throws ContainerExceptionInterface + * @throws Exception + * @throws NotFoundExceptionInterface + */ + #[Test] + public function deleteMultiplePartialFailure() + { + $this->databaseQueryResolver = function (QueryData $queryData): QueryResult { + // Only 1 of the 2 requested ids was actually removed. + return new QueryResult([], 1, 0); + }; + + $container = $this->buildContainer( + IntegrationTestCase::buildRequest('get', 'index.php', ['r' => 'client/delete', 'items' => [100, 200]]) + ); + + IntegrationTestCase::runApp($container); + + $this->expectOutputString('{"status":"ERROR","description":"Error while deleting the clients","data":null}'); + } + /** * @throws ContainerExceptionInterface * @throws Exception diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/CustomField/CustomFieldTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/CustomField/CustomFieldTest.php index 4a1cabcdf..ac3929e61 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/CustomField/CustomFieldTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/CustomField/CustomFieldTest.php @@ -242,11 +242,8 @@ public function deleteMultiple() } /** - * Unlike Tag's batch delete (which requires every requested id to be affected), - * CustomFieldDefinitionService::deleteByIdBatch() only checks for `$affectedNumRows === 0` - * — see CustomFieldDefinition.php lines 77-89 in src/Application/CustomField/Services/. - * So when none of the requested ids matched a row, the transactionAware() closure throws, - * the transaction rolls back, and the batch is reported as a failure. + * When none of the requested ids matched a row, the transactionAware() closure throws, the + * transaction rolls back, and the batch is reported as a failure. * * @throws ContainerExceptionInterface * @throws Exception @@ -270,17 +267,19 @@ public function deleteMultipleNoneFound() } /** - * ...but a PARTIAL match (some requested ids existed, some didn't) is NOT caught by that - * `=== 0` check, so it is reported as a full "Fields deleted" success even though some of - * the requested rows were never removed. This pins the current (weaker-than-Tag) behaviour - * in place — see the note above for the exact guard that lets it through. + * ...and so is a PARTIAL match, where some of the requested ids existed and some did not. + * + * It used not to be. The `=== 0` guard refused only when nothing at all matched, so a + * selection of which one item had already been deleted came back as "Fields deleted" while + * some of the requested rows were never removed. This test recorded that as the current, + * weaker-than-Tag behaviour; it now asserts the refusal, which is what Tag has always done. * * @throws ContainerExceptionInterface * @throws Exception * @throws NotFoundExceptionInterface */ #[Test] - public function deleteMultiplePartialMatchIsNotDetected() + public function deleteMultiplePartialMatchIsRefused() { $this->databaseQueryResolver = function (QueryData $queryData): QueryResult { // Only 1 of the 2 requested ids actually matched a row. @@ -293,7 +292,7 @@ public function deleteMultiplePartialMatchIsNotDetected() IntegrationTestCase::runApp($container); - $this->expectOutputString('{"status":"OK","description":"Fields deleted","data":null}'); + $this->expectOutputString('{"status":"ERROR","description":"Error while deleting the fields","data":null}'); } /** diff --git a/tests/Unit/Application/Auth/Services/AuthTokenTest.php b/tests/Unit/Application/Auth/Services/AuthTokenTest.php index d09812ed8..9aee1d9cd 100644 --- a/tests/Unit/Application/Auth/Services/AuthTokenTest.php +++ b/tests/Unit/Application/Auth/Services/AuthTokenTest.php @@ -90,7 +90,33 @@ public function testDeleteByIdBatch() ->expects(self::once()) ->method('deleteByIdBatch') ->with($ids) - ->willReturn(new QueryResult(null, 1)); + ->willReturn(new QueryResult(null, count($ids))); + + $this->authToken->deleteByIdBatch($ids); + } + + /** + * A batch of five ids of which one was already gone is not a removal of five. + * + * This service refused only when *nothing* matched, so an administrator selecting several + * tokens while a colleague deleted one of them was told the whole selection had been + * removed. Nine of the thirteen services already compare the count against the ids they were + * given; these four did not. + * + * @throws ServiceException + */ + public function testDeleteByIdBatchWithOneAlreadyGone() + { + $ids = array_map(fn() => self::$faker->numberBetween(1, 1000), range(0, 4)); + + $this->authTokenRepository + ->expects(self::once()) + ->method('deleteByIdBatch') + ->with($ids) + ->willReturn(new QueryResult(null, count($ids) - 1)); + + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('Error while removing the tokens'); $this->authToken->deleteByIdBatch($ids); } diff --git a/tests/Unit/Application/Category/Services/CategoryTest.php b/tests/Unit/Application/Category/Services/CategoryTest.php index dd42f2ecb..e3e0fd75b 100644 --- a/tests/Unit/Application/Category/Services/CategoryTest.php +++ b/tests/Unit/Application/Category/Services/CategoryTest.php @@ -249,7 +249,33 @@ public function testDeleteByIdBatch() ->expects(self::once()) ->method('deleteByIdBatch') ->with($ids) - ->willReturn(new QueryResult(null, 1)); + ->willReturn(new QueryResult(null, count($ids))); + + $this->category->deleteByIdBatch($ids); + } + + /** + * A batch of five ids of which one was already gone is not a removal of five. + * + * This service refused only when *nothing* matched, so an administrator selecting several + * categories while a colleague deleted one of them was told the whole selection had been + * removed. Nine of the thirteen services already compare the count against the ids they were + * given; these four did not. + * + * @throws ServiceException + */ + public function testDeleteByIdBatchWithOneAlreadyGone() + { + $ids = array_map(fn() => self::$faker->numberBetween(1, 1000), range(0, 4)); + + $this->categoryRepository + ->expects(self::once()) + ->method('deleteByIdBatch') + ->with($ids) + ->willReturn(new QueryResult(null, count($ids) - 1)); + + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('Error while deleting categories'); $this->category->deleteByIdBatch($ids); } diff --git a/tests/Unit/Application/Client/Services/ClientTest.php b/tests/Unit/Application/Client/Services/ClientTest.php index 7cd72c4d2..c0379b6f2 100644 --- a/tests/Unit/Application/Client/Services/ClientTest.php +++ b/tests/Unit/Application/Client/Services/ClientTest.php @@ -251,7 +251,33 @@ public function testDeleteByIdBatch() ->expects(self::once()) ->method('deleteByIdBatch') ->with($ids) - ->willReturn(new QueryResult(null, 1)); + ->willReturn(new QueryResult(null, count($ids))); + + $this->client->deleteByIdBatch($ids); + } + + /** + * A batch of five ids of which one was already gone is not a removal of five. + * + * This service refused only when *nothing* matched, so an administrator selecting several + * clients while a colleague deleted one of them was told the whole selection had been + * removed. Nine of the thirteen services already compare the count against the ids they were + * given; these four did not. + * + * @throws ServiceException + */ + public function testDeleteByIdBatchWithOneAlreadyGone() + { + $ids = array_map(fn() => self::$faker->numberBetween(1, 1000), range(0, 4)); + + $this->clientRepository + ->expects(self::once()) + ->method('deleteByIdBatch') + ->with($ids) + ->willReturn(new QueryResult(null, count($ids) - 1)); + + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('Error while deleting the clients'); $this->client->deleteByIdBatch($ids); } diff --git a/tests/Unit/Application/CustomField/Services/CustomFieldDefinitionTest.php b/tests/Unit/Application/CustomField/Services/CustomFieldDefinitionTest.php index e1d2891c7..e9b0eb376 100644 --- a/tests/Unit/Application/CustomField/Services/CustomFieldDefinitionTest.php +++ b/tests/Unit/Application/CustomField/Services/CustomFieldDefinitionTest.php @@ -112,6 +112,46 @@ public function testDeleteByIdBatch() $this->customFieldDefinition->deleteByIdBatch($ids); } + /** + * A batch of five ids of which one was already gone is not a removal of five. + * + * This service refused only when *nothing* matched, so an administrator selecting several + * definitions while a colleague deleted one of them was told the whole selection had been + * removed. Nine of the thirteen services already compare the count against the ids they were + * given; these four did not. Here the refusal happens inside the transaction, so it takes the + * partial delete with it. + * + * @throws ServiceException + * @throws SPException + */ + public function testDeleteByIdBatchWithOneAlreadyGone() + { + $ids = array_map(fn() => self::$faker->numberBetween(1, 1000), range(0, 4)); + + $this->customFieldDefinitionRepository + ->expects(self::once()) + ->method('transactionAware') + ->with( + new Callback(function (callable $callable) { + try { + $callable(); + } catch (ServiceException $e) { + return $e->getMessage() === 'Error while deleting the fields'; + } + + return false; + }) + ); + + $this->customFieldDefinitionRepository + ->expects(self::once()) + ->method('deleteByIdBatch') + ->with($ids) + ->willReturn(new QueryResult(null, count($ids) - 1)); + + $this->customFieldDefinition->deleteByIdBatch($ids); + } + /** * @throws ServiceException * @throws SPException