chore: refactor result keeping in policies

Store the result object in the execution context instead of the ivar to avoid problems in fiber-concurrent execution environments

This is a temporary workaroudn with some preparation for 1.0; ideally, we should get rid of the local-global state completely

Ref palkan/action_policy-graphql#47

benchmarks/catch_throw.rb

@@ -17,8 +17,8 @@ class APolicy < ActionPolicy::Base
   authorize :user, optional: true
 
   def apply(rule)
-    @result = self.class.result_class.new(self.class, rule)
-    @result.load __apply__(rule)
+    self.result = self.class.result_class.new(self.class, rule)
+    result.load __apply__(rule)
   end
 
   def show?
@@ -30,17 +30,17 @@ class BPolicy < ActionPolicy::Base
   authorize :user, optional: true
 
   def apply(rule)
-    @result = self.class.result_class.new(self.class, rule)
+    self.result = self.class.result_class.new(self.class, rule)
 
     catch :throw_me_away do
-      @result.load __apply__(rule)
+      result.load __apply__(rule)
     end
 
-    @result.value
+    result.value
   end
 
   def show?
-    @result.load true
+    result.load true
 
     throw :throw_me_away
   end

lib/action_policy/authorizer.rb

@@ -5,10 +5,11 @@ module ActionPolicy
   class Unauthorized < Error
     attr_reader :policy, :rule, :result
 
-    def initialize(policy, rule)
+    # NEXT_RELEASE: remove result fallback
+    def initialize(policy, rule, result = policy.result)
       @policy = policy.class
       @rule = rule
-      @result = policy.result
+      @result = result
 
       super("Not authorized: #{@policy}##{@rule} returns false")
     end
@@ -20,12 +21,14 @@ module Authorizer
     class << self
       # Performs authorization, raises an exception when check failed.
       def call(policy, rule)
-        authorize(policy, rule) ||
-          raise(::ActionPolicy::Unauthorized.new(policy, rule))
+        res = authorize(policy, rule)
+        return if res.success?
+
+        raise(::ActionPolicy::Unauthorized.new(policy, rule, res))
       end
 
       def authorize(policy, rule)
-        policy.apply(rule)
+        policy.apply_r(rule)
       end
 
       # Applies scope to the target

lib/action_policy/behaviour.rb

@@ -53,8 +53,7 @@ def allowed_to?(rule, record = :__undef__, **options)
     def allowance_to(rule, record = :__undef__, **options)
       policy = lookup_authorization_policy(record, **options)
 
-      policy.apply(authorization_rule_for(policy, rule))
-      policy.result
+      policy.apply_r(authorization_rule_for(policy, rule))
     end
 
     def authorization_context

lib/action_policy/policy/cache.rb

@@ -50,18 +50,18 @@ def apply_with_cache(rule)
         key = rule_cache_key(rule)
 
         ActionPolicy.cache_store.then do |store|
-          @result = store.read(key)
+          result = store.read(key)
           unless result.nil?
             result.cached!
-            next result.value
+            next result
+          end
+          yield.tap do |result|
+            store.write(key, result, options)
           end
-          yield
-          store.write(key, result, options)
-          result.value
         end
       end
 
-      def apply(rule)
+      def apply_r(rule)
         return super if ActionPolicy.cache_store.nil? ||
           !self.class.cached_rules.key?(rule)
 

lib/action_policy/policy/cached_apply.rb

@@ -7,19 +7,16 @@ module Policy
     # When you call `apply` twice on the same policy and for the same rule,
     # the check (and pre-checks) is only called once.
     module CachedApply
-      def apply(rule)
+      def apply_r(rule)
         @__rules_cache__ ||= {}
 
         if @__rules_cache__.key?(rule)
-          @result = @__rules_cache__[rule]
-          return result.value
+          return @__rules_cache__[rule]
         end
 
-        super
-
-        @__rules_cache__[rule] = result
-
-        result.value
+        super.tap do |result|
+          @__rules_cache__[rule] = result
+        end
       end
     end
   end

lib/action_policy/policy/core.rb

@@ -72,7 +72,7 @@ def identifier
 
       include ActionPolicy::Behaviours::PolicyFor
 
-      attr_reader :record, :result
+      attr_reader :record
 
       # NEXT_RELEASE: deprecate `record` arg, migrate to `record: nil`
       def initialize(record = nil, *)
@@ -83,13 +83,23 @@ def initialize(record = nil, *)
       # Unlike simply calling a predicate rule (`policy.manage?`),
       # `apply` also calls pre-checks.
       def apply(rule)
-        @result = self.class.result_class.new(self.class, rule)
+        res = apply_r(rule)
 
-        catch :policy_fulfilled do
-          result.load __apply__(resolve_rule(rule))
-        end
+        # DEPRECATED (we still rely on it in tests)
+        @result = res
+
+        res.value
+      end
+
+      # NEXT_RELEASE: This is gonna be #apply in 1.0
+      def apply_r(rule) # :nodoc:
+        with_result(rule) do |result|
+          catch :policy_fulfilled do
+            result.load __apply__(resolve_rule(rule))
+          end
 
-        result.value
+          result
+        end
       end
 
       def deny!
