Skip to content

fix: page the notification search over a total order - #897

Merged
blaipr merged 1 commit into
mainfrom
fix/the-notification-search-pages-a-total-order
Aug 30, 2026
Merged

fix: page the notification search over a total order#897
blaipr merged 1 commit into
mainfrom
fix/the-notification-search-pages-a-total-order

Conversation

@blaipr

@blaipr blaipr commented Aug 30, 2026

Copy link
Copy Markdown
Member

Notification::getBaseSearch() — which both searchForUserId() and searchForAdmin() build on —
paged with ORDER BY date DESC and nothing else. Notification.date is a one-second epoch
(int(10) unsigned, no unique index), so notifications raised together by one bulk operation all
carry the same stamp and tie on the only sort key. Under LIMIT/OFFSET the database is free to
order 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

PagedSearchesAreTotallyOrderedTest holds every paged repository to the rule, from a list written
by hand — and it builds each repository and calls search(). Notification has no search(): it
has searchForUserId() and searchForAdmin(). So it could not simply be added to the list, and it
wasn'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 containing
a query that reads getLimitCount() must be covered by the provider, or named as a deliberate
exception with its reason (AccountSearch, whose ordering is per-request and which has its own
test). 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 be
listed.

The change

  • getBaseSearch() orders by date DESC, id DESC — the primary key settles the tie, and id DESC
    keeps the newest-first reading date DESC already asks for.
  • The three unpaged notification lists (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

mutation result
revert the ordering fix the two new Notification cases fail, naming the method
drop Notification from the provider everyPagedSearchIsListedHere fails, naming the class

`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.
@blaipr
blaipr merged commit 508cfee into main Aug 30, 2026
8 checks passed
@blaipr
blaipr deleted the fix/the-notification-search-pages-a-total-order branch August 30, 2026 14:50
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