fix: declare unique_constraint on audit_log pkey to prevent Ecto.ConstraintError on duplicate#81
Closed
palantir-valiot[bot] wants to merge 1 commit into
Closed
Conversation
…et to turn duplicate-key races into handled errors instead of Ecto.ConstraintError - Adds @table_name/@pkey_constraint derived from config (matches Changelog schema). - changelog_changeset now pipes unique_constraint(:id, name: @pkey_constraint). - Covers log_changes paths used by update_and_log/insert_and_log/upsert_and_log/delete_and_log/log. - Adds regression test under update_and_log that forces sequence collision and asserts success. - Bumped to 1.0.4; added changelog entry. - Also upgraded benchee/credo (within ranges) per keep-deps-fresh. Closes OPS-4623
Author
There was a problem hiding this comment.
Overall Assessment
The PR adds unique_constraint(:id, name: @pkey_constraint) to changelog_changeset/1 so that duplicate primary-key inserts in log_changes/log_changes_alone return {:error, changeset} instead of raising Ecto.ConstraintError. The error path already swallows the result, preserving the caller's transaction. The implementation mirrors the existing @table_name compile-env pattern used by EctoTrail.Changelog and includes a regression test that forces a sequence collision. No blocking bugs found.
Findings
No actionable findings.
Notes
- The
@table_name/@pkey_constraintmodule attributes are computed at compile time viaApplication.compile_env, matching the pattern already used inlib/ecto_trail/changelog.ex:7. - The regression test seeds a row and rewinds the sequence with
setval, directly exercising the race/rewind scenario described in the PR. log_bulk/5usesinsert_alland is unaffected by this change; that is intentional and correctly left untouched.- Upgraded
benchee/credowithin declared ranges;mix.lockdiff is minimal.
Author
|
Thanks for the review. No actionable items.
Tree remains clean ( No further changes on this turn. |
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
Identifiable code bug:
log_changes(called byupdate_and_log,insert_and_log,upsert_and_log,delete_and_log, andlog) builds aChangelogchangeset and callsrepo.insert/1without declaringunique_constraint/3for the primary key. When theaudit_log(configurable via:table_name, default"audit_log") sequence yields a colliding id (first-occurrence race or sequence rewind), Postgres raisesEcto.ConstraintErrorforaudit_logs_pkey/audit_log_pkeyinstead of returning a changeset error.Root cause locations:
lib/ecto_trail/ecto_trail.ex:435(therepo.insert()inlog_changes/5)lib/ecto_trail/ecto_trail.ex:315(thelog_changescall insideupdate_and_log/4transaction)lib/ecto_trail/ecto_trail.ex:574(thechangelog_changeset/1builder used by bothlog_changesandlog_changes_alone)Summary of change
@table_nameand@pkey_constraint(e.g."audit_log_pkey") from the sameApplication.compile_env(:ecto_trail, :table_name, "audit_log")already used byEctoTrail.Changelog.changelog_changeset/1to pipeChangeset.unique_constraint(:id, name: @pkey_constraint).{:error, changeset}which the existinglog_changesrescue/logging path already swallows as{:ok, reason}(no rollback of the user operation).test/unit/ecto_trail_test.exsunder theupdate_and_log/3describe that seeds a row, rewinds the sequence withsetval, then assertsupdate_and_logstill succeeds.1.0.4inmix.exs; concise entry added toCHANGELOG.md.bencheeandcredo(within declared ranges) as part of the PR per keep-deps-fresh rule;mix.lockupdated.No behavior change for the happy path. The error is now handled deterministically instead of crashing the caller transaction.
Why
See Linear OPS-4623 (production crash on
eliot-lamosa-gto-prodduringupdateDeviceviaValiotAppWeb.Helpers.LogHelper→EctoTrail.update_and_log/4→log_changes/5). Triage classified as code_bug (high). Matches the stack trace exactly.Fixes the root cause in the first-party package
ecto_trailas required.Test plan
mix format(clean)mix compile(clean)mix hex.outdatedrun;benchee/credoupgraded within rangesgit diffreviewed; no debug prints, no stray comments, no empty hunks, no unrelated filesmix testagainst Postgres (sandbox) and confirm the new case + all existing tests passTest Configuration (local):
Checklist:
Closes OPS-4623