Skip to content
Browse files

Remove unnecessary code from define_read_method and add assertion to …

…make sure the underscored version is actually generated
  • Loading branch information...
1 parent 3d44e45 commit bbe0a507f287c20ab4ae8a244fbfc810665deda5 @jonleighton jonleighton committed
View
11 activerecord/lib/active_record/attribute_methods/read.rb
@@ -70,13 +70,10 @@ def define_read_method(symbol, attr_name, column)
if cache_attribute?(attr_name)
access_code = "@attributes_cache['#{attr_name}'] ||= (#{access_code})"
end
- if symbol =~ /^[a-zA-Z_]\w*[!?=]?$/
- generated_attribute_methods.module_eval("def _#{symbol}; #{access_code}; end; alias #{symbol} _#{symbol}", __FILE__, __LINE__)
@spastorino Ruby on Rails member

This was there for performance reasons.
It's faster module_eval with a string than using a block.

@josevalim Ruby on Rails member

Yes, yes. Please do revert. :heart:

@jonleighton Ruby on Rails member

Hey,

This code is confusing the hell out of me, partly because of really subtle differences between master and 3-0-stable. For example, in 3-0-stable symbol here is a Symbol, and under 1.8 Symbol#=~ is always false, so the first branch would never run. I just realised this is not the case on master though because symbol is actually exactly the same object (a string) as attr_name. (Well, except for the primary key where symbol will be :id if the attr_name is not "id".)

Also, why do we even allow attributes which do not match the regexp above? While it's technically possible to define a method with spaces in etc, surely we should just raise an error for this? (If it's to support weird database schemas, could we not just silently ignore such attributes (not defining any method), and let the user use read_attribute to access the attributes (which is hopefully what those few people are doing anyway, rather than using send() or something.

Questions, questions... I agree we should switch back to the string module eval though (and make sure the branch does actually get run), just wanna get my head around it all before I make more changes.

Jon

@josevalim Ruby on Rails member

Attributes that do not match the Regexp above are sometimes things generated by the database, that you cannot override. Like "my_column(omg)", and if you don't allow it, it simply explodes. About the differences between master and 3-0-stable I was unaware, but indeed they should behave the same. maybe we should add some comments when we revert it. Thanks Jon! :heart:

@jonleighton Ruby on Rails member

Right, but how about simply returning from define_read_method if the attr_name doesn't match the regexp? Simplicity++

@jonleighton Ruby on Rails member

To be clear, I am suggesting that we would return from define_read_method and not define anything, in this case.

@josevalim Ruby on Rails member

Yes, that may work. Because using self["weird(method(name))"] is easier than self.send("weird(method(name))"). Sending the method and accessing it through self[] returns the same result, right?

@jonleighton Ruby on Rails member

Base#[] delegates to read_attribute which will call _weird(method(name)) if it exists, or else just get the attribute and type cast it (in _read_attribute).

So doing this would lose us the caching of the type-casted values for oddly named attributes. I am not sure if this is a problem. Do you know if these types of attributes are actually used much or is it just a theoretical edge case? Perhaps it is better to leave as-is after all though... :/

@jonleighton Ruby on Rails member

Okay, I fixed this in:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- else
- generated_attribute_methods.module_eval do
- define_method("_#{symbol}") { eval(access_code) }
- alias_method(symbol, "_#{symbol}")
- end
+
+ generated_attribute_methods.module_eval do
+ define_method("_#{attr_name}") { eval(access_code) }
+ alias_method(attr_name, "_#{attr_name}")
end
end
end
View
1 activerecord/test/cases/attribute_methods_test.rb
@@ -77,6 +77,7 @@ def test_set_attributes_with_block
def test_respond_to?
topic = Topic.find(1)
assert_respond_to topic, "title"
+ assert_respond_to topic, "_title"
assert_respond_to topic, "title?"
assert_respond_to topic, "title="
assert_respond_to topic, :title

0 comments on commit bbe0a50

Please sign in to comment.
Something went wrong with that request. Please try again.