freshly / flow Goto Github PK

So I'm feeling like the failure handling is too flexible.

I'm trying to think about this from the perspective of providing error feedback from like, controllers.

Here's what I really really want to avoid:

result = SomeFlow.trigger(input: stuff)
if result.success?
  redirect_to success_path
else
  failure = result.failed_operation.operation_failure
  if failure.problem == :maybe_this
    @thing = failure.details.thing
    @other_thing = failure.details.other_thing
    render edit
  elsif failure.problem == :maybe_that
    if @thing = failure.details.thing
      render edit
    elsif @other_thing = failure.details.other_thing
      render idk_something_else
    else
      flash = "it's the wild west out here"
      redirect_to interactors_path
    end
  end
end

It's not that the access pattern is so wrong, it's that you're completely allowed to end up with that kind of code.

Here's something closer to the interface I'd rather see:

result = SomeFlow.trigger(input: stuff)

# just the one object. everything else on state
result.failure.object

# allow implicit I18n reads if the failure is defined with a I18n key to read from
# if `result.failure.object` responds to `errors.full_messages`, do that instead
# if neither are provided, maybe a default fallback idk
result.failure.message

The goal would be that the flow could usually literally tell you what happened, so you wouldn't have to scavenger-hunt through the data.

It still doesn't feel quite right though. Just wanted to document mostly the bad example. Not sure how best to avoid that.

Right now we have flow, which implies state. Operations have to use state, whether we want them or not, they can't be called without it. If we want to write to state inside of any operations, we must explicitly name the state option from the very beginning. Also this whole metaprogramming implicit naming (OneUsefulFlow and OneUsefulState)...

That feels conceptually wrong. I feel like state should exist not for the flow, but rather for operations and their interconnections.

My suggestion is to use Monads (also check out dry-monads, they did it right but with wonky DSL). This way we can pass around objects, that have outputs/state of previous operation inside, and also explicitly say if the operation was a success or a failure.

Debugging states is really hard generally. Something needs to be done about it.

Maybe there should be a debug log of the state between operations?

Currently the FlowBase concerns are siblings of everything cause they weren't in their own module

Development Tasks

• Wrap Flow concerns in a Flow namespace

• define matcher
• Switch over integration test to use output

Seems like Operation's _failures is supposed to be private or protected, but it's public.

Development Tasks

• Remove deprecation warning for direct state access
• Remove allow_direct_state_access and thus support for direct state access
• Celebrate the 1.0.0 release! 🎉 🍾

• Modularization refactoring
• State status
• State output
• State output matcher

[1] pry(main)> f=CreateListedProductFlow.trigger(price_cents: "fdsa", tax_code: "fdsafs", name: "the best thing ever", item_token: "fdsafdsa")

=> #<CreateListedProductFlow:0x00007f80874e3400
 @executed_operations=[],
 @rewound_operations=[],
 @state=
  #<CreateListedProductState item_token="fdsafdsa" price_cents="fdsa" tax_code="fdsafs" name="the best thing ever" price_currency=nil product=nil product_listing=nil>>
