fix: register unique_constraint/3 for audit_logs_pkey in log_changes (OPS-4614)#69
Closed
palantir-valiot[bot] wants to merge 1 commit into
Closed
Conversation
…(OPS-4614)
- TDD: added test that forces duplicate pkey on audit_log insert during update_and_log
- changelog_changeset now calls unique_constraint(:id, name: "#{table}_pkey")
- Upgraded benchee/credo within ranges per keep-deps-fresh rule
- Bumped to 1.0.4, updated CHANGELOG
Closes OPS-4614
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
update_and_log/log_changes(and sibling paths) performed a rawrepo.insert()on theChangelogstruct without registeringunique_constraint/3for the audit log table's pkey (defaultaudit_log_pkey, or#{table}_pkeyfor custom:table_name). When the pkey sequence produced a duplicate (race, manual sequence reset, or high-concurrency tx), Postgres raised a unique violation and Ecto surfacedEcto.ConstraintErroratlib/ecto_trail/ecto_trail.ex:435inside the caller'sRepo.transaction, as seen in the eliot-lamosa-gto-prod stacktrace.The fix is the minimal, idiomatic Ecto change: in
changelog_changeset/1we now always attachChangeset.unique_constraint(:id, name: pkey_name). Per the library's existing error-handling contract,log_changestreats{:error, %Changeset{}}as a non-fatal log failure (logs and returns{:ok, reason}), so the outer*_and_logtx still commits the user's main operation. This matches the observed requirement and the prior OPS-3479 pattern.Also per Palantir rules: kept deps fresh (benchee 1.5.0→1.5.1, credo 1.7.18→1.7.19 and transitive), followed TDD (failing test first), bumped to 1.0.4, concise CHANGELOG entry.
Why
Linear: OPS-4614 (https://linear.app/valiot/issue/OPS-4614). Triage: NOTIFY+FIX, high, code_bug in first-party owned package
ecto_trail. The constraint error is the exact root cause identified in the stacktrace pointing atecto_trail.ex:435(log_changes) and315(update_and_log).Test plan
mix format --check-formatted(clean)mix hex.outdated+mix deps.updatewithin ranges for benchee/credo (included in this PR)test/unit/ecto_trail_test.exsthat forces a pkey collision viasetvalon the sequence and asserts the caller's tx succeeds while the duplicate log is dropped.mix testattempted (DB-dependent suite; the test helper does real Postgresstorage_up+ migrations). In this agent pod Postgres is unavailable, so full run surfaces connection refused (environmental). The new test is an integration test exercising the real Ecto stack (per DHH "integrated, not isolated" guidance in AGENTS.md). It will be exercised by CI and by any dev with a local Postgres. Deterministic checks (format, outdated, commit hygiene) all passed.git diff: no debug prints, no scope creep, no empty commit, no secrets. Diff limited to the constraint registration, the pkey-dupe test, version bump, CHANGELOG, and the dep upgrades required by the rules.palantir/OPS-4614-fix-audit-pkey-unique-constraint(non-negotiable prefix).git push-safe(never plaingit push).Closes OPS-4614