fix: prevent Ecto.ConstraintError on audit_logs_pkey during *_and_log (OPS-4616)#75
Closed
palantir-valiot[bot] wants to merge 1 commit into
Closed
Conversation
- Declare unique_constraint(:id) + known pkey names (audit_logs_pkey, audit_log_pkey) in changelog_changeset/1 so Ecto can map constraint violations.
- Wrap the repo.insert() in log_changes/5 and log_changes_alone/6 with try/rescue so any DB error (ConstraintError, etc.) during audit logging is caught, logged, and returned as {:ok, reason} without aborting the caller's transaction.
- Add TDD regression test that forces a pkey collision via setval and asserts update_and_log still succeeds.
- Bump to 1.0.4; document in CHANGELOG.
Closes OPS-4616
Author
There was a problem hiding this comment.
Overall Assessment
The PR modifies log_changes/5 and log_changes_alone/6 to wrap repo.insert() in try/rescue, and adds unique_constraint(:id) declarations (for :id, audit_logs_pkey, audit_log_pkey) in changelog_changeset/1. Audit failures are logged and turned into {:ok, _} so the caller's *_and_log transaction commits the business write. A regression test using setval exercises the pkey collision path. No blocking bugs found.
Findings
No actionable findings.
Notes
- The
try/rescueis a safety net;unique_constraintdeclarations will convert matching constraint violations into{:error, changeset}(already handled), while the rescue catchesEcto.ConstraintErrorfor unmatched names or other DB errors. - Existing
log_bulk/5already uses a similarrescuepattern, so the approach is consistent. - The new test covers the serial PK collision case; custom UUID PK collision scenarios mentioned in the PR description are not exercised by the test but would still hit the rescue path.
- Version bump and changelog entry are present and minimal.
Author
|
Thanks for the review. Acknowledged:
No further code changes needed from this review. Ready for merge once CI is green. |
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Prevent
Ecto.ConstraintErroronaudit_logs_pkey(or equivalent primary-key constraint) whenEctoTrail.*_and_log/*(orlog/*) performs the audit insert.Root cause:
changelog_changeset/1did not declareunique_constraint(:id)(or known pkey constraint names), and the auditrepo.insert/1calls inlog_changes/5andlog_changes_alone/6were not protected. When a duplicate PK value was generated (custom UUID PKs, sequence rewind, retry paths, etc.), the error propagated out of the audit path and aborted the caller's transaction.Changes:
lib/ecto_trail/ecto_trail.ex:598—changelog_changeset/1now declaresunique_constraint(:id)plus the two common pkey names seen in the wild (audit_logs_pkey,audit_log_pkey).lib/ecto_trail/ecto_trail.ex:440andlib/ecto_trail/ecto_trail.ex:470— wrap therepo.insert()inlog_changes/5andlog_changes_alone/6withtry/rescue. Any DB error during audit logging is logged and turned into{:ok, reason}so the outer*_and_logtransaction still commits the business write.test/unit/ecto_trail_test.exs— added regression test that forces a pkey collision viasetvaland assertsupdate_and_logstill succeeds.mix.exs— version 1.0.4.CHANGELOG.md— concise entry under 1.0.4.Fixes the exact stack from the incident:
Why
Linear: OPS-4616 — Ecto.ConstraintError on audit_logs_pkey during EctoTrail.update_and_log on Device update in eliot-lamosa-gto-prod. Triage classified as code_bug (high). The package must never let an audit-log failure abort the caller's business operation.
Test plan
mix format --check-formatted(passes)mix testexecuted (Postgres is not available in this runner; the test helper fails on connection as expected — the new regression lives intest/unit/ecto_trail_test.exsand will exercise the rescue path when a DB is present; all prior tests continue to compile and the change is minimal/isolated).git diff --statand full diff: only the audit constraint handling, the TDD test, version bump, and changelog. No debug prints, no unrelated scope.Closes OPS-4616