search

peburrows / goth

Elixir package for Oauth authentication via Google Cloud APIs
http://hexdocs.pm/goth
MIT License
284 stars 108 forks source link

Goth redesign #82

Closed wojtekmach closed 2 years ago

wojtekmach commented 3 years ago

Hi, I'd like to propose a Goth redesign that replaces the global, application env-based, configuration with a more direct approach. This will in general give more control to the users (e.g. when exactly Goth starts, how it's configured, etc) and in particular it will solve one of the long standing issues of supporting multiple credentials. The new design is also more scalable as it replaces a single GenServer with process-less lookups. Finally, I'd like to also make the http client swappable and reduce the number of dependencies.

Here's the plan I have in mind:

I decided to keep the existing code as is (with @moduledoc false, @doc false, etc) to not break existing clients.

I'll send the PR for the first two items shortly.

dantswain commented 3 years ago

I am running into exactly the same sorts of issues that this is meant to solve.

peburrows commented 3 years ago

This is awesome, thank you so much for putting in the time on this; I'm totally aligned on everything in your plan ☺️

wojtekmach commented 3 years ago

Before shipping 1.3, we should do one more change. Currently the name must be an atom and we use it for the process and for the ets table name. This is not ideal when users might want to dynamically start many servers. We can solve this by registering the processes with Registry. The registry will also handle the storage of the token and the config, it's an ETS table too so nothing will change in that regard.

pacoguzman commented 3 years ago

@wojtekmach do you think we're ready for v1.13? or at least release an rc version which includes the registry thingy? Thanks in advance

chasers commented 3 years ago

Thanks for this. I've got Logflare on the latest RC and all looks good for far! Hitting about 300 times per second roughly.

wojtekmach commented 3 years ago

@pacoguzman I just published v1.3.0-rc.3. There are a few things left before the final, the pending PRs, but we're really close.

@chasers thanks for taking the time to test it and report feedback. Really appreciate it!

pacoguzman commented 3 years ago

@wojtekmach thanks, we're already using this in our side and no issue so far

superruzafa commented 3 years ago

@pacoguzman I just published v1.3.0-rc.3. There are a few things left before the final, the pending PRs, but we're really close.

@wojtekmach

ATTOW, it seems that version cannot be found in hex.pm:

https://hex.pm/packages/goth/1.3.0-rc.3

wojtekmach commented 3 years ago

oops, sorry about that. It is out on Hex now.

feliperenan commented 3 years ago

I'm also using 1.3.0-rc.3 here and everything looks fine until now 🎉

Just one question though: With legacy version we were able to get the project_id with either: Goth.Client.retrieve_metadata_project or Goth.Config.get("project_id") so that the second fetches it from a GenServer state if I'm not mistaken.

Could you guys tell me if that will still be supported?

wojtekmach commented 3 years ago

We plan not to support it to keep the library more focused. See https://github.com/peburrows/goth/issues/107.

dsdshcym commented 3 years ago

Hi @wojtekmach We encountered a minor issue with 1.3.0-rc.3 that occurs when the app is shutting down:

Since Goth.Registry starts with the :goth application, but Goth.Server is started by our application, Goth.Registry would shutdown before Goth.Server.

This would lead to an ** (ArgumentError) unknown registry: Goth.Registry error (when the app is still calling Goth.Server.fetch before completely shutdown, correct?)

then this ArgumentError would gets rescued: https://github.com/peburrows/goth/blob/711adee6976096830bd56b862468cba4683ddfb9/lib/goth/server.ex#L28-L30 but an {:error, RuntimeError.exception("no token")} would be returned: https://github.com/peburrows/goth/blob/711adee6976096830bd56b862468cba4683ddfb9/lib/goth/server.ex#L37-L38

This is fine most of the time, but I find it in our app's error logs (since we are pattern matching {:ok, token} = Goth.fetch(...)) Then it took me a while to figure it out, so I hope we can fix this so others won't run into the same confusion as I did

Thanks @wojtekmach again for this great redesign!

Proposal

First I think @pacoguzman already considered this issue in https://github.com/peburrows/goth/pull/103#issue-674286053

I'd propose that:

  1. we start the Goth.Registry before Goth.Server in Goth.child_spec, (currently it's started in Goth.Application)
  2. update error message no token to something like Goth.Registry is down (or some other better ways to handle it?)
tidusIO commented 2 years ago

Any ideas, when 1.3 will be fully released? :)

wojtekmach commented 2 years ago

https://github.com/peburrows/goth/issues/113 is a little bit worrying so after that one is fixed we'll be good to go. Any help diagnosing and/or fixing the issue would be very appreciated.

wojtekmach commented 2 years ago

The last planned RC, v1.3.0-rc.4, is out! See:

Unless anything comes up, I'm planning to release 1.3.0 final on Tuesday, June 1st.

Thank you everyone for testing this out and reporting bugs and improvements.

lkananowicz commented 2 years ago

@wojtekmach What is the planned release date of version 1.3.0 final?

wojtekmach commented 2 years ago

Unless anything else comes up I plan to cut the release after the one outstanding PR lands.

wojtekmach commented 2 years ago

Unfortunately the release is postponed indefinitely given we've accidentally introduced a breaking change: #136. Not yet sure if there's a way to fix it in a backwards compatible way or we'd need to ship v2.0.