DRYing up parameter checks

— March 15, 2006 at 10:46 PST


Here is a very simple way to check that the parameters coming into an action are useful. Remember, it's important to guard against both coding errors and people constructing bogus URLs.

class RecipeController < ApplicationController
  def recipe
    return bogus("Unknown Recipe") unless @recipe = Recipe.find_by_id(params[:id])
  end
  def bogus(message)
    flash[:warning] = message
    redirect_to :action => "home" 
  end
end

The cool thing is that find_by_id(params[:id]) returns nil if there is no id param, or if there is one but it is invalid. I've been putting this one-liner in all my actions and now I feel much safer.

8 comments — rails

Comments
  1. Łukasz Piestrzeniewicz – 2006-03-16 13:09:46

    Wouldn't it be better to render a 404 page instead of redirecting?

    Something among the lines of:

    def bogus(message)
  flash[:warning] = message
  render :action => "not_found", :status => 404
end
  2. Josh Susser – 2006-03-16 13:25:59

    @ Łukasz: Yes, that's an option to the redirect. Just depends on the user experience you want.

  3. Jon Bauman – 2006-04-10 21:44:49

    Wouldn't it be easier to do this as a before filter? Something like this (not tested):

    class FooController < ApplicationController
  before_filter :bogosity_check

  def bogosity_check
    unless instance_variable_set(params[:action].to_sym, self.class.find_by_id(params[:id]))
      return bogus("Unknown #{params[:action].humanize}")
    end
  end
end
  4. Josh Susser – 2006-04-10 21:55:03

    @Jon: First off, thanks for using the word bogosity in your code :-)

    My one-liner bogosity check is flexible enough to deal with any kind of parameter to an action. It seems like your filter approach would be limited to a certain set of parameter checks, so if an action needs to deal with a different kind of parameter scheme your filter wouldn't work.

  5. Jon – 2006-04-12 09:51:40

    @josh:

    It's true that this check woludn't handle all cases, I just envisioned the same one-liner being written many times. It would be simple to combine these with :only or :except parameters to the before filter.

  6. kelly felkins – 2006-04-17 08:48:04

    This may be controversial, but what are you trying to accomplish here? Provide a nice UI for someone constructing URLs? I edit URLs all the time and love to do it and love people that do it -- but I don't think the application must provide a nice UI for invalid parameters. I think you can generate a lot of code trying to protect against this stuff and the code is fragile and the bloat slows down development.

    In my opinion, the models should protect against invalid or unapproved access by throwing acceptions. The controllers and views should guide people to appropriate access. If someone edits the url and specifies an ID that is invalid or inappropriate, then a stack trace is fine....

    I have a feeling I will regret saying this publicly.. ;-)

    -Kelly

    railsinator at gee mail

  7. Josh Susser – 2006-04-17 09:04:29

    @kelly: nothing wrong with being controversial here :-) What I'm trying to accomplish is prevent the user from seeing a stack trace or WSOD. A user wouldn't have to hack a URL manually to get a bogus id - just returning to a bookmark for an item that is later deleted would do it. Since the controller is responsible for deciding what to do in response to a URL, I put the check there. My goal is that the user not think my app is broken just because he used an old bookmark.

    ActiveRecord already protects against invalid IDs. find() will either throw an exception or return nil or [] depending on how it's called, but that doesn't automatically provide a nice user experience.

  8. Kelly – 2006-05-18 00:33:09

    My goal is that the user not think my app is broken just because he used an old bookmark.

    I see your point. Thanks.

    -Kelly

Sorry, comments for this article are closed.