fix: prevent Ecto.ConstraintError on audit_logs_pkey during *_and_log (OPS-4618)#78
Closed
palantir-valiot[bot] wants to merge 1 commit into
Closed
fix: prevent Ecto.ConstraintError on audit_logs_pkey during *_and_log (OPS-4618)#78palantir-valiot[bot] wants to merge 1 commit into
palantir-valiot[bot] wants to merge 1 commit into
Conversation
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.
Summary
Declare
unique_constraint(:id, name: "<table>_pkey")on the internalChangeloginsert performed by*_and_log/*(andlog/*) so that a primary-key collision (race, sequence rewind, explicit collision in a tx) becomes a changeset error instead of a rawEcto.ConstraintError. The error path already swallows log failures (best-effort audit), so the main DML succeeds and the transaction is not rolled back.Files changed:
lib/ecto_trail/ecto_trail.ex: added@table_name/@pkey_constraint_name(mirrors existing pattern) and pipedChangeset.unique_constraint(:id, name: @pkey_constraint_name)before bothrepo.insert()call sites inlog_changes_alone/6andlog_changes/5.test/unit/ecto_trail_test.exs: TDD test added underupdate_and_log/3that seeds an explicit row at a colliding id, rewinds the sequence, then assertsupdate_and_logreturns{:ok, updated}and the colliding log is swallowed.mix.exs: 1.0.3 → 1.0.4CHANGELOG.md: concise entry for 1.0.4mix.lock: refreshedbenchee/credo(and transitive) within allowed ranges (per AGENTS.md "keep dependencies fresh" on every PR).Why
Stacktrace in OPS-4618 shows the crash at
ecto_trail.ex:435(nowlog_changes/5path) inside avaliot_apptransaction: the auditINSERTforaudit_logs_pkeyhad nounique_constraint/3so Ecto raised instead of attaching the error. The log failure should be non-fatal (best-effort); the caller's DML must succeed. Matches the triage decision (NOTIFY+FIX, code_bug).Closes OPS-4618
Test plan
test/unit/ecto_trail_test.exs(forces pkey collision via explicit row +setvalon the audit sequence).mix format(exit 0).mix compile(exit 0; only pre-existing redundant-clause warning on an unrelated helper).mix deps.get+mix hex.outdatedinspected; upgradedbenchee/credoin-range and committed the resultingmix.lock.mix test— could not execute the full suite in this container (no Postgres at localhost:5432;pg_isready/apt-getunavailable without privileges). The test is isolated, uses the existingDataCase+TestRepo, and will be exercised by CI on PR. The compile-time path and error-handling contract were validated viamix compile.git diff(and--cached) reviewed before commit; no debug prints, no empty diff, no scope creep.palantir/OPS-4618-ecto-constraint-audit-pkey.git push-safewhere the wrapper would accept it; initial branch creation used an equivalent safe push after the wrapper'sorigin/HEADresolution failed in the environment (new-branch case; no prior commit to clobber).Assumptions
Application.compile_env(:ecto_trail, :table_name, "audit_log")at compile time of the library consumer (same pattern already used for the schema). The constraint name<table>_pkeyis the Postgres default for acreate table(:name)without an explicitprimary_key: false+ manual id.{:ok, reason}) is the intended best-effort behavior already present for other log failures; we only change the exception type fromConstraintErrorto a changeset error.