fix: `Spear.append/3` `raw?: true` raw return #97

byu · 2024-05-02T21:44:14Z

Context

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.

Issue NFIBrokerage#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.

coveralls · 2024-05-03T13:41:25Z

Pull Request Test Coverage Report for Build a291c791a6e7786e9868d68811a6871b39058db7-PR-97

Details

2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build a08dfa90d210ded99dbe5dbcbc9e0fc0f3af0996:	0.0%
Covered Lines:	711
Relevant Lines:	711

💛 - Coveralls

byu · 2024-05-03T14:24:25Z

I dev'd against image: eventstore/eventstore:24.2.0-alpha-arm64v8

I copy-pasta'd from other append_batch test...

so, those errors between 20.10.2 vs 22.6 "Compatibility / Bless"? Maybe I misunderstood compatibility, and should've copied in the tag too?

    @tag compatible(">= 21.6.0")
    test "append_batch/5 appends a batch of events", c do
      assert {:ok, batch_id, request_id} =
               random_events()
               |> Stream.take(5)
               |> Spear.append_batch(c.conn, :new, c.stream_name, expect: :empty)

Also, not sure why "CI / Bless" is saying test/spear_test.exs is failing the format?

A local mix format is showing spear_test.exs unchanged from what i PR'd?

adding compat tag to the append_batch tests, which weren't carried over from the append_batch test copy pasting.

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.

the-mikedavis · 2024-05-06T14:02:37Z

The formatting might be caused by changes to mix format in newer Elixir versions. I have Elixir/Erlang pinned at relatively old versions in https://github.com/NFIBrokerage/spear/blob/main/.tool-versions

I think it would be ok to bump those but it should be a separate change from this fix

the-mikedavis · 2024-05-06T14:04:10Z

Yeah the compatibility checks are there to make sure we can run against older EventStoreDB releases and with older Elixir/Erlang versions. I believe the batch append RPC was introduced in 21.6.0 so we can't run tests against it on all versions of the matrix

lib/spear.ex

Before, it was pulling out the "result", now we're returning the raw proto

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.

test/spear_test.exs

`raw?: true` cases regardless of `:success` or `:wrong_expected_version` Also, cleans up the test cases to use `Streams.append_resp` pattern matching.

test/spear_test.exs

the-mikedavis

Thanks for working on this!

In retrospect I wish the return of Spear.append/3 was {ok, metadata} with a small map or struct that had the extra metadata in the append response rather than just ok. It would be a breaking change though so I don't think it's worth making it at this point.

the-mikedavis · 2024-05-09T19:08:28Z

test/spear_test.exs

+        revision: 4
+      }
+
+      assert {:success, Streams.batch_append_resp_success()} = result


It's weird that this behaves differently on Elixir 1.7. I want to bump the versions anyway though so I will merge this as-is

Thanks for your patience in the collaboration. It was also a learning experience for me, e.g. grpc in the erlang ecosystem, to gain more knowledge about event store db and this spear library (which will be a key part in my new project).

I'm definitely open to contribute again if/when things pop up as I use spear. 👍

the-mikedavis self-requested a review May 3, 2024 13:38

byu added 2 commits May 3, 2024 08:40

fix: append_batch raw test, add compat tag

168ba91

adding compat tag to the append_batch tests, which weren't carried over from the append_batch test copy pasting.

fix: spear_test mix format

f86a938

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.

the-mikedavis reviewed May 6, 2024

View reviewed changes

lib/spear.ex Outdated Show resolved Hide resolved

byu added 2 commits May 6, 2024 11:35

fix: Spear.append raw?: returns full AppendResp

a1038cc

Before, it was pulling out the "result", now we're returning the raw proto

the-mikedavis reviewed May 7, 2024

View reviewed changes

test/spear_test.exs Outdated Show resolved Hide resolved

fix: append/3 returns {:ok, raw_resp} for all

02e9ba3

`raw?: true` cases regardless of `:success` or `:wrong_expected_version` Also, cleans up the test cases to use `Streams.append_resp` pattern matching.

the-mikedavis reviewed May 9, 2024

View reviewed changes

test/spear_test.exs Outdated Show resolved Hide resolved

cleanup: use record helper for append_batch/5 raw test

a291c79

the-mikedavis approved these changes May 9, 2024

View reviewed changes

the-mikedavis merged commit eb8d5b6 into NFIBrokerage:main May 9, 2024
5 of 6 checks passed

byu deleted the fix-append-raw-true-result branch May 9, 2024 20:36

the-mikedavis mentioned this pull request May 13, 2024

Spear.append with raw?: true does not do what docs say? #96

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: `Spear.append/3` `raw?: true` raw return #97

fix: `Spear.append/3` `raw?: true` raw return #97

byu commented May 2, 2024

coveralls commented May 3, 2024 •

edited

byu commented May 3, 2024 •

edited

the-mikedavis commented May 6, 2024

the-mikedavis commented May 6, 2024

the-mikedavis left a comment

the-mikedavis May 9, 2024

byu May 9, 2024

fix: Spear.append/3 raw?: true raw return #97

fix: Spear.append/3 raw?: true raw return #97

Conversation

byu commented May 2, 2024

Context

Solution

Additional Notes

coveralls commented May 3, 2024 • edited

Pull Request Test Coverage Report for Build a291c791a6e7786e9868d68811a6871b39058db7-PR-97

Details

💛 - Coveralls

byu commented May 3, 2024 • edited

the-mikedavis commented May 6, 2024

the-mikedavis commented May 6, 2024

the-mikedavis left a comment

Choose a reason for hiding this comment

the-mikedavis May 9, 2024

Choose a reason for hiding this comment

byu May 9, 2024

Choose a reason for hiding this comment

fix: `Spear.append/3` `raw?: true` raw return #97

fix: `Spear.append/3` `raw?: true` raw return #97

coveralls commented May 3, 2024 •

edited

byu commented May 3, 2024 •

edited