fix: page the notification search over a total order - #897
Merged
Conversation
`Notification::getBaseSearch()`, which both `searchForUserId()` and `searchForAdmin()` build on, paged with ORDER BY date DESC and nothing else. `date` is a one-second epoch with no unique index, so notifications raised together by one bulk operation tie on the only sort key — and under LIMIT/OFFSET the database may order those ties differently for each page, putting one notification on two pages and another on none. Every other paged repository already ends its ordering with the primary key. The reason this one did not is that PagedSearchesAreTotallyOrderedTest builds each repository and calls `search()`, and Notification has no `search()` — so it could not simply be added to the hand-written list, and it wasn't. The one paged repository the guard did not cover is the one that was wrong. The provider now carries a method name per entry, and `everyPagedSearchIsListedHere()` holds the list to the source: a repository whose query reads getLimitCount() must be covered, or named as a deliberate exception with its reason. The three unpaged notification lists get the same tie-break. They cannot lose a row the way a paged query can, but they reshuffle between reads while the tie is undecided.
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.
Notification::getBaseSearch()— which bothsearchForUserId()andsearchForAdmin()build on —paged with
ORDER BY date DESCand nothing else.Notification.dateis a one-second epoch(
int(10) unsigned, no unique index), so notifications raised together by one bulk operation allcarry the same stamp and tie on the only sort key. Under
LIMIT/OFFSETthe database is free toorder those ties differently for each page: one notification arrives on two pages and another on
none. It is the same defect measured on the account search — 63 of 104 accounts on no page, 34 on
two — reached through the notification manager, for admins and regular users alike.
Every other paged repository here already ends its ordering with the primary key. This one didn't,
and the reason it didn't is the second half of the change.
Why the guard missed it
PagedSearchesAreTotallyOrderedTestholds every paged repository to the rule, from a list writtenby hand — and it builds each repository and calls
search().Notificationhas nosearch(): ithas
searchForUserId()andsearchForAdmin(). So it could not simply be added to the list, and itwasn't. The one paged repository the guard did not cover is the one that turned out to be wrong,
which is what a hand-maintained list of things to check will do eventually.
everyPagedSearchIsListedHere()now holds the list to the source: every repository file containinga query that reads
getLimitCount()must be covered by the provider, or named as a deliberateexception with its reason (
AccountSearch, whose ordering is per-request and which has its owntest). It is a static check, because reaching these methods needs their arguments and those differ;
it only has to answer which files page.
The provider's entries are now
[class, method], so a repository that pages under any name can belisted.
The change
getBaseSearch()orders bydate DESC, id DESC— the primary key settles the tie, andid DESCkeeps the newest-first reading
date DESCalready asks for.getAllForUserId,getAllActiveForUserId,getAllActiveForAdmin) get the same tie-break. They cannot lose a row the way a paged query can,but they reshuffle between reads for as long as the tie is undecided, and leaving three of four
ordering differently is how the next reader concludes the paged one is special.
Mutation-verified, both halves
Notificationcases fail, naming the methodNotificationfrom the providereveryPagedSearchIsListedHerefails, naming the class