13 May 2009

before_save callback be careful accidentally do not return false

Following code has been tested with Rails 2.3.2

You have an app with User model. The model has only two columns: salary (integer) , eligible_for_loan(boolean).

The business logic is that if user is earning more than $150,000 then user is not eligible for bonus. You might code like this.

class User < ActiveRecord::Base
  
  before_save :set_eligible_for_loan_status
  
  private
  
  def set_eligible_for_loan_status
    if self.salary > 150000
      self.eligible_for_loan = false
    else
      self.eligible_for_loan = true      
    end
  end
  
end

Do you see anything wrong with the above code?

> sc
Loading development environment (Rails 2.3.2)
>> User.create(:salary => 32000)
  User Create (0.4ms)   INSERT INTO "users" ("updated_at", "salary", "eligible_for_loan", "created_at") VALUES('2009-05-13 18:16:53', 32000, 't', '2009-05-13 18:16:53')
=> #<User id: 2, salary: 32000, eligible_for_loan: true, created_at: "2009-05-13 18:16:53", updated_at: "2009-05-13 18:16:53">
>> User.create(:salary => 232000)
=> #<User id: nil, salary: 232000, eligible_for_loan: false, created_at: nil, updated_at: nil> 

Notice that no user record was created for the user earning $232000. However use record was created for the user earning $32000.

The thing is that looking at the errors does not help either. Errors is empty.

>> u = User.create(:salary => 232000)
=> #<User id: nil, salary: 232000, eligible_for_loan: false, created_at: nil, updated_at: nil>
>> u.errors.full_messages

Explanation

ActiveRecord provides before_save callback which is immensely useful. As per the Rails guide there are two ways to stop the operation from before_save.

One way is to explicitly return false. If you return nil then the operation will continue to proceed. To stop the operation you must return false. In this case returning false is not the same as returning true.

The other way to stop the operation it to raise an exception.

In ruby any operation must return something. Check out in irb .

> irb
>> foo = 'ruby on rails'
=> "ruby on rails"
>> foo = 5
=> 5

In the first case foo was assigned a value of ‘ruby on rails’ and the operation returned a string with the value of ‘ruby on rails’. When foo is assigned a value of 5 then the operation returns a value of 5.

Guess what will the operation return if I assign false to foo.

>> foo = false
=> false

Back to original problem

The original code was

class User < ActiveRecord::Base
  
  before_save :set_eligible_for_loan_status
  
  private
  
  def set_eligible_for_loan_status
    if self.salary > 150000
      self.eligible_for_loan = false
    else
      self.eligible_for_loan = true      
    end
  end
  
end

In the above case when the salary is greater than 150000 then expression self.eligible_for_loan = false is evaluated. The return value of this expression is false as it was discussed earlier.

Notice that the evaluation of expression self.eligible_for_loan = false also happens to be the last expression in that method. The end result is that the method set_eligible_for_loan_status returns a value of false if the salary is greater then 150000.

As it was discussed when false is returned from a before_save callback then the whole transaction is aborted.

Solution

The solution is to not to return false. Adding nil at the very end of the method is one way to fix this problem.

def set_eligible_for_loan_status
  if self.salary > 150000
    self.eligible_for_loan = false
  else
    self.eligible_for_loan = true      
  end
  nil
end