from (pry):2:in `__pry__'
[3] pry(main)> f.failed?
=> false
[4] pry(main)> f.state.valid?
=> false
[5] pry(main)> f.state.errors
=> #<ActiveModel::Errors:0x00007f80875f9c18
 @base=
  #<CreateListedProductState item_token="fdsafdsa" price_cents="fdsa" tax_code="fdsafs" name="the best thing ever" price_currency=nil product=nil product_listing=nil>,
 @details={:price_cents=>[{:error=>:not_a_number, :value=>"fdsa"}]},
 @messages={:price_cents=>["is not a number"]}>

Is this correct?

Development Tasks

• Move class comments over to valid RDoc format
• Add method documentation

Wanted to pull a separate comment aside to talk about the validations. I love the idea, but I dunno if it makes sense to put another set of validations in operations as well as states. That to me feels a bit duplicative.

I think instead I would prefer maybe an imperative form of the existing declarative failure methods. How would something like this strike you?

class Operation1 < ApplicationOperation
  state_reader :weekly_orders

  failure :uneditable_weekly_orders, unless: -> { weekly_orders.all?(&:editable?) }
end

Would support the expected unless: and if: trailing conditionals with symbol support for methods as well.

I just would prefer this rather than having to trudge in the concept of validations again and tie them to a more generic kind of failure. Having states validate and operations fail because of "bad" states feels like a cleaner division of concerns to me.

Thoughts?

Originally posted by @garside in #46 (comment)

class Operation1 < ApplicationOperation
  state_reader :foo, required: true
end

where:

Operation1.new(OpenStruct.new(foo: nil)) 
# => raises Flow::Operation::Errors::MissingState

I'm not sold on the name MissingState. Just like the idea of an explicit raise if it's untrue.

Maybe we should build in some "default" failures somehow? Might make sense to not bother with this missing state thing and just have this induce a :missing_required_state_failure?

Would also be a nice place to put something like an irreversible_operation_failure so that can just become at thing too.

I have such State:

class PurchaseState < ApplicationState
  option :date
  option :total_amount
  option :number_of_items
  option :comment

  validates :total_amount, :number_of_items, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true

  output :purchase
end

and such Operation:

class Purchase::Create < ApplicationOperation
  def behavior
    state.purchase = Purchase.create!(date: state.date, total_amount: state.total_amount,
                                      comment: state.comment, number_of_items: state.number_of_items)
  end
end

Then I have following code:

result = Purchase::CreateFlow.trigger(date: Date.current)
result.state.purchase # #<Purchase id: 25, ... >
result.state.valid? # true
result.state.purchase # return nil right there

I really can't understand. Is it normal behavior that after valid? method call output returns nil or not?

When designing a flow or operation that doesn't support rewindability, it'd be nice if there was a way to set that. Otherwise we'll see a lot of this:

def undo
  raise NonReversibleError
end

It'd be nice if that was handled with a magic class method, with its own custom matcher to boot.

We're seeing a lot of this:

failure :something_wrong

def behavior
  something_wrong_failure!(thing: state.thing) unless thing_valid?
end

Just thinking, it'd be nice if we could do this:

failure :something_wrong, :thing

def behavior
  something_wrong_failure! unless thing_valid?
end

(such that extra keys passed to a failure would be retrieved from state and set on operation_failure.details by default)

Can't really think of a use case for setting on details anything that's not on state, so I feel like it would get used pretty often.

Integration testing flows with comprehensively tested operations feels like a pain in the ass, but you never really know otherwise right now.

Let's add some way to give you that assurance.

My gut is telling me this is probably gonna manifest as something like a custom matcher for whatever shakes out of #61

The point is there should be something that fluxes your operations and does a dry run of "could this ever work, regardless of your input".

Ideally the same technology that powers this at runtime will be what powers the "lite integration" test for flow.

I'm at the point where I feel the best way to integration test flow is with a full-scale integration test, like around the controller and everything too.

tl;dr:

That + "could this ever work" assertion + state validations + good operation unit testing = the winning combo for flow test coverage

agree/disagree?

Development Tasks

• Make Operation descend from RootObject
• Make Flow descend from RootObject

Development Tasks

• Include SignInFlow test

Currently, nil is allowed as the value for an argument. While that's good default behavior, we may sometimes want to prevent nil.

I'm envisioning something like this:

argument :definitely_need, allow_nil: false
argument :might_need

Flow.new definitely_need: "asdf", might_need: "asdf"
=> good

Flow.new definitely_need: "asdf", might_need: nil
=> also good

Flow.new definitely_need: nil, might_need: nil
=> error!

(😄)

For real though, the reasoning is that I'd rather the state tell me it's invalid, than run into NoMethodError's

Currently, nil is not allowed as the value for an argument. While that's good default behavior, we may sometimes want to allow nil. option doesn't cover it either, if we do want the kwarg itself to be mandatory.

I'm envisioning something like this:

argument :definitely_need
argument :might_need, allow_nil: true

Flow.new definitely_need: "asdf", might_need: "asdf"
=> good

Flow.new definitely_need: "asdf", might_need: nil
=> also good

Flow.new definitely_need: "asdf"
=> error!

I've just faced with one flow test-case which I think we could improve
basically I've named method behaviors instead of behavior by accident
and my specs were not passing because behaviors was never called

I think we might want to raise error to let developer know that operation does nothing.

So I think I have an idea for the consolidation of the disaster of complexity that is flow failures.

Generally speaking, I feel a lot of the things reported as problems here are actually "cost of doing business". A flow with an invalid state 🤓 technically speaking 🤓 doesn't have a failure? because nothing went wrong because nothing happened because things didn't get that far.

How you, the developer, chose to handle this is ultimately your decision. Given the complexity of application development, there will be times where you will want to handle the nuance of a flow failure.

Practically, given no alternative, my approach here is best described as "path of least resistance", which is instead of trying to come up with some state_valid? || failed? || etc.., I just flip my normal innate pessimism and go "happy path" and start with success.

1. If a flow was successful, then nothing bad happened.
2. If it wasn't successful, maybe it has a failure. If so, that's what went wrong.
3. If there isn't a failure, that's probably cause you have an invalid state.

And the list of things that can go wrong is incomplete. There's also a "malformed state" problem that isn't even soluble at the time of this writing.

This, granted, sucks. I just couldn't think of a better more compelling approach.

UNTIL TODAY!

Submitted for the approval of the midnight society, I present the "Malfunction".

flow = MyGreatFlow.trigger(input: yes)

flow.malfunction? # => true 😱 

malfunction = flow.malfunction
malfunction.problem # => :incompatible_state / :invalid_state / op_failure problem
malfunction.context # => incongruities / validation errors / op_failure details

The goal is to have this problem be a symbol key representing what went wrong and the context being a hash-like object of data salient to the malfunction.

Under the hood, I'm expecting this to be 3 different classes that all inherit from the same malfunction root, ex:

class CompatibilityMalfunction < Flow::MalfunctionBase; end
class ValidationMalfunction < Flow::MalfunctionBase; end
class OperationMalfunction < Flow::MalfunctionBase; end

This would then empower a much cleaner pattern for processing error states in flow, ex:

case flow.malfunction
when CompatibilityMalfunction
  blame_the_developer_this_will_never_work
when ValidationMalfunction
  blame_the_user_its_their_input
when OperationMalfunction
  blame_the_robots
else
  abandon_all_hope
end

So yea, thoughts??

I'm exciting about it { is_expected.to be_malfunction } :P

There's a dangling it "does something" in AppOperation generator spec that should be removed.

Development Tasks

• Remove direct state access deprecation warning
• Require allow_direct_state_access for operations to access state

1. Put up a PR that introduces that allow_direct_state_access with a default of true and a baked in deprecation warning that this will soon be default false unless you explicitly call it.
2. Release a version of the gem.
3. Put up a PR that changes the default to false which is a breaking change fulfilling (and removing) that deprecation warning.
4. Release a version of that gem.
5. Put up this PR which finally makes use of allow_direct_state_access but adds a deprecation warning to it being called.
6. Release a version of that gem.
7. Put up a final PR (later in the future) which drops support for the allow_direct_state_access concept.

consider following code example

class AddStripePaymentSource < ApplicationOperation

  handle_error Stripe::InvalidRequestError
  handle_error Stripe::AuthenticationError
  handle_error Stripe::APIConnectionError
  handle_error Stripe::APIError
  handle_error Stripe::PermissionError
  handle_error Stripe::RateLimitError
  handle_error Stripe::SignatureVerificationError

Readme section for Contributing/Development suggest using bin/console for experimenting. But most examples are not available from console, because
ApplicationState, ApplicationFlow, ApplicationOperation are not defined.

possible fix could be adding the:

class ApplicationState < Flow::StateBase; end
class ApplicationFlow < Flow::FlowBase; end
class ApplicationOperation < Flow::OperationBase; end

to the bin/console script.
but it feels like there should be a better alternative.

Once #61 is done and we have a way to fetch what the attributes a state ought to have, it should be possible to create a one-stop-shop generator for a flow from existing operations. Consider:

$ rails generate flow:for_operations Example Operation1, Operation2, Operation3

And this used the heart of #61 to get the expected shape of the state and uses it to draw it out in the generator, ex:

class ExampleFlow < ApplicationFlow
  operations Operation1, Operation2, Operation3
end

class ExampleState < ApplicationState
  def foo
    # reader for Operation1
    # accessor for Operation2
  end

  def bar
    # writer for Operation1
    # reader for Operation2
  end

  def baz
    # writer for Operation3
  end

  def gaz
    # writer for Operation3
  end
end

This should hopefully go a lonnnnnng way to bridging the gap for what ought to go into a state I think!

Development Tasks

• Introduce the StateProxy class for operations #128
• Make allow_direct_state_access ignore the state proxy and maintain behavior
• Add a deprecation warning for use of allow_direct_state_access

I've been campaigning heavily for the adoption of the following pattern offline, so it probably belongs here too for discussion:

class SomeOperation < ApplicationOperation
  ...
  def behavior
    some_model.save

    invalid_something_failure!(errors: some_record.errors) unless some_model.valid?
end

Conceptually the idea is such - in the case that an invalid record is the reason for an operation failure (which is often), we should have a consistent and predictable place to keep the offending errors and/or record. There's been some conversation about whether this should be either _failure(errors: record.errors) or _failure(invalid_record: record - and I'm in the camp of the former, for 2 reasons:

1. it doesn't matter anyway, since both objects have references to each other
2. errors just feels cleaner than invalid_model

I do admit, however, that reserving the errors namespace in a failure instance for specifically only activemodel errors is maybe a little draconian - nonetheless, we plan on adopting this pattern as a team in the short term for the sake of some consistency.

As a longer-term play, it might be nice to have a subclass of OperationFailure called InvalidModelFailure which required a model:/record: (to keep parity with ActiveRecord::RecordInvalid) parameter.

Or maybe we could even take it a step further, with a save_or_fail kinda thing:

save_or_fail!(record, :some_failure_type)

this would, upon failure, fail with an instance of the above InvalidModelFailures or something like it, tying all the threads in a neat little bow.

....off to the races!

Per discussion yesterday, it turns out I've been writing flows wrong the entire time.

How embarrassing!

Need to update the documentation to be a little more transparent as to the role flow plays and the proper way to use it.

The big miss is encapsulating blocks of work within the command pattern.

Encompass the gist that states which do not take input are a code smell.

Leverage jobs for the recommended way to encapsulate a flow and be where query meets state and batches occur.

failure_explanation = flow.failed_operation&.operation_failure || flow.state.errors

😢

It's 2 entirely different pathways to get to the object representing why the flow failed.

For one, Flow needs a #failure method, but I'm thinking invalid state should inherently trigger a "failure" (even a StateFailure), that's at least shaped like an operation failure - so that the above code is just

failure_explanation = flow.failure

 
If anything it'll make it easier to pursue resolving things like #88

Can we add how to test failures to the readme? I'm not sure if this is the correct way - but here's what I have..
`context "when the response is not successful" do
let(:response) { error_response }

      it "fails with braintree_request" do
        expect{execute.call}.to raise_error(Flow::Operation::Failures::OperationFailure)
        expect(customer).to have_received(:create).with(email: email)
      end
    end

