Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

- `log_changes` / `update_and_log` (and siblings) now catch `Ecto.ConstraintError` (e.g. duplicate audit_log pkey) and other raised errors from the audit insert so they do not abort the caller's transaction. Closes OPS-4572.

## [1.0.3] - 2026-05-28

### Fixed
Expand Down
47 changes: 21 additions & 26 deletions lib/ecto_trail/ecto_trail.ex
Original file line number Diff line number Diff line change
Expand Up @@ -391,19 +391,7 @@ defmodule EctoTrail do
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(repo)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log_changes_alone/6 (used by the public log/5) now passes a plain map directly to insert_changelog/2, which does repo.insert(changeset). Ecto.Repo.insert/1 requires a changeset or struct, not a bare map — this will raise ArgumentError at runtime for any call to Repo.log/4. The new helper was applied inconsistently: log_changes/5 correctly wraps via changelog_changeset/1 first, but log_changes_alone/6 skips that step.

end

defp log_changes(repo, %{operation: operation} = _multi_acc, struct_or_changeset, actor_id, operation_type) do
Expand Down Expand Up @@ -432,19 +420,7 @@ defmodule EctoTrail do
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(repo)
end

defp prepare_struct_or_changeset(%Changeset{data: data} = _changeset, :delete), do: data
Expand Down Expand Up @@ -574,4 +550,23 @@ defmodule EctoTrail do
defp changelog_changeset(attrs) do
Changeset.cast(%Changelog{}, attrs, @changelog_fields)
end

defp insert_changelog(changeset, repo) do
changeset
|> repo.insert()
|> case do
{:ok, changelog} ->
{:ok, changelog}

{:error, reason} ->
Logger.error("Failed to store changes in audit log. Reason: #{inspect(reason)}")

{:ok, reason}
end
rescue
error ->
Logger.error("Failed to store changes in audit log (raised). Reason: #{inspect(error)}")

{:ok, error}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rescue block in insert_changelog/2 returns {:ok, error} (the exception struct). Callers (*_and_log/*, log/*) treat this as success and proceed, which matches the intended contract. However, the function no longer distinguishes a rescued exception from a normal {:error, reason} tuple in its return value, so any downstream code that pattern-matches on the second element will see an exception struct instead of a changeset or atom reason.

end
end
2 changes: 1 addition & 1 deletion mix.exs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
defmodule EctoTrail.Mixfile do
use Mix.Project

@version "1.0.3"
@version "1.0.4"

def project do
[
Expand Down
8 changes: 4 additions & 4 deletions mix.lock
Original file line number Diff line number Diff line change
@@ -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"},
Expand All @@ -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"},
}
61 changes: 61 additions & 0 deletions test/unit/ecto_trail_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -328,5 +328,66 @@ defmodule EctoTrailTest do
:count
)
end

test "update_and_log succeeds and commits business change even when audit log insert raises on pkey unique violation" do
{:ok, schema} = TestRepo.insert(%Resource{name: "name-for-collision"})

# Seed one audit log row via the public path so it gets a generated PK id.
TestRepo.insert!(%Changelog{
actor_id: "seed-collision",
resource: "resources",
resource_id: to_string(schema.id),
changeset: %{},
change_type: :update,
inserted_at: DateTime.utc_now() |> DateTime.truncate(:second)
})

seed =
TestRepo.one(
from(c in Changelog, where: c.actor_id == "seed-collision", order_by: [desc: c.id], limit: 1)
)

seed_id = seed.id

# Rewind the sequence so the *next* audit insert (from update_and_log) will be assigned the same id -> pkey collision.
# This forces the exact ConstraintError path seen in OPS-4572 inside the library's log_changes.
TestRepo.query!("SELECT setval('audit_log_id_seq', $1, false)", [seed_id - 1])

# Call under test: the log_changes inside will hit duplicate pkey on insert and must not rollback the tx.
result =
schema
|> Changeset.change(%{name: "updated-despite-audit-collision"})
|> TestRepo.update_and_log("collision-actor")

assert {:ok, %Resource{name: "updated-despite-audit-collision"}} = result
assert TestRepo.get(Resource, schema.id).name == "updated-despite-audit-collision"
end
end

describe "log/4 (public, non-transactional)" do
test "logs without raising and returns {:ok, struct} even on audit collision" do
{:ok, schema} = TestRepo.insert(%Resource{name: "log-direct"})

# Seed a colliding id for the next audit row.
TestRepo.insert!(%Changelog{
actor_id: "log-direct-seed",
resource: "resources",
resource_id: to_string(schema.id),
changeset: %{},
change_type: :insert,
inserted_at: DateTime.utc_now() |> DateTime.truncate(:second)
})

seed =
TestRepo.one(
from(c in Changelog, where: c.actor_id == "log-direct-seed", order_by: [desc: c.id], limit: 1)
)

TestRepo.query!("SELECT setval('audit_log_id_seq', $1, false)", [seed.id - 1])

# Public log/4 path (exercises log_changes_alone) must not raise; it returns {:ok, input}.
result = TestRepo.log(schema, %{"name" => "log-direct"}, "log-direct-actor", :insert)
assert {:ok, ^schema} = result
end
end
end