fix: recognize 500 unique-constraint conflicts on import replay - #35
Conversation
…dy_present Widen is_conflict to also treat a 500 whose body names a unique- constraint violation as "already exists" — some already-deployed servers surface a duplicate realm name that way instead of a proper 409, which stopped a replay from converging on its first line. Add ImportReport.already_present, incremented at every skip site, so a converging replay is distinguishable from a run that did nothing.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughImport replay now counts entities that already exist, recognizes legacy ChangesImport replay accounting
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to The import now skips a broader class of server errors across roles, redirects, users, and assignments. If a matching 500 does not actually prove that the intended resource or relationship exists, the import could report success while leaving identity or authorization configuration incomplete; this requires explicit owner acceptance or tighter validation before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bug
is_conflictonly recognized a409, or a400whose body contains "exist". Some already-deployed servers surface a duplicate-key unique-constraint violation as a raw500instead (e.g.realms_name_keyon a duplicate realm name), so replaying an import stopped at its first line instead of converging.Fix
is_conflictto also treat a500whose body names a unique-constraint violation as a skip, not a hard failure.ImportReport.already_present, incremented at all 7 skip sites (realm, realm role, client, redirect, client role, user, role assignment), and render it inrealm import's output — so a converging replay is now distinguishable from a run that did nothing, per the issue's ask.Issue
Refs #27 — not fully closing it. See verification below: this server's realm-name conflict is already fixed server-side (returns 409 cleanly), but I found a different, still-live instance of the same class of bug — duplicate role creation returns a 500 with a body containing zero diagnostic information (
{"message":"Internal Server Error: Internal server error"}). There is nothing in that body to safely distinguish "role already exists" from a genuine server error, so I did not attempt to pattern-match it — doing so would risk silently swallowing real failures during import. That case can only be fixed server-side (returning 409, which is exactly point 2 of this issue); leaving it here rather than guessing.Verification
Ran a real import against a local FerrisKey server, replayed it, and inspected each conflict path directly:
realm create <existing>→ clean 409 (already fixed server-side here).realm importreplay of a realm with one role → 500 on the role-recreation step, generic body (see above) — confirms the shape of the underlying class of bug is still present, even though this specific occurrence isn't fixable from the CLI.is_conflict's new branch is covered by unit tests using the exactunique constraintwording from the issue's cited Postgres violation, since I couldn't reproduce that literal case live (already patched here).Test plan
cargo build --workspacecargo test --workspace(73 passed, +5 new:is_conflictcases)cargo clippy --workspace --all-targets --all-features -- -D warningsSummary by CodeRabbit
New Features
Bug Fixes