AaronLasseigne / active_interaction

:briefcase: Manage application specific business logic.
MIT License
2.06k stars 136 forks source link

Order dependency of filters depending on other filters #524

Closed smcabrera closed 2 years ago

smcabrera commented 2 years ago

Similar but not identical to the issue https://github.com/AaronLasseigne/active_interaction/issues/507. Here it's not a matter of a circular dependency but the order that filters are added when using import filters or a module.

I have an example reproduction of this issue in this repo

Say we have an interaction like this

class CreatePost < ActiveInteraction::Base
  string :title, default: -> { "#{user.name}'s post" }
  record :user

  def execute
    Post.create(user: user, title: title)
  end
end

This interaction worked fine with active_interaction 3.8. However, in 4.1, we'll get an error:

$ tst spec/interactions/create_post_spec.rb
WARNING: Redefining CreatePost#title filter
.F

Failures:

  1) CreatePost run without a title argument
     Failure/Error: string :title, default: -> { "#{user.name}'s post" }

     NoMethodError:
       undefined method `name' for nil:NilClass
     # ./app/interactions/create_post.rb:7:in `block in <class:CreatePost>'
     # ./spec/interactions/create_post_spec.rb:13:in `block (4 levels) in <top (required)>'

Finished in 0.20043 seconds (files took 2.39 seconds to load)
2 examples, 1 failure

Failed examples:

rspec ./spec/interactions/create_post_spec.rb:15 # CreatePost run without a title argument

We can fix this by changing the order of the filters, so this works

class CreatePost < ActiveInteraction::Base
  record :user
  string :title, default: -> { "#{user.name}'s post" }

  def execute
    Post.create(user: user, title: title)
  end
end

I was poking around in the code and it looks like this works because by the time the title proc is called the user is already available in the context so it's able to be used by title, when previously it was not yet available since they're created in the order they appear in the file.

Where it gets tricky is that we have some places in our code that share filters with each other through ImportFilters and including concerns. These sometimes break because it looks like a filter can be added in one place but even though it is overridden later, its key still now appears earlier in hash, which means it can still break some of our filters that are dependent in this way. For example

class CreatePost < ActiveInteraction::Base
  include PostConcern

  record :user
  string :title, default: -> { "#{user.name}'s post" }

  def execute
    Post.create(user: user, title: title)
  end
end

module PostConcern
  extend ActiveSupport::Concern

  included do
    string :title, default: 'default title'
  end
end

This fails the same was as previously noted even if we have title after user in the CreatePost interaction because it appears before the (non-existent) user filter in the concern.

It looks like this is fixed if we update the module to include an optional filter like this

module PostConcern
  extend ActiveSupport::Concern

  included do
    record :user, default: nil
    string :title, default: 'default title'
  end
end

This workaround is ok, and I'm not sure if there is a good way to better support this behavior in the library, but it's a bit of a surprising situation and it's something that appears to be broken when upgrading so at minimum maybe this edge case could be documented and a breaking change noted in the release notes?

AaronLasseigne commented 2 years ago

This turns out to be an interesting issue. Prior to 4.0.0 we would populate the object with the raw value provided in the input and then override them with the filtered value. I think we did it that way at the time to preserve the raw values for use in errors but I don't exactly remember. The result of this is that in some cases what you noticed would work. The raw value given was available and used in the provided default proc and functioned correctly. This however, was luck.

Here's an example (using 3.8.3):

class Test < ActiveInteraction::Base
  integer :a, default: -> { b + 1 }
  integer :b

  def execute
    a
  end
end

> Test.run!(b: 2) # => 3
> Test.run!(b: '2') # => TypeError (no implicit conversion of Integer into String)

This happens because the b value was not converted into an integer so when we try to add 1 to it we get an error. If you flip the order of :a and :b it does work.

The real error here is that we wrote improper values in the first place. In a way this error was accidentally fixed in 4.0.0.

I'm sorry it caused confusion. That's certainly frustrating. I've thought about ways to help this and the best I can think of is to process the inputs first by filters without default procs, then any inputs passed, and finally filters that received no input and have a default proc. The issue is that we still have the same issue with any of those final filters with no inputs and default procs. We've shrunk the chances of it happening but it's much more confusing for someone to reason about and much more prone to failing in a surprising way. I think having them work in order is probably the best bet.