fix: declare unique_constraint on audit_log pkey to swallow Ecto.ConstraintError in *_and_log (OPS-4600)#60
Closed
palantir-valiot[bot] wants to merge 1 commit into
Conversation
… to swallow Ecto.ConstraintError in log_changes during *_and_log. TDD test + version + changelog. Closes OPS-4600
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
Please include a summary of the change and/or which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Summary
changelog_changeset/1(used by all*_and_log/*andlog/*paths) performed a barerepo.insertof the audit log row without declaring aunique_constraint(:id, name: "<table>_pkey"). When a primary key collision occurs on the audit table inside a surrounding transaction (e.g. sequence rewind from prior insert + setval in the same tx, or concurrent writers), Postgres raises a unique violation. Ecto turns that into anEcto.ConstraintErrorbecause the changeset never listed the constraint. The exception escapeslog_changes/5(called fromupdate_and_logetc.) and aborts the caller's transaction, matching the reported:Fix: compute the pkey constraint name from the (already configurable)
:table_name(audit_logby default), expose it viaEctoTrail.Changelog.pkey_constraint_name/0, and always attachChangeset.unique_constraint(:id, name: ...)inchangelog_changeset/1. The pre-existing{:error, reason} -> Logger + {:ok, reason}handling inlog_changesandlog_changes_alonethen swallows the constraint violation exactly as it does for other log failures, so the outer*_and_logcall succeeds and the caller's work is not rolled back.Why
Linear: OPS-4600 (https://linear.app/valiot/issue/OPS-4600). First-party package
valiot/ecto_trailwas missing theunique_constraintthe error message itself tells callers to add. Stack trace and triage both point atecto_trail.ex:435inside the tx wrapper.Type of change
How Has This Been Tested?
update_and_log/3("does not raise Ecto.ConstraintError when audit log pkey is violated (swallowed)") that:update_and_log(creates one audit row and consumes one sequence value),setval(..., used_id, false)so the next audit insert will collide on the pkey,update_and_logagain and asserts the outer operation still returns{:ok, updated_resource}(no exception escapes).Ecto.ConstraintErrorreported in prod.mix format(clean).Note: the agent pod had no reachable Postgres, so a full
mix testrun could not be completed in this environment. The test was written and the diff reviewed to ensure it would have failed for the right reason pre-fix and passes post-fix. All other local checks (format, compilation of changed modules) succeeded.Test Configuration:
Checklist:
Closes OPS-4600