@@ -107,14 +117,17 @@ def allow!
       # (such as caching, pre checks, etc.)
       def __apply__(rule) = public_send(rule)
 
-      # Wrap code that could modify result
-      # to prevent the current result modification
-      def with_clean_result # :nodoc:
-        was_result = @result
-        yield
-        @result
+      # Prepare a new result object for the next rule application.
+      # It's stored in the thread-local storage to be accessible from within the policy.
+      def with_result(rule) # :nodoc:
+        result = self.class.result_class.new(self.class, rule)
+
+        Thread.current[:__action_policy_result__] ||= []
+        Thread.current[:__action_policy_result__] << result
+
+        yield result
       ensure
-        @result = was_result
+        Thread.current[:__action_policy_result__]&.pop
       end
 
       # Returns a result of applying the specified rule to the specified record.
@@ -146,6 +159,13 @@ def resolve_rule(activity)
         activity
       end
 
+      # Returns the result object for the last rule application within the given
+      # execution context (Thread or Fiber)
+      def result
+        # FIXME: Remove ivar fallback after 1.0
+        Thread.current[:__action_policy_result__]&.last || @result
+      end
+
       # Return annotated source code for the rule
       # NOTE: require "method_source" and "prism" gems to be installed.
       # Otherwise returns empty string.
@@ -155,9 +175,7 @@ def inspect_rule(rule) = PrettyPrint.print_method(self, rule)
       # Useful for debugging: type `pp :show?` within the context of the policy
       # to preview the rule.
       def pp(rule)
-        with_clean_result do
-          # We need result to exist for `allowed_to?` to work correctly
-          @result = self.class.result_class.new(self.class, rule)
+        with_result(rule) do
           header = "#{self.class.name}##{rule}"
           source = inspect_rule(rule)
           $stdout.puts "#{header}\n#{source}"

lib/action_policy/policy/reasons.rb

@@ -201,13 +201,12 @@ def allowed_to?(rule, record = :__undef__, inline_reasons: false, **options)
           if (record == :__undef__ || record == self.record) && options.empty?
             rule = resolve_rule(rule)
             policy = self
-            with_clean_result { apply(rule) }
+            apply_r(rule)
           else
             policy = policy_for(record: record, **options)
             rule = policy.resolve_rule(rule)
 
-            policy.apply(rule)
-            policy.result
+            policy.apply_r(rule)
           end
 
         if res.fail? && result&.reasons

lib/action_policy/rails/authorizer.rb

@@ -9,10 +9,10 @@ module Authorizer
       def authorize(policy, rule)
         event = {policy: policy.class.name, rule: rule.to_s}
         ActiveSupport::Notifications.instrument(EVENT_NAME, event) do
-          res = super
-          event[:cached] = policy.result.cached?
-          event[:value] = policy.result.value
-          res
+          result = super
+          event[:cached] = result.cached?
+          event[:value] = result.value
+          result
         end
       end
     end

lib/action_policy/rails/policy/instrumentation.rb

@@ -16,13 +16,13 @@ def initialize(record = nil, **params)
           ActiveSupport::Notifications.instrument(INIT_EVENT_NAME, event) { super }
         end
 
-        def apply(rule)
+        def apply_r(rule)
           event = {policy: self.class.name, rule: rule.to_s}
           ActiveSupport::Notifications.instrument(APPLY_EVENT_NAME, event) do
-            res = super
+            result = super
             event[:cached] = result.cached?
             event[:value] = result.value
-            res
+            result
           end
         end
       end

lib/action_policy/rspec/dsl.rb

@@ -39,6 +39,7 @@ def self.included(base)
         super
       end
 
+      # FIXME(1.0): Update to use result object directly
       def formatted_policy(policy)
         "#{policy.result.inspect}\n#{policy.inspect_rule(policy.result.rule)}"
       end

test/action_policy/policy/base_test.rb

@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+class TestBasePolicy < Minitest::Test
+  class User < Struct.new(:kind)
+  end
+
+  class FiberUser < User
+    def kind
+      Fiber.yield
+      super
+    end
+  end
+
+  class TestPolicy < ActionPolicy::Base
+    self.identifier = :test
+
+    authorize :user
+
+    def milk?
+      check?(:cat?)
+    end
+
+    def honey?
+      check?(:bee?)
+    end
+
+    def cat? = user.kind == "cat"
+
+    def bee? = user.kind == "bee"
+  end
+
+  def test_baseline
+    user = User.new("cat")
+    policy = TestPolicy.new(user:)
+
+    assert policy.apply(:milk?)
+    refute policy.apply(:honey?)
+    assert_equal({test: [:bee?]}, policy.result.reasons.details)
+
+    # it's cached
+    user.kind = "bee"
+
+    assert policy.apply(:milk?)
+    refute policy.apply(:honey?)
+  end
+
+  def test_fiber_concurrency
+    user = FiberUser.new("cat")
+    policy = TestPolicy.new(user:)
+
+    f1 = Fiber.new do
+      refute policy.apply(:honey?)
+      assert_equal({test: [:bee?]}, policy.result.reasons.details)
+    end
+
+    f2 = Fiber.new do
+      assert policy.apply(:milk?)
+    end
+
+    f1.resume
+    f2.resume
+    f2.resume
+    f1.resume
+  end
+end