Skip to content

fix: a partly stale batch delete is not a success - #899

Merged
blaipr merged 1 commit into
mainfrom
fix/a-partly-stale-batch-delete-is-not-a-success
Sep 2, 2026
Merged

fix: a partly stale batch delete is not a success#899
blaipr merged 1 commit into
mainfrom
fix/a-partly-stale-batch-delete-is-not-a-success

Conversation

@blaipr

@blaipr blaipr commented Sep 2, 2026

Copy link
Copy Markdown
Member

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 — by a colleague,
or in an earlier attempt — removed four and reported all five as removed.

This is the shape the record already names as an update that matched nothing, reported as saved,
asked as the wrong question rather than not asked at all: the count was being read, and compared
against something that only catches the total failure.

It was known

Three of the four unit tests for these services 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 saying it "pins the current
(weaker-than-Tag) behaviour in place". That is a description of the defect, not a reason for it —
Tag, which it names as the stronger sibling, has always compared against count($ids), and has
both a deleteMultiple and a deleteMultiplePartialFailure test.

So the pinning test now asserts the refusal and is renamed deleteMultiplePartialMatchIsRefused,
and the other three services get the deleteMultiplePartialFailure test Tag already had.

The change

Four === 0 comparisons become !== count($ids). Nothing else moves: CustomFieldDefinition's
check stays inside its transactionAware() closure, so the refusal takes the partial delete with
it; the other three have no transaction, and behave exactly as Tag, User and the rest already do
— some rows removed, and an error saying the batch was not completed.

Tests

  • Four new unit tests, one per service, each stubbing count($ids) - 1 affected. Mutation-verified:
    reverting the four services fails all four, and only those four.
  • Three new integration tests matching TagTest::deleteMultiplePartialFailure.
  • The three integration deleteMultiple tests now stub a full match explicitly, the way Tag's
    already did, instead of relying on the harness default of one affected row.

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.
@blaipr
blaipr merged commit 4f08619 into main Sep 2, 2026
8 checks passed
@blaipr
blaipr deleted the fix/a-partly-stale-batch-delete-is-not-a-success branch September 2, 2026 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant