Three Things To Avoid When Building Rails Controllers

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.