Three Things To Avoid When Building Rails Controllers

The controller-layer in any MVC architecture performs a pivotal role, leveraging the domain logic captured by your models and using this to build the views returned to the application user. Controllers in a Rails app offer a wealth of functionality and conveniences to help you complete this job. But amongst these conveniences there lie a number of prickly parts which can do more harm than good. In this post we will present three practices which are best avoided.

Introduction

The Model-View-Controller (MVC) architectural design has cemented itself as a solid choice for building web applications. It forms the foundation for many popular frameworks such as Django, ASP.NET and Rails.

The Model layer is responsible for persisting and retrieving the application data.

The View layer is what is presented to the user; it comprises a representation of the data contained in the application, and may also include the interface to interact with this data, e.g. forms, links, buttons etc.

The Controller layer is what the user interacts with; either to request data to view, or to insert or update data. The controller will receive user requests, will access the model layer to retrieve or update data, and will return an appropriate view to the user. In this way the controller can be viewed as co-ordinating the model and view layers to create a functional application.

In the context of a Rails application the router accepts the incoming request and is responsible for determining which controller action (if any) should hande the request. Once the request is passed off to the controller, it will often have responsibility for a myriad of important tasks, such as ensuring that the user in question has permission to access the action, parsing and sanitizing the request parameters, interacting with domain models and building the view (often HTML) to be returned to the user.

Rails offers a bag-full of functionalities to support this important and varied work of the controller. Most of these functions are necessary and convenient, but there are few which I consider to be, at least questionable, if not a downright bad choice.

Disclaimer: This post is basically personal opinion. I aim to provide some justification for the opinions expressed herein, but they are all formed by my own (arguably, limited) experience. There may be example use cases for which these particular behaviours have proven to be exceptionally useful to others. So, if you want to defend any of these functionalities please let us know in the comments!

On with the list …

Limit controller interface

Rails controllers plug into the framework by exposing actions (controller methods), which can be referenced from the application routes configured in config/routes.rb. These actions are the public API of each controller class, they should be both necessary and sufficient; that is to say:
- Necessary: There should be a no public controller method that does not support an application route.
- Sufficient: There should be a controller action to support each route exposed by your application.
The sufficent criterion will be addressed in point 2, below. Violation of the necessary criterion is illustrated in our fictitious BarsController, below. Note, the code samples presented in this post can be found on GitHub here.
```
  
# config/routes.rb
Rails.application.routes.draw do
  …
  resource :bar, only: [:show]
  …
end

# app/controllers/bars_controller.rb
class BarsController < ApplicationController
  def show
    create unless @bar
    # This action will render app/views/bars/show.html.erb
  end

  def create
    # Create the Bar instance
    # This method is just here to support the show action
  end
end
        
      
```
We see from the config/routes.rb that there should be a single action of show in the BarsContoller. However we have unintentionally polluted the public interface of the BarsController with the create method. This method is only intended to support the show action, but it is available as a public method. To fix this simply requires that we make the method private, no big deal, so why get worked up about it? Well, failing to properly control the API of your controller classes has two drawbacks:
- Unintentionally exposing controller methods as public (i.e. actions), means we are a short step away from an unintentional exposure in our application. Exposure just requires the unfortunate coincidence of a misconfigured route; at this point a malicious user would be able to hit our controller method as though it was an action. The damage they could do at this point depends on the details of your system, but it is definitely not something that we would choose to expose ourselves to! It must be said that, to realize this vulnerability requires a very unfortunate coincidence between the misconfigured route name and the name of your controller method. In the example above I have used a controller method with the generic name of create, which could be very easily exposed by a simple misconfiguration in your config/routes.rb. Custom method names in your controllers are going to be much less likely to unintentionally collide with route actions, nonetheless, this is not a risk we need to take.
- The second drawback relates to developer ergonomics. Any teammates coming to read the controller source code (or when you read it yourself, at some point in the future), will rightly have the expectation that each of the public methods on the controller class should correlate with exposed routes on the application. By violating this contract the controller code is going to be harder to understand and maintain.
The traceroute gem can help to find violations of this principle in our project. It will consider all the routes configured in your application and will highlight any controller actions which are unreachable via these routes. Usually this will mean that you have either exposed a method on your controller as public, when it should in fact be private or you previously supported a route which has now been removed from the router config but you failed to tidy up the associated controller action. Both cases should be addressed as a matter of urgency. Running traceroute on our dummy project will successfully identify the fact that the BarsController#create has no configured route, and is therefore unreachable:
```
  
> rake traceroute
…
Unreachable action methods (3):
  …
  bars#create
        
      
```
If you have already accidentally exposed a non-private controller method via a misconfigured route, then obviously traceroute can't help you here :S
Always provide an action method

