Rails 5 — ActiveRecord Suppress — a step too far?

One of the new additions to ActiveRecord that’s gonna be coming out with the Rails 5 release is the ActiveRecord::Base.suppress method.

I have some criticism of this new feature and would like to articulate those in this post. Before that I’d like to explain the ‘advertised’ use cases for this new method and also walk you through the implementation of this method. (which I think is pretty cool).

Some random blog post feature image! :)

When to use suppress

Alright lets say we’re building a social network application where users can follow each other. And when someone follows someone else we send a notification to the followed user.

In-terms of code it would look something like this —

That’s the code we have currently and it fits our needs. So far so good. But suddenly we are required to implement a crazy feature where we need to perform the follow but not notify the other user.

There are a ton of ways to do this. But since this blog is about the Suppress method, I’m gonna show you how to solve this using this new feature.

Let’s modify our code to add a new method follow_without_notification making use of suppress —

That’s it. This will solve our requirement.

Now I want you to take a brief moment (don’t take too much time though lest you forget to read the rest of this post) and think about this suppress method and guess what it actually does internally and how exactly it solves our problem. If you come up with an explanation do leave a response to this paragraph now. Also leave a response if you feel (like the way I felt) that this code is not intuitive enough.

We’ll talk about this in the last part of the post.

How it’s been implemented

The source code of this new method was pretty interesting to me and I managed to learn a few things from this. So let me walk you through the code for ActiveRecord::Base.suppress.

Suppressor module in ActiveRecord

As you can see Suppressor is an ActiveSupport::Concern module which gets mixed into all of our ActiveRecord models in Rails.

This concern adds a class method called suppress to our models and it takes in a block as an argument. That’s why we’re able to do things like —

Notification.suppress do 
  # stuff
end

