← Back to Upcase

Callbacks


(Upcase ) #1
Ben and Joe discuss the dos and (mostly) don'ts when it comes to ActiveRecord callbacks. You'll find out why you're generally better off not using this feature, with several suggestions for better alternatives.
This is a companion discussion topic for the original entry at https://thoughtbot.com/upcase/videos/callbacks

(Crispin Cornett) #2

What about observers? I haven’t personally used them, but would put them in the same category as callbacks?

Seems like they hook into the object the same way callbacks do, so would be subject to the same failings/dangers?

Also, what do you all think about a pub/sub type architecture, something like the wisper gem?

I haven’t personally used either of these methods, and have gone the route of something similar to what Ben & Joe did in the video. However, I have come across these other strategies and would love to get other people’s opinions on them.


(Alex Bush) #3

@jferris @benorenstein I agree in general on callbacks being evil(most of the times) but consider this example:

We have a model NewsSource which represents a blog with corresponding info like author email, site_url, title, etc.

Every news source also has a rss feed. The feed could be atom, rss, or whatever type.

There is a parser in the app that fetches feeds periodically and parses the posts.

Initially we kept feed_url value in news_source itself but after constructing parsers certain way we decided to extract feed data out to have some validations and other checks.

So at the end we have two very tightly coupled models:

class NewsSource
   has_one :feed
end

class Feed
   belongs_to :news_source
end

Every time we create or update a news_source we need create or update corresponding nested/child object feed and this is where before_create :generate_feed and before_update :update_feed callbacks came in handy.

Ideally we should’ve used accepts_nested_attributes_for but in our case the client apps(namely our Ember.js admin panel) doesn’t send nested objects to our API endpoints so we had to wrap it this way.

What do you think, is this one of those “exception” cases when it’s ok to use callbacks or we can somehow improve it?

Thanks.


(Frank West) #4

What perfect timing for this episode. I was faced with a situation where I had to shutdown an application another dev programmed that I did not understand on Wednesday. Long story short was that it is an app that added numbers into our inventory system via an API. It ran into a situation where a specific attribute was changed and there was a before_save callback. Details omitted for brevity, the callback created a transaction issue, but the API call was already made. 24+ hours later before it was noticed…, blah. I wish it was as simple as reversing the inventory, but as of today, the numbers are still not correct after several attempts.

I rewrote that part of the application and extracted a new model and it is much more explicit as to what is going on now.


(Joe Ferris) #5

This is actually one of those situations I find particularly confusing with callbacks. I can never remember which model will be saved when, or what ActiveRecord does with unsaved associations, validations, and foreign keys. I prefer to tell ActiveRecord to save single models, and handle larger transactions in their own object.

If you’re doing this from a form, making an object which encapsulates the form and all the objects it updates is nice. You can delegate and combine associations, and then clearly and concisely save all the objects in one transaction.

If you’re doing it in a backend service, you can do the same thing, except you don’t really have to worry about validations or messages. Any failures can be handled by exceptions (usually raised by save!).


(Joe Ferris) #6

ActiveRecord’s observers have never really received first-class treatment in Rails, and they were removed from Rails itself in 4.0.

I haven’t used Wisper, but I’ve had good luck with similar architectures. There are some common things which make observer/pubsub/events different than callbacks. The ActiveRecord callback API is a way to hook into the lifecycle of an object, modifying its behavior. For example, you can add new validation errors and perform other side effects which actually change the return value of a save call. Events are best when used exclusively for side effects. For example, triggering emails or updating remote service after a save is successful is a good use for an observer or event.

In a way, using observers or something like Wisper is going to the extreme, OOP end of the “Tell, Don’t Ask” philosophy: everything is phrased as a command, and dependencies are managed through events rather than composition. Besides dependency injection, events are the only way I know of to implement Inversion of Control.


(Samnang Chhun) #7

I tried to avoid callbacks, then I create a service class to handle logics after model created, but I still feel not happy about it.

# comments_controller.rb
def create
    post    = find_post
    comment = post.comments.new(protected_params)
    if comment.valid?
      CreateComment.new(current_user, comment).create
      render_created(comment)
    else
      render_bad_request(comment)
    end
end

# create_comment.rb
class CreateComment
  def initialize(user, comment)
    @user = user
    @comment = comment
  end

  def create
    save_comment
    subscribe_to_post
    update_rank_score
    create_activity
  end

  private

  def save_comment
    @comment.save!
  end

  def subscribe_to_post
    @comment.post.add_subscriber(@user)
  end

  def create_activity
    CreateActivity.create(user: @user, action: 'new_comment', trackable: @comment)
  end

  def update_rank_score
    UpdateRankScore.update(@comment.post)
  end
end

I’m not sure, but I feel CreateComment its job doing too much, and it makes hard to tests each functionality, and my test feel very tight couple to implementation:

require 'spec_helper'

