Make Touch plugin run after_commit on touched associations #566

Closed
djanowski opened this Issue Oct 8, 2012 · 2 comments

Comments

Projects
None yet
2 participants
@djanowski
Contributor

djanowski commented Oct 8, 2012

My use case is: model A has many models B. Whenever B touches A, I need the after_commit in A to be run so that I can update a key in Redis.

@jeremyevans if you think this is a good idea I can prepare a patch (it's really simple).

@jeremyevans

This comment has been minimized.

Show comment Hide comment
@jeremyevans

jeremyevans Oct 8, 2012

Owner

The reason it doesn't call after_commit in A already is that it modifies the A instance using the underlying dataset. This is mostly for performance reasons for *_to_many associations, since it is faster than saving each individually. The touch plugin is designed for updating timestamps, not for more general case.

I don't think we should add special support for after_commit in the touch plugin, as it is too specific, and may not cover all cases (e.g. adding a new B instance). However, I'd consider changes to the touch plugin to just update the associated model instance instead of using the dataset for *_to_one associations, or an option to choose whether to use the dataset-level update for speed or the model-level update for situations like yours.

Another way to handle your use case is to add an after_commit hook to B that calls after_commit for A:

class B
  def after_commit
     super
     a.after_commit
  end
end
Owner

jeremyevans commented Oct 8, 2012

The reason it doesn't call after_commit in A already is that it modifies the A instance using the underlying dataset. This is mostly for performance reasons for *_to_many associations, since it is faster than saving each individually. The touch plugin is designed for updating timestamps, not for more general case.

I don't think we should add special support for after_commit in the touch plugin, as it is too specific, and may not cover all cases (e.g. adding a new B instance). However, I'd consider changes to the touch plugin to just update the associated model instance instead of using the dataset for *_to_one associations, or an option to choose whether to use the dataset-level update for speed or the model-level update for situations like yours.

Another way to handle your use case is to add an after_commit hook to B that calls after_commit for A:

class B
  def after_commit
     super
     a.after_commit
  end
end
@djanowski

This comment has been minimized.

Show comment Hide comment
@djanowski

djanowski Oct 8, 2012

Contributor

I realized it's using the dataset directly and I assumed this was for performance reasons.

To be honest, your proposed solution is even simpler than my monkeypatched version of Touch and should get the job done equally well.

I'll close this for now, thanks for your feedback!

Contributor

djanowski commented Oct 8, 2012

I realized it's using the dataset directly and I assumed this was for performance reasons.

To be honest, your proposed solution is even simpler than my monkeypatched version of Touch and should get the job done equally well.

I'll close this for now, thanks for your feedback!

@djanowski djanowski closed this Oct 8, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment