Skip to content

fix: an account is not saved with only part of its sharing - #929

Merged
blaipr merged 1 commit into
mainfrom
fix/an-account-is-not-saved-with-only-part-of-its-sharing
Sep 17, 2026
Merged

blaipr merged 1 commit into
mainfrom
fix/an-account-is-not-saved-with-only-part-of-its-sharing

Conversation

@blaipr

@blaipr blaipr commented Sep 17, 2026

Copy link
Copy Markdown
Member

AccountItems::addItems() wrapped all five sharing writes — view/edit groups, view/edit users, tags
— in a catch (SPException) that called processException() and returned. So a failure part-way
left the account saved with whatever subset had been inserted first, or with nothing shared at all
if the first call was the one that threw, and the create answered with plain success.

AccountToUserGroup, AccountToUser and AccountToTag each carry a foreign key to the row they
name, so a group, user or tag deleted between the form being drawn and the form being submitted
throws a ConstraintException from inside one of them. That is an ordinary thing for two
administrators to do to each other on a busy installation, not a contrived race.

The edit path already does it right

updateItems()'s replaceUserGroups() and replaceUsers() run their delete-and-insert inside
transactionAware() and let failures propagate. Account::create() likewise already wraps the
whole create in transactionAware() — what was missing was letting the failure reach it. Removing
the catch is the entire fix: the exception now rolls the account back with its sharing.

The test said so

testAddItemsWithException called addItems() with a throwing repository and no
expectException, so it passed only because nothing propagated — it was pinning the swallow as
intended behaviour. It now asserts the failure is reported, and is renamed to say what it checks.

That is the second time in this repo a test has turned out to be describing a defect rather than a
decision (deleteMultiplePartialMatchIsNotDetected was the first), which is worth knowing as a
search strategy.

Tests

  • testAddItemsReportsAFailureRatherThanSavingPartOfTheSharing — mutation-verified: it fails
    against the old catch.
  • testCreateIsAbandonedWhenItsSharingCannotBeApplied — asserts the account create abandons the
    rest of its work and lets the failure out. This one is not a distinguisher: it mocks
    AccountItemsService, so it passes before and after. It is here to pin that Account::create()
    does not itself catch, which is the other half of the guarantee.

AccountItems::addItems() wrapped all five sharing writes — view/edit groups, view/edit
users, tags — in a catch (SPException) that logged and returned. So a failure part-way left
the account saved with whatever subset had been inserted first, or with nothing shared at
all if the first call threw, and the create answered with plain success.

AccountToUserGroup, AccountToUser and AccountToTag each carry a foreign key to the row they
name, so a group, user or tag deleted between the form being drawn and submitted throws a
ConstraintException from inside one of them — an ordinary thing for two administrators to
do to each other, not a contrived race.

The edit path already does it right: replaceUserGroups() and replaceUsers() run inside
transactionAware() and let failures propagate. Account::create() already wraps the whole
create in transactionAware() too; what was missing was letting the failure reach it.

testAddItemsWithException called addItems() with a throwing repository and no
expectException, so it passed only because nothing propagated — it was pinning the swallow
as intended behaviour. It now asserts the failure is reported.
@blaipr
blaipr merged commit f5f3441 into main Sep 17, 2026
8 checks passed
@blaipr
blaipr deleted the fix/an-account-is-not-saved-with-only-part-of-its-sharing branch September 17, 2026 22:31
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