Skip to content

Commit

Permalink
Always cast enummer values to symbols
Browse files Browse the repository at this point in the history
Support for using strings as setters was added in 441cdf6, but the
initial implementation suffers from an idempotency issue, as changes
would be committed to the DB even if the values were the same. Consider
the following scenario:

    u = User.last
    => #<User:0x00007c747d137810 id: 3, permissions: [:read, :write], facial_features: [], diets: [], transport: [], home: []>

    u.permissions = u.permissions.map(&:to_s)
    => ["read", "write"]

    u.changes
    => {"permissions"=>[[:read, :write], ["read", "write"]]}

Because we cast the values to symbols when serializing, we set the
correct bitmask but this comes with the drawback of altering the DB
even if the new values are the same.

A better way to handle this issue is to delegate the responsibility to
the `cast` method. If we do this we can drop the initial fix, as values
will _always_ be symbols.

It's tricky to test something like this, so I've opted to hook into
ActiveRecord notifications and confirm no SQL UPDATE is being triggered
if the values are the same.
  • Loading branch information
GabrielNagy committed Aug 29, 2024
1 parent 007a36f commit b051e9e
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 2 deletions.
8 changes: 7 additions & 1 deletion lib/enummer/enummer_type.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def type
def serialize(value)
return unless value

Array.wrap(value).sum { |value_name| @values.fetch(value_name.to_sym, 0) }
Array.wrap(value).sum { |value_name| @values.fetch(value_name, 0) }
end

# @param [Numeric] value Numeric representation of values
Expand All @@ -36,5 +36,11 @@ def deserialize(value)
value_names << pair_name
end
end

# @param [Array<Symbol>] value Current value represented as one or more symbols or strings
# @return [Array<Symbol>] Current value represented as symbols
def cast(value)
Array.wrap(value).map(&:to_sym)
end
end
end
11 changes: 10 additions & 1 deletion test/enummer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,18 @@ def setup
end

test "setting the attribute with strings adds the values" do
@user3.update(permissions: ["read", "write"])
@user3.update(permissions: %w[read write])

assert_equal %i[read write], @user3.permissions

updated = false
callback = lambda { |_name, _start, _finish, _id, payload| updated = true if payload[:sql].starts_with?("UPDATE") }

ActiveSupport::Notifications.subscribed(callback, "sql.active_record") do
@user3.update(permissions: %w[read write])
end

refute updated, "subsequent updates with the same values should be idempotent"
end

test "using a bang method properly updates the persisted field" do
Expand Down

0 comments on commit b051e9e

Please sign in to comment.