Skip to content

Commit

Permalink
Apply the cache strategy for all policy lookups
Browse files Browse the repository at this point in the history
This should be backwards-compatible because:

* `Pundit.policy` and `Pundit.policy!` are unaffected by this, since they use a non-cache cache.
* `Pundit::Context` is new, so nobody is relying on these methods yet.
* `Pundit::Authorization#policy` already cached the policy anyway.
* `policies` is explicitly discouraged, but even if it is relied upon it works as it always has, by default.

This is an improvement because we can change the cache strategy to introduce new features without breaking backwards-compatibility.
  • Loading branch information
Burgestrand committed Mar 26, 2024
1 parent 43b1d87 commit d7e97c3
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 13 deletions.
4 changes: 2 additions & 2 deletions lib/pundit/authorization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,9 @@ def policy_scope(scope, policy_scope_class: nil)
#
# @see https://github.com/varvet/pundit#policies
# @param record [Object] the object we're retrieving the policy for
# @return [Object, nil] instance of policy class with query methods
# @return [Object] instance of policy class with query methods
def policy(record)
policies[record] ||= pundit.policy!(record)
pundit.policy!(record)
end

# Retrieves a set of permitted attributes from the policy by instantiating
Expand Down
27 changes: 16 additions & 11 deletions lib/pundit/context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@ def authorize(possibly_namespaced_record, query:, policy_class:)
policy = if policy_class
policy_class.new(user, record)
else
policy_cache.fetch(possibly_namespaced_record) do
policy!(possibly_namespaced_record)
end
policy!(possibly_namespaced_record)
end

raise NotAuthorizedError, query: query, record: record, policy: policy unless policy.public_send(query)
Expand Down Expand Up @@ -86,10 +84,7 @@ def policy_scope!(scope)
# @raise [InvalidConstructorError] if the policy constructor called incorrectly
# @return [Object, nil] instance of policy class with query methods
def policy(record)
policy = policy_finder(record).policy
policy&.new(user, pundit_model(record))
rescue ArgumentError
raise InvalidConstructorError, "Invalid #<#{policy}> constructor is called"
cached_policy(record, &:policy)
end

# Retrieves the policy for the given record. Raises if not found.
Expand All @@ -101,14 +96,24 @@ def policy(record)
# @raise [InvalidConstructorError] if the policy constructor called incorrectly
# @return [Object] instance of policy class with query methods
def policy!(record)
policy = policy_finder(record).policy!
policy.new(user, pundit_model(record))
rescue ArgumentError
raise InvalidConstructorError, "Invalid #<#{policy}> constructor is called"
cached_policy(record, &:policy!)
end

private

def cached_policy(record, &)
policy_cache.fetch(record) do
policy = yield policy_finder(record)
next unless policy

begin
policy.new(user, pundit_model(record))
rescue ArgumentError
raise InvalidConstructorError, "Invalid #<#{policy}> constructor is called"
end
end
end

def policy_finder(record)
PolicyFinder.new(record)
end
Expand Down

0 comments on commit d7e97c3

Please sign in to comment.