Skip to content

Commit

Permalink
Drop the setter methods cache in favour of respond_to? (see #472)
Browse files Browse the repository at this point in the history
The cache was not being updated when new attributes were added,
causing very strange behaviour. Any previously unseen attributes
passed to new with send_only_modified_attributes enabled would be
missing from the request parameters. This is because change tracking
is only effective for setter methods.

Updating the cache turned out to be more expensive than not having a
cache at all. Using respond_to? would be fastest but this breaks
things as respond_to_missing? returns true for any assignment method.

If method(:foo=).source_location returns nil then this may indicate
that the method is missing but this is also true for methods in native
code.

A faster and more reliable approach is using a fiber-local variable to
make respond_to_missing? return false while doing these checks. It's a
tad ugly but it's the best I can come up with.
  • Loading branch information
chewi committed Sep 19, 2018
1 parent 8b7c6e9 commit 0632835
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 11 deletions.
22 changes: 11 additions & 11 deletions lib/her/model/attributes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,19 @@ def method_missing(method, *args, &blk)

# @private
def respond_to_missing?(method, include_private = false)
return false if Thread.current[:her_respond_to_missing]
method.to_s =~ /[?=]$/ || @_her_attributes.include?(method) || super
end

def respond_to_without_missing?(method, include_private = false)
Thread.current[:her_respond_to_missing] = true
respond_to?(method, include_private)
ensure
# Normally we would use nil to delete the variable but this is
# slightly more expensive and performance counts here.
Thread.current[:her_respond_to_missing] = false
end

# Assign new attributes to a resource
#
# @example
Expand Down Expand Up @@ -201,10 +211,9 @@ def use_setter_methods(model, params = {})
reserved = [:id, model.class.primary_key, *model.class.association_keys]
model.class.attributes *params.keys.reject { |k| reserved.include?(k) }

setter_method_names = model.class.setter_method_names
params.each_with_object({}) do |(key, value), memo|
setter_method = "#{key}="
if setter_method_names.include?(setter_method)
if model.respond_to_without_missing?(setter_method)
model.send setter_method, value
else
memo[key.to_sym] = value
Expand Down Expand Up @@ -278,15 +287,6 @@ def store_metadata(value = nil)
store_her_data(:metadata, value)
end

# @private
def setter_method_names
@_her_setter_method_names ||= begin
instance_methods.each_with_object(Set.new) do |method, memo|
memo << method.to_s if method.to_s.end_with?('=')
end
end
end

private

# @private
Expand Down
12 changes: 12 additions & 0 deletions spec/model/attributes_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,18 @@ def document=(document)
@user = Foo::User.find(1)
expect(@user.document).to eq("http://example.com")
end

it 'exposes the method to respond_to? and respond_to_without_missing?' do
@user = Foo::User.find(1)
expect(@user.respond_to?(:document=)).to be_truthy
expect(@user.respond_to_without_missing?(:document=)).to be_truthy
end

it 'exposes a non-existent method to respond_to? but not respond_to_without_missing?' do
@user = Foo::User.find(1)
expect(@user.respond_to?(:nonexistent=)).to be_truthy
expect(@user.respond_to_without_missing?(:nonexistent=)).to be_falsey
end
end

context "for predicate method" do
Expand Down

0 comments on commit 0632835

Please sign in to comment.