Thoughtbot folks have a great article on not expecting exceptions – save bang your head, active record will drive you mad. I’ll admit, just like the poster, I used to use save! in controllers to DRY my code. And have a global rescue_from in application.rb. But over the time, I changed the camp and now I’m fully in that “Don’t expect expectations” camp. Some things are more important that DRYing 3 lines of code.
But I’d want to take this a step further. When you’re not expecting something to fail, always use the methods that raise exceptions on failure.
So I strongly disagree with the poster on this :
I think ActiveRecord::Base#save! and ActiveRecord::Base.update_attributes! should be pulled from the public API
I would advocate just the opposite for certain cases. In many of the code reviews we’ve done via ActionRails, the following pattern was seen in many of the models :
1 2 3 4 5 6 7 8 |
def do_something self.foo = 'bar' save end def create_items names.each {|n| self.items.create :name => n } end |
In the snippets above, it’s not checking for cases where the save fails. And for good reasons that they’re not likely to fail as code is changing some very minor. But in these scenarios, a failure would be an unexpected situation. Hence you should always use save! or create!.
There could be easily be any unexpected reasons the above save could fail. And using save! protects you from those situations and help catch those minor programming mistakes early, which otherwise could prove to be very costly in terms of time/efforts. So the above code should really be :
1 2 3 4 5 6 7 8 |
def do_something self.foo = 'bar' save! end def create_items names.each {|n| self.items.create! :name => n } end |
However, if you’re using exceptions for flow control, this practise won’t always help you :
1 2 3 4 5 6 7 |
def create @user = User.create! params[:user] redirect_to @user rescue ActiveRecord::RecordNotSaved flash[:notice] = 'Unable to create user' render :new end |
As this catches the exception ActiveRecord::RecordNotSaved, unexpected save! failures from your model methods/callbacks will get caught too. And hide the real error.
Moral of the story :
- Don’t expect exceptions
- Use methods throwing exceptions when you’re not expecting a failure. For example, everywhere you’re not checking if save or create fails when working with Active Record objects, always use save! and create! instead.
Excellent blogging as of late – thank you.
Thanks!
I’m far from being a Rails guru, but always used save! for exactly this reason. With so many opinionated opinions floating around, it’s nice to at least once see your own practices validated.
Another rant masterpiece.
+1 verified.
Yeah, relying on Exceptions is mighty convenient until it occurs in a context you weren’t expecting. I’ve been burned by that bad. Even the relatively innocuous rendering of 404 for Model.find(id) can end up causing a mass of pain (what? that page has been broken for thousands of people for months? Why didn’t I get an exception notification? Ohhhhh….)
“When youre not expecting something to fail, always use the methods that raise exceptions on failure.”
Yes, yes, and yes. This is the real insight here. Waaay too many developers seem inclined to treat exceptions (or their potential) as unexceptional code. Unpunctuated Save is just more one way to bury the natural order of life, death, and exceptions in blind protective code.
Just testing !
Agree whole-heartedly.
This also causes transactions to rollback (without having to specify the conditional logic) when save! or update! throw exceptions.
In practice, if you want to ensure data integrity it might pay to break the “don’t expect exceptions” rule. ActiveRecord is good at validating simple cases like ‘presence of’ (= NOT NULL constraint on a database field); it is not reliable when validating trickier cases like ‘uniqueness of’, which a (decent) database management system is very good at handling. A given model can have ‘simple’ validations (presence-of, etc.), but should explicitly not include ‘tricky’ ones (uniqueness-of) – the latter should be handled by the database only (why get ActiveRecord to check when you can’t rely on it in such cases?).
So, a typical controller will not use exception handling for the ActiveRecord end of things, but will have to handle exceptions raised by the database management system. E.g.:
def create # A before_filter sets @person @friendship = @person.friendships.new(params[:friendship]) if @friendship.save # not save! – we are handling ActiveRecord errors via the IF-ELSE logic here flash[:notice] = ‘Friendship was successfully created.’ redirect_to @person else render :action => ‘new’ end end
Remember: in theory there is no difference between theory and practice, but in practice there is.
Aargh! Try that code again:
def create # A before_filter sets @person @friendship = @person.friendships.new(params[:friendship]) if @friendship.save # not save! – we are handling ActiveRecord errors via the IF-ELSE logic here flash[:notice] = ‘Friendship was successfully created.’ redirect_to @person else render :action => ‘new’ end end
Aaaaaarggghh!!!
Formatted code for comment above is here: http://pastie.textmate.org/658637
Thanks, this reminder just preserved my sanity in handling errors across model associations.