Merge pull request #6 from factorialco/fix-recursive-loop

Fix infinite loop when a method calls super

lib/captain_hook.rb

@@ -25,33 +25,20 @@ def self.inherited(subclass)
   # need to be executed in a stack-like way.
   def run_around_hooks(method, *args, **kwargs, &block)
     self.class.get_hooks(:around).to_a.reverse.inject(block) do |chain, hook_configuration|
-      next chain if hook_configuration.skip_when&.call(args, kwargs)
-      next chain if hook_configuration.exclude.include?(method)
+      next chain if hook_configuration.skip?(method, args, kwargs)
 
       instance = self
 
-      hook_proc = proc {
+      proc {
         run_hook(method, hook_configuration, chain, instance, *args, **kwargs)
       }
-
-      next hook_proc if hook_configuration.methods.empty?
-
-      next chain unless hook_configuration.methods.include?(method)
-
-      hook_proc
     end.call
   end
 
   # Runs non-around hooks for a given method.
   def run_hooks(method, hooks, *args, **kwargs)
     hooks.each do |hook_configuration|
-      next if hook_configuration.exclude.include?(method)
-      next if hook_configuration.skip_when&.call(args, kwargs)
-
-      body = run_hook(method, hook_configuration, -> {}, self, *args, **kwargs) if hook_configuration.methods.empty?
-
-      return body if hook_error?(body)
-      next unless hook_configuration.methods.include?(method)
+      next if hook_configuration.skip?(method, *args, **kwargs)
 
       body = run_hook(method, hook_configuration, -> {}, self, *args, **kwargs)
 
@@ -176,6 +163,8 @@ def decorate_method!(method_name)
 
       original_method_name = :"#{method_name}__without_hooks"
 
+      return if method_defined?(original_method_name)
+
       alias_method original_method_name, method_name
 
       # We decorate the method with the before, after and around hooks

lib/hook_configuration.rb

@@ -19,4 +19,17 @@ def initialize(
   end
 
   attr_reader :hook, :methods, :inject, :exclude, :skip_when, :param_builder
+
+  # This determines if this specific hook should be skipped
+  # depending on the method or arguments.
+  def skip?(method, *args, **kwargs)
+    # binding.pry if hook.class.name == "PrepareHook"
+
+    return true if skip_when&.call(args, kwargs)
+    return true if exclude.include?(method)
+
+    return false if methods.empty?
+
+    !methods.include?(method)
+  end
 end

spec/captain_hook_spec.rb

@@ -14,7 +14,7 @@ def call(_klass, _method, _one, _two, _three, &_block)
 end
 
 class PrepareHook
-  def call(klass, method); end
+  def call(klass, method, dto:); end
 end
 
 class ServeHook
@@ -92,6 +92,11 @@ def policy_context
 
 class ResourceChildWithHooks < ResourceWithHooks
   def foo(dto:); end
+
+  def prepare(dto:)
+    super(dto: dto)
+    "child #{dto}"
+  end
 end
 
 describe CaptainHook do
@@ -107,9 +112,9 @@ def foo(dto:); end
   end
 
   context "when the method is not defined in the hook" do
-    it do
+    it "calls all hooks with not methods defined" do
       expect_any_instance_of(CookHook).not_to receive(:call).once.and_call_original
-      expect_any_instance_of(PrepareHook).to receive(:call).once
+      expect_any_instance_of(PrepareHook).to receive(:call).once.and_call_original
       expect_any_instance_of(BeforeAllHook).to receive(:call).once.and_call_original
       expect_any_instance_of(ManyParametersHook).to receive(:call).once.and_call_original
       expect_any_instance_of(ServeHook).to receive(:call).once.and_call_original
@@ -144,9 +149,11 @@ def foo(dto:); end
     subject { ResourceChildWithHooks.new }
 
     it do
-      expect_any_instance_of(BeforeAllHook).to receive(:call).once
-
+      expect_any_instance_of(BeforeAllHook).to receive(:call).twice
       subject.foo(dto: "fooing")
+
+      expect_any_instance_of(BeforeAllHook).to receive(:call).once
+      expect(subject.prepare(dto: "foo")).to eq("child foo")
     end
   end
 end

spec/hook_configuration_spec.rb

@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+require "pry"
+require "pry-byebug"
+
+class DummyHook
+  def call(klass, method, dto:); end
+end
+
+describe HookConfiguration do
+  let(:hook) { DummyHook.new }
+  let(:args) { {} }
+  let(:kwargs) { {} }
+  let(:method) { :foo }
+  let(:excluded_method) { :excluded_foo }
+  let(:methods) { [method] }
+  let(:skip_when) { nil }
+
+  subject do
+    HookConfiguration.new(
+      hook: hook,
+      methods: methods,
+      exclude: [excluded_method],
+      skip_when: skip_when
+    )
+  end
+
+  describe "#skip?" do
+    context "when the method is included" do
+      it do
+        expect(subject.skip?(method, args, kwargs)).to be_falsey
+      end
+    end
+
+    context "when methods is empty should always run" do
+      let(:methods) { [] }
+
+      it do
+        expect(subject.skip?(method, args, kwargs)).to be_falsey
+      end
+    end
+
+    context "when the method is excluded" do
+      it do
+        expect(subject.skip?(excluded_method, args, kwargs)).to be_truthy
+      end
+    end
+
+    context "when skip_when block is provided" do
+      let(:skip_when) { ->(_args, kwargs) { kwargs[:dto].nil? } }
+
+      it "it evaluates the skip_when to decide if its shown" do
+        expect(subject.skip?(method, args, kwargs)).to be_truthy
+      end
+    end
+  end
+end