Refactor Guardian.DB.Sweeper #130

doomspork · 2021-05-16T16:10:14Z

I'd like to slowly start improving the code quality and test coverage across the Ueberauth org projects. Since we've been focusing on Guardian.DB recently I thought I'd start by proposing some changes here. For this PR I looked to tackle the Sweeper functionality first:

We currently have Sweeper and SweeperServer but their roles are not clearly separated (Sweeper manages the SweeperServer's state for instance). I combined these together as there's no real benefit to keeping them separate and this cleans up some of their code.
Updated the tests to use the GenServer directly and test the 2 major functions it provides: purge/0 and reset_timer/0.
Moved the Sweeper from Guardian.DB.Token.SweeperServer to Guardian.DB.Sweeper, I'm not entirely sure why this was ever nested under Token but it seems rather unnecessary. This is a breaking change.
Stop accessing configuration directly from the library code. It's generally a better practice for the application to have its own configuration which it passes in rather than libraries doing that themselves.

I may look at adding some addition test coverage for each of the GenServer callbacks just to cover our bases.

This PR is a place to start this conversation, hence the draft. 🎉

test/guardian/sweeper_test.exs

alexfilatov · 2021-05-16T18:17:36Z

lib/guardian/db/sweeper.ex

+  @sixty_minutes 60 * 60 * 1000
+
+  def start_link(opts) do
+    interval = Keyword.get(opts, :interval, @sixty_minutes)


looks like it's : sweep_interval in the config, @doomspork

config :guardian, Guardian.DB, repo: MyApp.Repo, # Add your repository module schema_name: "guardian_tokens", # default token_types: ["refresh_token"], # store all token types if not set sweep_interval: 60 # default: 60 minutes

oh I got it now, so it's not configurable anymore?

It is configurable @alexfilatov but as I mentioned in this point:

Stop accessing configuration directly from the library code. It's generally a better practice for the application to have its own configuration which it passes in rather than libraries doing that themselves.

I think we should be moving away from our libraries accessing application configuration themselves directly. Applications should be able to pass in their configurations.

I'm open to not doing this, I think it's just a better design practice.

children = [ ... {Guardian.DB.Sweeper, interval: configured_sweep_interval()} ... ]

I love this part for sure

yordis · 2021-05-16T22:39:10Z

We currently have Sweeper and SweeperServer but their roles are not clearly separated (Sweeper manages the SweeperServer's state for instance). I combined these together as there's no real benefit to keeping them separate and this cleans up some of their code.

I disagree with the promise of no real benefits.

SweeperServer is impure, it deals with Erlang message passing and gen-server stuff. While Sweeper is pure since it is just the implementation of the Sweeper. Functional Core with Imperative Interface.

That being said, you are the boss.

I just would try to be clear on what is wrong as of today based on the objective issues since it wasn't been a problem so far before we start removing things just because it is deemed unimportant based on your skills set or your perspective, which it isn't hard to understand where you are coming from for sure. I totally understand your reasoning.

test/guardian/sweeper_test.exs

yordis · 2021-05-16T22:43:42Z

Overall, don't mind the changes, I can maintaining them either way, if you feel strong about it, you have my 1+

doomspork · 2021-05-17T00:02:36Z

That being said, you are the boss.

It's not that at all. I'm open to ideas and suggestions.

just would try to be clear on what is wrong as of today based on the objective issues since it wasn't been a problem so far before we start removing things just because it is deemed unimportant based on your skills set or your perspective, which it isn't hard to understand where you are coming from for sure. I totally understand your reasoning.

I'm not sure "wasn't been a problem so far" is ever good reason to avoid changes, would we ever improve code with that mentality? If your concern is having to test the GenServers I can update the test to use the GenServer callbacks directly and avoid that. I actually want to test that this stuff works accordingly so these tests are intentional but I'm happy to remove them.

Simplifying the code to be easier to understand makes things better for everyone, especially us maintainers. A code base that's difficult to grok keeps contributors away.

I'm okay to not incorporating this change but I think it's misguided to assume that just because we can have "pure" functions to test that there isn't a better solution. As-is responsibility is spread throughout modules making understanding the code base far more complicated/

yordis · 2021-05-17T00:24:05Z

Works for me either way to me, I leave this one to your judgment, I trust your judgments.

doomspork · 2021-05-17T00:43:12Z

Once #129 is merged I will update this further 👍

doomspork · 2021-05-17T01:12:33Z

@yordis I updated the tests and made use of Mox for 2 reasons:

Updated the tests to use the GenServer callbacks directly. This should address you concern with impure functions.
Using Mox we can test the functionality works without having to actually interact with the database.

doomspork · 2021-05-17T01:13:19Z

lib/guardian/db/sweeper.ex

+  scheduled work.
+  """
+  def purge do
+    GenServer.cast(__MODULE__, :sweep)


These were also changed to cast from call since we don't need or care about a reply.

doomspork · 2021-05-17T01:14:54Z

lib/guardian/db/sweeper.ex

@@ -0,0 +1,70 @@
+defmodule Guardian.DB.Sweeper do


For backwards compatibility we could add a Guardian.DB.Token.SweeperServer alias but I'd like to encourage folks to move away from that module.

@doomspork wouldn't be better to use SweeperServer following some GenServer lingos?

It is a stateful thing (hint the server part of it) that you send messages to it 🤔 💭

I don't often seen things called "Server" in Elixir/Erlang space, do you? Looking at a few of my applications there's nothing in the application tree with that suffix 🤔

It depends if you have the Server being the message passing, while the thing without Server (as it was before), was the implementation of the thing.

Dave has an amazing course about it, https://codestool.coding-gnome.com/courses/elixir-for-programmers

I appreciate Dave's pattern but as much as he would like to see this catch on I can't say I see this having gained any adoption. I prefer to keep code simple and easy to grok for the human reader over trying to be "academically right".

I'd like to leave this.

Moved the Sweeper functionality out from inside the Token namespace/folder. Also simplified the code by combining the two separate but unnecessary modules. Updated and added test coverage.

doomspork requested a review from a team May 16, 2021 16:10

doomspork added Kind:Enhancement Note:Discussion labels May 16, 2021

doomspork commented May 16, 2021

View reviewed changes

test/guardian/sweeper_test.exs Show resolved Hide resolved

alexfilatov reviewed May 16, 2021

View reviewed changes

yordis reviewed May 16, 2021

View reviewed changes

test/guardian/sweeper_test.exs Outdated Show resolved Hide resolved

doomspork force-pushed the refactor-sweeper branch from bff7f9d to a0bad53 Compare May 17, 2021 01:10

doomspork marked this pull request as ready for review May 17, 2021 01:11

doomspork commented May 17, 2021

View reviewed changes

doomspork requested a review from yordis May 17, 2021 01:13

doomspork commented May 17, 2021

View reviewed changes

doomspork added 2 commits May 16, 2021 19:16

Refactor Guardian.DB.Sweeper

cb7972b

Moved the Sweeper functionality out from inside the Token namespace/folder. Also simplified the code by combining the two separate but unnecessary modules. Updated and added test coverage.

Expand on test coverage

fd5fce5

doomspork force-pushed the refactor-sweeper branch from a0bad53 to 5fb60f2 Compare May 17, 2021 01:16

Call GenServer callbacks directly

cdebe9a

doomspork force-pushed the refactor-sweeper branch from 5fb60f2 to cdebe9a Compare May 17, 2021 01:17

yordis approved these changes May 17, 2021

View reviewed changes

alexfilatov approved these changes May 17, 2021

View reviewed changes

doomspork merged commit e3a4c6b into master May 17, 2021

doomspork deleted the refactor-sweeper branch May 17, 2021 13:30

RichDom2185 mentioned this pull request Sep 17, 2023

Fix: documentation #143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Guardian.DB.Sweeper #130

Refactor Guardian.DB.Sweeper #130

doomspork commented May 16, 2021 •

edited

Loading

alexfilatov May 16, 2021

alexfilatov May 16, 2021

doomspork May 16, 2021 •

edited

Loading

yordis May 16, 2021

yordis commented May 16, 2021

yordis commented May 16, 2021

doomspork commented May 17, 2021

yordis commented May 17, 2021 •

edited

Loading

doomspork commented May 17, 2021

doomspork commented May 17, 2021

doomspork May 17, 2021

doomspork May 17, 2021

yordis May 17, 2021

doomspork May 17, 2021

yordis May 17, 2021

doomspork May 17, 2021

Refactor Guardian.DB.Sweeper #130

Refactor Guardian.DB.Sweeper #130

Conversation

doomspork commented May 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doomspork May 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yordis commented May 16, 2021

yordis commented May 16, 2021

doomspork commented May 17, 2021

yordis commented May 17, 2021 • edited Loading

doomspork commented May 17, 2021

doomspork commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doomspork commented May 16, 2021 •

edited

Loading

doomspork May 16, 2021 •

edited

Loading

yordis commented May 17, 2021 •

edited

Loading