fix(api): filter notifications before ordering and pagination (#243)

[thomasluizon]

Problem

GetNotificationsQuery routes its OrderByDescending(CreatedAtUtc).Take(50) through the FindAsync(predicate, transform, ct) overload, but that overload applied the caller-supplied transform before the Where(predicate):

IQueryable<T> query = _dbSet.AsNoTracking();
if (includes is not null)
    query = includes(query).AsSplitQuery();   // OrderBy + Take run here, over ALL rows
return await query.Where(predicate).ToListAsync(ct);  // predicate applied AFTER

EF pushes the Take into a subquery evaluated across every user's notifications, then filters. So the page is taken from the wrong set — a user receives zero of their own notifications whenever the global newest 50 rows all belong to other users.

Fix

Apply the predicate before the caller transform in all three transform overloads (FindAsync, FindTrackedAsync, FindOneTrackedAsync) — the identical structural flaw lives in each:

IQueryable<T> query = _dbSet.AsNoTracking().Where(predicate);
if (includes is not null)
    query = includes(query).AsSplitQuery();
return await query.ToListAsync(ct);

Every other caller of these overloads passes a pure .Include(...) (verified across the solution). Where().Include() and Include().Where() emit identical SQL, so the reorder is behavior-preserving for those callers and corrects the filtered + ordered + paged result for notifications.

No API/DTO/endpoint contract change — response shape is unchanged, so no client-side (orbit-ui-mobile) update is required.

Tests

New FindAsyncPredicateBeforeTransformTests (SQLite-backed real query provider, mirroring the existing DB-test harness):

• Handle_ReturnsTargetUsersNotifications_WhenAnotherUserOwnsAllNewestRows — the failure mode: another user owns all 60 newest rows; the target's 5 older rows must still be returned. Fails on old code (returns 0).
• Handle_CapsAtMaxReturned_AndKeepsNewestWithinUserScope — caps at 50 within the user's scope and returns the newest 50 in descending order. Fails on old code (returns 0).
• Handle_ExcludesSoftDeletedNotifications — soft-deleted rows excluded.
• FindAsync / FindTrackedAsync / FindOneTrackedAsync _WithIncludeTransform_... — predicate filters correctly and the .Include(...) navigation still loads after the reorder (behavior-preservation guard for the ~40 include callers).

Verified the two behavior tests fail against the pre-fix ordering (each returns 0 rows) and pass after — so they genuinely catch the bug. Full suites green: Infrastructure 1656 passed, Application 2651 passed.

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Code Review: PR #348 (thomasluizon/orbit-api)

Scope: PR #348 — fix(api): filter notifications before ordering and pagination (#243)
Recommendation: APPROVE

Summary

This PR fixes a real, well-diagnosed bug: GenericRepository's three transform-accepting overloads (FindAsync, FindOneTrackedAsync, FindTrackedAsync) applied the caller's includes transform (which can carry OrderBy/Take) before the Where(predicate) filter, so GetNotificationsQuery's OrderByDescending().Take(50) ran over every user's notifications before scoping to the caller — a user could get zero of their own notifications. The fix reorders Where before the transform in all three overloads. I independently verified the root-cause claim and the "every other caller passes a pure .Include(...)" claim by grepping all ~50 call sites of these three overloads across Orbit.Application; none besides GetNotificationsQuery embed OrderBy/Take/Skip in the transform, so the reorder is behavior-preserving everywhere else. The new test suite genuinely exercises the failure mode (another user owns all newest rows) and the fix.

Findings

Critical

None.

High

None.

Medium

[MEDIUM] Third copy-paste of the identical SqliteCompatOrbitDbContext test-harness shim
· dimension: 3 (SOLID / clean architecture — DRY, CLAUDE.md rule 10)
· location: orbit-api/tests/Orbit.Infrastructure.Tests/Persistence/FindAsyncPredicateBeforeTransformTests.cs:201-219
· issue: The private nested `SqliteCompatOrbitDbContext` (strips Postgres `::` cast default-value SQL and index filters for SQLite compatibility) is byte-for-byte identical to the same nested class already duplicated in tests/Orbit.Infrastructure.Tests/Persistence/AccountResetRepositoryTests.cs:280-298 and tests/Orbit.Infrastructure.Tests/Behaviors/IdempotencyBehaviorDbTests.cs:242-260. This PR adds the third occurrence.
· risk: Per CLAUDE.md rule 6/10, three near-identical uses should be extracted rather than copy-pasted again; a future EF/Postgres-cast change now has to be hand-applied in three places, and it's easy to update two and miss the third, causing silent SQLite/Postgres test drift.
· fix: Extract `SqliteCompatOrbitDbContext` into a shared internal test helper (e.g. `tests/Orbit.Infrastructure.Tests/Persistence/SqliteCompatOrbitDbContext.cs` or a `TestHelpers/` folder) and have all three test classes reference it. Not a blocker for this PR given the pattern predates it, but flagging now that the threshold is crossed.
· reference: CLAUDE.md rule 6 ("extract on the third real use, not the second"), rule 10 (DRY at the right level)

This is pre-existing-pattern duplication that this PR's new file pushes past the "third use" DRY threshold — flagged per the rubric's own signal gate as a concretely-actionable Medium, not a blocker.

Low / Info

None posted (per the rubric's signal gate, Low/Info are not surfaced on a PR review).

Subagents

Agent	Verdict
security-reviewer	PASS — reorder strictly improves tenant isolation (predicate now scopes the query before any pagination/ordering runs over it); no bypass path found; FindOneTrackedAsync's FirstOrDefaultAsync(predicate, ct) → .Where(predicate).FirstOrDefaultAsync(ct) change is EF-semantically identical; no injection risk (pure LINQ expression trees); no secrets/PII in the new test file.
contract-aligner	N/A — diff touches only Orbit.Infrastructure's repository implementation and a test file; the IGenericRepository interface signatures are unchanged, and no DTO/Controller route/packages/shared surface is touched. Not gated in.

Validation

Check	Result
Build (dotnet)	N/A — skipped per CI job scope; separate required check covers it
Tests (dotnet)	N/A — skipped per CI job scope; separate required check covers it

Deferred — N/A dimensions & files not verdicted

• Dimension 8 (DESIGN.md/AI-slop) — N/A, no apps/* UI files in this diff (backend-only repo).
• Dimension 9 (Parity web↔mobile) — N/A, orbit-api-only diff; also not verifiable in CI (orbit-ui-mobile not checked out).
• Dimension 10 (i18n) — N/A, no user-facing strings changed.
• Dimension 11 (Contract drift) — N/A on substance (interface signatures unchanged, no DTO/endpoint touched, confirmed by reading IGenericRepository.cs); additionally not fully verifiable in CI since orbit-ui-mobile is not checked out.
• Dimension 14 (FEATURES.md parity) — N/A, pure bugfix with no new/changed/removed user-facing feature, tool, or plan-gating behavior.
• Both changed files (src/Orbit.Infrastructure/Persistence/GenericRepository.cs, tests/Orbit.Infrastructure.Tests/Persistence/FindAsyncPredicateBeforeTransformTests.cs) received a full verdict — nothing deferred there.
• Phase 6 adversarial/second-opinion pass: no Critical or High findings survived to trigger it (protocol scopes the skeptic + cross-model step to Critical/High only).

What's good

• Precise root-cause diagnosis with a minimal, surgical fix — no workaround, no defensive branch, exactly the three affected overloads touched.
• The claim "every other caller passes a pure .Include(...)" was independently verified across all ~50 call sites in Orbit.Application; it holds, so the reorder is behavior-preserving everywhere except the one place it needed to change.
• Tests genuinely reproduce the bug (author states, and the scenario design confirms, the two behavior tests fail against pre-fix ordering) rather than just asserting the fix's output in isolation — including a within-scope pagination-cap test and a soft-delete exclusion test.
• Behavior-preservation guard tests added for all three overloads' include-transform path, covering the ~40 other callers by construction rather than by inspection alone.
• No API/DTO/contract surface touched, so correctly no orbit-ui-mobile changes are needed — the PR body's own parity claim checks out.

Recommendation

Merge as-is. The one Medium finding (extracting the now-thrice-duplicated SqliteCompatOrbitDbContext test shim) is a good follow-up but not a blocker — it doesn't affect correctness or the fix's validity, and touches only test infrastructure.

[claude]

Code Review: PR #348 (thomasluizon/orbit-api)

Scope: PR #348 — fix(api): filter notifications before ordering and pagination (#243)
Recommendation: APPROVE

Nothing to review — diff is unchanged since the last review

This PR already has a prior review from this bot (claude), submitted 2026-07-12T17:40:58Z, recommending APPROVE. Verified:

• Head is 7752dc60c79d57c19b82a0dd04fd3f9c44df410c, matching the current PR head exactly.
• The only commit added after the prior review's timestamp is 7752dc6 Merge branch 'main' into fix/notifications-query-filter-before-pagination (17:56:31Z, after the 17:40:58Z review).
• git diff ec84849 7752dc6 -- src/Orbit.Infrastructure/Persistence/GenericRepository.cs tests/Orbit.Infrastructure.Tests/Persistence/FindAsyncPredicateBeforeTransformTests.cs shows the merge only pulled in unrelated content from main (SumAsync / FindPagedAsync from the separately-merged #347 perf PR) — it did not touch the Where(predicate) reorder hunks or the new test file this PR actually changes.
• gh pr diff 348 is byte-identical to the diff the prior review evaluated: the three-overload reorder in GenericRepository.cs plus the new FindAsyncPredicateBeforeTransformTests.cs.
• No unresolved review threads — only a sonarqubecloud Quality-Gate-Passed comment plus the prior claude APPROVE review exist.

Independently re-read the current GenericRepository.cs — the three transform overloads (FindAsync, FindOneTrackedAsync, FindTrackedAsync) all now apply .Where(predicate) before the includes(query) transform, matching the described fix.

Findings

Critical / High: None.

Medium (carried over from prior review, non-blocking):

Third copy-paste of the identical SqliteCompatOrbitDbContext test-harness shim
· location: tests/Orbit.Infrastructure.Tests/Persistence/FindAsyncPredicateBeforeTransformTests.cs:201-219
· issue: nested SqliteCompatOrbitDbContext is byte-for-byte identical to copies in
  AccountResetRepositoryTests.cs:280-298 and IdempotencyBehaviorDbTests.cs:242-260 — third occurrence.
· fix: extract to a shared internal test helper. Not a blocker.

Low / Info: None (signal gate).

Validation

Not re-run — no new diff content since the prior review already covered security-reviewer (PASS) and contract-aligner (N/A, no contract surface touched). Build/Unit Tests/SonarCloud run as separate required CI checks.

Recommendation

APPROVE — unchanged from the prior review. No new commits altered the reviewed code; only an unrelated main merge landed on the branch afterward. The outstanding Medium (test-shim duplication) is a good non-blocking follow-up, not a merge blocker.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud