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; }