From e64eb903de90b52b7f4864681b8847d46455d67d Mon Sep 17 00:00:00 2001 From: blaipr Date: Sun, 30 Aug 2026 16:38:08 +0200 Subject: [PATCH] fix: page the notification search over a total order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .../Repositories/Notification.php | 17 ++- .../PagedSearchesAreTotallyOrderedTest.php | 110 ++++++++++++++++-- 2 files changed, 112 insertions(+), 15 deletions(-) diff --git a/src/Infrastructure/Adapter/Out/Notification/Repositories/Notification.php b/src/Infrastructure/Adapter/Out/Notification/Repositories/Notification.php index e676f03b7..b3d7b25eb 100644 --- a/src/Infrastructure/Adapter/Out/Notification/Repositories/Notification.php +++ b/src/Infrastructure/Adapter/Out/Notification/Repositories/Notification.php @@ -284,7 +284,13 @@ private function getBaseSearch(ItemSearchDto $itemSearchData): SelectInterface ->newSelect() ->from(NotificationModel::TABLE) ->cols(NotificationModel::getCols()) - ->orderBy(['date DESC']) + // The primary key settles ties, or the pages are not a partition of the results: + // `date` is a one-second epoch and nothing else here is unique, so notifications + // raised together by one bulk operation all carry the same stamp. Under LIMIT/OFFSET + // the database may then order those ties differently for each page, putting one + // notification on two pages and another on none. `id DESC` keeps the newest-first + // reading the date already asks for. + ->orderBy(['date DESC', 'id DESC']) ->limit($itemSearchData->getLimitCount()) ->offset($itemSearchData->getLimitStart()); @@ -385,7 +391,8 @@ public function getAllForUserId(int $userId): QueryResult ->cols(NotificationModel::getCols()) ->where('(userId = :userId OR (userId IS NULL AND sticky = 1)) AND onlyAdmin = 0') ->bindValues(['userId' => $userId]) - ->orderBy(['date DESC']); + // Ties on the one-second stamp, so the list keeps one order between reads. + ->orderBy(['date DESC', 'id DESC']); $queryData = QueryData::buildWithMapper($query, NotificationModel::class); @@ -406,7 +413,8 @@ public function getAllActiveForUserId(int $userId): QueryResult ->cols(NotificationModel::getCols()) ->where('(userId = :userId OR sticky = 1) AND onlyAdmin = 0 AND checked = 0') ->bindValues(['userId' => $userId]) - ->orderBy(['date DESC']); + // Ties on the one-second stamp, so the list keeps one order between reads. + ->orderBy(['date DESC', 'id DESC']); $queryData = QueryData::buildWithMapper($query, NotificationModel::class); @@ -428,7 +436,8 @@ public function getAllActiveForAdmin(int $userId): QueryResult ->cols(NotificationModel::getCols()) ->where('(userId = :userId OR sticky = 1 OR userId IS NULL) AND checked = 0') ->bindValues(['userId' => $userId]) - ->orderBy(['date DESC']); + // Ties on the one-second stamp, so the list keeps one order between reads. + ->orderBy(['date DESC', 'id DESC']); $queryData = QueryData::buildWithMapper($query, NotificationModel::class); diff --git a/tests/Unit/Infrastructure/Adapter/Out/PagedSearchesAreTotallyOrderedTest.php b/tests/Unit/Infrastructure/Adapter/Out/PagedSearchesAreTotallyOrderedTest.php index 3a6bf31a2..f0031cb6c 100644 --- a/tests/Unit/Infrastructure/Adapter/Out/PagedSearchesAreTotallyOrderedTest.php +++ b/tests/Unit/Infrastructure/Adapter/Out/PagedSearchesAreTotallyOrderedTest.php @@ -30,6 +30,8 @@ use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\Exception; +use RecursiveDirectoryIterator; +use RecursiveIteratorIterator; use ReflectionClass; use ReflectionMethod; use SP\Domain\Core\Context\Context; @@ -61,12 +63,18 @@ class PagedSearchesAreTotallyOrderedTest extends UnitaryTestCase { /** - * Every repository whose `search()` pages with LIMIT/OFFSET. + * Every repository whose search pages with LIMIT/OFFSET, and the method that does it. + * + * Most of them call it `search()`. `Notification` does not — it has `searchForUserId()` and + * `searchForAdmin()`, both building on one private `getBaseSearch()` — and that is exactly how + * it came to be missing from this list while its paged query ordered by `date DESC` alone. + * `everyPagedSearchIsListedHere()` below now fails when a repository pages and is absent, + * whatever it calls the method. * * `AccountSearch` is absent because its ordering is chosen per request rather than fixed here, * and `AccountSearchTest::testEveryOrderingIsTotal` covers each of its sort keys instead. * - * @return array + * @return array */ public static function pagedRepositoryProvider(): array { @@ -92,12 +100,91 @@ public static function pagedRepositoryProvider(): array $cases = []; foreach ($classes as $class) { - $cases[substr((string)strrchr($class, '\\'), 1)] = [$class]; + $cases[substr((string)strrchr($class, '\\'), 1)] = [$class, 'search']; + } + + foreach (['searchForUserId', 'searchForAdmin'] as $method) { + $cases['Notification::' . $method] = [ + \SP\Infrastructure\Adapter\Out\Notification\Repositories\Notification::class, + $method, + ]; } return $cases; } + /** + * The list above is written by hand, and a repository that pages under a method named anything + * else is invisible to it — which is what happened to `Notification`, for as long as its search + * ordered by a one-second stamp with no tie-break. + * + * So the list is held to the source: every repository with a query that reads + * `getLimitCount()` must be covered above, or named here as a deliberate exception with the + * reason. Static, because reaching these methods needs their arguments, which differ; this only + * has to answer which files page. + */ + #[Test] + public function everyPagedSearchIsListedHere(): void + { + $covered = array_unique(array_map(static fn(array $case) => $case[0], self::pagedRepositoryProvider())); + + // Its ordering is chosen per request; AccountSearchTest::testEveryOrderingIsTotal covers it. + $exempt = [\SP\Infrastructure\Adapter\Out\Account\Repositories\AccountSearch::class]; + + $missing = []; + + foreach (self::repositoryFiles() as $file) { + if (!str_contains((string)file_get_contents($file), 'getLimitCount()')) { + continue; + } + + $class = self::classFor($file); + + if (!in_array($class, $covered, true) && !in_array($class, $exempt, true)) { + $missing[] = $class; + } + } + + self::assertSame( + [], + $missing, + 'these repositories page and are covered by nothing: ' . implode(', ', $missing) + ); + } + + /** + * @return string[] + */ + private static function repositoryFiles(): array + { + $files = []; + + $directory = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator(REAL_APP_ROOT . '/src/Infrastructure/Adapter/Out') + ); + + foreach ($directory as $file) { + if ($file->isFile() + && $file->getExtension() === 'php' + && str_contains($file->getPathname(), DIRECTORY_SEPARATOR . 'Repositories' . DIRECTORY_SEPARATOR)) { + $files[] = $file->getPathname(); + } + } + + sort($files); + + return $files; + } + + private static function classFor(string $file): string + { + $source = (string)file_get_contents($file); + + preg_match('/^namespace\s+([^;]+);/m', $source, $namespace); + + return $namespace[1] . '\\' . basename($file, '.php'); + } + /** * @param class-string $repositoryClass * @@ -105,9 +192,9 @@ public static function pagedRepositoryProvider(): array */ #[Test] #[DataProvider('pagedRepositoryProvider')] - public function aPagedSearchOrdersByThePrimaryKeyLast(string $repositoryClass): void + public function aPagedSearchOrdersByThePrimaryKeyLast(string $repositoryClass, string $method): void { - $statement = $this->captureSearchStatement($repositoryClass); + $statement = $this->captureSearchStatement($repositoryClass, $method); self::assertStringContainsStringIgnoringCase( 'LIMIT', @@ -118,7 +205,7 @@ public function aPagedSearchOrdersByThePrimaryKeyLast(string $repositoryClass): self::assertSame( 'id', self::lastOrderedColumn($statement), - $repositoryClass . ': a paged search must order by the primary key last, or its pages' + $repositoryClass . '::' . $method . ': a paged search must order by the primary key last, or its pages' . ' are not a partition of the results' ); } @@ -128,7 +215,7 @@ public function aPagedSearchOrdersByThePrimaryKeyLast(string $repositoryClass): * * @throws Exception */ - private function captureSearchStatement(string $repositoryClass): string + private function captureSearchStatement(string $repositoryClass, string $method = 'search'): string { $statement = null; @@ -148,14 +235,15 @@ static function (QueryData $queryData) use (&$statement) { $arguments = [new ItemSearchDto(null, 10, 10)]; - if ((new ReflectionMethod($repositoryClass, 'search'))->getNumberOfRequiredParameters() > 1) { - // Track::search() also takes the window it counts attempts within. + if ((new ReflectionMethod($repositoryClass, $method))->getNumberOfRequiredParameters() > 1) { + // Track::search() also takes the window it counts attempts within, and + // Notification's two take the user whose notifications they are. $arguments[] = time(); } - $repository->search(...$arguments); + $repository->{$method}(...$arguments); - self::assertIsString($statement, $repositoryClass . ': no query was run'); + self::assertIsString($statement, $repositoryClass . '::' . $method . ': no query was run'); return $statement; }