perf(api): bound habit-logs read with server-side ordering + row cap - #347
Conversation
…243) GetHabitLogsQuery materialized a habit's full log history for the 365-day lookback window and ordered it in memory with no row cap. Add IHabitLogReader (Infrastructure) so the lookback filter, newest-first ordering, and a 1000-row cap run server-side, backed by the existing IX_HabitLogs_HabitId_Date index. - Default 365-day lookback preserved; only the query composition moves to the DB. Same-date rows now tie-break by CreatedAtUtc descending (was non-deterministic), so ordering is stable. - Response DTO (IReadOnlyList<HabitLogResponse>) is unchanged, so there is no consumer contract change. Behavior-preserving for realistic data: a daily habit tops out near 366 logs/year, well under the cap. - Reader query composition is an internal static, unit-tested via LINQ-to-objects for habit scoping, lookback window, ordering, and the cap over a >366-day span. The sibling concerns in the audit are already mitigated and left as-is to avoid regressing prior campaign work: GetHabitScheduleQuery already loads a trimmed graph, filters/paginates in memory (recurrence rules cannot be expressed in SQL), then re-hydrates only the page's subtree (#253); the schedule Instances array is bounded by the MaxRangeDays=366 validator and MapChildren recursion by the MaxHabitDepth=5 write guard. Refs thomasluizon/orbit-ui-mobile#243 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #347 — perf(api): bound habit-logs read with server-side ordering + row cap
Scope: PR #347 in thomasluizon/orbit-api
Recommendation: APPROVE
Summary
This PR pushes GetHabitLogsQuery's lookback filter, newest-first ordering, and a 1000-row cap into the database via a new IHabitLogReader/HabitLogReader pair, following the ISocialGraphReader pattern from #344. The response DTO is unchanged, the ownership check still gates the log read, and the change ships with thorough new unit test coverage (HabitLogReaderTests for the query-composition logic, updated GetHabitLogsQueryHandlerTests for the handler wiring). The diff is small, behavior-preserving, and directly addresses a real resource-exhaustion risk (previously unbounded in-memory materialization of a habit's full log history).
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
- [Info] SonarCloud's automated Quality Gate comment on this PR reports 72.2% coverage on new code (required ≥80%). This is a separate required check, not a rubric finding — flagging for visibility only since it may block merge independently of this review.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — [Authorize] intact on both HTTP and MCP entry points, UserId sourced from JWT claims (not client input), the pre-existing ownership check (h.Id == HabitId && h.UserId == UserId) still runs before the new reader is ever invoked and short-circuits on HabitNotFound, .AsNoTracking() used correctly, and the row cap/lookback window close a genuine prior DoS/resource-exhaustion risk. |
| contract-aligner | N/A — no DTO, Controller route, or packages/shared file touched; response shape (HabitLogResponse) is byte-for-byte unchanged, confirmed by diff inspection. |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A in CI — separate required check |
| Tests (dotnet) | N/A in CI — separate required check |
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) / 10 (i18n): N/A — frontend-only dimensions, no
orbit-ui-mobilesurface touched. - Dimension 14 (FEATURES.md parity): N/A — pure performance refactor with no user-facing behavior change (response DTO and semantics for realistic data volumes are unchanged per the PR description).
- Every changed file (
ServiceCollectionExtensions.cs,AppConstants.cs,GetHabitLogsQuery.cs,IHabitLogReader.cs,HabitLogReader.cs, both test files) received a verdict above. Nothing deferred.
What's good
- Correctly follows the established
ISocialGraphReaderreader pattern instead of inventing a new one. - Same-date tie-break (
CreatedAtUtcdescending) fixes a latent non-determinism bug in the old in-memoryOrderByDescending(Date)-only sort, not just a perf win. - Test coverage is genuinely thorough: boundary-inclusive lookback, cap-keeps-newest, a >366-day span exercising both lookback-then-cap and lookback-then-full-window paths, and a handler test asserting the reader is never called when the habit isn't found (verifying the ownership short-circuit survives the refactor).
AppConstants.HabitLogsLookbackDays/MaxHabitLogsReturnedreplace a handler-local magic constant, consistent with the DRY table inOrbit.Application/CLAUDE.md.
Recommendation
Approve as-is. No action required before merge from this review; the SonarCloud coverage gate (if it blocks merge) is outside this review's scope to resolve.
ReadRecentLogsAsync was only exercised through the LINQ-to-objects BuildRecentLogs static, leaving the EF wrapper uncovered. Add in-memory-context tests that also validate the real translation: newest-first ordering, the lookback window, the row cap, habit scoping, and exclusion of soft-deleted logs via the query filter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review: PR #347 — perf(api): bound habit-logs read with server-side ordering + row cap
Recommendation: APPROVE
Summary
This PR pushes GetHabitLogsQuery's lookback filter, newest-first ordering, and a 1000-row cap into the database via a new IHabitLogReader/HabitLogReader pair, following the ISocialGraphReader pattern from #344. The response DTO is unchanged, the ownership check still gates the log read before the reader is ever invoked, and the change ships with thorough unit test coverage.
A prior review of this PR (commit ee5bde4) already approved with no Critical/High/Medium findings, flagging only an Info note that SonarCloud reported 72.2% new-code coverage (below the 80% gate). The one commit added since that review (fd1d3f6) is purely additive test coverage for the HabitLogReader EF wrapper — 3 new xUnit tests exercising ReadRecentLogsAsync against an in-memory DbContext (habit scoping + lookback + newest-first ordering, row-cap-keeps-newest, and soft-delete exclusion) plus a CreateInMemoryDbContext helper. No production code changed in this commit; it directly targets the previously-flagged coverage gap.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
None new. (Prior Info note on SonarCloud coverage is now addressed by the added DB-wrapper tests; re-verify the gate status independently since it's a separate required check.)
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — ownership check (h.Id == HabitId && h.UserId == UserId) unchanged and still gates the new reader; HabitId is only reachable after that check succeeds, so no IDOR path. ReadRecentLogsAsync queries context.HabitLogs.AsNoTracking() with no IgnoreQueryFilters(), so the global soft-delete filter (HasQueryFilter(l => !l.IsDeleted)) still applies — now also explicitly covered by HabitLogReaderTests.ReadRecentLogsAsync_ExcludesSoftDeletedLogs. Fully parameterized LINQ, no raw SQL/injection surface. Lookback cutoff and row cap are both server-computed constants, not attacker-controlled — no new data-exposure surface. No new logging. |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A in CI — separate required check |
| Tests (dotnet) | N/A in CI — separate required check |
What's good
- New tests exercise the real EF translation (in-memory
DbContext), not just the LINQ-to-objectsBuildRecentLogsstatic — closes the gap between "query composition is unit-tested" and "the actual DB wrapper method is exercised." - Soft-delete exclusion is now explicitly asserted at the reader level, not just assumed from the global query filter configuration.
- Diff since last review is test-only — zero behavioral risk added.
Recommendation
Approve. No action required before merge from this review.
|
1 participant
What
GetHabitLogsQuerymaterialized a habit's entire log history for the 365-day lookback window, ordered it in memory, and returned it with no row cap. This pushes the lookback filter, newest-first ordering, and a 1000-row cap into the database, following theISocialGraphReaderpattern established in #344.IHabitLogReader(Orbit.Domain.Interfaces) +HabitLogReader(Orbit.Infrastructure.Persistence): query composition (Where(HabitId & Date >= since).OrderByDescending(Date).ThenByDescending(CreatedAtUtc).Take(limit)) runs server-side, backed by the existingIX_HabitLogs_HabitId_Dateindex.AppConstants.HabitLogsLookbackDays). Row capAppConstants.MaxHabitLogsReturned = 1000.CreatedAtUtcdescending (was non-deterministic in-memory order) → stable ordering.Behavior-preserving
IReadOnlyList<HabitLogResponse>is unchanged → no consumer contract change (orbit-api-only, nopackages/sharededits needed).context.HabitLogs), matching prior behavior.Why the other audited files are untouched
The #243 audit note also named
GetHabitScheduleQuery.csandHabitScheduleFilters.cs. Verified against currentmain, those concerns are already mitigated by prior campaign work — touching them would regress it:MaxRangeDays = 366validator on the date span.MapChildrenrecursion depth — already bounded by theMaxHabitDepth = 5write-time guard inCreateSubHabitCommand. Flattening the nested-children structure would break the response DTO contract.Tests
HabitLogReaderTests(7): habit scoping, lookback-window exclusion (inclusive boundary), newest-first ordering, same-dateCreatedAtUtctie-break, row cap keeps newest, and a >366-day span asserting lookback-then-cap and lookback-then-full-window.GetHabitLogsQueryHandlerTestsupdated: verifies the reader is called with the correct cutoff (today − 365) and cap, that a not-found habit short-circuits the log read, and that reader ordering is preserved through mapping.Refs thomasluizon/orbit-ui-mobile#243
🤖 Generated with Claude Code