This may sound like a bizarre statement. If we have a particular route, we will obviously have to have a corresponding controller method to support it, right?

Actually this is not the case. The controller does not need to have an appropriately named method for each route, provided an appropriately named view template is available. For example, consider the following entry in our config/routes.rb and corresponding PagesController implementation:
```
  
# config/routes.rb
Rails.application.routes.draw do 
  resources :pages, only: [:index] do
    get :has_template, on: :collection
  end
end

# app/controller/pages_controller.rb
class PagesController < ApplicationController
  before_action { @other_stuff = "Yes action still fires" }

  def index
  end
end
        
      
```
Now suppose we have a template existing at app/views/pages/has_template.html.erb , such that the name of the template matches the action name in config/routes.rb. In our sample we will just have a simple template as follows:
```
  
<h3>Template with no action</h3>
<p>Hi, I'm a template with no action</p>
<p>Here is a non-existant instance variable <%= @stuff %></p>
<p>Does before_action fire?  <%= @other_stuff %></p>
<%= link_to "<< Back to index", pages_path %>
        
      
```
Now if we spin up our Rails server and navigate to /pages/has_tempate we can see the rendered has_template.html.erb template! In other words, if we have the route defined and an appropriately named template then the template will get rendered, even if the controller doesn't have the supported action defined.

A couple of points to note:
- The controller does need to be present. If the controller, itself, is absent we will get an error raised: uninitialized constant PagesController
- The rendering process is quite forgiving, and will often render without error, even when instance variables are missing, e.g. @stuff in the example above
- Controller actions continue to fire as normal. The controller is behaving as though the missing action is present, just with an empty method definition
```
  
class PagesController < ApplicationController
  before_action { @other_stuff = "Yes action still fires" }

  def index
  end

  # Behaves the same as when the method definition is present, but empty
  def has_template
  end
end
            
          
```
I find this behaviour confusing and a potential security banana-skin. As a developer, when trying to understand any Rails application, the router provides the connection between the public interface (i.e. the URLs exposed to the users) and the server logic. Typically the router achieves this by connecting a URL to a specific controller action. Non-existant controller actions break this fundamental expectation. What is more, it is easy to imagine a refactoring exercise that removes a controller action but forgets to tidy up the associated route and template. It would seem all too easy to make unwanted exposures to our users in such circumstances.

I would never rely on this behaviour. I think there is a strong argument to remove this 'magic' from Rails, as I can't think of a strong use case where this behaviour is desirable. Even if it were required in some obscure case, there are other more explicit ways to achieve the same behaviour without making it a default behaviour for the framework.

In summary, each route in your application should point to an existing controller action. This action method can be empty but, for sanity, it should exist. Once again, the traceroute gem can help you uncover routes in your Rails app that do not have a corresponding controller action. This can help you to identify violations of this advice, and avoid exposing routes that do not have a corresponding controller action method. Running traceroute against our dummy project will flag the fact that PagesController#has_template looks like an 'unused route', because it has no corresponding controller action:
```
  
> rake traceroute
Unused routes (6):
  pages#has_template
  …
        
      
```
Don't use except for filters

Love them or hate them, controller filter actions are a powerful mechanism for implementing cross-cutting concerns for a request. Consider, for example, that we wish to manage some caching with an after_action filter in our fictitious FoosController:
```
  
class FoosController < ApplicationController
   after_action :clear_cache, except: [:show]

   def show
      # Get the record and display it
   end

   def update
      # Update the record
   end

   private

   def clear_cache
      # Remove the cache
   end
end
        
      
```
This controller exposes two actions, show and update. On the update action we want to clear the cache for the model that we have just updated. We don't want to clear the cache when we are just viewing the model, so we have achieved this using the pattern:
```
  
after_action :clear_cache, except: [:show]    
        
      
```
The problem with using an exclusion list is that, if we add a new action to the controller, we can easily forget to update the exclusion list:
```
  
class FoosController < ApplicationController
   after_action :clear_cache, except: [:show]
   …
   def edit
      # Just displaying edit form should *not* trigger clearing cache
   end
   …
end
        
      
```
In this case, the edit action just displays a form for updating the model. It doesn't actually represent any update to the underlying model, so it should not lead to the cache being cleared. However, as we forgot to update the except clause, this action will now trigger a needless (possibly costly) cache purge.

