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.

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!