dashbitco / broadway_cloud_pub_sub

A Broadway producer for Google Cloud Pub/Sub
Apache License 2.0
70 stars 24 forks source link

Unable to use `Google.fetch/1` from Goth 1.3 #61

Closed feliperenan closed 3 years ago

feliperenan commented 3 years ago

Hi šŸ‘‹šŸ»

After reading this awesome blog post: https://dashbit.co/blog/goth-redesign

I'm trying to setup Goth 1.3 with this lib but I can't just use token_generator: MyApp, :fetch, [MyApp.Goth]} since it throws the following error:

[error] GenServer MyApp.Subscriptions.SyncEvent.Broadway.Producer_0 terminating
** (FunctionClauseError) no function clause matching in GoogleApi.PubSub.V1.Connection.new/1
    (google_api_pub_sub 0.32.1) lib/google_api/pub_sub/v1/connection.ex:25: GoogleApi.PubSub.V1.Connection.new(%Goth.Token{token: "my-token", ....})

As far as I get, that happens because it sends the whole struct instead of the string token.

https://github.com/dashbitco/broadway_cloud_pub_sub/blob/4f1cbbabda5a326b38a9b4d818fcef4395ed54bf/lib/broadway_cloud_pub_sub/google_api_client.ex#L46-L48

Notice that when using Goth.Token.for_scope it only uses the string token.

https://github.com/dashbitco/broadway_cloud_pub_sub/blob/4f1cbbabda5a326b38a9b4d818fcef4395ed54bf/lib/broadway_cloud_pub_sub/google_api_client.ex#L320-L323

Let me know if that makes sense. I'm happy to send a PR :)

cc @wojtekmach

feliperenan commented 3 years ago

For the time being, I'm using a custom function extracting only the token in case someone else is facing this:

  def fetch_token(server) do
    with {:ok, %{token: token}} <- Goth.fetch(server) do
      {:ok, token}
    end
  end

Then:

    producer: [
      module: {
        producer_module,
        subscription: subscription, token_generator: {__MODULE__, :fetch_token, [MyApp.Goth]}
      }
    ]
wojtekmach commented 3 years ago

Good catch. And thank you for trying out Goth v1.3-rc!

Our docs say:

:token_generator - Optional. An MFArgs tuple that will be called before each request to fetch an authentication token. It should return {:ok, String.t()} | {:error, any()}. Default generator uses Goth.Token.for_scope/1 with "https://www.googleapis.com/auth/pubsub".

(emphasis mine)

which is technically the truth, we're saying that we use Goth.Token, we're not saying that the default is equal to setting token_generator: {Goth.Token, :for_scope, ["https://www.googleapis.com/auth/pubsub"]}. But yeah, I don't think this is good enough.

One solution would be to change the contract to return {:ok, %{token: String.t}} | {:error, any}, which happens to match the shape of Goth response, but I am not sure we should make that change. FWIW we could do that in a backwards compatible way and do a deprecation, but yeah, I don't know.

Another solution is to allow token_generator to be a 0-arity function so it is convenient enough to use with Goth without having to define a separate function:

token_generator: fn ->
  with {:ok, token} <- Goth.fetch(server) do
    {:ok, token.token}
  end

WDYT?

cc @josevalim @mcrumm

wojtekmach commented 3 years ago

Given the contract is clear enough and the docs are correct, we decided to improve the docs. Thanks for the report!

mcrumm commented 3 years ago

@wojtekmach :+1: Good call. Once Goth v1.3 gets a full release we could decide to bump the version requirement and update the internal helper, but either way I think we're covered.

feliperenan commented 3 years ago

That sounds good. Thank you for the quick response!

Cheers šŸ» šŸ»