Blog
0 pixels scrolled
  • Home
  • Work
  • Services
  • About
  • Blog
  • Contact
You Can't Save Everyone:
Some Exceptions Should Be Left Alone
Jared Norman on January 23, 2018
I take a look at a fun case of permissive error handling that causes a Rails controller to accidentally catch an exception from RSpec.
You Can't Save Everyone:
Some Exceptions Should Be Left Alone
Jared Norman on January 23, 2018
Posted in Software-development
← Back to the blog

While exceptions are an important tool in creating reliable programs, they also introduce what is arguably the least intuitive form of control flow in mainstream languages, including Ruby.

Conventional control flow structures like loops and conditionals move execution within some very limited context. Unlike exception handling, they never leave you in another part of the call stack altogether.

Exceptions handling is itself an exception to what many would consider “good language design.” Exceptions are like mystery goto statements, and in Ruby they have substantially more complicated behaviour.

Goto statements arbitrarily move execution from one place to another. It’s a one-direction transfer of the flow of execution to another explicitly stated place.

Exceptions are far harder to reason about; they send execution up the call stack until a caller is found in a context that defines exception handling for that type of exception. The code raising an exception is not tied to the code that will be handling it. In a dynamic language like Ruby, identifying how a given exception is going to be caught can be super hard.

This blog post doesn’t seek to slander exceptions. They’re a powerful tool that allows us to write our programs where our high-level abstractions can ignore many of the potential low-level exceptions that might be encountered. Alternative solutions like callbacks tend to force the intermediate layers to handle the lower-level exceptions so that they can be communicated to the caller in some way.

Unlike callback-based approaches where errors must be passed around and either handled or passed up to callers, exceptions give application developers some freedom in which errors to handle and where to handle them. Being too broad in what exceptions you choose to handle can get you into trouble. Permissively catching all exceptions is a dangerous pattern, and can have unintended results. You might have heard this before, but I encountered a very unexpected result not too long ago that’s worth sharing.

Be Careful

I’ve done tons of work with Spree and its successor Solidus. They offer an E-commerce platform as a mountable Rails engine. Once installed in your Rails application, they serve up a storefront, admin interface, and API to power it. It’s a great way to build a highly-customized store.

The controllers that power the API all share a superclass named Spree::Api::BaseController. It helps deal with nitty-gritty details like authentication, authorization, Content-Type handling, and exception handling.

For years, the Spree::Api::BaseController was rescuing any exception, so that it could respond with a status code of 422 (Unprocessable Entity). The code that did this was eventually completely removed, but the old code looked basically like this:

module Spree
  module Api
    class BaseController < ActionController::Base
      # ...

      rescue_from Exception, with: :error_during_processing

      # ...

      private

      # ...

      def error_during_processing(exception)
        Rails.logger.error exception.message
        Rails.logger.error exception.backtrace.join("\n")

        error_notifier.call(exception, self) if error_notifier

        render text: { exception: exception.message }.to_json,
          status: 422 and return
      end

      # ...
    end
  end
end

At least the author was kind enough to log the details of the exception. This made developing subclasses bearable, but rescuing Exception will still rescue pretty much everything and having to read through your logs in development to diagnose a NoMethodError because of a typo seems excessive.

It turns out, this code can also lead to false positives in your automated test suite, if you construct your tests just right.

The Perfect Test

Spree and Solidus both use RSpec for their test suites, and if you’re building Spree/Solidus based store, you’re encouraged to use it too as they provide some nice testing support facilities.

In RSpec, if you’re stubbing a method on an object and you need to some non-trivial verification of its arguments, you’ll need to do something like this:

expect(PaymentStore).to receive(:save) do |payment|
  expect(payment.order_id).to eq order.id
end

In the above code asserts that when we store a payment, that payment must be associated with the right order. It’s a slightly contrived example, but it happens occasionally.

Now if you were writing a custom API controller for your Spree application that inherited from Spree::Api::BaseController, and you did something like this in one of the specs, the assertion in that block would never fail.

Well, that’s not exactly true. RSpec would never report that exception as having failed. When RSpec assertions fail they raise exceptions, and because exceptions travel up the call stack, and PaymentStore.save(payment) is presumably being called somewhere inside your controller, the rescue_from Exception will kick in and call the error_during_processing method.

The assertion would fail, but it would be handled by your controller code, resulting in a response with a 422 status code. This means you could write the following spec which would pass even if the payments were getting associated with some other order:

describe "POST #record_payment" do
  subject do
    post :record_payment, params: { id: order_id, payment: payment_params }
  end

  let(:order_id) do
    # ...
  end
  let(:payment_params) do
    # ...
  end

  it { is_expected.to have_http_status :created }

  it "associates the payment with the correct order" do
    expect(PaymentStore).to receive(:save) do |payment|
      expect(payment.order_id).to eq order.id
    end
    subject
  end

  it "saves a payment" do
    expect { subject }.to change { Payment.count }.from(0).to(1)
  end
end

Because the stub is only defined in the one example, you would never catch that the controller was erroneously responding with a 422 (Unprocessable Entity) status code.

Note: If you’ve encountered issues with rescuing Exception before, you might also know that the preferred strategy is to rescue from StandardError instead. This is true, and would protect you from this particular bug (RSpec’s assertion-related exceptions inherit directly from Exception), but would still expose you to potential problems. NoMethodError, NameError, NoMethodError, TypeError, all inherit from StandardError and often represent unexpected issues you should be aware of.

You Can’t Expect The Unexpected

I’m sure the author meant well, but when it comes to exception handling, you should only handle exceptions that you understand. All modern web application frameworks provide facilities for exception reporting that can be configured to notify external services that record and group the exceptions encountered by your application.

You, the application developer, should monitor the exceptions your application encounters and decide how to handle them individually. You’ll need to write handlers for some exceptions that get reported, while others you can safely ignore or monitor. Sometimes you’ll want to catch a whole class of exceptions (network errors when contacting a third party service, for example). Just be as conservative as you can in your error handling. Modern software systems are simply too complex for blanket strategies like Spree used. Reasoning about everything that could cause an exception in any given part of your application is simply not possible.

Work ServicesAboutBlogCareersContact


Privacy policyTerms and conditions© 2018 Super Good Software Inc. All rights reserved.