fix(api): restore EF-mapped private setters unmapped by #389 (startup PendingModelChangesWarning crash) - #390
Conversation
…tartup PendingModelChangesWarning crash) #389's S1144 smell burn-down removed the `private set` from two EF-mapped entity properties -- Report.ReviewedAtUtc and PendingClarification.ResolvedAtUtc -- turning them into read-only auto-properties. EF Core's property-discovery convention does not map read-only auto-properties, so the runtime model dropped both columns while OrbitDbContextModelSnapshot.cs still declared them. The startup DB-migrate step is configured to throw on PendingModelChangesWarning, so every orbit-api deploy since #389 crashes (exit 139) at WebApplicationExtensions.ConfigureOrbitPipeline. Restoring the private setters re-maps both columns with ZERO schema change (no new migration, no snapshot edit): `dotnet ef migrations has-pending-model-changes` returns false. Adding a migration instead would DropColumn both live production columns (data loss), which #389 never intended. The setters are unused from C# (EF sets them via reflection), so a URL-linked WHY note keeps a future S1144 pass from re-removing them and re-breaking the deploy. Refs #389, thomasluizon/orbit-ui-mobile#243 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
There was a problem hiding this comment.
Code Review: PR #390 — fix(api): restore EF-mapped private setters unmapped by #389
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 1 |
Summary
This is a byte-exact revert of the two entity-property changes PR #389 made (Report.ReviewedAtUtc and PendingClarification.ResolvedAtUtc going from { get; private set; } to { get; }), plus a one-line URL-linked WHY comment on each property. #389's unused-private-setter cleanup silently un-mapped both EF Core columns (EF's convention doesn't map read-only auto-properties), desyncing the runtime model from OrbitDbContextModelSnapshot.cs and crashing startup on PendingModelChangesWarning. This is the minimal, correct root-cause fix — no schema change, no destructive migration.
Verified directly against the working tree:
git diff 56c75b9 bd8bcff -- Report.cs PendingClarification.csis the exact textual inverse of #389's two hunks, nothing else touched.- Each file contains exactly one
ReviewedAtUtc/ResolvedAtUtcdeclaration — confirmed no duplicate-member compile break exists (a background tool briefly flagged a possible duplicate from a garbled diff render; checked the actual file content and it's a false alarm). grep -rn "{ get; }" src/Orbit.Domain/Entities/*.csreturns no matches post-fix — no third broken property left behind.
Findings
Critical / High / Medium: None.
Low / Info:
- No CI/test step currently runs an EF model/snapshot-parity check (e.g.
dotnet ef migrations has-pending-model-changes), which is exactly what let #389 merge with this desync in the first place. Worth a non-blocking follow-up: a small xUnit test that builds theOrbitDbContextmodel and assertsGetPendingModelChangesAsync()is empty, wired into CI. - The SonarCloud Quality Gate check on this PR is currently failing ("B Maintainability Rating on New Code"), most likely from the two files sharing an identical WHY-comment line (minor duplication signal). This is a separate required check outside this review's rubric-based decision — not a functional or design concern, and the PR body already notes the underlying S1144 flag is an accepted re-open on these two members for a later non-model-breaking cleanup.
Security / Contract
- security-reviewer: PASS — restoring an already-
privatesetter adds no new mutation surface; no auth/injection/data-exposure impact. The added comment is a compliant URL-linked WHY-note, not narration. - contract-aligner: N/A — no DTO, route, or
packages/sharedtype changed.
What's good
- Correctly root-caused the actual mechanism (EF's read-only-auto-property mapping convention) rather than papering over it with a migration that would emit destructive
DropColumns against live prod data. - Tightly scoped — touches only the two broken properties.
- Comments are exactly the sanctioned shape under this repo's ORBIT0001 policy: URL-linked WHY note tied to the causing PR.
Recommendation
Approve and merge. Consider the CI parity-check follow-up (non-blocking) so this exact class of regression can't recur.


Incident (deploy-blocking)
Every orbit-api deploy since #389 crashes at startup:
exit 139, at
Orbit.Api.Extensions.WebApplicationExtensions.ConfigureOrbitPipeline(the startup DB-migrate step). The app is configured to throw onPendingModelChangesWarning, so any deploy now aborts. Prod is still safe on deploy #388, but every future deploy is blocked until the EF model matches its migration snapshot again.Root cause
#389 was a behavior-preserving SonarCloud smell burn-down. Its S1144 fix ("delete unused private setters") removed the
private setfrom two EF-mapped entity properties, converting them to read-only auto-properties:Report.ReviewedAtUtc{ get; private set; }→{ get; }PendingClarification.ResolvedAtUtc{ get; private set; }→{ get; }EF Core's property-discovery convention does not map read-only auto-properties. So the runtime model silently dropped both columns while
OrbitDbContextModelSnapshot.csstill declared them — a model-vs-snapshot mismatch. A scratchdotnet ef migrations addagainst #389's HEAD confirms exactly this:The other model-adjacent edit in #389 (OrbitDbContext S1192, swapping the
'[]'::jsonbliteral for an existing const of the identical value) is model-neutral, and the S3260sealed record UserDatePreferencesis a private in-service cache DTO, not an entity — neither contributes.Fix
Restore the
private seton both properties. This re-maps the columns with zero schema change — no new migration, no snapshot edit. The rest of #389's smell fixes are untouched.Why restore instead of adding a migration: the model change was unintended (#389 was behavior-preserving). Adding a migration would emit the two
DropColumns above against the live production DB — dropping real columns and their data — which was never intended and risks data loss.The setters are unused from C# (EF materializes them via reflection), so each carries a URL-linked WHY note to stop a future S1144 pass from re-removing them and re-breaking the deploy. Consequence: SonarCloud S1144 re-opens for these two members — accepted; it will be handled a non-model-breaking way later.
Verification (local, foreground)
dotnet ef migrations has-pending-model-changes→ "No changes have been made to the model since the last migration." (was "Changes have been made..." at chore(sonar): burn down csharpsquid smell cluster across API #389 HEAD)dotnet build Orbit.slnx→ 0 errorsdotnet test Orbit.slnx→ 5284 passed, 0 failed (Domain 512, Application 2773, Infrastructure 1992, Analyzers 7)Refs #389, thomasluizon/orbit-ui-mobile#243