Rescuing RecordNotFound Is Too Broad

- - posted in ruby | Comments

Because rescuing ActiveRecord::RecordNotFound rescues missing records from all models it can produce unexpected behaviour which is hard to trace.

I recommend doing yourself a favour and logging any exceptions which are rescued by not re-raised and being explicit about the model from which you want to rescue RecordNotFound using the NotFound gem.

We had a fairly regular controller, here is an extract.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
rescue_from ActiveRecord::RecordNotFound do
  render action: 'not_found'
end

def show
  @study = Study.find(study_id)
end

def destroy
  command = DeleteStudy.new
  command.on(:success) { flash[:notice] = "The study was successfully deleted" }
  command.on(:failed)  { flash[:notice] = "The study was NOT deleted" }
  command.execute(study_id)
  redirect_to studies_path
end

Looks okay, right? The problem was that the destroy action was not redirecting to the index action. Instead we where getting the not found page being rendered. At first I thought it was redirecting to the show action and then rendering the not found because the model had been deleted. I didn’t write some of the code in the controller and that which I did I hadn’t looked at in years.

It took some time to figure out the problem.

It turns out that we made a change to an engine which was subscribing to a Wisper event broadcast from the DeleteStudy command.

Due to a bug in the listener a ActiveRecord::RecordNotFound exception was being raised on a different model. But this exception was being rescued by rescue_from and the not found page was being rendered. Because the exception was being silently swallowed it was not apparent what was going on.

I decided two things, one if you rescue an exception and don’t re-raise the same exception it should be logged and second we shouldn’t rescue such a broad exception as ActiveRecord::RecordNotFound. I wrote a tiny gem which raises a model specific RecordNotFound exception, so we can be explicit about the model from which we want to rescueReecordNotFound. The new code looked something like this:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
rescue_from Study::RecordNotFound do |e|
  log_swallowed_exception(e)
  render action: 'not_found'
end

def destroy
  command = DeleteStudy.new
  command.on(:success) { flash[:notice] = "The study was successfully deleted" }
  command.on(:failed)  { flash[:notice] = "The study was NOT deleted" }
  command.execute(study_id)
  redirect_to studies_path
end

private

# in ApplicationController
def log_swallowed_exception(e)
  logger.debug("#{e.class.name} #{e.message}\n #{e.backtrace.take(10).join("\n"))
end

We log the exception and rescue Study::RecordNotFound instead of ActiveRecord::RecordNotFound.

Comments