fix: autogenerate binary_id UUID PK + unique_constraint(:id) to prevent audit_logs_pkey ConstraintError in log_changes/update_and_log (OPS-4607)#68
Closed
palantir-valiot[bot] wants to merge 1 commit into
Conversation
…(:id) on changeset to turn pkey violations into errors instead of ConstraintError raises (update_and_log/log_changes). Bump 1.1.0. Closes OPS-4607
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
First-party
ecto_trailraisedEcto.ConstraintErroronaudit_logs_pkeyinsidelog_changes/update_and_log(and equivalent paths) when called from within a transaction (e.g.valiot_appupdateDevice). Root cause: theChangelogschema (used foraudit_logrows) did not declare an autogenerating@primary_key(so no server-side UUID was generated on insert), andchangelog_changeset/1did not registerunique_constraint(:id). The result was either missing ID generation or raw constraint exceptions bubbling up as 500s instead of changeset errors.Changes:
lib/ecto_trail/changelog.ex:8: add@primary_key {:id, :binary_id, autogenerate: true}so every inserted audit row gets a generated UUIDv4.lib/ecto_trail/ecto_trail.ex:575:changelog_changeset/1now pipes throughChangeset.unique_constraint(:id)so pkey collisions become changeset errors (not raisedConstraintError).__changelog_changeset__/1(used to assert constraint registration in a no-DB context).priv/repo/migrations/20170419082821_create_log_changes_table.exs) to match the documented UUID PK shape used inREADME.mdexamples and the module docs.test/unit/ecto_trail_test.exsasserting binary UUID PK on producedChangelogrows.1.1.0(minor: explicit ID generation + constraint handling behavior).CHANGELOG.md.benchee,credo, transitivedeep_merge/statistex) per Palantir baseline rules.This matches the exact stack in the Linear issue (ecto_trail 1.0.3
lib/ecto_trail/ecto_trail.ex:435inlog_changes/5called fromupdate_and_log/4).Why
Closes OPS-4607 (https://linear.app/valiot/issue/OPS-4607). High-severity code bug: missing/insufficient unique handling + ID generation for audit rows under concurrent/rapid updates inside transactions. The triage decision was NOTIFY+FIX.
Test plan
mix format --check-formatted(clean)mix compile(clean; only pre-existing redundant-clause warning unrelated to this diff)mix run -e(no DB) that__changelog_changeset__now registersunique_constraint(:id)on the:idfield; temporarily removed the constraint to demonstrate red/green.insert_and_logpath and assertingis_binary(id) and byte_size==36(UUID string length) on the producedChangelogrow.mix hex.outdated+mix deps.updateperformed; only in-range upgrades landed (benchee 1.5.0→1.5.1,credo 1.7.18→1.7.19and transitives).mix testcannot run in this pod (no Postgres reachable;test/test_helper.exsrequires a live DB for sandbox/migrations). The DB-dependent path was exercised via the ID-generation test (which will run in CI) and the no-DB constraint registration check. The core fix is the schema + changeset change; the error path change is proven by the constraint list assertion.palantir/OPS-46xx-*audit-pkey variants) exist; this is the canonical fix for the reported symptom (autogenerate + unique_constraint on the changeset).Files changed (from
git diff --stat)Closes OPS-4607