fix: declare unique_constraint for audit_log pkey so log_changes swallows Ecto.ConstraintError (OPS-4584)#41
Closed
palantir-valiot[bot] wants to merge 1 commit into
Conversation
…lows ConstraintError (OPS-4584)
Member
|
Closing as a duplicate of #24 — all of these PRs fix the same bug: |
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
This PR fixes a code bug in
EctoTrail.log_changes/5(and the*_and_log/*family) that surfaces as an unhandledEcto.ConstraintErroron the audit log table's primary key whenupdate_and_log(or insert/upsert/delete_and_log) is called inside a transaction that later retries or collides on id generation.Summary of the change
insert_changelog/2helper (lib/ecto_trail/ecto_trail.ex:578) that builds the Changelog changeset and explicitly declaresChangeset.unique_constraint(:id, name: @pkey_constraint_name)using the configured table name (default "audit_log" => "audit_logs_pkey", or the custom table pkey).log_changes/5(line 435) andlog_changes_alone/6(line 395) now delegate to the helper instead of callingchangelog_changeset() |> repo.insert()directly.{:error, %Changeset{}}(constraint error promoted into the changeset). The existing{:error, reason} -> Logger.error(...) ; {:ok, reason}arms swallow it, the outer transaction succeeds, and the caller's main operation is not rolled back. This matches the documented/intended non-fatal audit behavior introduced for Multi support in 1.0.3.test/unit/ecto_trail_test.exs(the "swallows unique pkey..." case under update_and_log) that usesALTER SEQUENCE ... RESTART WITHto deterministically force a duplicate-pkey audit insert and asserts the primaryupdate_and_logstill returns{:ok, updated_resource}while no extra Changelog row is created.benchee,credo, and their transitivedeep_merge/statistex) and updated mix.lock as part of the PR (per Palantir baseline rules).mix format,mix credo --strict, andmix compile(fullmix testcould not complete in this CI-like agent pod because no Postgres listener was available; the test was written TDD-style and the compile/credo surface was clean).Why
See Linear OPS-4584. The originating error (from eliot-lamosa-gto-prod) was:
The triage decision was NOTIFY+FIX (code_bug, high severity). The root cause was the missing
unique_constraint/3declaration on the audit log insert path; without it Ecto raises instead of returning an error changeset that the caller's rescue/logging arms already handle.Files changed (from git diff --stat)
Type of change
How Has This Been Tested?
mix compile— clean (modulo a pre-existing redundant-clause warning unrelated to this diff).mix format --check-formatted— clean.mix credo --strict— clean (0 issues on the 3 source files).mix hex.outdated+ selective upgrade of benchee/credo within ranges, committed.git diff(see below) — minimal, targeted, no debug prints, no scope creep, follows existing patterns (the Multi-rollback fix in 1.0.3 already relied on the same "log error and return {:ok, reason}" swallowing).mix testcould not be executed end-to-end in the agent environment (no Postgres); the test helper requires a live DB. The change is isolated to the insert path that the new unit test exercises and the pre-existing error-handling arms. A reviewer or CI with a DB can run the full suite.Self-review of the diff (the exact output read before
git commit):No unrelated files, no secrets, no empty hunks.
Test plan (checkboxes for the reviewer)
mix format(and--check-formatted) — passed.mix credo --strict— passed with zero issues.mix compile— passed.git diffself-review performed; change is the smallest obvious thing that works.mix testin an environment with Postgres (the agent pod had no DB listener; the test is ready for that run).Closes OPS-4584