Software Development | Ruby on Rails
altered_beast Error Handling

When I was looking at the permalink_fu_plugin I noticed that altered_beast fails to catch some fairly basic errors. Requests for existing resources works fine but, for example, altered_beast doesn't cope with any of these URLs:

http://localhost:3000/forums/dodgy
http://localhost:3000/forums/dodgy.xml
http://localhost:3000/forums/realforum/topics/dodgy
http://localhost:3000/forums/realforum/topics/dodgy.xml
http://localhost:3000/forums/dodgyforum/topics/dodgy
http://localhost:3000/forums/dodgyforum/topics/dodgy.xml

In all cases "dodgy" means something that doesn't exist in the system.

Dodgy Forums

Hacking the URL to give a dodgy forum (e.g. http://localhost:3000/forums/dodgy) blows up on line 22 of the show action within controllers/forums_controller.rb. This is the line where Time.now.utc is stored in the session. But actually the problem is the line before where the call to find_by_permalink is made.

app/controllers/forums_controller.rb (snippet)

def show
  @forum = current_site.forums.find_by_permalink(params[:id])
  (session[:forums] ||= {})[@forum.id] = Time.now.utc
  (session[:forums_page] ||= Hash.new(1))[@forum.id] = current_page if current_page > 1

  respond_to do |format|
    format.html do # show.html.erb
      @topics = @forum.topics.paginate :page => current_page
    end
    format.xml { render :xml => @forum }
  end
end

Active Record will return a ActiveRecord::RecordNotFound exception when a finder driven by a primary key cannot find the row in question. In contrast when search criteria are used, for example find_by, then a nil is returned if the expected row doesn't exist. This is what is happening here. There is no permalink called "dodgy" to find so find_by_permalink returns nil.

Dodgy Topics

This url http://localhost:3000/forums/realforum/topics/dodgy will blow up on line 25 of controllers/topics_controller.rb. Not surprisingly the problem in in the show action. Basically it tries to evaluation @topic.hit! as nil.hit! and explodes with a no method found error.

app/controllers/topics_controller.rb (snippet)

def show
  respond_to do |format|
    format.html do
      if logged_in?
        current_user.seen!
        (session[:topics] ||= {})[@topic.id] = Time.now.utc
      end
      @topic.hit! unless logged_in? && @topic.user_id == current_user.id
      @posts = @topic.posts.paginate :page => current_page
      @post = Post.new
    end
    format.xml { render :xml => @topic }
  end
end

XML

altered_beast copes with requests for XML no better than for HTML when a request arrives for dodgy data. If you try either of these then rails will blow up.

http://localhost:3000/forums/dodgy.xml
http://localhost:3000/forums/realforum/topics/dodgy.xml

The latter example, with forums/realforum/topics/dodgy.xml, produces this error message:

Template is missing
Missing template topics/show.erb in view path app/views:vendor/plugins/brain_buster/views/brain_busters

Now that is an odd message and certainly not web service friendly.

Fixing Forum

This is what I think altered_beast should do for a show action on forum when the requested forum doesn't exist:

Log this request.
When returning HTML put an error message in the flash notice and redirect to the index action.
When returning XML return appropriate HTTP status code, in this case 404 not found.

To do this I deleted from forums_controller.rb all the lines that said:

app/controllers/forums_controller.rb (snippet)

  @forum = current_site.forums.find_by_permalink(params[:id])

This is because I also added a before filter at the beginning of forums_controller.rb:

before_filter :find_forum, :only => [:show, :edit, :update, :destroy]

And a protected find_forum method at the end.

protected
  def find_forum
    @forum = current_site.forums.find_by_permalink(params[:id])
    if @forum.nil? 
      logger.error("Attempt to access invalid forum #{params[:id]}")
      respond_to do |format|
        format.html do 
          flash[:notice] = "Invalid forum"
          redirect_to :action => 'index'
        end
        format.xml { render :nothing => true, :status => 404 } # Not Found
      end
    end 
  end

Using exceptions

So why did I use if @forum.nil? rather than an exception. You could argue that the only way to get this condition is by a user or machine hacking a URL and that warrants an exception. Following the pattern in Ruby et al (2009, p. 108-112) the find_forum method would look like this with an exception:

protected
  def find_forum
    @forum = current_site.forums.find_by_permalink(params[:id])
    raise ActiveRecord::RecordNotFound  if @forum.nil? 
  rescue ActiveRecord::RecordNotFound 
      logger.error("Attempt to access invalid forum #{params[:id]}")
      respond_to do |format|
        format.html do 
          flash[:notice] = "Invalid forum"
          redirect_to :action => 'index'
        end
        format.xml { render :nothing => true, :status => 404 } # Not Found
      end
    end 
  end

Alternative you could use rescue_from and move the exception handling code to a different part of the controller, in this case the method dodgy_id. You could even put that method in the ApplicationController as a generic solution to the problem.

rescue_from ActiveRecord::RecordNotFound, :with => :dodgy_id

protected
  def find_forum
    @forum = current_site.forums.find_by_permalink(params[:id])
    raise ActiveRecord::RecordNotFound  if @forum.nil? 
  end 

  def dodgy_id
      logger.error("Attempt to access invalid forum #{params[:id]}")
      respond_to do |format|
        format.html do 
          flash[:notice] = "Invalid forum"
          redirect_to :action => 'index'
        end
        format.xml { render :nothing => true, :status => 404 } # Not Found
      end
    end 
  end

I decided not to because ActiveRecord obviously doesn't view this scenario as an exception because it returns a nil if no record is found. By implication, ActiveRecord is encouraging the approach using the if statement. Latter I found these blog entries which, with their associated comments, support the approach I used:

Basically "exceptions should not be expected" so "don't use exceptions for program flow".

Fixing Topics

The fix to topics is fairly similar. topics_controller.rb already has the appropriate before_filter so we just have to change the protected methods at the end.

app/controllers/topics_controller.rb (snippet)

def find_forum
  @forum = current_site.forums.find_by_permalink(params[:forum_id])
  if @forum.nil? 
    logger.error("Attempt to access invalid forum #{params[:forum_id]}")
    respond_to do |format|
      format.html do 
        flash[:notice] = "Invalid forum"
        redirect_to '/forums'
      end
      format.xml { render :nothing => true, :status => 404 } # Not Found
    end
  end 
end

def find_topic
  @topic = @forum.topics.find_by_permalink(params[:id])
  if @topic.nil? 
    logger.error("Attempt to access invalid topic #{params[:id]}")
    respond_to do |format|
      format.html do 
        flash[:notice] = "Invalid topic"
        redirect_to :action => 'index'
      end
      format.xml { render :nothing => true, :status => 404 } # Not Found
    end
  end 
end

DRYing the code

You'll have noticed that I used the same pattern of code three times to fix the issues. This is not DRY.

TODO

DRY it up

Users

A separate but related issue is that the version of altered_beast I've got installed blows up on any link to a user. It comes up with URLs along the lines of

http://localhost:3000/users/2

References

Effectively using rescue_from

Ruby, S., Thomas, D., and Heinemeier, D. (2009). Agile Web Development with Rails [3rd Ed.]. Raleigh, NC: The Pragmatic Bookshelf.

Save Bang Your Head Active Record Will Drive You Mad

Software Development | Ruby on Rails altered_beast Error Handling