Strategy pattern and implications

[nicooga]

TL;DR:

1. the matcher raise_validation_error does not work with errors from nested actions
2. I propose adding a new validator like validates :some_nested_action, nested_action: {merge_errors: true}. By merging the errors we'd also circunvent the problems of point 1, but the problem would still persist.
3. I propose adding a DSL for stratgies

I've used nested actions to implement a strategy pattern, but found some problems that forced me to introduce some terrible monkeypatches.

Consider this example of a strategy pattern implemented using nested actions:

module CommonStuff
  extend ActiveSupport::Concern

  included do
    allow_if { true }

    attribute :soldiers, Array
    attribute :target, String
    attribute :strategy, Object
  end
end

class AssaultPosition < Granite::Action
  include CommonStuff

  validates :strategy_action, nested: true

  def strategy=(val)
    case val
    when Granite::Action then super(val)
    when Symbol
      super({
        flanking: AssaultPosition::Flanking,
        front_assault: AssaultPosition::FrontAssault,
      }.fetch(val))
    end
  end

  private

  def execute_perform!(*)
    strategy_action.perform!
  end

  def strategy_action
    @strategy_action ||= strategy.as(performer).new(attributes)
  end
end

class AssaultPosition::FrontAssault < Granite::Action
  include CommonStuff

  private def execute_perform!(*)
    puts "Performing front assault on #{target}"
  end
end

class AssaultPosition::Flanking < Granite::Action
  include CommonStuff

  validates :soldiers, length: { minimum: 6, message: 'You need more soldiers to perform this strategy' }

  private def execute_perform!(*)
    puts "Flanking #{target}!"
  end
end

action = AssaultPosition.new(strategy: :flanking, soldiers: [1,2,3])
action.valid?
action.errors
=> #<ActiveModel::Errors:0x00007fbffacf2e40
 @base=#<AssaultPosition soldiers: [1, 2, 3], target: nil, strategy: AssaultPosition::Flanking(soldiers: Array, target: String, strategy: Object)>,
 @details={},
 @messages={:"strategy_action.soldiers"=>["You need more soldiers to perform this strategy"]}>

The problem that forces me to monkey patch around is that the provided matcher raise_validation_error relies on error.details, which is not populated in the same manner that with non nested validations. This is most probably related to how the validator works, which actually comes from active_data gem.

The other problem is also more or less of a blocker for using this kind of approach: the messages are namespaced under strategy_action, which may make sense in most situations, but not here, because both the strategies and the parent caller action would accept the same attributes, and the code that must do something with the errors does not expect the error not to be namespaced.

I propose adding our own version of the nested action validator, which accepts the option merge_errors.

In other notes, I think is worth considering adding a DSL for strategies so we could convert my first code example into something like this (with the option to move the implementation of the strategies into another class):

class AssaultPosition < Granite::Action
  allow_if { true }

  attribute :soldiers, Array
  attribute :target, String

  strategy :front_assault do
    private def execute_perform!(*)
      puts "Performing front assault on #{target}"
    end
  end

  strategy :flanking do
    validates :soldiers, length: { minimum: 6, message: 'You need more soldiers to perform this strategy' }

    private def execute_perform!(*)
      puts "Flanking! #{target}"
    end
  end
end

[IvanShamatov]

Please correct me if I wrong, but why not to use inheritance for different "strategies"?

[nicooga]

@IvanShamatov I don't have anything against it, it's another viable option.

[nicooga]

Confirmed the reason that error.details is not properly populated. They should be populated manually in the same way activerecord's accept_nested_attributes_for does:

                errors.details[reflection_attribute] << error
                errors.details[reflection_attribute].uniq!

This is needed to be able to match the type of the error with:

raise_validation_error.of_type(:blank).on(:"some_nested_action.some_attribute")

... on nested errors.

[pirj]

What if you avoid raise_validation_error in specs and prefer good old .to not_be_valid and expect a message namespaced to the nested action?

the code that must do something with the errors does not expect the error not to be namespaced.

Not sure what code do you mean exactly, the presentation of the error to the user?

What problem are you solving with merging the validation messages to the same namespace? I see a good space for ambiguity, e.g. when root action and nested action accept the parameters with identical names, but that are semantically different, e.g. codename, that would be the codename of the battle and the codename of the attack. The error messages should be different in those two cases (e.g. "Battle should have a codename" vs "Attack should have a codename").

I side with Ivan,

AssaultPosition::Flanking.new(soldiers: [1,2,3])

seems cleaner to me than:

AssaultPosition.new(strategy: :flanking, soldiers: [1,2,3])

[nicooga]

@pirj I think you are completely right about the matcher. I was migrating the specs which already did this style of testing when you .perform! the action and match on the raised validation error. But the validation matchers from ActiveModel should be just as good (or better). I think we should probably drop the provided matcher.

About the strategies, the point with the pattern is that it should be decided at runtime, so the user might not even know which one is going to be used, and does not necessarily need to know either. In my real use case, the strategy is decided at runtime based on some other attribute, not passed as an argument.

When I talk about namespaced errors as a problem, I mean that the keys of the errors are part of the interface of the action. If your keys are namespaced just be cause of how the action works, you are changing the interface of the action for no reason, and forcing everyone to work around it. IRL, my migration broke some specs on other files because they expected the errors to be on other keys.

[pirj]

Does this issue still stand?

[nicooga]

Hey, I've pinged a few people in search of more input about this topic. I'm sure there will be some more comments. Otherwise, I'll be happy to close this.

[nicooga]

Closing due to lack of input.