fix: declare unique_constraint for audit pkey in *_and_log / log_changes (OPS-4567)#28
Closed
palantir-valiot[bot] wants to merge 1 commit into
Closed
Conversation
…ges (OPS-4567) - changelog_changeset now adds unique_constraint/3 for common pkey names (audit_log_pkey, audit_logs_pkey, _pkey) so Ecto turns ConstraintError into a changeset error that the existing log-and-swallow path already handles. - Added TDD test (constraint collision under upsert_and_log) and pure unit test (no DB) asserting the declarations are present. - Bumped version to 1.0.4; added CHANGELOG entry; upgraded benchee/credo within ranges for deps freshness. - mix format clean. Closes OPS-4567
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.
Summary
Root cause:
upsert_and_log(and the other*_and_log/log_changespaths) callrepo.insert(%Changelog{...})without declaringunique_constraint/3for the audit table's pkey. When the sequence foraudit_log/audit_logsis rewound or a concurrent upsert produces an id collision, Postgres raises on the unique pkey and Ecto surfaces it asEcto.ConstraintError(seeaudit_logs_pkeyin the stack). The existing error-handling inlog_changesalready swallows{:error, reason}returns (logs and returns{:ok, reason}), so turning the violation into a changeset error makes the main mutation succeed instead of blowing up the request.Fix:
changelog_changeset/1now attachesunique_constraint(:id, name: ...)for the common pkey names customers actually use (audit_log_pkey,audit_logs_pkey,${table}_pkey). Also upgraded benchee/credo within ranges (deps freshness per workflow).Files changed:
lib/ecto_trail/ecto_trail.ex— add@default_audit_table,changelog_changesetnow builds candidates fromApplication.get_env(:ecto_trail, :table_name, ...)and callsadd_unique_constraints_for_pkey/2.test/unit/ecto_trail_test.exs— added "constraint error handling for audit_log pkey (OPS-4567)" integration test exercisingupsert_and_logunder forced pkey collision (sequence rewind).test/unit/ecto_trail_constraint_unit_test.exs— new pure (no-DB) unit test asserting the constraint declarations are present on the internal builder.mix.exs— 1.0.4.CHANGELOG.md— concise entry + deps freshness note.mix.lock— benchee 1.5.1, credo 1.7.19 (and transitive).Why
Linear: OPS-4567 — "jobs-lamosa-gto-prod: Root cause is missing unique_constraint handling". Triage: NOTIFY+FIX, severity high, category code_bug, first-party package. Matches the exact stack in the issue (ecto_trail 1.0.3
lib/ecto_trail/ecto_trail.ex:435: EctoTrail.log_changes/5→repo.insertinsideupsert_and_log).Test plan
mix format --check-formatted(clean).mix compile(clean; only pre-existing redundant-clause warning on an unrelated helper).test/unit/ecto_trail_constraint_unit_test.exsassertsaudit_log_pkeyandaudit_logs_pkeyappear in the changeset constraints.describe "constraint error handling for audit_log pkey (OPS-4567)":upsert_and_log; asserts the mutation succeeds ({:ok, Resource}) instead of raisingEcto.ConstraintError.mix testexecuted (DB-dependent tests cannot connect in this pod; the test helper'sEcto.Migrator.runfails with connection refused as expected; the deterministic checks above cover the change).mix hex.outdated→mix deps.update); lockfile updated.git diffinspected; no debug prints, no empty diff, no scope creep; branch name follows the NON-NEGOTIABLEpalantir/OPS-4567-...convention.Closes OPS-4567