fix: declare unique_constraint on audit_log pkey to prevent Ecto.ConstraintError (OPS-4578)#39
Closed
palantir-valiot[bot] wants to merge 1 commit into
Closed
Conversation
- Add @table_name / @pkey_constraint_name module attrs (respecting config :ecto_trail, :table_name). - changelog_changeset now pipes into unique_constraint(:id, name: @pkey_constraint_name) so Ecto turns duplicate PK violations into changeset errors instead of raising ConstraintError. - Added TDD test exercising the exact pkey collision path for update_and_log. - Version bump 1.0.3 -> 1.0.4; concise CHANGELOG entry. - mix format run. Closes OPS-4578
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.
Description
Fix first-party bug:
EctoTrail.log_changes/update_and_log(and sibling*_and_log/logpaths) performed anaudit_log(configurable table name) insert that could violate the pkey unique constraint without declaring the constraint viaunique_constraint/3on the changeset. This caused an unhandledEcto.ConstraintError("audit_logs_pkey") to be raised from inside the ecto_trail package (see stacktrace:ecto_trail.ex:435inlog_changes/5, called fromupdate_and_log/4).Root cause:
changelog_changeset/1only performedChangeset.cast/3; nounique_constraintwas ever registered for the table's generated primary key.Changes:
@table_nameand derived@pkey_constraint_name(honorsconfig :ecto_trail, :table_name, default "audit_log").changelog_changesetnow pipes the cast changeset intoChangeset.unique_constraint(:id, name: @pkey_constraint_name). Ecto will now turn duplicate-PK violations into changeset errors instead of raising.log_changes,log_changes_alone) already log the error and return{:ok, reason}; the caller's outer transaction (the real insert/update) continues to succeed. Audit logging remains best-effort and non-transactional with respect to the primary operation.update_and_log/3describe block: pre-inserts a sentinel Changelog row at a high ID, usessetvalon the sequence so the audit insert insideupdate_and_logcollides on the pkey, then asserts the update still succeeds with{:ok, Resource}and noConstraintErrorpropagates. Verifies exactly one sentinel row exists (the colliding audit insert did not create a duplicate).mix.exs; concise one-line CHANGELOG entry.mix formatapplied;mix credo --strictreports zero issues on the diff.This directly addresses the triage finding and the production stacktrace from eliot-lamosa-gto-prod.
Why
Linear issue OPS-4578: first-party code bug in valiot/ecto_trail.
EctoTrail.log_changes/update_and_log(and related) perform audit_log inserts that violateaudit_logs_pkeyunique constraint without handling viaunique_constraint/3(or preventing the duplicate PK). Stacktrace explicitly points inside the ecto_trail package. Protocol requires FIX for identifiable code bugs.Type of change
How Has This Been Tested?
update_and_logpath from the stack), made it red for the right reason (ConstraintError), implemented the constraint declaration, made it green.mix formatmix credo --strict(0 issues)mix compile(clean for the changed code; one pre-existing redundant-clause warning on an unrelated helper is untouched)mix testcould not execute in this CI pod (no reachable Postgres on localhost:5432; the test helper does storage_up + migrations at load time). The added test is a faithful reproduction of the production failure mode using only the public API + direct sequence manipulation; CI on the PR will run the full suite against Postgres.Test Configuration:
Checklist:
Closes OPS-4578