Load the user from the database each time it's fetched from the cache by default

[danschultzer]

As described in the docs the fetched user object assigned as current_user may be stale. This is more performant as a db lookup isn't necessary for each load. However, I think for the majority of use cases the user is expected to have been fetched from the database. This default is probably confusing for many. So I think it's better to load the current_user from the database by default, and let it be easy for devs to optimize later on.

[Kurisu3]

Hi @danschultzer ^^
I would suggest to make it configurable. For example we can add a key to pow config like :force_sync that will default to true so that we have by default the current_user fetched from the database on every request. This way people who already rely on the current behaviour will just set this key to false when upgrading their projects dependies. For example in a project I have in production I reload the current_user on specific pages. If Pow defaults change the current_user will be loaded twice on that pages so I can just set the new config to false to keep my project working as before.

[danschultzer]

Yup, it will be configurable. I just have a strong distaste for configs, I prefer explicit logic where-ever possible so have to think about what can be a good approach here 😄 I might also push this to 1.1.0 release which will have all breaking changes.

For example in a project I have in production I reload the current_user on specific pages. If Pow defaults change the current_user will be loaded twice on that pages so I can just set the new config to false to keep my project working as before.

The reload logic could also be removed?

[Kurisu3]

The reload logic could also be removed?

Yep of course it can be. I was just a bit lazzy about reviewing it. ^^

With less configs, we have a cleaner code for sure.
I'm kind excited to see thoses breaking changes in a sens... This means also we will have a more robust library.

[jesselathamdev]

Hey @danschultzer, had a two questions/observations here with this change that maybe you could provide some insight into both which relate to changes currently on master...

Possible to delete the session from within a custom get_by/1 if no record is found

I had previously left you a question Help in understanding custom contexts/purpose of get_by/1 where you had recently introduced a fix (works great by the way).

With the changes here with (which are great and removes the need to have a separate plug for reloading the user) I'm wondering if you have a way such that if the custom get_by/1 doesn't return a record we can somehow delete the session.

The reason being that I want to be able to better support my app by modifying active and deleted_at records at the db level and as I have those as where statements in the get_by/1 itself. So while I can make current_user not find a record by making the two values falsey and I can get redirected to the sign in page, the session is not actually deleted.

Should the custom get_by/1 and built in get_by/2 be followed with a check within Pow itself to delete the session if they resolve to no record found?

get_by/1 clauses vary between :email and :id on sign in and subsequent requests

I've noticed with the newest changes that when I sign in the single keyword within clauses for the custom get_by/1 is :email, but for each subsequent request, the value of clauses contains only :id. Was there something I'm missing here as with the custom get_by/1 I have to check to see which keyword is included (:email or :id) and adjust my where clause accordingly.

Thoughts on both/more information needed?

Thanks again.

[danschultzer]

I'm wondering if you have a way such that if the custom get_by/1 doesn't return a record we can somehow delete the session.
Should the custom get_by/1 and built in get_by/2 be followed with a check within Pow itself to delete the session if they resolve to no record found?

Great point! I've opened PR #573 to solve this. At first I was a bit unsure if it could introduce race condition, but that shouldn't be an issue here at all. Also, #567 kinda touches this, where all sessions for a user that's being deleted should be expired.

Was there something I'm missing here as with the custom get_by/1 I have to check to see which keyword is included (:email or :id) and adjust my where clause accordingly.

The get_by/1 is generic, so Pow just sends in various where clauses (e.g. for PowEmailConformation it's [email_confirmation_token: token]). It would be best if you just feed the clauses into the query rather than checking what the clauses are.

It could look like:

MyApp.Users.User
|> Repo.get_by(clauses)
|> case do
  %{deleted_at: nil} = user -> user
  _any -> nil
end

Or maybe it's possible with a where clause (not sure if the macro accepts this though):

MyApp.Users.User
|> where([u], ^clauses)
|> where([u], is_nil(u.deleted_at))
|> Repo.one()