`

Just a reminder to do the spec changes you outlined in #90, they sound awesome @jkminneti !

Development Tasks

• Move errors up from explicit files into the root Flow namespace

Allow syntax alike to following:

class SomeState < ApplicationState
  argument :whack_a_mole, default: "No fun allowed!"
end

Probably involves some keyword argument magic in two places: argument method definition and define_attribute method definition

Development Tasks

• Add .allow_direct_state_access to operations as an inheritable flag
• Create a deprecation warning for states which do not call allow_direct_state_access
• Add new default to generators
• Update specs to ensure most things are using this
• Update README

it { is_expected.to read_state :foo }
it { is_expected.to write_state :bar }
it { is_expected.to access_state :baz }

Also, add description of use to the README

it { is_expected.to wrap_in_transaction } is not a thing 😟

make a matcher called raise_operation_failure or something that does this under the hood:

it { is_expected.to raise_error(Operation::Failures::OperationFailure) { |error| expect(error.problem).to eq :stripe_response_no_id } }

Once @arettberg gets the state_* stuff hooked up (as chattered about in #50) we should be able to super easily add a pre-validation step in the flow which sanity checks that the state has the right "shape" to it.

ex:

class ExampleFlow < ApplicationFlow
  operations Operation1, Operation2, Operation3
end

class ExampleState < ApplicationState
  # nothing defined right now
end

class Operation1 < ApplicationOperation
  state_reader :foo
  state_writer :bar
end

class Operation2 < ApplicationOperation
  state_reader :bar
  state_accessor :foo
end

class Operation3 < ApplicationOperation
  state_writer :baz
  state_writer :gaz
end

ExampleFlow.trigger
# => raises Flows::Errors::MalformedState, "Missing attributes: foo, bar, baz, gaz"