describe CreateComment, '#create' do
  let(:user) { double(:user) }
  let(:post) { double(:post, add_subscriber: true) }
  let(:comment) { double(:comment, post: post, post_id: 1, save!: true) }

  before do
    UpdateRankScore.stub(:update)
    CreateActivity.stub(:create)
  end

  it 'creates comment' do
    create_comment(user, comment)

    expect(comment).to have_received(:save!)
  end

  it 'subscribes to the post' do
    create_comment(user, comment)

    expect(post).to have_received(:add_subscriber).with(user)
  end

  it 'updates post rank score' do
    create_comment(user, comment)

    expect(UpdateRankScore).to have_received(:update).with(comment.post)
  end

  it 'tracks activity' do
    create_comment(user, comment)

    expect(CreateActivity).to have_received(:create).with(
      user: user,
      action: 'new_comment',
      trackable: comment
    )
  end

  def create_comment(user, comment)
    CreateComment.new(user, comment).create
  end
end

(Aaron Mc Adam) #8

@samnang I think this is a case where multiple event listeners or subscribers would be a good fit. You’d fire an event/publish a topic if the save is successful. Then your spec only needs to test that the event gets fired, or you could write a slightly higher level spec that tests the behaviour from the integration between CreateComment and its related listeners


(Dan Croak) #9

In this episode, a decorator is used to avoid a callback:

class Comment < ActiveRecord::Base
end

class FacebookNotifyingComment < SimpleDelegator
  def initialize(comment)
    super(comment)
  end

  def save
    if __getobj__.save
      post_to_wall
    end
  end

  private

  def post_to_wall
    Facebook.post(title: title, user: author)
  end
end

class CommentsController < ApplicationController
  def create
    @comment = FacebookNotifyingComment.net(Comment.new(params[:comment]))

    if @comment.save
      redirect_to blog_path, notice: "Your comment was posted."
    else
      render "new"
    end
  end
end

Why is the decorator advantageous over inlining the Facebook notification within the controller?

class Comment < ActiveRecord::Base
end

class CommentsController < ApplicationController
  def create
    @comment = Comment.new(params[:comment])

    if @comment.save
      Facebook.post(title: @comment.title, user: @comment.author)
      redirect_to blog_path, notice: "Your comment was posted."
    else
      render "new"
    end
  end
end

(Charushin Roman) #10

I think that FacebookNotifyingComment is extracted in the name of Single Responsibility Principle. With that principle in mind you create thin one-task-focused classes which are easily testable, reusable, simple etc…

Thin focused controller does two things (not including authentication stuff):

  • Translates HTTP request to your application’s language, sending a message to one of your app’s classes.
  • Translates response from that message to HTML or whatever format you’re using.

On the other hand you get 15 lines in your FacebookNotifyingComment instead of single simple Facebook.post in your controller. In the latter case controller is still pretty thin, you can quickly figure out what is going on and test it with Facebook stabbed out. But now controller is a part of your app, not a simple translator.

At the end of the day it’s all about design, balance and consistency.


(Dan Croak) #11

@lompy Got it, good points. I appreciate the decorator more with that knowledge.

I’ve seen this style of decorator (“decorator”?) implementation recently in a real-world app:

class Comment < ActiveRecord::Base
end

class FacebookNotifyingComment < SimpleDelegator
  def initialize(params)
    super(Comment.new(params))
  end

  def save
    if __getobj__.save
      post_to_wall
    end
  end

  private

  def post_to_wall
    Facebook.post(title: title, user: author)
  end
end

class CommentsController < ApplicationController
  def create
    @comment = FacebookNotifyingComment.new(params[:comment])

    if @comment.save
      redirect_to blog_path, notice: "Your comment was posted."
    else
      render "new"
    end
  end
end

What do people think of that versus wrapping the comment object like a classic decorator?


(Aaron Mc Adam) #12

The Draper gem allows you do something similar. If you include the decorates_finders macro, you can do something like @article = ArticleDecorator.find(params[:id]).

I think maybe creating a builder/factory might be a better option than adding Comment within the FacebookNotifyingComment initializer. It would keep the comment Role dependency abstract rather than concrete within FacebookNotifyingComment


(Mark Joel Chavez) #13

Very awesome screencast about callbacks!

One question though, will it be much better to always approach every callback problem using
a decorator or is there a scenario wherein a decorator/delegator is not applicable?

P.S Very cool vim theme. May I know what it is?


(Himshwet Gaurav) #14

I have a Rails app with a model AssessmentForm which has many AssessmentItems. Whenever an AssessmentItem is updated I have to touch its AssessmentForm model. I have implemented it using callbacks. @jferris would you consider this as an acceptable case for using callbacks?

As per your discussion you pointed out that callbacks make it difficult to isolate two actions.In my use case if we consider the domain model it seems logical to me that every time an AssessmentItem changes the AssessmentForm has changed so I don’t foresee a need to isolate the two actions(updating an AssessmentItem and changing the updated_at attribute of the AssessmentForm). What pitfalls do you see in using callbacks in this case?