From 15eecf317e00c4c15329475e827b641f907f7726 Mon Sep 17 00:00:00 2001 From: Jeroen Bourgois Date: Fri, 24 Apr 2026 14:00:19 +0200 Subject: [PATCH] fix: cursor pagination on events index repeating same rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Flop schema sorted on `[:inserted_at, :id]` but `search_events/2` selected a partial field list that did not include `inserted_at`. The extracted cursor therefore had `inserted_at: nil`, which Flop's Ecto adapter drops, leaving only `id > cursor_id` as the cursor predicate. Combined with the manually-prepended `order_by(desc: timestamp)` (which Flop was already warning about), each "Next" click peeled one record off the bottom of the same set instead of advancing. Have Flop own the ordering and base the cursor on `[:timestamp, :id]` — both already in the SELECT — and replace the leftover `(timestamp, inserted_at, id)` index with a `(timestamp, id)` one that matches the new ORDER BY. Co-Authored-By: Claude Opus 4.7 (1M context) --- .gitignore | 1 + lib/mailgun_logger/events/event.ex | 4 +- lib/mailgun_logger/events/events.ex | 2 - ...081601_replace_events_pagination_index.exs | 15 ++++ .../lib/mailgun_logger/events/events_test.exs | 68 +++++++++++++++++++ test/support/factory.ex | 16 +++++ 6 files changed, 102 insertions(+), 4 deletions(-) create mode 100644 priv/repo/migrations/20260424081601_replace_events_pagination_index.exs create mode 100644 test/lib/mailgun_logger/events/events_test.exs diff --git a/.gitignore b/.gitignore index a155b1a..5e5c3fa 100644 --- a/.gitignore +++ b/.gitignore @@ -35,3 +35,4 @@ docker-compose.yml node_modules .serena .playwright-mcp +.claude diff --git a/lib/mailgun_logger/events/event.ex b/lib/mailgun_logger/events/event.ex index 968d46f..2a0c8cd 100644 --- a/lib/mailgun_logger/events/event.ex +++ b/lib/mailgun_logger/events/event.ex @@ -60,9 +60,9 @@ defmodule MailgunLogger.Event do :message_subject, :account_id ], - sortable: [:inserted_at, :id], + sortable: [:timestamp, :id], default_order: %{ - order_by: [:inserted_at, :id], + order_by: [:timestamp, :id], order_directions: [:desc, :asc] }, default_pagination_type: :first, diff --git a/lib/mailgun_logger/events/events.ex b/lib/mailgun_logger/events/events.ex index 51f9af6..7fc9bcd 100644 --- a/lib/mailgun_logger/events/events.ex +++ b/lib/mailgun_logger/events/events.ex @@ -18,7 +18,6 @@ defmodule MailgunLogger.Events do ) Event - |> order_by([e], desc: e.timestamp) |> select([e], ^fields) |> Flop.validate_and_run(params, for: Event) end @@ -192,5 +191,4 @@ defmodule MailgunLogger.Events do %{count: count} -> count end end - end diff --git a/priv/repo/migrations/20260424081601_replace_events_pagination_index.exs b/priv/repo/migrations/20260424081601_replace_events_pagination_index.exs new file mode 100644 index 0000000..7437696 --- /dev/null +++ b/priv/repo/migrations/20260424081601_replace_events_pagination_index.exs @@ -0,0 +1,15 @@ +defmodule MailgunLogger.Repo.Migrations.ReplaceEventsPaginationIndex do + use Ecto.Migration + + def change do + execute( + "CREATE INDEX IF NOT EXISTS events_timestamp_id_idx ON events (timestamp DESC, id)", + "DROP INDEX IF EXISTS events_timestamp_id_idx" + ) + + execute( + "DROP INDEX IF EXISTS events_timestamp_inserted_at_id_idx", + "CREATE INDEX IF NOT EXISTS events_timestamp_inserted_at_id_idx ON events (timestamp DESC, inserted_at DESC, id)" + ) + end +end diff --git a/test/lib/mailgun_logger/events/events_test.exs b/test/lib/mailgun_logger/events/events_test.exs new file mode 100644 index 0000000..980b244 --- /dev/null +++ b/test/lib/mailgun_logger/events/events_test.exs @@ -0,0 +1,68 @@ +defmodule MailgunLogger.EventsTest do + use MailgunLogger.DataCase + + alias MailgunLogger.Events + + describe "search_events/2 cursor pagination" do + test "next page returns rows that are not on the previous page" do + account = insert(:account) + + for i <- 1..30 do + insert(:event, + account: account, + timestamp: NaiveDateTime.add(~N[2026-01-01 00:00:00], i, :second) + ) + end + + {:ok, {page1, meta1}} = Events.search_events(%{"first" => 10}) + assert length(page1) == 10 + assert meta1.has_next_page? + assert is_binary(meta1.end_cursor) + + {:ok, {page2, meta2}} = Events.search_events(%{"first" => 10, "after" => meta1.end_cursor}) + assert length(page2) == 10 + + page1_ids = Enum.map(page1, & &1.id) + page2_ids = Enum.map(page2, & &1.id) + + assert MapSet.disjoint?(MapSet.new(page1_ids), MapSet.new(page2_ids)), + "page 2 must not repeat ids from page 1, got overlap: " <> + inspect(MapSet.intersection(MapSet.new(page1_ids), MapSet.new(page2_ids))) + + {:ok, {page3, _meta3}} = Events.search_events(%{"first" => 10, "after" => meta2.end_cursor}) + page3_ids = Enum.map(page3, & &1.id) + + assert MapSet.disjoint?(MapSet.new(page2_ids), MapSet.new(page3_ids)), + "page 3 must not repeat ids from page 2, got overlap: " <> + inspect(MapSet.intersection(MapSet.new(page2_ids), MapSet.new(page3_ids))) + + assert MapSet.equal?( + MapSet.new(page1_ids ++ page2_ids ++ page3_ids), + MapSet.new(page1_ids ++ page2_ids ++ page3_ids) |> MapSet.to_list() |> MapSet.new() + ) + + assert length(page1_ids ++ page2_ids ++ page3_ids) == 30 + end + + test "rows are ordered by timestamp descending across pages" do + account = insert(:account) + + for i <- 1..30 do + insert(:event, + account: account, + timestamp: NaiveDateTime.add(~N[2026-01-01 00:00:00], i, :second) + ) + end + + {:ok, {page1, meta1}} = Events.search_events(%{"first" => 10}) + {:ok, {page2, _meta2}} = Events.search_events(%{"first" => 10, "after" => meta1.end_cursor}) + + last_of_page1 = List.last(page1).timestamp + first_of_page2 = List.first(page2).timestamp + + assert NaiveDateTime.compare(first_of_page2, last_of_page1) in [:lt, :eq], + "expected page 2 to continue chronologically after page 1, " <> + "but page 1 ends at #{last_of_page1} and page 2 starts at #{first_of_page2}" + end + end +end diff --git a/test/support/factory.ex b/test/support/factory.ex index 816b30e..387b42d 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -2,6 +2,7 @@ defmodule MailgunLogger.Factory do use ExMachina.Ecto, repo: MailgunLogger.Repo alias MailgunLogger.Account + alias MailgunLogger.Event alias MailgunLogger.User alias MailgunLogger.Role alias MailgunLogger.UserRole @@ -16,6 +17,21 @@ defmodule MailgunLogger.Factory do } end + def event_factory() do + %Event{ + api_id: sequence(:api_id, &"api_id_#{&1}"), + event: "delivered", + log_level: "info", + method: "http", + recipient: sequence(:recipient, &"to_#{&1}@example.com"), + message_from: "from@example.com", + message_subject: sequence(:subject, &"subject #{&1}"), + message_id: sequence(:message_id, &"mid_#{&1}"), + timestamp: ~N[2026-01-01 00:00:00], + account: build(:account) + } + end + def user_factory() do %User{ email: sequence(:email, &"joe-#{&1}@email.com"),