Proposal: rename or remove class-level update/update!

I’ll start this discussion with an initializer I just added to my Rails app:

ActiveSupport.on_load(:active_record) do
  ActiveRecord::Persistence::ClassMethods.module_eval do
    [:update, :update!].each do |m|
      remove_method(m)

      define_method(m) do |*_|
        raise <<~TXT.squish
          The class method `#{m}` has been banned.
          This method will load and then mutate every single record
          in the database. This is probably not what you want to be doing,
          and if it is, there are safer and faster ways of doing it.
        TXT
      end
    end
  end
end

You can probably guess what happened. I was fixing a couple bits of data in production. I had my record to fix in a variable user and typed user.update(attrs). Or, that’s what I meant to type. I actually typed User.update(attrs). The next hour of my day was pulling a point-in-time database backup and recovering all the data I had clobbered.

Having update and update! as class-level persistence methods is making a dangerous weapon readily available. You’re one character away from clobbering every record in a table. If the application doesn’t have robust audit logging or you don’t have point-in-time backups, this will do irreparable damage to your database.

IMO dangerous operations should not be convenient and easy. If someone really wants to load and then update every single record in the database, let them do it with a loop. Then it’s blindingly obvious what you’re doing.

If there’s a desire to keep these methods around, how about renaming them so it’s clear what you’re doing? Perhaps mass_update or update_each? update_all might be nice, but the behaviour would not be the same as when used with scopes/relations like User.where(x: "y").update_all(x: "z") which just fires-off a single UPDATE.

4 Likes

Bravo. What an amazing find. I realize that it is only luck that has kept me from finding this particular “sharp knife” the hard way. I wonder what the original intent was for this method?

Wow! I had never thought that this was possible, because we have the update_all(..) class method to update records in bulk, I never noticed that it was possible to use just update.

I could be the victim of such a fatality. I will be extremely more attentive when making occasional data updates.