chore(#107): backend code-health — error-code contract, EF migration consolidation, perf, validators, deps - #205
Conversation
…arCloud reliability)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
One contract break to resolve before merge.
High-signal finding: The 410 Gone response body on both GET /api/sync/changes and GET /api/sync/changes/v2 silently changed the field name from code to errorCode (line 156 / 238 in SyncController.cs). Every other controller in this PR goes from no error-code field to adding errorCode — genuinely additive. These two endpoints are the exception: they already had a field named code, and this PR renames it. Any client that branches on .code === 'SYNC_WINDOW_EXCEEDED' to trigger a full re-sync now gets undefined instead. The inline comment has the specifics and the resolution options.
Everything else in this PR is solid — the uniform errorCode contract across ~36 controller sites, filtered includes on the hot log/skip paths, N+1 gamification batching, new validators and rate limits on account-deletion, migration consolidation, and the FileSignatures DoS advisory fix all look correct and well-tested. 3,255 tests passing, zero warnings. Happy to approve once the sync 410 body contract question is answered.
|
There was a problem hiding this comment.
Solid, well-scoped code-health sweep. The error-code contract is clean: AppError records pair code + message, Result.Failure(AppError) propagates both, and ToErrorResult/ToErrorBody unify the controller surface — every failure path now emits { error, errorCode } consistently. The migration consolidation is safe: the three pre-existing partial Habits indexes are guarded with IF NOT EXISTS, and the new IX_Notifications_UserId_CreatedAtUtc index is genuinely new so no collision risk. Filtered Include on the log/skip/bulk hot paths and the gamification N+1 batch fix are correct and non-breaking. All five new validators (ConfirmAccountDeletion, GetAllHabitLogs, GetCalendarMonth, GetDailySummary, UnsubscribePush) ship with test files. The GET /api/habits/logs REST endpoint removal is intentional and explicitly paired with the consumer PR (#172). The SonarCloud reliability gate tripped before the last commit (the null-check fix) — the build and 3,255-test suite are green, so I'm not treating the stale gate result as a blocker.
1 participant
What
Consolidated backend code-health PR for the #107 umbrella (orbit-api side). Paired with the frontend PR in thomasluizon/orbit-ui-mobile.
Consumer PR: thomasluizon/orbit-ui-mobile#172
Refs thomasluizon/orbit-ui-mobile#107
Squash-merge this first — the
errorCodechange is additive on the wire (errorstring preserved +errorCodeadded), so the consumer PR depends on it being live.Highlights
errorCodealongside the English fallback (ToErrorResult/ToErrorBodyuniform path; ~36 controller sites + AuthController migrated). NewAppErrorcatalog pairs each message with a code inErrorMessages/DomainErrors; clients localize on the code.Persistence/Migrationsfolder merged intoMigrationsunder one namespace viagit mv— migration ids, class names, and the model snapshot untouched, so no migration re-applies and the production schema is unaffected. Also adopted the previously-orphaned performance indexes into a realAdoptPerformanceIndexesmigration (they were never in the EF chain) + added a notifications(UserId, CreatedAtUtc)index.Include(h => h.Logs)on the hot log/skip/bulk paths (was loading full 400-day history); bulk-log gamification batched (was N×6 queries);FindAsync().Countmaterializations →Any/CountAsync.FileSignatures6→7.2.1 clears the OpenMcdf DoS advisories (0 vulnerable across all 8 projects); JWT/test-tooling bumps.BuildCapabilities794→25 + named builders, etc.); audit-write swallows now log; naming fixes; analyzer (ORBIT0001) clean.Validation
dotnet build Orbit.slnx0 errors / 0 warnings;dotnet test Orbit.slnx3,255 passing.Deferral
SyncController.GetChanges(v1 sync) kept, not deleted — old Play app builds may still call it, the agent capability catalog references it, and two tests exercise it. Revisit when min-supported-version telemetry confirms it's safe to drop (justification on issue #107).🤖 Generated with Claude Code