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 commentsrails

Comments
  1. Łukasz Piestrzeniewicz2006-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 Susser2006-03-16 13:25:59

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

  3. Jon Bauman2006-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 Susser2006-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. Jon2006-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 felkins2006-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 Susser2006-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. Kelly2006-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.