fix: a history purge reports what it removed - #878
Open
blaipr wants to merge 2 commits into
Open
Conversation
`AccountHistory::delete()` throws when the row it was given did not exist. The batch beside it returned the affected count and left it there, and both callers — the history manager's delete and the bulk edit's "delete history" option — discard the return and answer success regardless. So selecting a set of history entries to destroy, where some of them no longer matched — a stale id, a row another session had already removed — was reported as a purge that had happened. History is where an account's previous passwords live, so the thing the operator believes they destroyed is precisely the thing that is still there. deleteByIdBatch() is given history row ids, so every one of them should have matched, and it now throws when the count differs — the check the single delete has always made and the one PublicLink::deleteByIdBatch() makes for the same reason. deleteByAccountIdBatch() is deliberately left alone, and now says so: those are *account* ids, and an account may have no history at all, so removing nothing from one is the right answer rather than a failure. There is no count there to compare against, and holding it to the same rule would fail a purge that had done exactly what it was asked. Checked by making the comparison always false: the new test fails and the one pinning the account-id case does not.
The integration harness answers every statement with one affected row, so a delete given three history ids reported one — which the service now reads, correctly, as a purge that did not remove what it was asked to. The double answers three for the delete in that test. This is the harness modelling the server rather than the test being bent around the change: a real DELETE matching three rows reports three, and the whole point of the check is that anything less is not a purge that happened.
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.
AccountHistory::delete()throws when the row it was given did not exist. The batchbeside it returned the affected count and left it there, and both callers — the history
manager's delete and the bulk edit's "delete history" option — discard the return and
answer success regardless.
So selecting a set of history entries to destroy, where some of them no longer matched —
a stale id, a row another session had already removed — was reported as a purge that had
happened. History is where an account's previous passwords live, so the thing the operator
believes they destroyed is precisely the thing that is still there.
deleteByIdBatch() is given history row ids, so every one of them should have matched, and
it now throws when the count differs — the check the single delete has always made and
the one PublicLink::deleteByIdBatch() makes for the same reason.
deleteByAccountIdBatch() is deliberately left alone, and now says so: those are account
ids, and an account may have no history at all, so removing nothing from one is the right
answer rather than a failure. There is no count there to compare against, and holding it
to the same rule would fail a purge that had done exactly what it was asked.
Checked by making the comparison always false: the new test fails and the one pinning the
account-id case does not.