ActiveInteraction::Base leaks _validators

[aaronjensen]

Some frameworks, like client_side_validations make use of ._validators. Because of the way ActiveInteraction::Base includes ActiveModel::Validations (which differs from the way that ActiveModel::Model does it) only one _validators method and associated class attribute is defined so they all share it. This causes problems.

The below test fails:

  it 'does not leak _validators' do
    first = Class.new(ActiveInteraction::Base) do
      string :foo
      validates_presence_of :foo
    end

    second = Class.new(ActiveInteraction::Base) do
      string :bar
      validates_presence_of :bar
    end

    expect(first._validators.keys).to eql [:foo]
    expect(second._validators.keys).to eql [:bar]
  end

I attempted to move includes to callbacks but this broke a couple tests.

In ActiveInteraction::Base

      def inherited(klass)
        klass.send :include, ActiveModelable
        klass.send :include, Runnable
        klass.send :validate, :input_errors
        klass.instance_variable_set(:@_interaction_filters, filters.dup)
      end

In ActiveModelable

    extend ActiveSupport::Concern
    included do #:nodoc:
      extend  ActiveModel::Naming
      include ActiveModel::Validations
      include ActiveModel::Conversion
    end

Failures:

Failures:

  1) ActiveInteraction::Base#compose with invalid composition has the correct errors
     Failure/Error: expect(outcome.errors[:base])
       expected collection contained:  ["X is required", "Y is required"]
       actual collection contained:    ["X translation missing: en.activemodel.errors.models.9f3d193a8a7083ca65b726895b1dfade.attributes.x.missing", "Y translation missing: en.activemodel.errors.models.9f3d193a8a7083ca65b726895b1dfade.attributes.y.missing"]
       the missing elements were:      ["X is required", "Y is required"]
       the extra elements were:        ["X translation missing: en.activemodel.errors.models.9f3d193a8a7083ca65b726895b1dfade.attributes.x.missing", "Y translation missing: en.activemodel.errors.models.9f3d193a8a7083ca65b726895b1dfade.attributes.y.missing"]
     # ./spec/active_interaction/base_spec.rb:317:in `block (4 levels) in <top (required)>'

  2) ActiveInteraction::Base#execute raises an error
     Failure/Error: expect { interaction.execute }.to raise_error NotImplementedError
       expected NotImplementedError, got #<NoMethodError: undefined method `execute' for #<ActiveInteraction::Base:0x007f8363d0f500>> with backtrace:
         # ./spec/active_interaction/base_spec.rb:325:in `block (4 levels) in <top (required)>'
         # ./spec/active_interaction/base_spec.rb:325:in `block (3 levels) in <top (required)>'
     # ./spec/active_interaction/base_spec.rb:325:in `block (3 levels) in <top (required)>'

[smdern]

I've ran into the same issue.

[aaronjensen]

I updated this with the actual changes I made to make it easier to see the bug.

[tfausak]

Thanks for finding this bug! I'd like to have a test case that doesn't use the "private" accessor _validators. This doesn't have exactly the same semantics, but it gets the point across:

class A < ActiveInteraction::Base
  string :a
  validates_presence_of :a
end

class B < ActiveInteraction::Base
  string :b
  validates_presence_of :b
end

A.validators == B.validators
# => true

I'll look at your proposed fix and see if I can figure out what's going on with the tests.

[tfausak]

Here's the test case version of my code snippet:

it 'does not leak validators' do
  a = Class.new(ActiveInteraction::Base) do
    string :a
    validates_presence_of :a
  end

  b = Class.new(ActiveInteraction::Base) do
    string :b
    validates_presence_of :b
  end

  expect(a.validators).to_not eq b.validators
end

And here's a minimal change that makes the test pass without affecting any of the other tests:

diff --git a/lib/active_interaction/base.rb b/lib/active_interaction/base.rb
index edcdb60..00f480d 100644
--- a/lib/active_interaction/base.rb
+++ b/lib/active_interaction/base.rb
@@ -134,6 +134,7 @@ module ActiveInteraction

       # @param klass [Class]
       def inherited(klass)
+        klass._validators = _validators.dup if self == Base
         klass.instance_variable_set(:@_interaction_filters, filters.dup)
       end

Does that seem like a reasonable fix?

Edit: That might have to be a deep dup, like Rails does. But _validators on Base should be empty anyway.

[tfausak]

The fix turned out to be even simpler! Check out #176.