Permalink
Browse files

Remove the need for type_cast_attribute.

This is good because it reduces duplication.
  • Loading branch information...
1 parent 47b97a7 commit f1a534af98950efd9969deea1540717c4516d673 @jonleighton jonleighton committed Dec 1, 2011
@@ -29,6 +29,10 @@ def attribute_methods_generated?
end
end
+ def generated_attribute_methods
@josevalim

josevalim Dec 2, 2011

Contributor

Do we really want the modules to be shared up the chain? Can it lead to false positives? (a method does not exist but calling it would work because the module is shared?

@jonleighton

jonleighton Dec 2, 2011

Member

Hmmmm.....

Usually if two classes have the same base_class then they use the same DB table, therefore they will have exactly the same generated_attribute_methods (as the generated methods are 1-1 with the columns of the table).

However, I suppose someone could do this:

class A < AR::Base
  self.table_name = 'foo'
end

class B < A
  self.table_name = 'bar'
end

Should that be supported? It's pretty fucked up.

Potentially we could also change base_class to pay attention to the table_name (such that A.base_class == A and B.base_class == B in the above example). I think that would deal with it, but I need to think about the implications a bit more.

Thoughts welcome...

@jonleighton

jonleighton Dec 2, 2011

Member

BTW the reason I added this is that without it the generation of methods happened differently depending on which class was added first. Consider:

class A < AR::Base; end
class B < A; end

Suppose there is a title column. If you do:

A.new.title
B.new.title
  1. Line 1 generates attribute methods.
  2. Line 2 already responds to title, so nothing generated.

But if you do:

B.new.title
A.new.title
  1. Line 1 generates title method in module for B
  2. Line 2 generates title method in module for A
  3. Now if you do A.reset_column_information, B will still have a title method. (This was causing issues with some tests during this change, hence why I added this method.)
@josevalim

josevalim via email Dec 2, 2011

Contributor
@jonleighton

jonleighton Dec 3, 2011

Member

Nope, base_class is the last non-abstract AR::Base descendant in the ancestor chain.

+ @generated_attribute_methods ||= (base_class == self ? super : base_class.generated_attribute_methods)
+ end
+
def undefine_attribute_methods
if base_class == self
super
@@ -33,7 +33,7 @@ def undefine_attribute_methods
if base_class == self
generated_attribute_methods.module_eval do
public_methods(false).each do |m|
- singleton_class.send(:undef_method, m) if m.to_s =~ /^cast_/
+ singleton_class.send(:undef_method, m) if m.to_s =~ /^attribute_/
end
end
end
@@ -49,27 +49,27 @@ def undefine_attribute_methods
# The second, slower, branch is necessary to support instances where the database
# returns columns with extra stuff in (like 'my_column(omg)').
def define_method_attribute(attr_name)
- access_code = attribute_access_code(attr_name)
- cast_code = "v && (#{attribute_cast_code(attr_name)})"
+ internal = internal_attribute_access_code(attr_name)
+ external = external_attribute_access_code(attr_name)
if attr_name =~ ActiveModel::AttributeMethods::NAME_COMPILABLE_REGEXP
generated_attribute_methods.module_eval <<-STR, __FILE__, __LINE__
def #{attr_name}
- #{access_code}
+ #{internal}
end
- def self.cast_#{attr_name}(v)
- #{cast_code}
+ def self.attribute_#{attr_name}(v, attributes_cache, attr_name)
+ #{external}
end
STR
else
generated_attribute_methods.module_eval do
define_method(attr_name) do
- eval(access_code)
+ eval(internal)
end
- singleton_class.send(:define_method, "cast_#{attr_name}") do |v|
- eval(cast_code)
+ singleton_class.send(:define_method, "attribute_#{attr_name}") do |v, attributes_cache, attr_name|
+ eval(external)
end
end
end
@@ -80,7 +80,7 @@ def cacheable_column?(column)
attribute_types_cached_by_default.include?(column.type)
end
- def attribute_access_code(attr_name)
+ def internal_attribute_access_code(attr_name)
access_code = "(v=@attributes['#{attr_name}']) && #{attribute_cast_code(attr_name)}"
unless attr_name == self.primary_key
@@ -94,6 +94,16 @@ def attribute_access_code(attr_name)
access_code
end
+ def external_attribute_access_code(attr_name)
+ access_code = "v && #{attribute_cast_code(attr_name)}"
+
+ if cache_attribute?(attr_name)
+ access_code = "attributes_cache[attr_name] ||= (#{access_code})"
+ end
+
+ access_code
+ end
+
def attribute_cast_code(attr_name)
columns_hash[attr_name].type_cast_code('v')
end
@@ -103,36 +113,26 @@ def attribute_cast_code(attr_name)
# "2004-12-12" in a data column is cast to a date object, like Date.new(2004, 12, 12)).
def read_attribute(attr_name)
attr_name = attr_name.to_s
- caster = "cast_#{attr_name}"
+ accessor = "attribute_#{attr_name}"
methods = self.class.generated_attribute_methods
- if methods.respond_to?(caster)
+ if methods.respond_to?(accessor)
if @attributes.has_key?(attr_name)
- @attributes_cache[attr_name] || methods.send(caster, @attributes[attr_name])
+ methods.send(accessor, @attributes[attr_name], @attributes_cache, attr_name)
end
+ elsif !self.class.attribute_methods_generated?
+ # If we haven't generated the caster methods yet, do that and
+ # then try again
+ self.class.define_attribute_methods
+ read_attribute(attr_name)
else
- _read_attribute attr_name
- end
- end
-
- def _read_attribute(attr_name)
- attr_name = attr_name.to_s
- attr_name = self.class.primary_key if attr_name == 'id'
-
- unless @attributes[attr_name].nil?
- type_cast_attribute(column_for_attribute(attr_name), @attributes[attr_name])
+ # If we get here, the attribute has no associated DB column, so
+ # just return it verbatim.
+ @attributes[attr_name]
end
end
private
- def type_cast_attribute(column, value)
- if column
- column.type_cast(value)
- else
- value
- end
- end
-
def attribute(attribute_name)
read_attribute(attribute_name)
end
@@ -77,14 +77,6 @@ def set_serialized_attributes
end
end
- def type_cast_attribute(column, value)
- if column && self.class.serialized_attributes[column.name]
- value.unserialized_value
- else
- super
- end
- end
-
def type_cast_attribute_for_write(column, value)
if column && coder = self.class.serialized_attributes[column.name]
Attribute.new(coder, value, :unserialized)
@@ -19,19 +19,32 @@ module ClassMethods
# Defined for all +datetime+ and +timestamp+ attributes when +time_zone_aware_attributes+ are enabled.
# This enhanced read method automatically converts the UTC time stored in the database to the time
# zone stored in Time.zone.
- def attribute_access_code(attr_name)
- if create_time_zone_conversion_attribute?(attr_name, columns_hash[attr_name])
+ def internal_attribute_access_code(attr_name)
+ column = columns_hash[attr_name]
+
+ if create_time_zone_conversion_attribute?(attr_name, column)
<<-CODE
cached = @attributes_cache['#{attr_name}']
return cached if cached
- time = _read_attribute('#{attr_name}')
+ v = @attributes['#{attr_name}']
+ time = #{column.type_cast_code('v')}
@attributes_cache['#{attr_name}'] = time.acts_like?(:time) ? time.in_time_zone : time
CODE
else
super
end
end
+ def external_attribute_access_code(attr_name)
+ column = columns_hash[attr_name]
+
+ if create_time_zone_conversion_attribute?(attr_name, column)
+ "attributes_cache[attr_name] ||= (#{attribute_cast_code(attr_name)})"
+ else
+ super
+ end
+ end
+
def attribute_cast_code(attr_name)
if create_time_zone_conversion_attribute?(attr_name, columns_hash[attr_name])
"v.acts_like?(:time) ? v.in_time_zone : v"
@@ -1821,7 +1821,7 @@ def attribute_for_inspect(attr_name)
# Returns true if the specified +attribute+ has been set by the user or by a database load and is neither
# nil nor empty? (the latter only applies to objects that respond to empty?, most notably Strings).
def attribute_present?(attribute)
- value = _read_attribute(attribute)
+ value = read_attribute(attribute)
!value.nil? || (value.respond_to?(:empty?) && !value.empty?)
end

1 comment on commit f1a534a

Contributor

rubys commented on f1a534a Dec 3, 2011

This commit causes any use of to_xml to fail when applied to an ActiveRecord::Base containing a datetime attribute (like created_at or updated_at). Look at the following stack traceback for details: http://intertwingly.net/projects/AWDwR4/checkdepot-193/section-12.4.html

Please sign in to comment.