diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index e257c4c..6001561 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -4,6 +4,9 @@ on: push: branches: - "master" + pull_request: + branches: + - "master" jobs: spec: diff --git a/interactor.gemspec b/interactor.gemspec index c99e59e..fd822cf 100644 --- a/interactor.gemspec +++ b/interactor.gemspec @@ -13,7 +13,6 @@ Gem::Specification.new do |spec| spec.files = `git ls-files`.split($INPUT_RECORD_SEPARATOR) - spec.add_dependency "ostruct" spec.add_development_dependency "bundler" spec.add_development_dependency "rake" end diff --git a/lib/interactor.rb b/lib/interactor.rb index 307234c..c76ca17 100644 --- a/lib/interactor.rb +++ b/lib/interactor.rb @@ -114,9 +114,7 @@ def initialize(context = {}) def run run! rescue Failure => e - if context.object_id != e.context.object_id - raise - end + raise unless context.equal?(e.context) end # Internal: Invoke an Interactor instance along with all defined hooks. The diff --git a/lib/interactor/context.rb b/lib/interactor/context.rb index 8e2181e..8fb64ba 100644 --- a/lib/interactor/context.rb +++ b/lib/interactor/context.rb @@ -1,4 +1,4 @@ -require "ostruct" +require "set" module Interactor # Public: The object for tracking state of an Interactor's invocation. The @@ -12,6 +12,11 @@ module Interactor # # The context may be manipulated using arbitrary getter and setter methods. # + # A context is not safe for concurrent writes: the first write of a key may + # define a singleton accessor (see RESERVED_NAMES), which mutates the + # instance's singleton class. Share a context across threads for reads only, + # or guard writes externally. + # # Examples # # context = Interactor::Context.new @@ -28,7 +33,7 @@ module Interactor # # => "baz" # context # # => # - class Context < OpenStruct + class Context # Internal: Initialize an Interactor::Context or preserve an existing one. # If the argument given is an Interactor::Context, the argument is returned. # Otherwise, a new Interactor::Context is initialized from the provided @@ -60,6 +65,109 @@ def self.build(context = {}) end end + def initialize(context = {}) + # Assign every instance variable up front, in a fixed order, so all + # contexts share one object shape. fail!/halt!/rollback! then only mutate + # existing slots instead of adding ivars, which would fork the shape and + # make @table and flag reads polymorphic (slower) under YJIT. + @table = {} + reset_state + context&.each { |key, value| self[key] = value } + end + + # Public: Read a context attribute by key. + def [](key) + @table[key.to_sym] + end + + # Public: Write a context attribute by key, normalising to symbol. + def []=(key, value) + key = key.to_sym + define_accessor(key) if define_accessor?(key) + @table[key] = value + end + + # Public: Return all user-set attributes as a Hash (excludes internal + # state). Mirrors OpenStruct#to_h: with a block, each key/value pair is + # transformed; without one, a shallow copy is returned. + def to_h(&block) + return @table.to_h(&block) if block + + @table.dup + end + + # Public: Extract a nested value, mirroring OpenStruct#dig (and Hash#dig). + # The first key is normalised to a symbol to match attribute storage. + def dig(key, *rest) + @table.dig(key.to_sym, *rest) + end + + def ==(other) + other.is_a?(Context) && @table == other.table + end + + def eql?(other) + other.is_a?(Context) && @table.eql?(other.table) + end + + def hash + @table.hash + end + + # Public: Freeze the context. Freezes the backing table too so that, as with + # OpenStruct, subsequent writes raise FrozenError rather than silently + # succeeding. + def freeze + @table.freeze + super + end + + # Public: Serialize only the attribute table. Because set keys define + # singleton methods, the default Marshal path raises "singleton can't be + # dumped"; dumping just @table keeps contexts marshallable (e.g. cached or + # enqueued) and rebuilds accessors on load. Transient state (called list, + # failure/halted flags) is intentionally not preserved. + def marshal_dump + @table + end + + def marshal_load(table) + @table = table + reset_state + rebuild_accessors + end + + def inspect + pairs = @table.map { |k, v| "#{k}=#{v.inspect}" } + "#<#{self.class}#{" #{pairs.join(", ")}" unless pairs.empty?}>" + end + + # OpenStruct aliased to_s to inspect; preserve that so Interactor::Failure + # messages (Exception#message calls context.to_s) stay readable rather than + # falling back to Object#to_s (#). + alias_method :to_s, :inspect + + def respond_to_missing?(method_name, include_private = false) + method_name.to_s.end_with?("=") || @table.key?(method_name.to_sym) || super + end + + # Dynamic getter/setter for arbitrary context attributes. + # Setters end with "="; getters return nil for unset keys. + def method_missing(method_name, *args) + name = method_name.to_s + if name.end_with?("=") + self[name.delete_suffix("=").to_sym] = args.first + else + # Mirror OpenStruct: a getter takes no arguments, so flag caller bugs + # rather than silently ignoring them. + unless args.empty? + raise ArgumentError, "wrong number of arguments (given #{args.size}, expected 0)" + end + + @table[method_name.to_sym] + end + end + # Public: Whether the Interactor::Context is successful. By default, a new # context is successful and only changes when explicitly failed. # @@ -143,7 +251,7 @@ def halted? # # Raises Interactor::Failure initialized with the Interactor::Context. def fail!(context = {}) - context.each { |key, value| self[key.to_sym] = value } + context.each { |key, value| self[key] = value } @failure = true raise Failure, self end @@ -171,7 +279,7 @@ def fail!(context = {}) # # Raises Interactor::Halt initialized with the Interactor::Context. def halt!(context = {}) - context.each { |key, value| self[key.to_sym] = value } + context.each { |key, value| self[key] = value } @halted = true raise Halt, self end @@ -204,6 +312,7 @@ def called!(interactor) # Returns true if rolled back successfully or false if already rolled back. def rollback! return false if @rolled_back || halted? + _called.reverse_each(&:rollback) @rolled_back = true end @@ -250,12 +359,105 @@ def _called # end # # Returns the context as a hash, including success, failure, and halted - def deconstruct_keys(keys) - to_h.merge( + def deconstruct_keys(_keys) + @table.merge( success: success?, failure: failure?, halted: halted? ) end + + protected + + # Readable by sibling contexts only, so #== / #eql? can compare tables + # without reaching into another instance's @table by name. + attr_reader :table + + private + + # Whether a singleton accessor should be installed for the given key. + # + # A plain key (one that does not shadow an existing method) is served by + # #method_missing, so no accessor is needed - keeping ordinary contexts free + # of per-key singleton methods. An accessor is installed only when the key + # would otherwise be intercepted by an inherited method (e.g. ActiveSupport's + # Object#to_json), so the stored value reads back. Reserved names and keys + # already backed by an accessor are skipped. + # + # The collision test reads the class's real method table rather than + # #respond_to?, which #respond_to_missing? would report true for any stored + # key, and avoids touching #singleton_class until we know shadowing is + # needed - materialising a per-instance singleton class for an ordinary key + # would give every context its own class and make attribute reads + # megamorphic under YJIT. + def define_accessor?(key) + return false if RESERVED_NAMES.include?(key) + return false unless shadows_inherited_method?(key) + + !singleton_class.method_defined?(key, false) + end + + # Whether a real (non-method_missing) method named key is inherited, so a + # plain getter would dispatch to it instead of the stored value. + def shadows_inherited_method?(key) + self.class.method_defined?(key) || self.class.private_method_defined?(key) + end + + # Define a getter on this instance's singleton class so it outranks the + # inherited method the key shadows. Only the reader needs overriding: setter + # names end in "=", which no inherited method does, so writes always route + # through #method_missing to #[]=. See #define_accessor? for when this runs. + def define_accessor(key) + define_singleton_method(key) { @table[key] } + end + + # Install singleton accessors for every stored key that needs one. Used + # after @table is replaced wholesale (dup/clone, Marshal load) rather than + # built up key-by-key through #[]=. + def rebuild_accessors + @table.each_key { |key| define_accessor(key) if define_accessor?(key) } + end + + def initialize_copy(orig) + super + @table = orig.to_h + rebuild_accessors + reset_state + # A copy starts fresh except that it carries over the called interactors, + # so it can still be rolled back. + @called = orig._called.dup + end + + # Set the transient state to that of a brand-new context, assigning the + # ivars in the order #initialize establishes so every construction path + # produces the same object shape. + def reset_state + @called = [] + @failure = false + @halted = false + @rolled_back = false + end + + # Internal: Inherited Object/Kernel methods the context itself calls and + # must never let a key shadow. Only this protocol half needs hand-listing - + # it lives on Object, not here; the class's own methods are folded in by + # RESERVED_NAMES below. + CORE_PROTOCOL = %i[ + dup clone frozen? class is_a? kind_of? instance_of? nil? + send __send__ singleton_class define_singleton_method + instance_variable_get instance_variable_set method methods + respond_to? object_id equal? + ].freeze + + # Internal: Method names a context key must never shadow with a singleton + # accessor. Derived from the class's own API (so adding or renaming a method + # can't drift out of sync with this list) plus the inherited protocol above. + # Such keys are still stored in @table and reachable via #[]/#to_h. Names + # from *other* libraries (e.g. ActiveSupport's Object#to_json) are absent on + # purpose: those are shadowed so a stored value reads back, which is the + # entire reason accessors exist. + RESERVED_NAMES = ( + instance_methods(false) + private_instance_methods(false) + CORE_PROTOCOL + ).to_set.freeze end end diff --git a/spec/interactor/context_spec.rb b/spec/interactor/context_spec.rb index 14c88bc..c1f1b1d 100644 --- a/spec/interactor/context_spec.rb +++ b/spec/interactor/context_spec.rb @@ -12,7 +12,7 @@ module Interactor context = Context.build expect(context).to be_a(Context) - expect(context.send(:table)).to eq({}) + expect(context.to_h).to eq({}) end it "doesn't affect the original hash" do @@ -144,11 +144,9 @@ module Interactor end it "makes the context available from the failure" do - begin - context.fail! - rescue Failure => error - expect(error.context).to eq(context) - end + context.fail! + rescue Failure => error + expect(error.context).to eq(context) end end @@ -288,6 +286,360 @@ module Interactor end end + describe "dynamic attributes" do + let(:context) { Context.build } + + it "sets and reads attributes via method syntax" do + context.foo = "bar" + expect(context.foo).to eq("bar") + end + + it "returns nil for unset attributes" do + expect(context.missing_key).to be_nil + end + + it "raises ArgumentError when a getter is called with arguments" do + context.foo = "bar" + expect { context.foo(1) }.to raise_error(ArgumentError) + end + + it "overwrites previously set attributes" do + context.foo = "bar" + context.foo = "baz" + expect(context.foo).to eq("baz") + end + + it "responds to setter methods" do + expect(context.respond_to?(:foo=)).to eq(true) + end + + it "responds to getter methods only after the key is set" do + expect(context.respond_to?(:foo)).to eq(false) + context.foo = "bar" + expect(context.respond_to?(:foo)).to eq(true) + end + + it "normalises string keys to symbols" do + context["foo"] = "bar" + expect(context.foo).to eq("bar") + expect(context[:foo]).to eq("bar") + end + end + + describe "#[] and #[]=" do + let(:context) { Context.build } + + it "reads and writes with symbol keys" do + context[:foo] = "bar" + expect(context[:foo]).to eq("bar") + end + + it "normalises string keys on write" do + context["foo"] = "bar" + expect(context[:foo]).to eq("bar") + end + + it "normalises string keys on read" do + context[:foo] = "bar" + expect(context["foo"]).to eq("bar") + end + end + + describe "#to_h" do + it "returns user-set attributes as a hash" do + context = Context.build(foo: "bar", baz: 42) + expect(context.to_h).to eq(foo: "bar", baz: 42) + end + + it "does not include internal state flags" do + context = Context.build(foo: "bar") + begin + context.fail! + rescue + nil + end + hash = context.to_h + expect(hash.keys).not_to include(:failure, :success, :halted) + end + + it "returns a copy that does not affect the context" do + context = Context.build(foo: "bar") + hash = context.to_h + hash[:foo] = "mutated" + expect(context.foo).to eq("bar") + end + + it "transforms pairs when given a block" do + context = Context.build(foo: "bar") + result = context.to_h { |key, value| [key.to_s, value.upcase] } + expect(result).to eq("foo" => "BAR") + end + end + + describe "#==" do + it "is equal to another context with the same attributes" do + context1 = Context.build(foo: "bar") + context2 = Context.build(foo: "bar") + expect(context1).to eq(context2) + end + + it "is not equal to a context with different attributes" do + context1 = Context.build(foo: "bar") + context2 = Context.build(foo: "baz") + expect(context1).not_to eq(context2) + end + + it "is not equal to a plain hash" do + context = Context.build(foo: "bar") + expect(context).not_to eq(foo: "bar") + end + + it "is equal regardless of attribute insertion order" do + context1 = Context.build(foo: "bar", baz: "qux") + context2 = Context.build(baz: "qux", foo: "bar") + expect(context1).to eq(context2) + end + end + + describe "#eql? and #hash" do + it "two contexts with the same attributes are eql?" do + context1 = Context.build(foo: "bar") + context2 = Context.build(foo: "bar") + expect(context1.eql?(context2)).to eq(true) + end + + it "two contexts with different attributes are not eql?" do + context1 = Context.build(foo: "bar") + context2 = Context.build(foo: "baz") + expect(context1.eql?(context2)).to eq(false) + end + + it "equal contexts have the same hash value" do + context1 = Context.build(foo: "bar") + context2 = Context.build(foo: "bar") + expect(context1.hash).to eq(context2.hash) + end + + it "can be used as a Hash key with value semantics" do + context1 = Context.build(foo: "bar") + context2 = Context.build(foo: "bar") + h = {context1 => :found} + expect(h[context2]).to eq(:found) + end + end + + describe "#dup (initialize_copy)" do + let(:instance1) { double(:instance1) } + let(:instance2) { double(:instance2) } + + it "dups the @table so attribute mutations are isolated" do + original = Context.build(foo: "bar") + copy = original.dup + copy.foo = "baz" + expect(original.foo).to eq("bar") + end + + it "dups @called so rollback lists are independent" do + original = Context.build + original.called!(instance1) + copy = original.dup + copy.called!(instance2) + expect(original._called).to eq([instance1]) + expect(copy._called).to eq([instance1, instance2]) + end + + it "resets failure state so a dup of a failed context starts fresh" do + original = Context.build(foo: "bar") + begin + original.fail! + rescue + nil + end + copy = original.dup + expect(copy.failure?).to eq(false) + expect(copy.success?).to eq(true) + end + + it "resets halted state so a dup of a halted context starts fresh" do + original = Context.build(foo: "bar") + begin + original.halt! + rescue + nil + end + copy = original.dup + expect(copy.halted?).to eq(false) + end + end + + describe "OpenStruct removal" do + it "does not inherit from OpenStruct" do + expect(Context.superclass).to eq(Object) + end + + # Regression guard for the real-world failure mode: in the application, + # ActiveSupport adds Object#to_json and ~32 endpoints store a rendered + # payload as context.to_json then read it back. A plain method_missing + # implementation lets that read fall through to the inherited method. + # ActiveSupport is not loaded in this gem's own suite, so we reproduce the + # same shadowing with Kernel#display, which exists on every Object here; + # the application-level to_json case is covered by the Docsketch suite. + it "returns the stored value for a key shadowing an inherited method" do + context = Context.build + context.display = "stored-payload" + expect(context.display).to eq("stored-payload") + end + + it "preserves the shadowing override across dup" do + context = Context.build + context.display = "stored-payload" + expect(context.dup.display).to eq("stored-payload") + end + + it "does not install a singleton accessor for a plain key" do + context = Context.build + context.foo = "bar" + expect(context.singleton_methods).not_to include(:foo) + expect(context.foo).to eq("bar") + end + + it "installs a singleton accessor only for keys shadowing a method" do + context = Context.build + context.display = "stored-payload" + expect(context.singleton_methods).to include(:display) + end + end + + describe "reserved names" do + it "stores a reserved key without overriding the method" do + context = Context.build + context[:hash] = "stored" + # #hash still returns the real Integer identity hash, not "stored", + # while the value remains reachable through #[]. + expect(context.hash).to eq({hash: "stored"}.hash) + expect(context[:hash]).to eq("stored") + end + + it "keeps #to_h working when a :to_h key is stored" do + context = Context.build(to_h: "stored", foo: "bar") + expect(context.to_h).to eq(to_h: "stored", foo: "bar") + expect(context[:to_h]).to eq("stored") + end + + it "keeps equality working when a reserved key is stored" do + context1 = Context.build(hash: "x") + context2 = Context.build(hash: "x") + expect(context1).to eq(context2) + end + + # Guards against silent divergence: if a new public method is added to + # Context without reserving its name, a user key of the same name would + # shadow it. This fails loudly instead. + it "reserves every method the class defines" do + own_methods = Context.instance_methods(false) + unreserved = own_methods.reject { |name| Context::RESERVED_NAMES.include?(name) } + expect(unreserved).to eq([]) + end + end + + describe "#freeze" do + it "raises when writing to a frozen context" do + context = Context.build(foo: "bar") + context.freeze + expect { context.baz = "qux" }.to raise_error(FrozenError) + end + + it "reports itself as frozen" do + context = Context.build.freeze + expect(context).to be_frozen + end + end + + describe "#dig" do + it "reads a top-level attribute" do + context = Context.build(foo: "bar") + expect(context.dig(:foo)).to eq("bar") + end + + it "digs into nested values" do + context = Context.build(foo: {a: {b: 1}}) + expect(context.dig(:foo, :a, :b)).to eq(1) + end + + it "normalises a string first key" do + context = Context.build(foo: {a: 1}) + expect(context.dig("foo", :a)).to eq(1) + end + + it "returns nil for a missing key" do + expect(Context.build.dig(:missing, :nope)).to be_nil + end + end + + describe "Marshal round-trip" do + it "dumps and loads an attribute-bearing context" do + context = Context.build(foo: "bar", count: 3) + restored = Marshal.load(Marshal.dump(context)) + expect(restored).to eq(context) + expect(restored.foo).to eq("bar") + end + + it "rebuilds accessors for shadowing keys after load" do + context = Context.build + context.display = "stored-payload" + restored = Marshal.load(Marshal.dump(context)) + expect(restored.display).to eq("stored-payload") + end + + it "starts a restored context in a fresh, successful state" do + context = Context.build(foo: "bar") + restored = Marshal.load(Marshal.dump(context)) + expect(restored.success?).to eq(true) + expect(restored._called).to eq([]) + end + end + + describe "object shape" do + # All construction paths must assign the same instance variables in the + # same order so contexts share one object shape; otherwise @table and + # flag reads go polymorphic (slower) under YJIT. + it "is identical regardless of state or construction path" do + fresh = Context.build(a: 1) + failed = Context.build(a: 1) + begin + failed.fail! + rescue + nil + end + halted = Context.build(a: 1) + begin + halted.halt! + rescue + nil + end + duped = fresh.dup + loaded = Marshal.load(Marshal.dump(fresh)) + + shapes = [fresh, failed, halted, duped, loaded].map(&:instance_variables) + expect(shapes.uniq.size).to eq(1) + end + end + + describe "#to_s" do + it "matches #inspect so attribute info is not lost" do + context = Context.build(foo: "bar") + expect(context.to_s).to eq(context.inspect) + expect(context.to_s).to include('foo="bar"') + end + + it "keeps Interactor::Failure messages readable" do + context = Context.build(foo: "bar") + context.fail! + rescue Failure => error + expect(error.message).to include('foo="bar"') + end + end + describe "#deconstruct_keys" do let(:context) { Context.build(foo: :bar) } @@ -318,7 +670,7 @@ module Interactor end it "supports rightward assignment for halted:" do - context => { halted: } + context => {halted:} expect(halted).to be(true) end end