fix: register unique_constraint on audit_log pkey to prevent Ecto.ConstraintError in *_and_log (OPS-4615)#76
Closed
palantir-valiot[bot] wants to merge 1 commit into
Conversation
…et (OPS-4615)
- TDD: added test that forces pkey collision via sequence reset and asserts update_and_log does not raise Ecto.ConstraintError.
- Root cause: changelog_changeset/1 only cast; no unique_constraint/3, so repo.insert of Changelog inside *_and_log tx raised ConstraintError on audit_logs_pkey when the app's tx reused an id (e.g. after prior log in same tx or Multi).
- Fix: read same @table_name compile_env as Changelog schema, add unique_constraint(:id, name: "#{table}_pkey") so Ecto converts the violation to changeset error; existing log_changes paths already swallow {:error, changeset} by logging and returning {:ok, reason}.
- Also bumped to 1.0.4 and updated CHANGELOG per release rules.
- mix format applied; pre-existing credo/compile warning unrelated (map_custom_ecto_type clause).
Author
There was a problem hiding this comment.
Overall Assessment
The PR adds unique_constraint(:id, name: "#{@table_name}_pkey") to changelog_changeset/1 so that primary-key violations on the audit log table are converted to changeset errors instead of raising Ecto.ConstraintError. The implementation reads the same compile_env key used by the Changelog schema, and existing error paths in log_changes* already swallow {:error, changeset} results. No blocking bugs found.
Findings
No actionable findings.
Notes
- The new test directly reproduces the pkey collision via
setvaland asserts the expected{:ok, ...}return path, following the same patterns as the existing "use inside Ecto.Multi" tests. - The fix is scoped correctly: only
changelog_changeset/1is modified, and the@table_nameconstant is defined once at module level to match theChangelogschema's compile-time config. - Considered flagging that the test assumes a serial
idcolumn with a{table}_id_seqsequence (not UUID pkey as shown in the moduledoc example); however, this matches the actualTestReposchema used by the test suite, so the test is valid for its environment.
Author
|
Thanks for the review.
Ready for any further CI results or approvals. |
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
Root cause was in
changelog_changeset/1(lib/ecto_trail/ecto_trail.ex:575): it only didChangeset.cast/3and never registeredunique_constraint(:id, name: "#{table}_pkey"). When any*_and_log/*(orlog/5) performed its internalrepo.insert(%Changelog{})inside an outer transaction (e.g.update_and_logcalled from app code insideRepo.transaction/1or fromEcto.Multi.run), a pkey collision (sequence reuse, custom table, or concurrent id assignment) produced anEcto.ConstraintErroronaudit_log_pkey(or the configured*_pkey) instead of a normal changeset error.Fix: read the same
@table_namecompile_env used by theChangelogschema, and chainunique_constraint/3using the conventional#{table}_pkeyname. Existing call sites inlog_changes*already treat{:error, changeset}by logging and returning{:ok, reason}, so the user mutation still succeeds and the audit row is simply not written for that op.Also added a TDD test that forces a pkey collision via
setvalon the sequence and assertsupdate_and_logreturns{:ok, updated}instead of raising.Why
Ecto.ConstraintErroronaudit_logs_pkeyduringlog_changescalled fromupdate_and_loginside a tx).ecto_trail(valiot/ecto_trail) was the source.Test plan
mix format(applied; only our new test + small refactor touched).mix compile(clean for our changes; one pre-existing redundant-clause warning onmap_custom_ecto_typeunrelated to this diff).git diffreviewed (only 4 files, real diff, no debug prints, no scope creep).mix test— could not execute in agent environment (no local Postgres; all connection attempts refused). The new test follows the exact patterns of the existing "use inside Ecto.Multi" tests and directly reproduces the pkey collision scenario described in the incident. CI on this PR will run the full suite against Postgres.palantir/OPS-4615-fix-audit-log-pkey-constraint; pushed viagit push-safe(never plaingit push).Closes OPS-4615