Let’s move into the suppress method. In the first line we see the usage of a class called SuppressorRegistry that’s been defined in the same file at the bottom. If you look at that class (line#20), you’ll see that it’s a plain Ruby class that’s extending ActiveSupport::PerThreadRegistry.

Quick Diversion — I don’t want to go in-depth with this in this post, but it’s basically a way to have thread-local global data in Rails. If you have a class in your code that extends ActiveSupport::PerThreadRegistry, then you can access that class anywhere in your application and when you do you’ll get a hold of a singleton object of that class in which you can store data and express behavior if needed. This is simply a nice improvement on Thread.current and I would recommend reading this post by Justin Weiss on this topic. Also I recommend you read the source code for this where you’ll see how this is implemented on top of Thread.current.

In our case, SuppressorRegistry extends PerThreadRegistry — thus making itself available globally. And it defines a single attribute called suppressed initialized to an empty hash. (line#26).

Going back to the suppress method’s definition (line#7), we see the the following line of code —

SuppressorRegistry.suppressed[name] = true

So this basically use the name of the class (obtained using the name method) on which suppress was called, ‘Notification’ in our example, as a key in the suppressed hash and sets it’s value to true such that the suppressed hash in SuppressorRegistry would look this —

{'Notification' => true}

And after that it simply yields the block that was passed as the argument and so all our code inside the block gets executed. And once the block completes, the execution moves into the ensure block (line#10) in which the value of the key (‘Notification’) is updated to false.

This is all that the suppress method does. The real action is in another method that’s been defined in this module. Which is the save! instance method at line 15. This simply overrides ActiveRecord::Base’s save! and it’s defined as —

def save!(*args) 
  SuppressorRegistry.suppressed[self.class.name] ? self : super 
end

You get it? Now everything falls into place. Here, if the class of the object on which save! is being called (Notification.create in our example), has a true flag in the suppressed hash, then its made to fail quietly and just return the object. And if there is no true flag for the class, super is called and execution proceeds as usual.

Now that we know what the suppress method actually does, we can see that when we do,

Notification.suppress do 
  # stuff
end

its basically blocking all ActiveRecord save! methods that are called on instances of the Notification class inside the block passed to the suppress! method.

Is this really a good solution?

I had asked you to think about this earlier whether this was intuitive and clear enough. Do leave your responses. Certainly I did not think this was clear by any means. It felt like this was a contrived solution to a non-existent problem.

Why? Let’s begin with the GitHub issue that got the ball rolling. It was raised by none other than DHH. In the issue he wrote about a use-case that could be solved by something like the suppress method and gave a rough implementation idea. Here’s the use-case DHH shared.

There’s a class Comment that upon creation, sends notifications by creating objects of Notification (similar to my example from above).

class Comment < ActiveRecord::Base
  belongs_to :commentable, polymorphic: true
  after_create -> { Notification.create! comment: self }
end

And then there is a certain use-case where the commentable entities can be copied along with their comments and in that case notifications are not supposed to be fired when comments are created afresh.

So he has this concern Copyable which is supposed to know how to copy the commentable entities. And in-order to prevent firing of notifications during copy, here’s what he did —

module Copyable
  def copy_to(destination)
    Notification.suppress do
      # Copy logic that creates new comments that we do not want triggering Notifications
    end
  end
end

Few things about this.

1. How does one figure out that suppress is supposed to affect ActiveRecord save!?

Last I checked, code is supposed to be clear and concise. And naming things properly goes a long way in achieving this goal. See — IntentionRevealingNames.

Also, imagine this being used widely in a large codebase and the ordeal a developer would need to go through to know exactly what kind of effects this has. The point being, its not immediately obvious what this piece of code does. One has to figure out that this is connected to the Notification.create in the after_create callback.

DHH’s response to a similar concern raised on GitHub by Cristian Bica —

The dictionary definition seems to fit what we’re doing to a tee:http://dictionary.reference.com/browse/suppress. I think it works with all sorts of classes. In my code base, I’m currently using three different versions of this is different places: Event.suppress, Mention.suppress, Notification.suppress. All those work for me.

I’ll let you be the judge of that.

2. How common is this use-case?

Another thing I felt was that this seemed to be a rather narrow use-case. Maybe its widely used inside Basecamp, but I can’t think of many scenarios where you’d want to create an object in response to creation of a related object and then wanting to not do this in some cases.

But this is based on my limited experience and it’s true that I am slightly biased against callbacks in general. Would rather go with a builder or factory — whichever is appropriate — and represent the concept in-terms of that object instead of encoding business logic in a model callback.

Henrik Nyh and Elliot Winkler had argued for similar things in the issue & PR.

So this again begs the question — Is this feature really needed in Rails?

3. When you add asynchrony?

What if the creation of the notification object (which I feel is a rather odd way to handle notifications) was scheduled to be done in a background job or in a separate thread maybe? That’s something to think about.

Well I am aware that suppressing record creation in callbacks is not the only use-case for this and I shouldn’t have a section to criticize just that specific example. But when the comments in the source code describe this as the typical usage of this method I think it does warrant some criticism.

4. Too much coupling

Another issue I had with this is the coupling that it might introduce in your code. I do understand that our community in general is not very much interested in dealing with coupling and I am not an expert at this either, but I do get a feeling that this suppress method would lead to some very bad usages.

For instance, why does the Copyable module needs to know about a class named Notification?

What if someone is creating a lot of different things in the callbacks? Then we’d have to do something like this —

A.suppress do
  B.suppress do
    C.suppress do
      # do the stuff
    end
  end
end

Which keeps adding dependencies. Instead, we could have had a method / object that represents this concept (in which A, B, C need not be created) and used that.

Another valid argument was raised by Elliot Winkler in his response and I’d like to quote him here —

if we are modifying the behavior of Comment, why should we have to tell Notification to do something? We’re reaching inside of Notification from a completely different class

I completely agree with this argument.

Anyway, these are some of the feelings that I am able to articulate and explain. It just feels wrong to do something that has an effect in a far away place.

Certainly I‘m no expert at this, so I could be wrong on a lot of things here. But as I was reading about the new addition I had this immediate bad feeling. And then when I went to the GitHub issue & PR I saw many people arguing about the same things — that kinda validated my initial feeling towards this.

What I do know for certain is that if I had read about this when I first started out a couple of years ago, I would never have had this reaction. And that is why I don’t want to see Rails encourage such things openly and leading beginners into misery.

I would love to know what you think about this. Do leave responses!