This is a very specific example, but the problem is a general one. It pertains to the general advice that, in such contexts we should stick to allow-lists rather than exclusion-lists. An allow-list approach requires that we explicitly state when we want the after_action to fire, and it prevents accidentally firing filters when they weren't intended:
```
  
class FoosController < ApplicationController
   after_action :clear_cache, only: [:update]
   …
   def edit
      # Just displaying edit form should *not* trigger clearing cache
   end
   …
end
        
      
```
There are legitimate arguments in support of the exclusion-list approach. One powerful counter-argument is when configuring security layers, e.g. authentication or authorization:
```
  
class PrivateStuffController < ApplicationController
   before_action :ensure_permission!, except: [:something_public]
   …
end
        
      
```
I understand the argument here, and have used this pattern myself. We are trying to play safe by running the :ensure_permission! before all actions, by default, then making an exception for a limited subset of actions. We are trying to future-proof ourselves in case we add another action in the future, but forget to apply the filter.

Over time I have come to dislike this approach. For something as fundamental as authentication, for example, I feel that a separation of auth and non-auth actions into separate controllers will yield a better solution. And, aside from that, surely it is better that, when we need a new action in the future, we choose how to implement the action at that time, rather than trying to solve the problem in advance? The discussion seems analogous to similar arguments against premature optimization, and is closely related to the YAGNI principle.

I would be more sympathetic to the future-proofing argument if we obtained the benefit without incurring any cost, but there are costs. The first cost I have mentioned is the very real possibility that you could unintentionally run superfluous, or unwanted, filters before/after/around your actions. At best these unintended filter actions represent wasted compute time, at worst they may actually impact the functionality or security of your controller actions.

The second cost is in maintainability. Take the example of our FoosController once more, but this time imagine we have added some additional actions over time
```
  
class FoosController < ApplicationController
   after_action :clear_cache, except: [:show]
   def update  
      # We decide we want to remove this update action
   end

   def bar
      # Some other action added over time
   end
   …
end
        
      
```
Now suppose we want to remove the update action. This is the only action that really requires our after_action filter, but we are reluctant to remove it. Does the bar action, which was added to the controller later, have some implicit reliance on this filter? A simple refactoring job has now become a bit more involved, and it might require some investigation before we can satisfy ourselves that, in fact, the bar action has no reliance on this :clear_cache filter. So we see that the use of the :except clause has allowed the filter logic to unintentionally leak out into other controller actions, adding a maintenance burden.

In summary, I feel that the :except clause on a filter action decays too rapidly, and can become a maintenance headache. It feels that the best approach is to either
- Deliberately and unambiguously apply a filter to all controller actions (i.e. with no :only nor :except clauses)
- If you really need to limit the filter to specific actions, you should explicitly allow-list those actions with an :only clause.
Use of the :except clause should be limited, or avoided where possible.

Conclusion

In this blog post we have outlined three patterns that are best avoided when crafting your rails controllers.

Each public method on a controller should correspond to an action which is accessible via your application routes.
Always provide an action method for each route, even if empty. Don't rely on the Rails feature that automatically renders the correspondingly named template, where action is not defined.
If you need to limit the actions to which a controller filter should apply, prefer to use the :only clause to allow-list the actions where the filter is needed. Avoid using :except.

Don't agree with these points, or have others to add to the list? Please let us know in the comments.

If you found this blog post interesting or useful, please consider subscribing to our mailing list, and we will send an email update when we publish any new content.

References

Wiki entry for MVC pattern
Rails docs for controllers
The YAGNI mantra
Rails docs for the router
The traceroute gem
Rails docs for controller action filters
Simple GitHub repo with Rails project displaying these controller behaviours

Three Things To Avoid When Building Rails Controllers

Introduction

Limit controller interface

Always provide an action method

Don't use `except` for filters

Conclusion

References

Comments

Got your own view or feedback? Share it with us below …

Subscribe

Three Things To Avoid When Building Rails Controllers

Introduction

Limit controller interface

Always provide an action method

Don't use except for filters

Conclusion

References

Comments

Got your own view or feedback? Share it with us below …

Related

Subscribe

Don't use `except` for filters