pboling / flag_shih_tzu

Bit fields for ActiveRecord
http://railsbling.com/flag_shih_tzu
MIT License
496 stars 64 forks source link

Suggestion flag_query_mode: bit_operator by default #87

Open giovannibonetti opened 4 years ago

giovannibonetti commented 4 years ago

Thanks for the awesome gem!

Anyway, after using it in production for a few months I would like to suggest (with a PR afterwards) to change the default query mode from :in_list to :bit_operator. Even though performance-wise it seems like :in_list is best for MySQL (haven't measured it with Postgres), I would like to argue that :bit_operator is safer.

The reason is that the :in_list query only works if the number of flags is fixed.

# reference state
class User < ApplicationRecord
  has_flags 1 => :warpdrive,
            2 => :shields
end

User.warprive.to_sql
=> "SELECT users.* FROM users WHERE users.flags in (1,3)

However, consider the case where a new flag is added:

# new state
class User < ApplicationRecord
  has_flags 1 => :warpdrive,
            2 => :shields,
            3 => :premium
            :flag_query_mode => :bit_operator
end

And in the same deploy a migration sets the flag for some records:

# db/migrate/...
class AddNewFlagToUsersTable < ApplicationRecord
  def up
    execute <<~SQL
      UPDATE users
        SET flags = flags | 4
        WHERE created_at < '...'
    SQL
  end

  def down
    # noop
  end
end

During the deploy the following query done in the old version of the app becomes invalid:

User.warprive.to_sql
=> "SELECT users.* FROM users WHERE users.flags in (1,3)

However, if the default query mode would be :bit_operator instead of :in_list, this problem wouldn't happen:

User.warprive.to_sql
=> "SELECT users.* FROM users WHERE users.flags & 1 = 1

The :bit_operator works regardless of the number of flags, so there would be no issue during a deploy setting a new flag.

Anyway, I decided to open this issue today because in my company we have been bitten by this a couple of times, and now we try to remember to always add :flag_query_mode => :bit_operator when using has_flags. I wonder if other users have been bitten by that, too. I'm a fan of the principle of least surprise, and think that safety should come before performance. If someone needs more performance he/she can always add the option later.