Verify origins do not collide with ServiceActor::Result object meth…

…ods (#138)

CHANGELOG.md

@@ -19,6 +19,8 @@ Fixes:
   are subclasses of `Exception` (#132)
 - Skip `default` check for actor outputs (#135)
 - Ensure provided `fail_on` arguments are subclasses of `Exception` (#136)
+- Ensure `input`, `output` and `alias_input` names do not collide with
+  `ServiceActor::Result` instance methods (#138)
 
 ## v3.7.0
 

lib/service_actor/arguments_validator.rb

@@ -3,6 +3,13 @@
 module ServiceActor::ArgumentsValidator
   module_function
 
+  def validate_origin_name(name, origin:)
+    return unless ServiceActor::Result.instance_methods.include?(name.to_sym)
+
+    raise ArgumentError,
+          "#{origin} `#{name}` overrides `ServiceActor::Result` instance method"
+  end
+
   def validate_error_class(value)
     return if value.is_a?(Class) && value <= Exception
 

lib/service_actor/attributable.rb

@@ -20,6 +20,10 @@ def inherited(child)
     end
 
     def input(name, **arguments)
+      ServiceActor::ArgumentsValidator.validate_origin_name(
+        name, origin: :input
+      )
+
       inputs[name] = arguments
 
       define_method(name) do
@@ -37,6 +41,10 @@ def inputs
     end
 
     def output(name, **arguments)
+      ServiceActor::ArgumentsValidator.validate_origin_name(
+        name, origin: :output
+      )
+
       outputs[name] = arguments
 
       define_method(name) do

lib/service_actor/playable.rb

@@ -20,6 +20,12 @@ def play(*actors, **options)
     end
 
     def alias_input(**options)
+      options.each_key do |new|
+        ServiceActor::ArgumentsValidator.validate_origin_name(
+          new, origin: :alias
+        )
+      end
+
       lambda do |actor|
         options.each do |new, original|
           define_alias_input(actor, new, original)
@@ -40,7 +46,7 @@ def inherited(child)
     private
 
     def define_alias_input(actor, new_input, original_input)
-      actor[new_input] = actor.delete(original_input)
+      actor[new_input] = actor.delete!(original_input)
     end
   end
 

lib/service_actor/result.rb

@@ -1,16 +1,19 @@
 # frozen_string_literal: true
 
-require "ostruct"
-
 # Represents the context of an actor, holding the data from both its inputs
 # and outputs.
-class ServiceActor::Result
+
+class ServiceActor::Result < BasicObject
   def self.to_result(data)
     return data if data.is_a?(self)
 
     new(data.to_h)
   end
 
+  %i[class is_a? kind_of? send].each do |method_name|
+    define_method(method_name, ::Kernel.instance_method(method_name))
+  end
+
   def initialize(data = {})
     @data = data.to_h
   end
@@ -22,17 +25,18 @@ def to_h
   def inspect
     "<#{self.class.name} #{to_h}>"
   end
+  alias pretty_print inspect
 
   def fail!(failure_class = nil, result = {})
-    if failure_class.nil? || failure_class.is_a?(Hash)
+    if failure_class.nil? || failure_class.is_a?(::Hash)
       result = failure_class.to_h
-      failure_class = ServiceActor::Failure
+      failure_class = ::ServiceActor::Failure
     end
 
     data.merge!(result)
     data[:failure] = true
 
-    raise failure_class, self
+    ::Kernel.raise failure_class, self
   end
 
   def success?
@@ -61,13 +65,13 @@ def []=(key, value)
     data[key] = value
   end
 
-  def delete(key)
+  def delete!(key)
     data.delete(key)
   end
 
-  # Defined here to override the method on `Object`.
-  def display
-    to_h.fetch(:display)
+  def respond_to?(method_name, include_private = false)
+    self.class.instance_methods.include?(method_name) ||
+      respond_to_missing?(method_name, include_private)
   end
 
   private
@@ -98,7 +102,7 @@ def method_missing(method_name, *args) # rubocop:disable Metrics/AbcSize
   end
 
   def warn_on_undefined_method_invocation(message)
-    Kernel.warn(
+    ::Kernel.warn(
       "DEPRECATED: Invoking undefined methods on `ServiceActor::Result` will " \
       "lead to runtime errors in the next major release of Actor. " \
       "Invoked method: `#{message}`",

spec/actor_spec.rb

@@ -1,6 +1,10 @@
 # frozen_string_literal: true
 
 RSpec.describe Actor do
+  shared_context "with mocked `Kernel.warn` method" do
+    before { allow(Kernel).to receive(:warn).with(kind_of(String)) }
+  end
+
   describe "#call" do
     context "when fail! is not called" do
       let(:actor) { DoNothing.call }
@@ -423,12 +427,6 @@
       end
     end
 
-    context "when using an output called display" do
-      it "returns it" do
-        expect(SetOutputCalledDisplay.call.display).to eq("Foobar")
-      end
-    end
-
     context "when setting an unknown output" do
       it "raises" do
         expect { SetUnknownOutput.call }
@@ -730,6 +728,61 @@
       end
     end
 
+    context "with `input` name that collides with result methods" do
+      include_context "with mocked `Kernel.warn` method"
+
+      let(:actor) do
+        Class.new(Actor) do
+          input :value, type: Integer
+          input :kind_of?, type: String
+        end
+      end
+
+      specify do
+        expect { actor }.to raise_error(
+          ArgumentError,
+          "input `kind_of?` overrides `ServiceActor::Result` instance method",
+        )
+      end
+    end
+
+    context "with `output` name that collides with result methods" do
+      include_context "with mocked `Kernel.warn` method"
+
+      let(:actor) do
+        Class.new(Actor) do
+          input :value, type: Integer
+          output :fail!, type: String
+        end
+      end
+
+      specify do
+        expect { actor }.to raise_error(
+          ArgumentError,
+          "output `fail!` overrides `ServiceActor::Result` instance method",
+        )
+      end
+    end
+
+    context "with `alias_input` that collides with result methods" do
+      include_context "with mocked `Kernel.warn` method"
+
+      let(:actor) do
+        Class.new(Actor) do
+          input :value, type: Integer
+
+          play alias_input(merge!: :value)
+        end
+      end
+
+      specify do
+        expect { actor }.to raise_error(
+          ArgumentError,
+          "alias `merge!` overrides `ServiceActor::Result` instance method",
+        )
+      end
+    end
+
     context "with `failure_class` which is not a class" do
       let(:actor) do
         Class.new(Actor) do
@@ -863,9 +916,9 @@
     end
 
     context "with sending unexpected messages" do
-      let(:actor) { PlayActors.result(value: 42) }
+      include_context "with mocked `Kernel.warn` method"
 
-      before { allow(Kernel).to receive(:warn) }
+      let(:actor) { PlayActors.result(value: 42) }
 
       it { expect(actor).to be_a_success }
       it { expect(actor).to respond_to(:name) }

spec/service_actor/arguments_validator_spec.rb

@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+RSpec.describe ServiceActor::ArgumentsValidator do
+  describe ".validate_origin_name" do
+    it "raises if collision present" do # rubocop:disable RSpec/ExampleLength
+      expect do
+        described_class.validate_origin_name(:fail!, origin: :input)
+      end.to raise_error(
+        ArgumentError,
+        "input `fail!` overrides `ServiceActor::Result` instance method",
+      )
+    end
+
+    it do
+      expect do
+        described_class.validate_origin_name(:some_method, origin: :output)
+      end.not_to raise_error
+    end
+  end
+
+  describe ".validate_error_class" do
+    it "with an exception class" do
+      expect { described_class.validate_error_class(ArgumentError) }
+        .not_to raise_error
+    end
+
+    it "with a non-class object" do
+      expect { described_class.validate_error_class("123") }
+        .to raise_error(
+          ArgumentError,
+          "Expected 123 to be a subclass of Exception",
+        )
+    end
+
+    it "with a non-exception class" do
+      expect { described_class.validate_error_class(Class.new) }
+        .to raise_error(
+          ArgumentError,
+          /Expected .+ to be a subclass of Exception/,
+        )
+    end
+  end
+end

spec/service_actor/result_spec.rb

@@ -9,6 +9,38 @@
     expect(result.respond_to?(:name?)).to be true
   end
 
+  describe ".instance_methods" do
+    it "stays the same across supported Rubies" do # rubocop:disable RSpec/ExampleLength
+      expect(described_class.instance_methods).to contain_exactly(
+        :merge!,
+        :send,
+        :delete!,
+        :key?,
+        :pretty_print,
+        :failure?,
+        :inspect,
+        :fail!,
+        :class,
+        :success?,
+        :[]=,
+        :[],
+        :kind_of?,
+        :is_a?,
+        :respond_to?,
+        :to_h,
+        :equal?,
+        :!,
+        :__send__,
+        :==,
+        :!=,
+        :__binding__,
+        :instance_eval,
+        :instance_exec,
+        :__id__,
+      )
+    end
+  end
+
   context "when input is String" do
     context "when is empty" do
       it "returns false" do

spec/service_actor/version_spec.rb

@@ -1,7 +1,5 @@
 # frozen_string_literal: true
 
 RSpec.describe ServiceActor::VERSION do
-  # rubocop:disable RSpec/DescribedClass
-  it { expect(ServiceActor::VERSION).not_to be_nil }
-  # rubocop:enable RSpec/DescribedClass
+  it { is_expected.not_to be_nil }
 end