Add optional database caching

[adampal]

No description provided.

[adampal]

@dansingerman @andrewculver would love any feedback from either of you on this change.
In my local testing, I was saving up to 100ms per request so there's definite performance improvements to be had here.
My main concern is making sure the cache gets expired in all the right places. It feels like we could introduce some nasty cache/permission bugs if we're not careful.
Also, what do you think about making it opt-in with the new cache option for the permit method? I think maybe we should just turn it on by default for everyone if we're confident it won't cause issues.

[dansingerman]

I think this is a good change. I like having the cache option, as I can imagine there may be cases where it's harder to know when to invalidate the ability cache, so we should leave that as a decision for the developer.

[adampal]

I think this is a good change. I like having the cache option, as I can imagine there may be cases where it's harder to know when to invalidate the ability cache, so we should leave that as a decision for the developer.

Good point @dansingerman. I was actually thinking maybe we should just always enable it.
Given that in most cases it should be on and just work for people, what do you think about enabling it by default but having the option to opt out for when you are in a situation where you don't want it?

[dansingerman]

I think if it's on by default, then it's not obvious there's caching going on. From a developer experience point of view I'd favour having it off by default, but have the reference implementation include the cache key set to true. i.e. the reference implementation (and everything created by super scaffolding if that were to happen) should be

permit user, through: :memberships, parent: :team, cache:true 

Then there is a breadcrumb trail for a developer to follow if they start getting weird permissions issues.

[adampal]

Makes sense. Unless @andrewculver has any other thoughts at this stage, I'll keep this PR moving forward.

[dansingerman]

Adding caching to the parent_ids_for method means when the ability_cache needs to be created it's going to do DB update for each unique call to parent_ids_for.

Instead, how about adding something like this to the end of Roles::Permit#permit:

user.flush_parent_ids_for_cache_to_db if cache

and then in Roles::User

def parent_ids_for_cache
  @_parent_ids_for_cache || {}
end

def flush_parent_ids_for_cache_to_db
  return unless respond_to?(:ability_cache)
  current_ability_cache = ability_cache || {}
  return if (parent_ids_for_cache.keys - current_ability_cache.keys).none?
  update_column :ability_cache, current_ability_cache.merge(parent_ids_for_cache)
end

Then you only need to do one update per permit statement, and the code can be a little cleaner because you don't have to pass the cache argument all the way through.

[adampal]

I like that.
However, won't we still need to pass the cache key all the way through so the parent_ids_for method knows if it should check the cache?

[dansingerman]

Yes I think you're right. We'd need that still especially in the case you had a permit statement with cache: false followed by a permit statement with cache: true.

In that case the cache: false queries would still get cached, but then ignored in the parent_ids_for method, which I think is ok, if possibly a little surprising to a developer looking at it.

[adampal]

Yeah I think given that we're making the cache happen "under the hood" as long as when they set cache: false the result is calculated live each time it's ok if the value exists unused in the cache. We should only be setting the cache when they have cache: true though so it will only happen in the scenario you described where someone wants cache: true on one permit call then cache: false on another.
I can't think of many reasons why you'd want or need to do that but I like the fact that we're giving the developers the choice and making the cache discoverable through the key. Cache bugs are tricky and I think we want to give people the flexibility to turn it off if they're trying to debug something.

[andrewculver]

@adampal Any chance we can resolve the failing tests?

[adampal]

@dansingerman after merging #21, using cache: true as the key here feels confusing.

Now that we've introduced caching at two different levels, I think we need to be explicit about each one to avoid confusion.
#21 adds caching at the Rails level
#20 adds caching at the database level

What do you think about changing the method param you introduced in #21 to be rails_cache_key: nil
We could make the param that this PR adds enable_db_cache: false

So a full permit method call with both levels of caching enabled would look like:

permit(user, through: :memberships, parent: :team, intermediary: :workspace, rails_cache_key: :a_unique_key, enable_db_cache: true)

It's a bit long but at least it's explicit and leaves the breadcrumbs we were talking about for developers to discover the caching options.

[dansingerman]

@dansingerman after merging #21, using cache: true as the key here feels confusing.

Now that we've introduced caching at two different levels, I think we need to be explicit about each one to avoid confusion. #21 adds caching at the Rails level #20 adds caching at the database level

What do you think about changing the method param you introduced in #21 to be rails_cache_key: nil We could make the param that this PR adds enable_db_cache: false

So a full permit method call with both levels of caching enabled would look like:

permit(user, through: :memberships, parent: :team, intermediary: :workspace, rails_cache_key: :a_unique_key, enable_db_cache: true)

It's a bit long but at least it's explicit and leaves the breadcrumbs we were talking about for developers to discover the caching options.

Makes sense to me 👍

[adampal]

@andrewculver there's some upstream changes we need to make to the bullet_train-base gem and the starter repo to get the tests here passing.
It's actually to correct a bug we didn't notice where the methods that this gem is supposed to add to the User class were already being added by the bullet_train-base gem. It wasn't a problem before because the methods were copied from the same source and were identical. We're now changing the methods with this PR so we need to fix it to get this passing.
The other associated PRs are:

• Remove methods that are now provided by the roles gem bullet_train-base#121
• Add the Roles::User module to User bullet_train#441

[andrewculver]

Merged bullet-train-co/bullet_train-base#121 .