search

ueberauth / guardian_db

Guardian DB integration for tracking tokens and ensuring logout cannot be replayed.
MIT License
367 stars 87 forks source link

Refactor Guardian.DB.Sweeper #130

Closed doomspork closed 3 years ago

doomspork commented 3 years ago

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:

  1. 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.
  2. Updated the tests to use the GenServer directly and test the 2 major functions it provides: purge/0 and reset_timer/0.
  3. 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.
  4. 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. :tada:

yordis commented 3 years ago

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.

yordis commented 3 years ago

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

doomspork commented 3 years ago

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 commented 3 years ago

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

doomspork commented 3 years ago

Once #129 is merged I will update this further :+1:

doomspork commented 3 years ago

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

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