When passing hackney without default_opts as http_client to Goth.Token.fetch it breaks

[prem-prakash]

Goth version: 1.3-rc

When passing hackney without default_opts as http_client to Goth.Token.fetch it breaks:

This does not work:

iex(3)> Goth.Token.fetch(%{source: {:metadata, []}, http_client: {Goth.HTTPClient.Hackney, []}})
** (ArgumentError) you attempted to apply :default_opts on []. If you are using apply/3, make sure the module is an atom. If you are using the dot syntax, such as map.field or module.function(), make sure the left side of the dot is an atom or a map
    :erlang.apply([], :default_opts, [])
    (goth 1.3.0-rc.3) lib/goth/http_client/hackney.ex:40: Goth.HTTPClient.Hackney.request/6
    (goth 1.3.0-rc.3) lib/goth/token.ex:175: Goth.Token.request/1

This does work:

Goth.Token.fetch(%{source: {:metadata, []}, http_client: {Goth.HTTPClient.Hackney, %{default_opts: []}}}) 

[prem-prakash]

Forgot to mention that the impact is that it became impossible to use other http clients that don't have a default_opts key in its config

[wojtekmach]

Token.fetch/1 assumes an "initialised" http client, that is, the result of calling init/1 on the callback module. The idea was the initialisation shouldn't happen in Token.fetch/1 because it would mean doing that for every request. We don't have that problem in Goth.start_link, there we do perform initialisation ourselves because it only has to happen once. Does that make sense?

This should help:

- http_client: {Goth.HTTPClient.Hackney, %{default_opts: []}}
+ http_client: Goth.HTTPClient.init({Goth.HTTPClient.Hackney, []})

A patch with docs improvement would be appreciated!

[prem-prakash]

Hey @wojtekmach thanks for taking the time to answer this issue.

Actually the problem I am facing is more related to other libs that make use of goth.

So for example the pigeon library would call Goth.Token.fetch directly without initializing Goth. And then I am not able to inject an HTTP mock client in my tests to mock the request done by Goth.Token.fetch. Or at least I was not able to figure out a way to do this, yet.

[prem-prakash]

As soon as I understand the desired approach better, I am very willing to open a PR to contribute in code and in docs.

[prem-prakash]

@wojtekmach WDYT about making HTTP client a Goth module configuration? I can submit a PR this change

[wojtekmach]

Could you show a code snippet with what you have in mind?

[prem-prakash]

Something like this: https://github.com/peburrows/goth/pull/111

[wojtekmach]

Thank you for sending the PR. Looking at your problem description again, it looks like dep A calls dep B, and you are solving limitation of A by changing B. I believe depending on implementation details like these is brittle, perhaps dep A will change how it uses dep B without warning and it would be right to do so because again, it is an implementation detail. Maybe making dep A more customizable is a better long term solution?

[prem-prakash]

Hey @wojtekmach, thanks for your time again!

I completely understand what you said. I think this discussion is tied to the current Goth use case. Having a genserver managing the connection isn't the only use case for Goth. Because there are people using Goth.Fetch directly, as I am myself in some internal dependencies, as well as this other dependency I mentioned. And the problem we face when we do this is that the HTTP adapter configuration is being injected using parameter, which requires this parameter to be passed through the whole chain of functions. But if the definition of the HTTP adapter were injected using module configuration there would not be this problem.

This change I'm proposing tries to make it possible to change the adapter. However I believe a better implemented change would be to define a single http adapter for Goth, using a module configuration and not allow it to be defined using arguments, as it is allowed today in Goth.Fetch. I didn't do this because it wasn't clear to me what the intention was to allow to inject the http client into the Goth.Fetch call.

So I leave these two questions. What is the intention of allowing the http client to be injected in the Goth.Fetch call? Do you see any problem in defining http adapter as a Goth module configuration?

[wojtekmach]

And the problem we face when we do this is that the HTTP adapter configuration is being injected using parameter, which requires this parameter to be passed through the whole chain of functions.

I think you have a good point that passing some http_options around is annoying. It also requires all layers to be aware of that. I believe there's another option for that too, instead of "dep A" accepting some http_options to be passed to Goth.Token.fetch, "dep A" could have an :token_fetcher option, defaulting to Goth.Token.fetch, and a user could configure that. This is what for example Broadway Cloud PubSub uses as a way to configure the underlying http call.

I think an advantage of this option is you have more control, besides changing just http options, maybe you want to use cached token fetching via Goth.fetch, a goth call with some additional tracing and, say, retries, or grabbing a token using a different library altogether.

However I believe a better implemented change would be to define a single http adapter for Goth, using a module configuration and not allow it to be defined using arguments, as it is allowed today in Goth.Fetch. I didn't do this because it wasn't clear to me what the intention was to allow to inject the http client into the Goth.Fetch call.

What is the intention of allowing the http client to be injected in the Goth.Fetch call?

I believe passing explicit configuration is a better and more flexible design than relying on global configuration. See e.g: https://hexdocs.pm/elixir/library-guidelines.html#avoid-application-configuration

Do you see any problem in defining http adapter as a Goth module configuration?

That's definitely an option we could pursue but given the extra context above I'm not sure it is the best approach.

What do you think?

[prem-prakash]

Nice! This makes perfect sense! I completely agree with you. I'm going to close this PR and I'm going to mirror the Broadway Cloud PubSub approach. Thanks for clearing up this matter.