From 53001de740827aec46d952445ca2f096c112e7a2 Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 00:31:29 +0200 Subject: [PATCH] fix: clearing one custom field must not delete the others MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The save path deleted a custom field's row when its posted value was empty, which is right — saving the blank instead would leave the old ciphertext behind. But the delete it made was `delete([$itemId], $moduleId)`, and that is keyed on the item and the module with no definition at all: it removes *every* custom field value on the item. So emptying one field on any item that has more than one wiped the rest of them in the same save, including encrypted values the person saving could not see and had not touched. Nothing reported it. The save succeeded and the other fields simply came back blank, which looks like the fields were never filled in rather than like data loss. deleteBatch() is keyed that way because it exists for the other job — deleting the item takes its fields with it — and the single-field case had been given the same method. This adds deleteForDefinition() through the repository, both ports and the service, and leaves deleteBatch() to the case it was written for. Also narrows updateCustomFieldsForItem()'s $itemId from `int|array` to `int`. Every caller passes a single id, and the array form was never workable here: it is written straight into the model's itemId, so an array would have been stored as one. The wider type came from the batch delete, where it does make sense. Checked by putting the old call back: the test that pins which row is deleted fails, and so does the one asserting the fields either side of a cleared one survive. --- .../Ports/CustomFieldDataService.php | 11 +++++ .../CustomField/Services/CustomFieldData.php | 12 ++++++ src/Domain/Common/Adapters/ItemTrait.php | 9 ++-- .../Ports/CustomFieldDataRepository.php | 13 ++++++ .../Repositories/CustomFieldData.php | 34 +++++++++++++++ .../Domain/Common/Adapters/ItemTraitTest.php | 43 ++++++++++++++++++- .../Repositories/CustomFieldDataTest.php | 39 +++++++++++++++++ 7 files changed, 157 insertions(+), 4 deletions(-) diff --git a/src/Application/CustomField/Ports/CustomFieldDataService.php b/src/Application/CustomField/Ports/CustomFieldDataService.php index 9788a9233..3778aa05b 100644 --- a/src/Application/CustomField/Ports/CustomFieldDataService.php +++ b/src/Application/CustomField/Ports/CustomFieldDataService.php @@ -68,6 +68,17 @@ public function updateOrCreate(CustomFieldData $customFieldData): void; */ public function delete(array $itemsId, int $moduleId): void; + /** + * Delete one field's value, leaving the item's other fields alone + * + * @param int $itemId + * @param int $moduleId + * @param int $definitionId + * + * @throws ServiceException + */ + public function deleteForDefinition(int $itemId, int $moduleId, int $definitionId): void; + /** * Creates an item * diff --git a/src/Application/CustomField/Services/CustomFieldData.php b/src/Application/CustomField/Services/CustomFieldData.php index 2aa402b93..81fc12518 100644 --- a/src/Application/CustomField/Services/CustomFieldData.php +++ b/src/Application/CustomField/Services/CustomFieldData.php @@ -197,6 +197,18 @@ public function delete(array $itemsId, int $moduleId): void } } + /** + * @inheritDoc + */ + public function deleteForDefinition(int $itemId, int $moduleId, int $definitionId): void + { + try { + $this->customFieldDataRepository->deleteForDefinition($itemId, $moduleId, $definitionId); + } catch (SPException $e) { + throw ServiceException::from($e); + } + } + /** * Updates an item * diff --git a/src/Domain/Common/Adapters/ItemTrait.php b/src/Domain/Common/Adapters/ItemTrait.php index e7a95df22..dc54ff479 100644 --- a/src/Domain/Common/Adapters/ItemTrait.php +++ b/src/Domain/Common/Adapters/ItemTrait.php @@ -191,7 +191,7 @@ protected function deleteCustomFieldsForItem( * Update the item's custom fields * * @param int $moduleId - * @param int|int[] $itemId + * @param int $itemId * @param RequestService $request * @param CustomFieldDataService $customFieldDataService * @@ -200,7 +200,7 @@ protected function deleteCustomFieldsForItem( */ protected function updateCustomFieldsForItem( int $moduleId, - int|array $itemId, + int $itemId, RequestService $request, CustomFieldDataService $customFieldDataService ): void { @@ -233,7 +233,10 @@ protected function updateCustomFieldsForItem( ); if (empty($customFieldData->getData())) { - $customFieldDataService->delete([$itemId], $moduleId); + // Only this field. delete() is keyed on the item and the module alone — which + // is what deleting the item itself wants — so clearing one field used to take + // every other custom field on the item with it, silently, in the same save. + $customFieldDataService->deleteForDefinition($itemId, $moduleId, $id); } else { $customFieldDataService->updateOrCreate($customFieldData); } diff --git a/src/Domain/CustomField/Ports/CustomFieldDataRepository.php b/src/Domain/CustomField/Ports/CustomFieldDataRepository.php index 47ce41b6a..753911bce 100644 --- a/src/Domain/CustomField/Ports/CustomFieldDataRepository.php +++ b/src/Domain/CustomField/Ports/CustomFieldDataRepository.php @@ -87,6 +87,19 @@ public function create(CustomFieldDataModel $customFieldData): QueryResult; */ public function deleteBatch(array $itemIds, int $moduleId): QueryResult; + /** + * Delete one field's value, leaving the item's other fields alone + * + * @param int $itemId + * @param int $moduleId + * @param int $definitionId + * + * @return QueryResult + * @throws QueryException + * @throws ConstraintException + */ + public function deleteForDefinition(int $itemId, int $moduleId, int $definitionId): QueryResult; + /** * Returns all the items that were encrypted * diff --git a/src/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldData.php b/src/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldData.php index 30c627d0c..f22204da9 100644 --- a/src/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldData.php +++ b/src/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldData.php @@ -157,6 +157,40 @@ public function deleteBatch(array $itemIds, int $moduleId): QueryResult return $this->db->runQuery(QueryData::build($query)); } + /** + * Delete one field's value, leaving the item's other fields alone. + * + * deleteBatch() is keyed on the item and the module only, which is what deleting the item + * itself wants. Clearing a single field needs the definition in the WHERE too, or it takes + * every other field on the item with it. + * + * @param int $itemId + * @param int $moduleId + * @param int $definitionId + * + * @return QueryResult + * @throws QueryException + * @throws ConstraintException + */ + public function deleteForDefinition(int $itemId, int $moduleId, int $definitionId): QueryResult + { + $query = $this->queryFactory + ->newDelete() + ->from(CustomFieldDataModel::TABLE) + ->where('moduleId = :moduleId') + ->where('itemId = :itemId') + ->where('definitionId = :definitionId') + ->bindValues( + [ + 'moduleId' => $moduleId, + 'itemId' => $itemId, + 'definitionId' => $definitionId, + ] + ); + + return $this->db->runQuery(QueryData::build($query)); + } + /** * Returns all the items * diff --git a/tests/Unit/Domain/Common/Adapters/ItemTraitTest.php b/tests/Unit/Domain/Common/Adapters/ItemTraitTest.php index 53f31afaf..c1966f0d1 100644 --- a/tests/Unit/Domain/Common/Adapters/ItemTraitTest.php +++ b/tests/Unit/Domain/Common/Adapters/ItemTraitTest.php @@ -229,17 +229,58 @@ public function updatingSavesTheFieldsThatHaveAValue() /** * Clearing a field deletes the row rather than saving an empty one. Only the delete removes the * stored value, so saving the blank would leave the old ciphertext behind. + * + * It deletes that field, keyed on its definition. It used to call delete(), which is keyed on + * the item and the module alone. */ #[Test] public function clearingAFieldDeletesItRatherThanSavingItEmpty() { $service = $this->createMock(CustomFieldDataService::class); $service->expects(self::never())->method('updateOrCreate'); - $service->expects(self::once())->method('delete')->with([7], self::MODULE_ID); + $service->expects(self::never())->method('delete'); + $service->expects(self::once())->method('deleteForDefinition')->with(7, self::MODULE_ID, 4); $this->host->updateCustomFields(self::MODULE_ID, 7, $this->givenThePostedFields([4 => '']), $service); } + /** + * Clearing one field leaves the item's other fields alone. + * + * The delete this used to make carried only the item and the module, so emptying a single + * field removed every custom field value on the item — including encrypted ones the person + * saving could not see and had not touched. Nothing reported it: the save succeeded and the + * other fields simply came back blank. + */ + #[Test] + public function clearingOneFieldLeavesTheOthersAlone() + { + $deleted = []; + $saved = []; + + $service = $this->createStub(CustomFieldDataService::class); + $service->method('deleteForDefinition')->willReturnCallback( + static function (int $itemId, int $moduleId, int $definitionId) use (&$deleted): void { + $deleted[] = $definitionId; + } + ); + $service->method('updateOrCreate')->willReturnCallback( + static function ($customFieldData) use (&$saved): void { + $saved[] = $customFieldData->getDefinitionId(); + } + ); + + $this->host->updateCustomFields( + self::MODULE_ID, + 7, + $this->givenThePostedFields([4 => 'kept', 5 => '', 6 => 'also kept']), + $service + ); + + self::assertSame([5], $deleted, 'only the cleared field is deleted'); + self::assertSame([4, 6], $saved, 'the others are still saved'); + } + /** * A masked value is not saved over the secret it stands in for. * diff --git a/tests/Unit/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldDataTest.php b/tests/Unit/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldDataTest.php index 781245e54..39de7dbad 100644 --- a/tests/Unit/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldDataTest.php +++ b/tests/Unit/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldDataTest.php @@ -104,6 +104,45 @@ public function testDeleteBatchWithNoItems() $this->assertEquals(0, $result->getNumRows()); } + /** + * The delete that clears one field carries the definition as well as the item and the module. + * + * deleteBatch() above is keyed on the item and the module only — right for deleting the item, + * wrong for clearing one of its fields, which used to take every other field with it. The + * assertion is on the emitted statement's bound values rather than on a row count, because a + * WHERE clause that is missing a condition still deletes successfully. + * + * @throws ConstraintException + * @throws QueryException + */ + public function testDeleteForDefinitionIsKeyedOnTheDefinitionToo() + { + $itemId = self::$faker->numberBetween(1, 1000); + $moduleId = self::$faker->numberBetween(1, 1000); + $definitionId = self::$faker->numberBetween(1, 1000); + + $callback = new Callback( + static function (QueryData $arg) use ($itemId, $moduleId, $definitionId) { + $query = $arg->getQuery(); + $bindValues = $query->getBindValues(); + + return count($bindValues) === 3 + && $bindValues['moduleId'] === $moduleId + && $bindValues['itemId'] === $itemId + && $bindValues['definitionId'] === $definitionId + && is_a($query, DeleteInterface::class) + && !empty($query->getStatement()); + } + ); + + $this->database + ->expects(self::once()) + ->method('runQuery') + ->with($callback); + + $this->customFieldData->deleteForDefinition($itemId, $moduleId, $definitionId); + } + /** * @throws ConstraintException * @throws QueryException