Use ActiveModel::Attributes instead of custom implementation

[aaronmallen]

Elevator pitch, describe and sell us your feature request

As a rails developer,
I want to be able to expect a similar api as ActiveModel::Attributes for Context::Base
so that I can utilize common testing libraries like shoulda-matchers

Is your feature request related to a problem

This was originally requested by @dark-panda on reddit: https://www.reddit.com/r/rails/comments/eu4z1b/activeinteractor_v100_release/ffnzmy3?utm_source=share&utm_medium=web2x

Have you considered any alternatives

Additional Comments

This should be done in a backwards compatible manor. I don't want to have to bump the major version for this functionality at this time.

The reason for the custom functionality is that Context::Base inherits from OpenStruct and the overall goal of the Context::Attributes was to provide a mechanism for explicitly returning the desired attributes on the context object instead of all the actual attributes defined on the object.

class Context < ActiveInteractor::Context::Base
  attributes :foo
end

context = Context.new(foo: 'foo', bar: 'bar')
context.attributes
#=> { foo: 'foo' }

context.bar
#=> 'bar'

[dark-panda]

I've discovered a bug in this PR that isn't covered by a spec: when you are using an organizer, the context values in the organizer's steps don't get initialized properly. Basic case:

class TestInteractor < ApplicationInteractor
  def perform
    p context
    p context['user']
    p context.user
  end
end

class TestInteractorContext < ApplicationContext
  attributes :user
end

class TestOrganizer < ApplicationInteractorOrganizer
  organize :test_interactor
end

TestOrganizer.perform(user: 'foo')

Output before change:

#<TestInteractorContext user="foo">
"foo"
"foo"
TestOrganizer::Context {
    :user => "foo"
}

Output after change:

#<TestInteractorContext user="foo">
"foo"
nil
TestOrganizer::Context {
    :user => "foo"
}

As we see here, accessing the user attribute via a method call fails and silently returns nil. The reason this is happening goes something like this:

• there is no TestOrganizerContext and we actually end up using a just-in-time TestOrganizer::Context that has no attributes defined.

• in the original code, this worked because the OpenStruct initializer handled copying over all key-pair values from the context, but in the new code, we copy over attributes, which is quite different. In the new code, since no attributes are defined on the just-in-time TestOrganizer::Context class that's created on-the-fly, no attributes are copied over to the new context.

This issue presumably affects any time that a context is being copied, because it now relies upon attributes being specified rather than all values in an enumerable structure being copied by iteration via OpenStruct. To fix the example code, I would have to manually create a context for TestOrganizer and give it a user attribute.

The question here becomes then: should attributes need to be explicit, rather than allowing basically any value to enter the context via OpenStruct? I see that this is what the interactor gem uses, but in that case, they don't have validations or explicitly defined contexts with explicitly defined attributes, so the two philosophies might not be compatible in this case.

I played around with a couple of ideas, and pushed a branch with a test case that works in both the original code and with the adjustments I've made, but I don't particularly like the code I've written (https://github.com/dark-panda/activeinteractor/tree/attribute-merging-fixes):

• it seems flaky, because it requires a call to #to_sym and makes the assumption that context keys are defined as symbols, which may not be the case.

• it has rather high complexity.

• there are likely further edge cases that I'm not accounting for.

This might sort of boil down to philosophy though -- are explicit contexts with implicit key-pair values going to work together in this structure?

[aaronmallen]

@dark-panda the on the fly context is not new functionality and is the intended behavior. See the first paragraph in https://github.com/aaronmallen/activeinteractor/wiki/Context however this is a bug and I think it is because of the private method merge_attribute_values. I thought this spec compensated for this https://github.com/aaronmallen/activeinteractor/blob/master/spec/active_interactor/context/base_spec.rb#L96 but it would appear I was wrong.

Could you do me a favor and open a bug with your use case. I look at it tomorrow.

[dark-panda]

I see that it's the intended behaviour, my point is more wondering aloud if the context should inherit all of the values in the incoming context or if it should only handle named attributes when initializing the context. The context should be modifiable once it has been received by an interactor since that is the intent of the interactor pattern, but should the initial context itself receive all values during initialization? In the patch I provided (which I'm not particularly fond of) I only copy over explicitly named attributes when they are set, rather than all attributes in the context.

Anyways, we can continue the discussion in the bug report. I'll post the example code from here.

[aaronmallen]

@dark-panda the feature has been released and is now available in v1.0.1