Validation Anti-Pattern
Validations in ActiveRecord and other mapping frameworks are used to ensure that the data being saved to the database meets the requirements of the app. More importantly, validations are used to communicate to the end user what they need to change about the data they entered.
I often see people using validations to check generated attributes or an attribute set by the controller, and I think that is wrong. Validations should only be used if you to provide your user an opportunity to correct the invalid data.
For example, take this MongoMapper model:
class ChatMessage include MongoMapper::Document key :text, String key :game_id, ObjectId belongs_to :game validates_presence_of :text, :game_id end
This model is created in an action that looks something like this:
def create @game = current_user.games.find(params[:game_id]) @chat_message = @game.chat_messages.build(params[:chat_message]) if @chat_message.save redirect_to @game else render :edit end end
The model is validating the presence of text, a value supplied by the user, and game_id. The only way that game_id can be blank is if there is something wrong the your controller logic. In that unlikely case, your user will be presented with the completely unhelpful error: Game cannot be blank. game_id should not be a validation that is shown to your user.
But I don’t want to save invalid data!
That’s fine. You can still use the validation callback as an opportunity to verify the data. Add an assertion that the attribute is set. Just don’t add validation errors that get displayed to your user, because no matter what they do, they won’t be able to fix the error that they are seeing. It is more helpful to raise an exception, which will show your user the standard 500 error, and notify you that you’ve got a problem with your application logic.
validate :validate_game_id def validate_game_id raise "game_id must be present" unless self.game_id? end
But what about people hacking?
Do you really want to return friendly error messages to someone trying to hack your app? When trying to secure your app, you want to expose as little information as possible to potential attackers. That’s why it’s even more important to raise errors when validating internal data.
Return friendly error messages for values supplied by your user, and raise big ugly errors when things go wrong.
3 Comments
Hmm.. valid point. Actually, I’ve noticed myself testing for the methaphorical game_id in the controller once or twice.. this would be much better.
I like the fail-fast nature of your strategy. I like the idea of validations only for user corrections and exceptions for everything else. Often in background processing I’ll use save! just to fail-fast if there are any problems, with this strategy save/save! doesn’t matter.
Great point – this is the error handling strategy I used now. If there’s a potential error that you don’t plan to handling right now, instead of doing a blind rescue or this anti-pattern, just fail fast and fix it if/when it happens (cause who knows, it might never actually happen)
Post a Comment