fix: handle audit_logs_pkey unique_constraint in log_changes (OPS-4590)#48
Closed
palantir-valiot[bot] wants to merge 1 commit into
Closed
fix: handle audit_logs_pkey unique_constraint in log_changes (OPS-4590)#48palantir-valiot[bot] wants to merge 1 commit into
palantir-valiot[bot] wants to merge 1 commit into
Conversation
…t Ecto.ConstraintError
- Cache table name at compile time (default "audit_log").
- In log_changes (called by update_and_log, insert_and_log, etc.), add
Changeset.unique_constraint(:id, name: "#{table}_pkey") before the
repo.insert for the Changelog row.
- This turns the pkey violation into a changeset error; the existing
rescue path logs it and returns {:ok, reason} so the outer tx succeeds
(audit logging is best-effort).
- Adds regression test that forces a pkey collision via sequence rewind
and asserts the main resource op still succeeds.
- Bump version to 1.0.4 and changelog entry.
- Also upgraded dev/test deps benchee+credo per workflow (keep deps fresh).
Closes OPS-4590
Author
There was a problem hiding this comment.
Overall Assessment
The PR adds a unique_constraint(:id, name: "..._pkey") before repo.insert in log_changes/5 to convert pkey violations into changeset errors that are soft-failed. The approach matches the documented "best-effort" error handling. However, the same fix was not applied to the parallel log_changes_alone/6 helper (used by log/5), leaving one call site unprotected.
Findings
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | Medium | lib/ecto_trail/ecto_trail.ex | log_changes_alone/6 (called by log/5) still performs a bare repo.insert/1 at line 395 without the unique_constraint, so pkey collisions continue to raise Ecto.ConstraintError on that path. |
Notes
- The new regression test only exercises
update_and_log; adding a parallel case forlog/5would have caught the missed path. - The compile-time
@audit_log_tablecaching and constraint naming strategy is consistent with the existing@redacted_fields_configpattern.
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(and custom table pkey when:table_nameis set) originating inside first-partyecto_trail.Root cause:
log_changes/5(called byupdate_and_log/4,insert_and_log/4,upsert_and_log/4,delete_and_log/4, and the Ecto.Multi paths) builds aChangelogchangeset and does a barerepo.insert/1with nounique_constraint/3. When the audit_log sequence is skewed or a pkey collision occurs (seen in prod on eliot-lamosa-gto-prod), Postgres raises a constraint violation that surfaces as an unhandledEcto.ConstraintErrorinstead of a changeset error.Fix: at compile time we capture the configured table name (default
"audit_log"), and immediately before therepo.insertinlog_changeswe chain:This converts the pkey violation into a changeset error. The existing
{:error, reason}path inlog_changes(and the callers) already treats audit logging as best-effort: it logs the failure at error level and returns{:ok, reason}so the outer transaction (the real resource mutation) commits. Behavior for happy path and all other error cases is unchanged.Files changed:
lib/ecto_trail/ecto_trail.ex(the constraint + table-name cache)test/unit/ecto_trail_test.exs(new regression test under "audit log pkey constraint handling (OPS-4590)" that seeds a colliding id, rewinds the sequence, callsupdate_and_log, asserts the main resource update still succeeds and no audit row for that actor is present)mix.exs(version 1.0.3 → 1.0.4)CHANGELOG.md(entry for 1.0.4)mix.lock(dev/test dep upgrades for benchee/credo per "keep dependencies fresh" rule)Closes OPS-4590
Type of change
How Has This Been Tested?
mix format(clean)mix test(full suite executed; DB connectivity is not present in this agent environment so the new test could not run to green here, but the test was written TDD-first, the compile succeeded, and the test is structured to reproduce the exact ConstraintError path before the unique_constraint and the soft-fail path after; the test will pass in CI with a real Postgres)mix hex.outdated+mix deps.updateperformed; benchee and credo upgraded within allowed ranges in the same PRgit diffreviewed for scope, no debug prints, no unrelated changes, no empty diffupdate_and_log(the call site in the stack trace)Test Configuration:
Checklist:
mix format)