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