fix: an edit that says nothing about sharing leaves it alone - #875
Merged
blaipr merged 1 commit intoAug 24, 2026
Merged
Conversation
`AccountItems::updateItems()` read `null` as "delete every row of this type" and an empty
array as nothing at all. Both are the wrong way round. `null` is what a caller sends when
they said nothing about a list, and an empty array is what they send to empty it.
The consequence on the API is the serious one. `AccountUpdateDto`'s four sharing lists are
never set by `Account/EditController` — there is no REST parameter for them and no
endpoint for account sharing anywhere — so they were always null. Every `PUT` to an
account, by a caller whose profile can manage permissions, silently deleted all four kinds
of sharing on the account it was editing. A script correcting a URL removed everyone's
access to the account, reported success, and left nothing in the response to say so.
The web reached the same place by a different route: `AccountForm::analyzeItems()` only
sets a list when the matching `_update` flag is posted, and the theme posts that only for
a select the user actually changed — so an edit that touched the name and nothing else
sent null for all four.
And the third case is the one that was supposed to work: the bulk edit's "Delete"
checkboxes post exactly the empty array, which matched neither branch. The one deliberate
way to clear an account's sharing was the one way that did nothing, while answering
"Accounts updated".
So: null leaves the list alone, an empty array clears it, a non-empty array replaces it.
The delete and the add are one transaction, as before, so a failure adding leaves the
existing rows rather than an account shared with nobody.
The tags carried the identical two branches and are aligned with the rest — a rule that
holds for four of five lists is not a rule. That needed the API's own default changing
too: `getParamArray('tagsId', false, [])` handed an empty array when the parameter was
absent, which under the corrected reading would mean "clear the tags". It is null now,
and an empty array from a caller who did supply one still clears them.
The existing test pinned the old contract — it asserted the two deletes that this change
removes — so it is rewritten rather than kept, alongside a new one for the empty-array
case that never had one.
blaipr
deleted the
fix/an-edit-that-says-nothing-about-sharing-leaves-it-alone
branch
August 24, 2026 00:29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AccountItems::updateItems()readnullas "delete every row of this type" and an emptyarray as nothing at all. Both are the wrong way round.
nullis what a caller sends whenthey said nothing about a list, and an empty array is what they send to empty it.
The consequence on the API is the serious one.
AccountUpdateDto's four sharing lists arenever set by
Account/EditController— there is no REST parameter for them and noendpoint for account sharing anywhere — so they were always null. Every
PUTto anaccount, by a caller whose profile can manage permissions, silently deleted all four kinds
of sharing on the account it was editing. A script correcting a URL removed everyone's
access to the account, reported success, and left nothing in the response to say so.
The web reached the same place by a different route:
AccountForm::analyzeItems()onlysets a list when the matching
_updateflag is posted, and the theme posts that only fora select the user actually changed — so an edit that touched the name and nothing else
sent null for all four.
And the third case is the one that was supposed to work: the bulk edit's "Delete"
checkboxes post exactly the empty array, which matched neither branch. The one deliberate
way to clear an account's sharing was the one way that did nothing, while answering
"Accounts updated".
So: null leaves the list alone, an empty array clears it, a non-empty array replaces it.
The delete and the add are one transaction, as before, so a failure adding leaves the
existing rows rather than an account shared with nobody.
The tags carried the identical two branches and are aligned with the rest — a rule that
holds for four of five lists is not a rule. That needed the API's own default changing
too:
getParamArray('tagsId', false, [])handed an empty array when the parameter wasabsent, which under the corrected reading would mean "clear the tags". It is null now,
and an empty array from a caller who did supply one still clears them.
The existing test pinned the old contract — it asserted the two deletes that this change
removes — so it is rewritten rather than kept, alongside a new one for the empty-array
case that never had one.