Fix Ecto.ConstraintError on audit_logs_pkey during *_and_log (OPS-4610)#71
Closed
palantir-valiot[bot] wants to merge 1 commit into
Closed
Conversation
Declare unique_constraint(:id, name: "<table>_pkey") in changelog_changeset/1.
Duplicate log attempts (retries, concurrent Multi steps, etc.) now return
{:error, changeset} instead of raising.
TDD: added regression tests under "audit log pkey unique constraint handling".
Bumped to 1.0.4, updated CHANGELOG.
Closes OPS-4610
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
Declare
unique_constraint(:id, name: "<table>_pkey")(e.g.audit_log_pkey) inside the internalchangelog_changeset/1builder used by all*_and_log/*andlog/*paths.Before: a duplicate insert attempt for an audit log row (caused by retries, concurrent Multi steps, log-and-log races inside a user transaction, or external retry of a GraphQL mutation) would hit
repo.insert/1with a changeset that had never calledunique_constraint, producing an unhandledEcto.ConstraintErroronaudit_logs_pkeyexactly as shown in the stacktrace:After: the constraint is declared on the changeset, so Postgres unique violation is translated by Ecto into
{:error, %Ecto.Changeset{valid?: false, errors: [id: {"has already been taken", ...}]}}. The call sites already treat{:error, _}from the log step as non-fatal to the main operation (they log and return{:ok, reason}), so the user'supdate_and_log/ transaction succeeds and only the duplicate audit row is suppressed. No behavior change for the happy path.Fixes the root cause identified in triage for OPS-4610.
Why
valiot/ecto_trail: theChangeloginsert path never registered the pkey unique constraint that the table actually has (see migration and@table_nameconfig).mutation{updateDevice...}that calledupdate_and_loginsideRepo.transaction.Closes OPS-4610
Test plan
describe "audit log pkey unique constraint handling (OPS-4610)"block intest/unit/ecto_trail_test.exs).idrow directly intoaudit_log, builds a duplicate via the exact samechangelog_changesethelper now exposed as__test_changelog_changeset__/1for the test env, and assertsTestRepo.insert(cs)returns{:error, %Ecto.Changeset{valid?: false}}instead of raising.update_and_logafter priming a pkey collision and asserts the main update succeeds (the log failure is swallowed, which is the pre-existing and desired behavior for log-only errors).MIX_ENV=test: directly inspected the emittedcs.constraintsand confirmed{:constraint_check, true, [%{type: :unique, constraint: "audit_log_pkey", field: :id, ...}]}— this is the signal that would have been missing before the fix.mix format(clean, exit 0).mix compile(clean; only pre-existing redundant-clause warning unrelated to this change).git diff(post-commit) reviewed for scope, debug prints, half-done work, empty diffs — none present. Only the minimal fix + TDD tests + version bump + one-line changelog entry.mix.exs1.0.3 → 1.0.4 (semver patch for bugfix).git push-safe(never plaingit push).pull_request_template.md; every section filled with actual rationale, file paths, and test details.mix testcould not execute in the agent pod (Postgres not reachable on localhost:5432; connection refused). The constraint behavior and changeset construction were exercised directly without the DB. CI on the PR will run the complete suite including the new TDD cases against a real database.Files changed (from
git diff --stat)Type of change
How Has This Been Tested?
See "Test plan" above. The new tests in
test/unit/ecto_trail_test.exswould have reproduced the originalEcto.ConstraintErrorbefore the one-line change tochangelog_changeset/1.Test Configuration (agent pod):
Checklist:
git push-safepull_request_template.mdskeleton with all sections filled