Rails' CurrentAttributes considered harmful

22 Jun 2017

I was made aware of this commit recently (thanks to Rebecca Skinner) to Rails which introduces what is effectively a global state to Rails applications.

Rather than writing why global state in-general is a bad thing myself, I will instead link you to this excellent question + answer on the Software Engineering Stack Exchange.

Very briefly, it makes program state unpredictable.

To elaborate, imagine you have a couple of objects that both use the same global variable. Assuming you're not using a source of randomness anywhere within either module, then the output of a particular method can be predicted (and therefore tested) if the state of the system is known before you execute the method.

This implementation also chooses to use thread-local variables which this answer over on Stack Overflow explains is not a good choice because:

  • It's harder to test them, as you will have to remember to set the thread-local variables when you're testing out code that uses it
  • Classes who use thread locals will need knowledge that these objects are not available for them but inside a thread-local variable and this kind of indirection usually breaks the law of demeter
  • Not cleaning up thread-locals might be an issue if your framework reuses threads (the thread-local variable would be already initiated and code that relies on ||= calls to initialize variables might fail

Not to mention that this also violates the well-established Law of Demeter too. Suddenly, Current is available anywhere in your application. Good code is explicit in how it makes data available to its methods / functions. This CurrentAttributes feature is not good code, nor is it making it explicitly clear how Current.user arrives in the model. It is just there "magically".

I enjoy and have benefited from Rails magic in the past. Some of my favourite features are render @collection and the polymorphic routing. Those are great features because their scope is limited. I know that I can render a collection in a view. I know I can use polymorphic routing in controllers, models and helpers.

This CurrentAttributes is much too magical for my liking because of how it introduces a thread-local global state which hides where the actual work of setting values on Current is done, and because it is implicit about where those values come from.

"They're just set in the controller!", may be a defense for this. But what if you don't have a controller? What if you have a background job instead? How about a test? True, in both cases you could use Current to provide the values:

def perform(user_id, post_id)
  Current.user = User.find(user_id)
  post = Post.find_by(post_id)
  post.run_long_running_thing
  # code to run the job goes here
end

Here, Post#run_long_running_thing can simply access the current user by accessing Current.user. But it is not immediately clear -- if all you're looking at is the Post#run_long_running_thing method -- where Current.user is being set in the first place. It's implied that it's set somewhere else, but to attempt to find where it's set in this context may be difficult. Doing a find in the project for Current.user = may turn up multiple places (controllers, jobs, etc.) where the variable is set. Which one is the right one for this context?

As for tests, for those you would need to setup Current.user before hand if you had any code relying on that. I'd imagine something like:

let(:user) { FactoryGirl.create(:user) }
before { Current.user = user }

it "runs the long running thing" do
  post.run_long_running_thing
end

Again, it is not explicit when you're looking at the run_long_running_thing method or its tests where Current.user is being set.

There doesn't appear to be anywhere in the CurrentAttributes code -- as far as I can tell -- where it would reset the state of this Current object in between each test. Therefore, setting it in one test like what I've done above now makes it bleed through "magically" into other tests. That behaviour seems like a horrible thing to have in a codebase. You could very well have situations where you're expecting Current.user to be nil but instead it's set to some vaule because some other test set it. Now which of the 500 tests in my application did that? Good luck finding it.

Convention over configuration, and perhaps explicitness over implicitness?

Rails is still a good framework. I know DHH's rebuttal to this will be "don't use it then" or something along those lines. Similar to his response to my reverting of callback suppression a while back.

Protecting programmers from themselves is explicitly not in the charter for Rails when it comes to providing features that have a valid use case but could be abused.

I just can't reason with the guy at all. We have vastly different opinions on this sort of thing.

I think Rails choosing to go with ultra-implicitness -- like in this Current case -- is a vastly wrong move that will lead to a lot of frustration with Rails codebases that use this feature. I think Rails should, in situations like this, choose to opt for explicitness over implicitness. Rails has enough magic in it and it certainly doesn't need any more.

This feature is not something that was sought after (it appears like DHH thought it was a good idea one day and just did it), and we have much better ways of doing this. For instance, in the job code above, it would be better to pass it explicitly:

def perform(user_id, post_id)
  user = User.find(user_id)
  post = Post.find_by(post_id)
  post.run_long_running_thing(user)
  # code to run the job goes here
end

Similarly, in the test explictness also wins:

let(:user) { FactoryGirl.create(:user) }

it "runs the long running thing" do
  post.run_long_running_thing(user)
end

In both of these cases, it is very clear how user arrives in the run_long_running_thing method: it is passed in as an argument.

Let's finish by taking a look at the code from the pull request and look at how it can be written more explicitly.

DHH's CurrentAttributes code vs my explicit code

# app/models/current.rb
class Current < ActiveSupport::CurrentAttributes
  attribute :account, :user
  attribute :request_id, :user_agent, :ip_address

  resets { Time.zone = nil }

  def user=(user)
    super
    self.account = user.account
    Time.zone = user.time_zone
  end
end

# app/controllers/concerns/authentication.rb
module Authentication
  extend ActiveSupport::Concern

  included do
    before_action :set_current_authenticated_user
  end

  private
    def set_current_authenticated_user
      Current.user = User.find(cookies.signed[:user_id])
    end
end

# app/controllers/concerns/set_current_request_details.rb
module SetCurrentRequestDetails
  extend ActiveSupport::Concern

  included do
    before_action do
      Current.request_id = request.uuid
      Current.user_agent = request.user_agent
      Current.ip_address = request.ip
    end
  end
end

class ApplicationController < ActionController::Base
  include Authentication
  include SetCurrentRequestDetails
end

Including the Authentication module into ApplicationController to add a single method seems like a bit of premature extraction. Let's ignore that for now.

This implementation with its before_action to set_current_authenticated_user will mean that Current.user is set on all requests, even those which don't refer to the current_user at all.

A better implementation would be a current_user method which evaluates its find when it is called. You'll see this pattern in a lot of Rails applications.

def current_user
  @current_user ||= User.find(cookies.signed[:user_id])
end

In fact, this is similar to how Devise presents its current_user method. It uses warden instead of cookies.signed, but it's implementation is similar enough:

def current_#{mapping}
  @current_#{mapping} ||= warden.authenticate(scope: :#{mapping})
end

Ok, so now we've got a current_user method which is available in the controllers but what if we want to use it in the view? For instance, if we want to say Hello, #{current_user.name} in a layout? Easy enough: make it a helper method.

def current_user
  @current_user ||= User.find(cookies.signed[:user_id])
end
helper_method :current_user

Great, so now it's available in controllers, helpers and views. All without making it available everywhere in the current thread.

Now I would like to focus on the second half of DHH's code:

class MessagesController < ApplicationController
  def create
    Current.account.messages.create(message_params)
  end
end

class Message < ApplicationRecord
  belongs_to :creator, default: -> { Current.user }
  after_create { |message| Event.create(record: message) }
end

class Event < ApplicationRecord
  before_create do
    self.request_id = Current.request_id
    self.user_agent = Current.user_agent
    self.ip_address = Current.ip_address
  end
end

Here, DHH is implicitly linking the message's creator to the Current.user by using the default option on belongs_to. I believe that this violates the MVC layer abstraction. The Current.user is just "magically" available in the model, with absolutely no context of how it got there in the first place.

A common pattern in Rails applications is not to do this, but instead to explicitly set the creator at the point of creation:

def create
  @message = current_account.messages.create(message_params)
  @message.creator = current_user

Let's assume current_account is a similar abstraction to the current_user one. It's clear here that in the controller that this is where the creator is assigned. With DHH's code, it is not immediately clear from the controller code itself that creator is assigned at all.

Not only that, but this also lends itself to being abstracted into a "service object" which is responsible for creating a message. Let's say that you want to log an Event whenever a Message is created. Oh, I see DHH's code does that already with an after_create callback. Well then.

In the case of DHH's code, the after_create callback will happen whenever a Message is created anywhere in your application. This might be suitable in a controller, but what if you want to test some database logic, or something else which requires a persisted message, and you don't care about an event being there at the same time? What if when you created an event you had extra logic on it which caused another record to be created too?

Having such a callback irrevocably ties together messages and events implicitly.

As I mentioned before, it would be better to abstract this into a "service object".

class CreateMessageWithCreator
  def self.run(params, current_user)
    message = current_account.messages.create(message_params)
    message.creator = current_user
    message.save
  end
end

You can then call this code in your controller like this:

def create
  if CreateMessageWithCreator.run(message_params, current_user)
    Event.create(record: record)
    flash[:notice] = "Message sent!"
    redirect_to :index
  else
    flash[:alert] = "Message failed to send."
    render :new
  end
end

This way, then you would know that in this particular case you're definitely creating a message with a linked creator and it frees you up to create messages without creators or events, if the need did arise.

I think having these dependencies clearly highlighted in the code rather than magically abstracted away is a much, much better solution.

Conclusion

Introducing a global state to Rails seems like a terrible idea and while I deeply, deeply wish this change is reverted, that is very likely not going to happen because it's DHH's change and it's his framework. DHH is allowed to be a footgun merchant if he wishes to be. I am just sad to see that, despite evidence that this is a genuinely bad idea, DHH carried on with it. I thought with his years of experience he would know better by now.

blog comments powered by Disqus