From 77dabbb565b9cd1631c2ffeb86e4e01e75d9d6c1 Mon Sep 17 00:00:00 2001 From: Benjamin <4800+byu@users.noreply.github.com> Date: Thu, 2 May 2024 15:32:02 -0600 Subject: [PATCH 1/7] fix: `Spear.append/3` `raw?: true` raw return Issue #96 Passing a `raw?: true` option to `Spear.append/3` or `Spear.Client/append` continued to return `:ok` instead of `{:ok, AppendResp.t()}`. This contradicted the docs and the `Spear.append_batch/5` behaviour. Solution: This commit adds the conditional case in `Spear.append/3` to return `{:ok, AppendResp.t()}` when `raw?` is `true`. `Spear.Client.append` will also be fixed as a result. Additional Notes: * Added to `SpearTest`: to test `append/3` for both raw true and false * Added to `SpearTest`: to test `append_batch/5` for both raw true and false. * Updated the `Spear.append/3` type spec to add the missing `{:ok, AppendResp.t()}` return type. * Updated the `Spear.Client.append/3` and `Spear.Client.append/4` callback specs to add the missing `{:ok, AppendResp.t()}` return type. --- lib/spear.ex | 7 ++++- lib/spear/client.ex | 4 +-- test/spear_test.exs | 62 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 3 deletions(-) diff --git a/lib/spear.ex b/lib/spear.ex index 34a21fa..4b28fe9 100644 --- a/lib/spear.ex +++ b/lib/spear.ex @@ -390,7 +390,8 @@ defmodule Spear do connection :: Spear.Connection.t(), stream_name :: String.t(), opts :: Keyword.t() - ) :: :ok | {:error, reason :: Spear.ExpectationViolation.t() | any()} + ) :: + :ok | {:ok, AppendResp.t()} | {:error, reason :: Spear.ExpectationViolation.t() | any()} def append(event_stream, conn, stream_name, opts \\ []) when is_binary(stream_name) do default_write_opts = [ expect: :any, @@ -400,6 +401,7 @@ defmodule Spear do opts = default_write_opts |> Keyword.merge(opts) params = Enum.into(opts, %{}) + raw? = Keyword.get(opts, :raw?, false) messages = [Spear.Writing.build_append_request(params)] @@ -413,6 +415,9 @@ defmodule Spear do messages, Keyword.take(opts, [:credentials, :timeout]) ) do + {:ok, Streams.append_resp(result: {:success, _} = response)} when raw? == true -> + {:ok, response} + {:ok, Streams.append_resp(result: {:success, _})} -> :ok diff --git a/lib/spear/client.ex b/lib/spear/client.ex index c834643..6085bf9 100644 --- a/lib/spear/client.ex +++ b/lib/spear/client.ex @@ -67,14 +67,14 @@ defmodule Spear.Client do """ @doc since: "0.1.0" @callback append(event_stream :: Enumerable.t(), stream_name :: String.t()) :: - :ok | {:error, any()} + :ok | {:ok, AppendResp.t()} | {:error, any()} @doc """ A wrapper around `Spear.append/4` """ @doc since: "0.1.0" @callback append(event_stream :: Enumerable.t(), stream_name :: String.t(), opts :: Keyword.t()) :: - :ok | {:error, any()} + :ok | {:ok, AppendResp.t()} | {:error, any()} @doc """ A wrapper around `Spear.append_batch/4` diff --git a/test/spear_test.exs b/test/spear_test.exs index 21c0a94..e34c990 100644 --- a/test/spear_test.exs +++ b/test/spear_test.exs @@ -923,6 +923,25 @@ defmodule SpearTest do assert Spear.cancel_subscription(c.conn, subscription) == :ok end + test "the append/3 with `raw?: false` returns :ok", c do + assert :ok = + random_events() + |> Stream.take(7) + |> Spear.append(c.conn, c.stream_name, expect: :empty, raw?: false) + end + + test "the append/3 with `raw?: true` returns the raw result", c do + assert {:ok, result} = + random_events() + |> Stream.take(7) + |> Spear.append(c.conn, c.stream_name, expect: :empty, raw?: true) + + assert {:success, + {:"event_store.client.streams.AppendResp.Success", {:current_revision, 6}, + {:position, {:"event_store.client.streams.AppendResp.Position", _p1, _p2}}}} = + result + end + @tag compatible(">= 21.6.0") test "append_batch/5 appends a batch of events", c do assert {:ok, batch_id, request_id} = @@ -978,6 +997,49 @@ defmodule SpearTest do assert Spear.stream!(c.conn, c.stream_name) |> Enum.map(& &1.body) == Enum.to_list(0..19) end + test "the append_batch/5 with `raw?: false` returns :ok", c do + assert {:ok, batch_id, request_id} = + random_events() + |> Stream.take(5) + |> Spear.append_batch(c.conn, :new, c.stream_name, expect: :empty, raw?: false) + + assert_receive %Spear.BatchAppendResult{ + result: result, + batch_id: ^batch_id, + request_id: ^request_id, + revision: 4 + } + + assert :ok = result + + assert Spear.cancel_subscription(c.conn, request_id) == :ok + + assert Spear.stream!(c.conn, c.stream_name) |> Enum.map(& &1.body) == Enum.to_list(0..4) + end + + test "the append_batch/5 with `raw?: true` returns the raw result", c do + assert {:ok, batch_id, request_id} = + random_events() + |> Stream.take(5) + |> Spear.append_batch(c.conn, :new, c.stream_name, expect: :empty, raw?: true) + + assert_receive %Spear.BatchAppendResult{ + result: result, + batch_id: ^batch_id, + request_id: ^request_id, + revision: 4 + } + + assert {:success, + {:"event_store.client.streams.BatchAppendResp.Success", {:current_revision, 4}, + {:position, {:"event_store.client.AllStreamPosition", _p1, _p2}}}} = + result + + assert Spear.cancel_subscription(c.conn, request_id) == :ok + + assert Spear.stream!(c.conn, c.stream_name) |> Enum.map(& &1.body) == Enum.to_list(0..4) + end + @tag compatible(">= 21.6.0") test "append_batch/5 can fragment with the :done? flag and :batch_id", c do assert {:ok, batch_id, request_id} = From 168ba91d9f4be1eb60676bba8eab58793607dc63 Mon Sep 17 00:00:00 2001 From: Benjamin <4800+byu@users.noreply.github.com> Date: Fri, 3 May 2024 08:40:58 -0600 Subject: [PATCH 2/7] fix: append_batch raw test, add compat tag adding compat tag to the append_batch tests, which weren't carried over from the append_batch test copy pasting. --- test/spear_test.exs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/spear_test.exs b/test/spear_test.exs index e34c990..f26390b 100644 --- a/test/spear_test.exs +++ b/test/spear_test.exs @@ -997,6 +997,7 @@ defmodule SpearTest do assert Spear.stream!(c.conn, c.stream_name) |> Enum.map(& &1.body) == Enum.to_list(0..19) end + @tag compatible(">= 21.6.0") test "the append_batch/5 with `raw?: false` returns :ok", c do assert {:ok, batch_id, request_id} = random_events() @@ -1017,6 +1018,7 @@ defmodule SpearTest do assert Spear.stream!(c.conn, c.stream_name) |> Enum.map(& &1.body) == Enum.to_list(0..4) end + @tag compatible(">= 21.6.0") test "the append_batch/5 with `raw?: true` returns the raw result", c do assert {:ok, batch_id, request_id} = random_events() From f86a9385caee525ba14eca414fc7fb6c96b3ec3a Mon Sep 17 00:00:00 2001 From: Benjamin <4800+byu@users.noreply.github.com> Date: Fri, 3 May 2024 09:38:37 -0600 Subject: [PATCH 3/7] fix: spear_test mix format Fixes a `mix format --check-formatted` issue in `spear_test.exs` that was caught with Elixir 1.14, vs not catching in Elixir 1.16.2. --- test/spear_test.exs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/spear_test.exs b/test/spear_test.exs index f26390b..5044e5d 100644 --- a/test/spear_test.exs +++ b/test/spear_test.exs @@ -1034,8 +1034,7 @@ defmodule SpearTest do assert {:success, {:"event_store.client.streams.BatchAppendResp.Success", {:current_revision, 4}, - {:position, {:"event_store.client.AllStreamPosition", _p1, _p2}}}} = - result + {:position, {:"event_store.client.AllStreamPosition", _p1, _p2}}}} = result assert Spear.cancel_subscription(c.conn, request_id) == :ok From a1038cc65a4ba72e5b4dac370987ab982db86b41 Mon Sep 17 00:00:00 2001 From: Benjamin <4800+byu@users.noreply.github.com> Date: Mon, 6 May 2024 11:35:11 -0600 Subject: [PATCH 4/7] fix: `Spear.append` raw?: returns full AppendResp Before, it was pulling out the "result", now we're returning the raw proto --- lib/spear.ex | 2 +- test/spear_test.exs | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/spear.ex b/lib/spear.ex index 4b28fe9..03264dd 100644 --- a/lib/spear.ex +++ b/lib/spear.ex @@ -415,7 +415,7 @@ defmodule Spear do messages, Keyword.take(opts, [:credentials, :timeout]) ) do - {:ok, Streams.append_resp(result: {:success, _} = response)} when raw? == true -> + {:ok, Streams.append_resp(result: {:success, _})} = response when raw? == true -> {:ok, response} {:ok, Streams.append_resp(result: {:success, _})} -> diff --git a/test/spear_test.exs b/test/spear_test.exs index 5044e5d..15ae476 100644 --- a/test/spear_test.exs +++ b/test/spear_test.exs @@ -936,9 +936,11 @@ defmodule SpearTest do |> Stream.take(7) |> Spear.append(c.conn, c.stream_name, expect: :empty, raw?: true) - assert {:success, - {:"event_store.client.streams.AppendResp.Success", {:current_revision, 6}, - {:position, {:"event_store.client.streams.AppendResp.Position", _p1, _p2}}}} = + assert {:ok, + {:"event_store.client.streams.AppendResp", + {:success, + {:"event_store.client.streams.AppendResp.Success", {:current_revision, 6}, + {:position, {:"event_store.client.streams.AppendResp.Position", _p1, _p2}}}}}} = result end From f2c5fb453d49d4510917aea8c6fb49594c08018e Mon Sep 17 00:00:00 2001 From: Benjamin <4800+byu@users.noreply.github.com> Date: Mon, 6 May 2024 14:09:52 -0600 Subject: [PATCH 5/7] fix append raw error (again) 1. fixes the unintentionally added extra nesting of `{:ok, {:ok, resp}}` Instead, when success and raw flag set, returns `{:ok, raw_response_pb}`. 2. Adds the extra raw response check so that `append/3` returns `{:error, raw_unexpected_version_response_pb}` if there is the wrong expected version response AND raw flag set. If I just did ``` {:ok, response} when raw? == true -> {:ok, response} ``` An `{:ok, raw_unexpected_version_response_pb}` could have been returned on unexpected version error response. --- lib/spear.ex | 6 +++++- test/spear_test.exs | 23 ++++++++++++++++------- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/lib/spear.ex b/lib/spear.ex index 03264dd..0b82ee1 100644 --- a/lib/spear.ex +++ b/lib/spear.ex @@ -415,12 +415,16 @@ defmodule Spear do messages, Keyword.take(opts, [:credentials, :timeout]) ) do - {:ok, Streams.append_resp(result: {:success, _})} = response when raw? == true -> + {:ok, Streams.append_resp(result: {:success, _}) = response} when raw? == true -> {:ok, response} {:ok, Streams.append_resp(result: {:success, _})} -> :ok + {:ok, Streams.append_resp(result: {:wrong_expected_version, _}) = response} + when raw? == true -> + {:error, response} + {:ok, Streams.append_resp(result: {:wrong_expected_version, expectation_violation})} -> {:error, Spear.Writing.map_expectation_violation(expectation_violation)} diff --git a/test/spear_test.exs b/test/spear_test.exs index 15ae476..ba67bec 100644 --- a/test/spear_test.exs +++ b/test/spear_test.exs @@ -931,17 +931,26 @@ defmodule SpearTest do end test "the append/3 with `raw?: true` returns the raw result", c do - assert {:ok, result} = + assert {:ok, response} = random_events() |> Stream.take(7) |> Spear.append(c.conn, c.stream_name, expect: :empty, raw?: true) - assert {:ok, - {:"event_store.client.streams.AppendResp", - {:success, - {:"event_store.client.streams.AppendResp.Success", {:current_revision, 6}, - {:position, {:"event_store.client.streams.AppendResp.Position", _p1, _p2}}}}}} = - result + assert {:"event_store.client.streams.AppendResp", + {:success, + {:"event_store.client.streams.AppendResp.Success", {:current_revision, 6}, + {:position, {:"event_store.client.streams.AppendResp.Position", _p1, _p2}}}}} = + response + end + + test "the append/3 with `raw?: true` returns expectation error as raw", c do + assert {:error, response} = + random_events() + |> Stream.take(1) + |> Spear.append(c.conn, c.stream_name, expect: 9999, raw?: true) + + assert {:"event_store.client.streams.AppendResp", + {:wrong_expected_version, _wrong_expected_version_pb_tuple}} = response end @tag compatible(">= 21.6.0") From 02e9ba395038a9361020abdc2e6df102aaee0638 Mon Sep 17 00:00:00 2001 From: Benjamin <4800+byu@users.noreply.github.com> Date: Tue, 7 May 2024 15:13:22 -0600 Subject: [PATCH 6/7] fix: `append/3` returns `{:ok, raw_resp}` for all `raw?: true` cases regardless of `:success` or `:wrong_expected_version` Also, cleans up the test cases to use `Streams.append_resp` pattern matching. --- lib/spear.ex | 6 +----- test/spear_test.exs | 27 ++++++++++++--------------- 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/lib/spear.ex b/lib/spear.ex index 0b82ee1..234f152 100644 --- a/lib/spear.ex +++ b/lib/spear.ex @@ -415,16 +415,12 @@ defmodule Spear do messages, Keyword.take(opts, [:credentials, :timeout]) ) do - {:ok, Streams.append_resp(result: {:success, _}) = response} when raw? == true -> + {:ok, response} when raw? == true -> {:ok, response} {:ok, Streams.append_resp(result: {:success, _})} -> :ok - {:ok, Streams.append_resp(result: {:wrong_expected_version, _}) = response} - when raw? == true -> - {:error, response} - {:ok, Streams.append_resp(result: {:wrong_expected_version, expectation_violation})} -> {:error, Spear.Writing.map_expectation_violation(expectation_violation)} diff --git a/test/spear_test.exs b/test/spear_test.exs index ba67bec..1bfb97c 100644 --- a/test/spear_test.exs +++ b/test/spear_test.exs @@ -7,6 +7,8 @@ defmodule SpearTest do import Spear.Uuid, only: [uuid_v4: 0] import VersionHelper + require Spear.Records.Streams, as: Streams + @max_append_bytes 1_048_576 @checkpoint_after 32 * 32 * 32 @@ -931,26 +933,21 @@ defmodule SpearTest do end test "the append/3 with `raw?: true` returns the raw result", c do - assert {:ok, response} = - random_events() - |> Stream.take(7) - |> Spear.append(c.conn, c.stream_name, expect: :empty, raw?: true) + result = + random_events() + |> Stream.take(7) + |> Spear.append(c.conn, c.stream_name, expect: :empty, raw?: true) - assert {:"event_store.client.streams.AppendResp", - {:success, - {:"event_store.client.streams.AppendResp.Success", {:current_revision, 6}, - {:position, {:"event_store.client.streams.AppendResp.Position", _p1, _p2}}}}} = - response + assert {:ok, Streams.append_resp(result: {:success, _})} = result end test "the append/3 with `raw?: true` returns expectation error as raw", c do - assert {:error, response} = - random_events() - |> Stream.take(1) - |> Spear.append(c.conn, c.stream_name, expect: 9999, raw?: true) + result = + random_events() + |> Stream.take(1) + |> Spear.append(c.conn, c.stream_name, expect: 9999, raw?: true) - assert {:"event_store.client.streams.AppendResp", - {:wrong_expected_version, _wrong_expected_version_pb_tuple}} = response + assert {:ok, Streams.append_resp(result: {:wrong_expected_version, _})} = result end @tag compatible(">= 21.6.0") From a291c791a6e7786e9868d68811a6871b39058db7 Mon Sep 17 00:00:00 2001 From: Benjamin <4800+byu@users.noreply.github.com> Date: Thu, 9 May 2024 09:42:19 -0600 Subject: [PATCH 7/7] cleanup: use record helper for `append_batch/5` raw test --- test/spear_test.exs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/spear_test.exs b/test/spear_test.exs index 1bfb97c..4e4f92a 100644 --- a/test/spear_test.exs +++ b/test/spear_test.exs @@ -1040,9 +1040,7 @@ defmodule SpearTest do revision: 4 } - assert {:success, - {:"event_store.client.streams.BatchAppendResp.Success", {:current_revision, 4}, - {:position, {:"event_store.client.AllStreamPosition", _p1, _p2}}}} = result + assert {:success, Streams.batch_append_resp_success()} = result assert Spear.cancel_subscription(c.conn, request_id) == :ok