NFIBrokerage / spear

A sharp EventStoreDB v20+ client backed by Mint :yum:
https://hex.pm/packages/spear
Apache License 2.0
85 stars 14 forks source link

`Spear.subscribe/4` can return an `{:ok, _}` tuple even when subscription failed #78

Closed antondyad closed 1 year ago

antondyad commented 1 year ago

This issue seems to make the API less predictable, unless there are unknown to me reasons for it working like this.

Imagine we create a subscription like this:

{:ok, subscription} = Spear.subscribe(conn, self(), :all)

This code could succeed even when there are problems with the connection - e.g. esdb://wrong:password@host:2113/. And this is because Spear.subscribe/4 does return an {:ok, _} tuple even in some failure cases:

Success (as documented) - subscription created:

{:ok, #Reference<0.508686625.3000238081.184532>}

Access denied (no events will be received by self()):

{:ok,
 %Spear.Connection.Response{
   status: 200,
   type: {:event_store_db_gpb_protobufs_streams,
    :"event_store.client.streams.ReadResp"},
   headers: [
     {"date", "Wed, 09 Nov 2022 13:48:31 GMT"},
     {"content-type", "application/grpc"},
     {"server", "Kestrel"},
     {"content-length", "0"},
     {"exception", "access-denied"},
     {"grpc-status", "7"},
     {"grpc-message", "Access Denied"}
   ],
   data: ""
 }}

401 (no events will be received by self()):

{:ok,
 %Spear.Connection.Response{
   status: 401,
   type: {:event_store_db_gpb_protobufs_streams,
    :"event_store.client.streams.ReadResp"},
   headers: [
     {"date", "Wed, 09 Nov 2022 13:48:03 GMT"},
     {"server", "Kestrel"},
     {"www-authenticate", "X-Basic realm=\"ESDB\""},
     {"content-length", "0"}
   ],
   data: ""
 }}
the-mikedavis commented 1 year ago

That should definitely be tagged as an error 👍. Spear.Connection.Response should also be used internally only, I don't think any function should end up returning that struct.

Spear.connect_to_persistent_subscription/5 has a nice pattern for this that I think could be reused in Spear.subscribe/4: https://github.com/NFIBrokerage/spear/blob/352bc5f38bcc77a3501f1ea07fb65a79c1d4a7da/lib/spear.ex#L2314-L2331

And we can add test for this case in test/spear/connection_test.exs:

  test "a connection with an incorrect password cannot subscribe to a stream" do
    config =
      update_in(@good_config[:connection_string], &String.replace(&1, "changeit", "foobarbaz"))

    conn = start_supervised!({Spear.Connection, config})
    assert {:error, reason} = Spear.subscribe(conn, self(), :all)
    assert reason.message == "Bad HTTP status code: 401, should be 200"
  end

Would you like to make a PR?