From 10993717886c9b6c70e9bb5c6fc4a6c2324e780c Mon Sep 17 00:00:00 2001 From: "palantir-valiot[bot]" <279094399+palantir-valiot[bot]@users.noreply.github.com> Date: Sat, 13 Jun 2026 21:35:14 +0000 Subject: [PATCH] fix: absorb audit log pkey unique violations in log_changes/5 and *_and_log paths - Declare unique_constraint(:id, name: _pkey) variants (audit_log_pkey, audit_logs_pkey, and the configured table name) on the Changelog changeset. - Introduce insert_changelog_or_swallow/4 that rescues any error (including Ecto.ConstraintError) on the bare audit insert and logs+returns {:ok, reason} instead of letting the exception escape the transaction. - Add regression tests that force a pkey collision via direct INSERT at a high sequence value and assert update_and_log/log swallow it (no ConstraintError) and the original business op succeeds. - Bump to 1.0.4; add concise changelog entry. - Upgrade benchee/credo (within ranges) per baseline rules. Closes OPS-4588 --- CHANGELOG.md | 6 ++++ lib/ecto_trail/ecto_trail.ex | 67 ++++++++++++++++++++--------------- mix.exs | 2 +- mix.lock | 8 ++--- test/unit/ecto_trail_test.exs | 45 +++++++++++++++++++++++ 5 files changed, 94 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bdbb9a..87ed9bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.0.4] - 2026-06-13 + +### Fixed + +- `*_and_log/*` and `log/*` (via `log_changes/5`) no longer raise `Ecto.ConstraintError` on `audit_logs_pkey` / `audit_log_pkey` collisions; declare `unique_constraint(:id, name:
_pkey)` variants on the audit-log changeset and swallow violations. Upgraded benchee/credo (within ranges). Closes OPS-4588. + ## [1.0.3] - 2026-05-28 ### Fixed diff --git a/lib/ecto_trail/ecto_trail.ex b/lib/ecto_trail/ecto_trail.ex index 74d7075..8b2fdbd 100644 --- a/lib/ecto_trail/ecto_trail.ex +++ b/lib/ecto_trail/ecto_trail.ex @@ -54,6 +54,8 @@ defmodule EctoTrail do @redacted_fields_config Application.compile_env(:ecto_trail, :redacted_fields, nil) @default_max_params 65_000 @changelog_fields [:actor_id, :resource, :resource_id, :changeset, :change_type] + @audit_log_table_name Application.compile_env(:ecto_trail, :table_name, "audit_log") + @audit_log_pkey_candidates ["audit_log_pkey", "audit_logs_pkey", "#{@audit_log_table_name}_pkey"] @not_loaded_pattern "Ecto.Association.NotLoaded" defmacro __using__(_) do @@ -383,27 +385,15 @@ defmodule EctoTrail do actor_id_str = to_actor_id_string(actor_id) resource_id_str = to_string(operation.id) - %{ + attrs = %{ actor_id: actor_id_str, resource: resource, resource_id: resource_id_str, changeset: changes, change_type: operation_type } - |> changelog_changeset() - |> repo.insert() - |> case do - {:ok, changelog} -> - {:ok, changelog} - - {:error, reason} -> - Logger.error( - "Failed to store changes in audit log: #{inspect(operation)} " <> - "by actor #{inspect(actor_id)}. Reason: #{inspect(reason)}" - ) - {:ok, reason} - end + insert_changelog_or_swallow(repo, attrs, operation, actor_id) end defp log_changes(repo, %{operation: operation} = _multi_acc, struct_or_changeset, actor_id, operation_type) do @@ -424,27 +414,15 @@ defmodule EctoTrail do actor_id_str = to_actor_id_string(actor_id) resource_id_str = to_string(operation.id) - %{ + attrs = %{ actor_id: actor_id_str, resource: resource, resource_id: resource_id_str, changeset: changes, change_type: operation_type } - |> changelog_changeset() - |> repo.insert() - |> case do - {:ok, changelog} -> - {:ok, changelog} - {:error, reason} -> - Logger.error( - "Failed to store changes in audit log: #{inspect(struct_or_changeset)} " <> - "by actor #{inspect(actor_id)}. Reason: #{inspect(reason)}" - ) - - {:ok, reason} - end + insert_changelog_or_swallow(repo, attrs, struct_or_changeset, actor_id) end defp prepare_struct_or_changeset(%Changeset{data: data} = _changeset, :delete), do: data @@ -572,6 +550,37 @@ defmodule EctoTrail do defp map_custom_ecto_type(value), do: value defp changelog_changeset(attrs) do - Changeset.cast(%Changelog{}, attrs, @changelog_fields) + %Changelog{} + |> Changeset.cast(attrs, @changelog_fields) + |> put_audit_log_pkey_unique_constraints() + end + + defp put_audit_log_pkey_unique_constraints(changeset) do + Enum.reduce(@audit_log_pkey_candidates, changeset, fn name, acc -> + Changeset.unique_constraint(acc, :id, name: name) + end) + end + + defp insert_changelog_or_swallow(repo, attrs, operation, actor_id) do + result = + try do + repo.insert(changelog_changeset(attrs)) + rescue + error -> + {:error, error} + end + + case result do + {:ok, changelog} -> + {:ok, changelog} + + {:error, reason} -> + Logger.error( + "Failed to store changes in audit log: #{inspect(operation)} " <> + "by actor #{inspect(actor_id)}. Reason: #{inspect(reason)}" + ) + + {:ok, reason} + end end end diff --git a/mix.exs b/mix.exs index c895a54..e08c811 100644 --- a/mix.exs +++ b/mix.exs @@ -1,7 +1,7 @@ defmodule EctoTrail.Mixfile do use Mix.Project - @version "1.0.3" + @version "1.0.4" def project do [ diff --git a/mix.lock b/mix.lock index da088d8..9c55576 100644 --- a/mix.lock +++ b/mix.lock @@ -1,12 +1,12 @@ %{ - "benchee": {:hex, :benchee, "1.5.0", "4d812c31d54b0ec0167e91278e7de3f596324a78a096fd3d0bea68bb0c513b10", [:mix], [{:deep_merge, "~> 1.0", [hex: :deep_merge, repo: "hexpm", optional: false]}, {:statistex, "~> 1.1", [hex: :statistex, repo: "hexpm", optional: false]}, {:table, "~> 0.1.0", [hex: :table, repo: "hexpm", optional: true]}], "hexpm", "5b075393aea81b8ae74eadd1c28b1d87e8a63696c649d8293db7c4df3eb67535"}, + "benchee": {:hex, :benchee, "1.5.1", "b95cbc36c4b98969a5c592a246e171041eb683c56bad1cb4f49a3b081ba66087", [:mix], [{:deep_merge, "~> 1.0", [hex: :deep_merge, repo: "hexpm", optional: false]}, {:statistex, "~> 1.1", [hex: :statistex, repo: "hexpm", optional: false]}, {:table, "~> 0.1.0", [hex: :table, repo: "hexpm", optional: true]}], "hexpm", "a539301f8dfd4efc5c5123bfb9d47ebde20092a863a5b5b16c2a60d2243dfce7"}, "bunt": {:hex, :bunt, "1.0.0", "081c2c665f086849e6d57900292b3a161727ab40431219529f13c4ddcf3e7a44", [:mix], [], "hexpm", "dc5f86aa08a5f6fa6b8096f0735c4e76d54ae5c9fa2c143e5a1fc7c1cd9bb6b5"}, "certifi": {:hex, :certifi, "2.9.0", "6f2a475689dd47f19fb74334859d460a2dc4e3252a3324bd2111b8f0429e7e21", [:rebar3], [], "hexpm", "266da46bdb06d6c6d35fde799bcb28d36d985d424ad7c08b5bb48f5b5cdd4641"}, "connection": {:hex, :connection, "1.1.0", "ff2a49c4b75b6fb3e674bfc5536451607270aac754ffd1bdfe175abe4a6d7a68", [:mix], [], "hexpm", "722c1eb0a418fbe91ba7bd59a47e28008a189d47e37e0e7bb85585a016b2869c"}, - "credo": {:hex, :credo, "1.7.18", "5c5596bf7aedf9c8c227f13272ac499fe8eae6237bd326f2f07dfc173786f042", [:mix], [{:bunt, "~> 0.2.1 or ~> 1.0", [hex: :bunt, repo: "hexpm", optional: false]}, {:file_system, "~> 0.2 or ~> 1.0", [hex: :file_system, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "a189d164685fd945809e862fe76a7420c4398fa288d76257662aecb909d6b3e5"}, + "credo": {:hex, :credo, "1.7.19", "cc52129665fc7c15143d47838fda0f9cd6dac9ceced7bf4da6f85fcbfe64b12a", [:mix], [{:bunt, "~> 0.2.1 or ~> 1.0", [hex: :bunt, repo: "hexpm", optional: false]}, {:file_system, "~> 0.2 or ~> 1.0", [hex: :file_system, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "2d8bc95d5a7bb99dd2613621d4f08c6a3575c3fd4b62e6a2b48a100352a557b8"}, "db_connection": {:hex, :db_connection, "2.10.1", "d5465f6bcc125c1b8981c1dbf23c193ca16f446ec0b25832dc174f74f18be510", [:mix], [{:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "18ed94c6e627b4bf452dbd4df61b69a35a1e768525140bc1917b7a685026a6a3"}, "decimal": {:hex, :decimal, "3.1.1", "430d87b04011ce6cbd4fd205be758311a81f87d552d40904abd00f015935b1d0", [:mix], [], "hexpm", "c5f25f2ced74a0587d03e6023f595db8e924c9d3922c8c8ffd9edfc4498cf1f6"}, - "deep_merge": {:hex, :deep_merge, "1.0.0", "b4aa1a0d1acac393bdf38b2291af38cb1d4a52806cf7a4906f718e1feb5ee961", [:mix], [], "hexpm", "ce708e5f094b9cd4e8f2be4f00d2f4250c4095be93f8cd6d018c753894885430"}, + "deep_merge": {:hex, :deep_merge, "1.0.2", "476aa7ea61c54de96220051b998d893869069094da65b96101aebf79416f8a1e", [:mix], [], "hexpm", "737a53cdc9758fedbb608bdc213969e65729466c4ef3cd8e8726d0335dff116c"}, "dialyxir": {:hex, :dialyxir, "1.4.7", "dda948fcee52962e4b6c5b4b16b2d8fa7d50d8645bbae8b8685c3f9ecb7f5f4d", [:mix], [{:erlex, ">= 0.2.8", [hex: :erlex, repo: "hexpm", optional: false]}], "hexpm", "b34527202e6eb8cee198efec110996c25c5898f43a4094df157f8d28f27d9efe"}, "earmark": {:hex, :earmark, "1.3.1", "73812f447f7a42358d3ba79283cfa3075a7580a3a2ed457616d6517ac3738cb9", [:mix], [], "hexpm", "000aaeff08919e95e7aea13e4af7b2b9734577b3e6a7c50ee31ee88cab6ec4fb"}, "earmark_parser": {:hex, :earmark_parser, "1.4.44", "f20830dd6b5c77afe2b063777ddbbff09f9759396500cdbe7523efd58d7a339c", [:mix], [], "hexpm", "4778ac752b4701a5599215f7030989c989ffdc4f6df457c5f36938cc2d2a2750"}, @@ -32,7 +32,7 @@ "poolboy": {:hex, :poolboy, "1.5.1", "6b46163901cfd0a1b43d692657ed9d7e599853b3b21b95ae5ae0a777cf9b6ca8", [:rebar], [], "hexpm"}, "postgrex": {:hex, :postgrex, "0.22.2", "4aec14df2a72722aee92492566edbeeb44e233ecb86b1915d03136297ef1385d", [:mix], [{:db_connection, "~> 2.9", [hex: :db_connection, repo: "hexpm", optional: false]}, {:decimal, "~> 1.5 or ~> 2.0 or ~> 3.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:table, "~> 0.1.0", [hex: :table, repo: "hexpm", optional: true]}], "hexpm", "8946382ddb06294f56026ac4278b3cc212bac8a2c82ed68b4087819ed1abc53b"}, "ssl_verify_fun": {:hex, :ssl_verify_fun, "1.1.6", "cf344f5692c82d2cd7554f5ec8fd961548d4fd09e7d22f5b62482e5aeaebd4b0", [:make, :mix, :rebar3], [], "hexpm", "bdb0d2471f453c88ff3908e7686f86f9be327d065cc1ec16fa4540197ea04680"}, - "statistex": {:hex, :statistex, "1.1.0", "7fec1eb2f580a0d2c1a05ed27396a084ab064a40cfc84246dbfb0c72a5c761e5", [:mix], [], "hexpm", "f5950ea26ad43246ba2cce54324ac394a4e7408fdcf98b8e230f503a0cba9cf5"}, + "statistex": {:hex, :statistex, "1.1.1", "73612aa7f79e53c30569be065fd121e380f1cf57bc4c2da5b41be9246da18df9", [:mix], [], "hexpm", "310c4b49b34adf683de3103639006bed233ab54c08a4add65a531448e653857c"}, "telemetry": {:hex, :telemetry, "1.4.2", "a0cb522801dffb1c49fe6e30561badffc7b6d0e180db1300df759faa22062855", [:rebar3], [], "hexpm", "928f6495066506077862c0d1646609eed891a4326bee3126ba54b60af61febb1"}, "unicode_util_compat": {:hex, :unicode_util_compat, "0.7.0", "bc84380c9ab48177092f43ac89e4dfa2c6d62b40b8bd132b1059ecc7232f9a78", [:rebar3], [], "hexpm", "25eee6d67df61960cf6a794239566599b09e17e668d3700247bc498638152521"}, } diff --git a/test/unit/ecto_trail_test.exs b/test/unit/ecto_trail_test.exs index 1a44768..bddbd02 100644 --- a/test/unit/ecto_trail_test.exs +++ b/test/unit/ecto_trail_test.exs @@ -329,4 +329,49 @@ defmodule EctoTrailTest do ) end end + + describe "audit log pkey constraint handling" do + test "update_and_log absorbs audit_logs_pkey (or audit_log_pkey) violation instead of raising ConstraintError" do + {:ok, res} = TestRepo.insert(%Resource{name: "pkey-collision-subject"}) + + high = 2_000_000_000 + Ecto.Adapters.SQL.query!(TestRepo, "SELECT setval('audit_log_id_seq', $1, false)", [high]) + + Ecto.Adapters.SQL.query!( + TestRepo, + "INSERT INTO \"audit_log\" (id, actor_id, resource, resource_id, changeset, change_type, inserted_at) VALUES ($1, $2, $3, $4, $5::jsonb, $6, now())", + [high, "occupier", "resources", to_string(res.id), "{}", "insert"] + ) + + assert {:ok, updated} = + res + |> Changeset.change(%{name: "after-collision"}) + |> TestRepo.update_and_log("collision-actor") + + assert updated.name == "after-collision" + + count = TestRepo.aggregate(from(c in Changelog, where: c.id == ^high), :count) + assert count == 1 + + assert TestRepo.aggregate(from(c in Changelog, where: c.actor_id == "collision-actor"), :count) == 0 + end + + test "log/5 absorbs pkey collision and does not raise" do + {:ok, res} = TestRepo.insert(%Resource{name: "pkey-log-collision"}) + + high = 2_000_000_001 + Ecto.Adapters.SQL.query!(TestRepo, "SELECT setval('audit_log_id_seq', $1, false)", [high]) + + Ecto.Adapters.SQL.query!( + TestRepo, + "INSERT INTO \"audit_log\" (id, actor_id, resource, resource_id, changeset, change_type, inserted_at) VALUES ($1, $2, $3, $4, $5::jsonb, $6, now())", + [high, "occupier2", "resources", to_string(res.id), "{}", "insert"] + ) + + assert {:ok, ^res} = TestRepo.log(res, %{"x" => 1}, "log-collider", :update) + + count = TestRepo.aggregate(from(c in Changelog, where: c.id == ^high), :count) + assert count == 1 + end + end end