Conditional callbacks

[jwoertink]

With the new addition of #481 we can now skip some callbacks. However, this implementation does have a drawback where it redefines the callback method to do nothing which means that if the skip callback is defined before the actual callback, then it won't actually be skipped.

class SaveUser < User::SaveOperation
  before_save :set_thing
  skip_before_save :set_thing

  private def set_thing
    # This is still executed
  end
end

# you'd have to do something funky like
class SaveUser < User::SaveOperation
  before_save :set_thing

  private def set_thing
    # This is still executed
  end

  # this would redefine the method later allowing you to actually skip it
  skip_before_save :set_thing
end

Now, I don't think anyone would actually write code like either of these examples, but if we had conditional callbacks they might. Imagine something like this:

class SaveUser < User::SaveOperation
  before_save :set_thing
  skip_before_save :set_thing, if: ->(user) { user.admin? }

  private def set_thing
    # This is still executed
  end
end

In this case, we may need to think about a system to register callbacks at the instance level. I kinda like what @matthewmcgarvey suggested here.

Now, I could have sworn we had some sort of conditional on the before_save before like where you could say before_save :whatever, on: :create... but I couldn't find it. I wanted to see if there was a reason we removed it so if anyone remembers if we had that before, let me know.

Another idea I just had, which may be a bit a crazy, is to change the callbacks to be segmented in to a more type-safe way... Just thinking out loud here, but what about this?

class SaveUser < User::SaveOperation
  before_save UserSetThingCallback.new

end

class UserSetThingCallback
  include Avram::CallbackBeforePipe

  def call
    # runs the before save
  end

  def skip_call(user)
    # if this is defined
    # and returns true, it would skip running the call.
    # you could just have it return `true` to always skip...
  end
end

module Avram::CallbackBeforePipe
  macro included
     abstract def call
     # whatever else
     
  end
end

Obviously a "rough" sketch, and would be another major refactor, but worth a shot possibly...

[jwoertink]

Found where those were removed b684ad3#diff-4b25d19af16746ff3a5cdb80fbd5f3a228d0385a7a197fd42d3a6bb4e0da6622R117

So we had before_create and before_update. There has been some talk on making a SaveOperation only run for a create or an update. I think that might cover this portion.

[paulcsmith]

I think for the , if: -> this is not necessary since you can use a regular if in the callback method. IMO that's just extra DSL that Crystal already has covered.

For example:

unless responds_to?(:user) && user.admin?
  # do something
end

For skipping in a type-safe way

I think we could do something similar to what was done for Actions https://github.com/luckyframework/lucky/blob/dd06b41926da1a162732c4c0d3ff83c22018543e/src/lucky/action_pipes.cr#L11-L23

It goes through an array of callbacks and raises if you try to skip a callback that doesn't exist. This means we may need to create an array to store a list of callbacks. I don't think we have that yet

I don't mind that the method is simply redefined to "skip" a callback. I think if you do your examples that's just something you shouldn't do haha. I'd also bet most people won't do it. But we can always change it if people run into it. IMO though it's not worth the work since I think most people will not do that. We should document the edge case in the guides and say "if you do this it will redefine the callback, just FYI"

Nervous about skipping callbacks

This could be a way to spaghetti things up if people aren't careful. For example, people might add a bunch of callbacks to a base class and then skip them in operations, when really it'd be clearer to leave them out of the base class and add them just to operations that need them.

I think skipping callbacks is fine when used sparingly, but we just need to make sure we document this when documenting "skipping callbacks". Something like, "prefer putting callbacks in the operations that need them, rather than in a base class. Reserve callbacks in base classes for things that should almost always run for all operations. Otherwise it can lead to a lot of confusion about what callbacks are actually running when you save an operation"

[jwoertink]

Yeah, I wasn't really a fan of this syntax if: ->(user) { user.admin? }, so just adding code in that method does seem a lot nicer.

class SaveUser < User::SaveOperation
  before_save :set_thing

  private def set_thing
    unless responds_to?(:user) && user.admin?
       # skips this callback
    end
  end
end

And I guess you could just override the method in a subclass if you need.

My original plan with the skipping was based on if you wanted to have create_only and update_only operations, then you might do something like:

class SaveUser < User::SaveOperation
end

class CreateUser < SaveUser
  permit_columns email, username
end

class UpdateUser < SaveUser
  # Assuming this is something SaveUser had set
  skip_before_save :validate_required
end

Maybe I should remove those skips for now, and then we can work out some specs on how to handle a skip in this sort of manner where we just override some method where needed?

[stephendolan]

I actually really like being able to define short guard clauses with callbacks. It helps me to avoid jumping around in a file for a long time just to figure out where my object is ending up.

Say I have an order processing operation:

class ProcessOrder < Order::SaveOperation
  before_save :normalize_card_number, if: :paid_with_card?
  before_save :determine_tax_strategy, if: :user_is_outside_united_states?
  before_save :validate_shipping_address, if: :order_requires_fulfillment?

  private def paid_with_card?
    order.payment_method == "card"
  end

  private def user_is_outside_united_states?
    order.payee.country_code == "US"
  end

  private def order_requires_fulfillment?
    order.contains_physical_goods?
  end
end

I prefer being able to essentially ignore all of those private methods, since I'm able to determine exactly what will prevent each of those callbacks from firing based on the method name.

If I'm debugging an issue with a PayPal order that contains digital goods, I can safely skip thinking about validate_shipping_address and normalize_card_number just from the before_save definition.

I remember finding this concept really intuitive when I ran into it in Rails.

I also think that using a symbol will make it a bit easier to read through than using a Proc. From Paul in Discord, it also sounds like adding an if {{if_method}} in the macro would be easier to implement, perhaps.

[jwoertink]

Ok, doing that might actually be better than having skip_... now that I see all this. Because this means you could do

class SaveUser < User::SaveOperation
  before_save :validate_required_for_new_people, if: :new_user?

  def new_user?
    record.try &.new_record?
  end
end

class CreateUser < SaveUser
  permit_columns email, username

  def new_user?
    true
  end
end

class UpdateUser < SaveUser
  def new_user?
    false
  end
end

This allows you to skip the before save where needed without adding in a new macro. If y'all are cool with it, then I'll remove the skips and add in this instead.

[paulcsmith]

I think that's a good idea. We can always add it back later if we need it and as we understand use cases better. For now the if: and unless: should get us pretty far I think!