Skip to content
Browse files

Refactoring: replace the mix of variables like @finder_sql, @counter_…

…sql, etc with just a single scope hash (created on initialization of the proxy). This is now used consistently across all associations. Therefore, all you have to do to ensure finding/counting etc is done correctly is implement the scope correctly.
  • Loading branch information...
1 parent 2a47e7e commit b82fab25f999dd6245c23a22f948048eef2d5d9a @jonleighton jonleighton committed with tenderlove
View
66 activerecord/lib/active_record/associations/association_collection.rb
@@ -19,11 +19,6 @@ module Associations
# If you need to work on all current children, new and existing records,
# +load_target+ and the +loaded+ flag are your friends.
class AssociationCollection < AssociationProxy #:nodoc:
- def initialize(owner, reflection)
- super
- construct_sql
- end
-
delegate :group, :order, :limit, :joins, :where, :preload, :eager_load, :includes, :from, :lock, :readonly, :having, :to => :scoped
def select(select = nil)
@@ -36,7 +31,7 @@ def select(select = nil)
end
def scoped
- with_scope(construct_scope) { @reflection.klass.scoped }
+ with_scope(@scope) { @reflection.klass.scoped }
end
def find(*args)
@@ -58,9 +53,7 @@ def find(*args)
merge_options_from_reflection!(options)
construct_find_options!(options)
- find_scope = construct_scope[:find].slice(:conditions, :order)
-
- with_scope(:find => find_scope) do
+ with_scope(:find => @scope[:find].slice(:conditions, :order)) do
relation = @reflection.klass.send(:construct_finder_arel, options, @reflection.klass.send(:current_scoped_methods))
case args.first
@@ -178,17 +171,18 @@ def sum(*args)
end
end
- # Count all records using SQL. If the +:counter_sql+ option is set for the association, it will
- # be used for the query. If no +:counter_sql+ was supplied, but +:finder_sql+ was set, the
- # descendant's +construct_sql+ method will have set :counter_sql automatically.
- # Otherwise, construct options and pass them with scope to the target class's +count+.
+ # Count all records using SQL. If the +:counter_sql+ or +:finder_sql+ option is set for the
+ # association, it will be used for the query. Otherwise, construct options and pass them with
+ # scope to the target class's +count+.
def count(column_name = nil, options = {})
column_name, options = nil, column_name if column_name.is_a?(Hash)
- if @reflection.options[:counter_sql] && !options.blank?
- raise ArgumentError, "If finder_sql/counter_sql is used then options cannot be passed"
- elsif @reflection.options[:counter_sql]
- @reflection.klass.count_by_sql(@counter_sql)
+ if @reflection.options[:counter_sql] || @reflection.options[:finder_sql]
+ unless options.blank?
+ raise ArgumentError, "If finder_sql/counter_sql is used then options cannot be passed"
+ end
+
+ @reflection.klass.count_by_sql(custom_counter_sql)
else
if @reflection.options[:uniq]
@@ -197,7 +191,7 @@ def count(column_name = nil, options = {})
options.merge!(:distinct => true)
end
- value = @reflection.klass.send(:with_scope, construct_scope) { @reflection.klass.count(column_name, options) }
+ value = @reflection.klass.send(:with_scope, @scope) { @reflection.klass.count(column_name, options) }
limit = @reflection.options[:limit]
offset = @reflection.options[:offset]
@@ -377,18 +371,6 @@ def proxy_respond_to?(method, include_private = false)
def construct_find_options!(options)
end
- def construct_counter_sql
- if @reflection.options[:counter_sql]
- @counter_sql = interpolate_sql(@reflection.options[:counter_sql])
- elsif @reflection.options[:finder_sql]
- # replace the SELECT clause with COUNT(*), preserving any hints within /* ... */
- @reflection.options[:counter_sql] = @reflection.options[:finder_sql].sub(/SELECT\b(\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" }
- @counter_sql = interpolate_sql(@reflection.options[:counter_sql])
- else
- @counter_sql = @finder_sql
- end
- end
-
def load_target
if !@owner.new_record? || foreign_key_present
begin
@@ -434,9 +416,9 @@ def method_missing(method, *args)
elsif @reflection.klass.scopes[method]
@_named_scopes_cache ||= {}
@_named_scopes_cache[method] ||= {}
- @_named_scopes_cache[method][args] ||= with_scope(construct_scope) { @reflection.klass.send(method, *args) }
+ @_named_scopes_cache[method][args] ||= with_scope(@scope) { @reflection.klass.send(method, *args) }
else
- with_scope(construct_scope) do
+ with_scope(@scope) do
if block_given?
@reflection.klass.send(method, *args) { |*block_args| yield(*block_args) }
else
@@ -446,9 +428,19 @@ def method_missing(method, *args)
end
end
- # overloaded in derived Association classes to provide useful scoping depending on association type.
- def construct_scope
- {}
+ def custom_counter_sql
+ if @reflection.options[:counter_sql]
+ counter_sql = @reflection.options[:counter_sql]
+ else
+ # replace the SELECT clause with COUNT(*), preserving any hints within /* ... */
+ counter_sql = @reflection.options[:finder_sql].sub(/SELECT\b(\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" }
+ end
+
+ interpolate_sql(counter_sql)
+ end
+
+ def custom_finder_sql
+ interpolate_sql(@reflection.options[:finder_sql])
end
def reset_target!
@@ -462,7 +454,7 @@ def reset_named_scopes_cache!
def find_target
records =
if @reflection.options[:finder_sql]
- @reflection.klass.find_by_sql(@finder_sql)
+ @reflection.klass.find_by_sql(custom_finder_sql)
else
find(:all)
end
@@ -494,7 +486,7 @@ def create_record(attrs)
ensure_owner_is_not_new
scoped_where = scoped.where_values_hash
- create_scope = scoped_where ? construct_scope[:create].merge(scoped_where) : construct_scope[:create]
+ create_scope = scoped_where ? @scope[:create].merge(scoped_where) : @scope[:create]
record = @reflection.klass.send(:with_scope, :create => create_scope) do
@reflection.build_association(attrs)
end
View
19 activerecord/lib/active_record/associations/association_proxy.rb
@@ -61,6 +61,7 @@ def initialize(owner, reflection)
reflection.check_validity!
Array.wrap(reflection.options[:extend]).each { |ext| proxy_extend(ext) }
reset
+ construct_scope
end
# Returns the owner of the proxy.
@@ -203,6 +204,24 @@ def with_scope(*args, &block)
@reflection.klass.send :with_scope, *args, &block
end
+ # Construct the scope used for find/create queries on the target
+ def construct_scope
+ @scope = {
+ :find => construct_find_scope,
+ :create => construct_create_scope
+ }
+ end
+
+ # Implemented by subclasses
+ def construct_find_scope
+ raise NotImplementedError
+ end
+
+ # Implemented by (some) subclasses
+ def construct_create_scope
+ {}
+ end
+
private
# Forwards any missing method call to the \target.
def method_missing(method, *args)
View
20 activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -50,19 +50,21 @@ def find_target
"find"
end
- options = @reflection.options.dup
- (options.keys - [:select, :include, :readonly]).each do |key|
- options.delete key
- end
- options[:conditions] = conditions
+ options = @reflection.options.dup.slice(:select, :include, :readonly)
- the_target = @reflection.klass.send(find_method,
- @owner[@reflection.primary_key_name],
- options
- ) if @owner[@reflection.primary_key_name]
+ the_target = with_scope(:find => @scope[:find]) do
+ @reflection.klass.send(find_method,
+ @owner[@reflection.primary_key_name],
+ options
+ ) if @owner[@reflection.primary_key_name]
+ end
set_inverse_instance(the_target, @owner)
the_target
end
+
+ def construct_find_scope
+ { :conditions => conditions }
+ end
def foreign_key_present
!@owner[@reflection.primary_key_name].nil?
View
22 activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
@@ -44,20 +44,20 @@ def set_inverse_instance(record, instance)
end
end
+ def construct_find_scope
+ { :conditions => conditions }
+ end
+
def find_target
return nil if association_class.nil?
- target =
- if @reflection.options[:conditions]
- association_class.find(
- @owner[@reflection.primary_key_name],
- :select => @reflection.options[:select],
- :conditions => conditions,
- :include => @reflection.options[:include]
- )
- else
- association_class.find(@owner[@reflection.primary_key_name], :select => @reflection.options[:select], :include => @reflection.options[:include])
- end
+ target = association_class.send(:with_scope, :find => @scope[:find]) do
+ association_class.find(
+ @owner[@reflection.primary_key_name],
+ :select => @reflection.options[:select],
+ :include => @reflection.options[:include]
+ )
+ end
set_inverse_instance(target, @owner)
target
end
View
37 activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -24,7 +24,7 @@ def has_primary_key?
protected
def construct_find_options!(options)
- options[:joins] = Arel::SqlLiteral.new @join_sql
+ options[:joins] = Arel::SqlLiteral.new(@scope[:find][:joins])
options[:readonly] = finding_with_ambiguous_select?(options[:select] || @reflection.options[:select])
options[:select] ||= (@reflection.options[:select] || Arel::SqlLiteral.new('*'))
end
@@ -80,27 +80,26 @@ def delete_records(records)
).delete
end
end
+
+ def construct_joins
+ "INNER JOIN #{@owner.connection.quote_table_name @reflection.options[:join_table]} ON #{@reflection.quoted_table_name}.#{@reflection.klass.primary_key} = #{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.association_foreign_key}"
+ end
- def construct_sql
- if @reflection.options[:finder_sql]
- @finder_sql = interpolate_sql(@reflection.options[:finder_sql])
- else
- @finder_sql = "#{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.primary_key_name} = #{owner_quoted_id} "
- @finder_sql << " AND (#{conditions})" if conditions
- end
-
- @join_sql = "INNER JOIN #{@owner.connection.quote_table_name @reflection.options[:join_table]} ON #{@reflection.quoted_table_name}.#{@reflection.klass.primary_key} = #{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.association_foreign_key}"
-
- construct_counter_sql
+ def construct_conditions
+ sql = "#{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.primary_key_name} = #{owner_quoted_id} "
+ sql << " AND (#{conditions})" if conditions
+ sql
end
- def construct_scope
- { :find => { :conditions => @finder_sql,
- :joins => @join_sql,
- :readonly => false,
- :order => @reflection.options[:order],
- :include => @reflection.options[:include],
- :limit => @reflection.options[:limit] } }
+ def construct_find_scope
+ {
+ :conditions => construct_conditions,
+ :joins => construct_joins,
+ :readonly => false,
+ :order => @reflection.options[:order],
+ :include => @reflection.options[:include],
+ :limit => @reflection.options[:limit]
+ }
end
# Join tables with additional columns on top of the two foreign keys must be considered
View
56 activerecord/lib/active_record/associations/has_many_association.rb
@@ -6,14 +6,10 @@ module Associations
# If the association has a <tt>:through</tt> option further specialization
# is provided by its child HasManyThroughAssociation.
class HasManyAssociation < AssociationCollection #:nodoc:
- def initialize(owner, reflection)
- @finder_sql = nil
- super
- end
protected
def owner_quoted_id
if @reflection.options[:primary_key]
- quote_value(@owner.send(@reflection.options[:primary_key]))
+ @owner.class.quote_value(@owner.send(@reflection.options[:primary_key]))
else
@owner.quoted_id
end
@@ -35,10 +31,10 @@ def owner_quoted_id
def count_records
count = if has_cached_counter?
@owner.send(:read_attribute, cached_counter_attribute_name)
- elsif @reflection.options[:counter_sql]
- @reflection.klass.count_by_sql(@counter_sql)
+ elsif @reflection.options[:counter_sql] || @reflection.options[:finder_sql]
+ @reflection.klass.count_by_sql(custom_counter_sql)
else
- @reflection.klass.count(:conditions => @counter_sql, :include => @reflection.options[:include])
+ @reflection.klass.count(@scope[:find].slice(:conditions, :joins, :include))
end
# If there's nothing in the database and @target has no new records
@@ -87,36 +83,32 @@ def target_obsolete?
false
end
- def construct_sql
- case
- when @reflection.options[:finder_sql]
- @finder_sql = interpolate_sql(@reflection.options[:finder_sql])
-
- when @reflection.options[:as]
- @finder_sql =
- "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " +
- "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
- @finder_sql << " AND (#{conditions})" if conditions
-
- else
- @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}"
- @finder_sql << " AND (#{conditions})" if conditions
+ def construct_conditions
+ if @reflection.options[:as]
+ sql =
+ "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " +
+ "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
+ else
+ sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}"
end
+ sql << " AND (#{conditions})" if conditions
+ sql
+ end
- construct_counter_sql
+ def construct_find_scope
+ {
+ :conditions => construct_conditions,
+ :readonly => false,
+ :order => @reflection.options[:order],
+ :limit => @reflection.options[:limit],
+ :include => @reflection.options[:include]
+ }
end
- def construct_scope
+ def construct_create_scope
create_scoping = {}
set_belongs_to_association_for(create_scoping)
- {
- :find => { :conditions => @finder_sql,
- :readonly => false,
- :order => @reflection.options[:order],
- :limit => @reflection.options[:limit],
- :include => @reflection.options[:include]},
- :create => create_scoping
- }
+ create_scoping
end
def we_can_set_the_inverse_on_this?(record)
View
16 activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -82,21 +82,7 @@ def delete_records(records)
def find_target
return [] unless target_reflection_has_associated_record?
- with_scope(construct_scope) { @reflection.klass.find(:all) }
- end
-
- def construct_sql
- case
- when @reflection.options[:finder_sql]
- @finder_sql = interpolate_sql(@reflection.options[:finder_sql])
-
- @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}"
- @finder_sql << " AND (#{conditions})" if conditions
- else
- @finder_sql = construct_conditions
- end
-
- construct_counter_sql
+ with_scope(@scope) { @reflection.klass.find(:all) }
end
def has_cached_counter?
View
39 activerecord/lib/active_record/associations/has_one_association.rb
@@ -2,11 +2,6 @@ module ActiveRecord
# = Active Record Belongs To Has One Association
module Associations
class HasOneAssociation < AssociationProxy #:nodoc:
- def initialize(owner, reflection)
- super
- construct_sql
- end
-
def create(attrs = {}, replace_existing = true)
new_record(replace_existing) do |reflection|
attrs = merge_with_conditions(attrs)
@@ -79,33 +74,31 @@ def owner_quoted_id
private
def find_target
- options = @reflection.options.dup
- (options.keys - [:select, :order, :include, :readonly]).each do |key|
- options.delete key
- end
- options[:conditions] = @finder_sql
+ options = @reflection.options.dup.slice(:select, :order, :include, :readonly)
- the_target = @reflection.klass.find(:first, options)
+ the_target = with_scope(:find => @scope[:find]) do
+ @reflection.klass.find(:first, options)
+ end
set_inverse_instance(the_target, @owner)
the_target
end
- def construct_sql
- case
- when @reflection.options[:as]
- @finder_sql =
- "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " +
- "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
- else
- @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}"
+ def construct_find_scope
+ if @reflection.options[:as]
+ sql =
+ "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " +
+ "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
+ else
+ sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}"
end
- @finder_sql << " AND (#{conditions})" if conditions
+ sql << " AND (#{conditions})" if conditions
+ { :conditions => sql }
end
- def construct_scope
+ def construct_create_scope
create_scoping = {}
set_belongs_to_association_for(create_scoping)
- { :create => create_scoping }
+ create_scoping
end
def new_record(replace_existing)
@@ -113,7 +106,7 @@ def new_record(replace_existing)
# instance. Otherwise, if the target has not previously been loaded
# elsewhere, the instance we create will get orphaned.
load_target if replace_existing
- record = @reflection.klass.send(:with_scope, :create => construct_scope[:create]) do
+ record = @reflection.klass.send(:with_scope, :create => @scope[:create]) do
yield @reflection
end
View
2 activerecord/lib/active_record/associations/has_one_through_association.rb
@@ -33,7 +33,7 @@ def create_through_record(new_value) #nodoc:
private
def find_target
- with_scope(construct_scope) { @reflection.klass.find(:first) }
+ with_scope(@scope) { @reflection.klass.find(:first) }
end
end
end
View
24 activerecord/lib/active_record/associations/through_association_scope.rb
@@ -5,16 +5,20 @@ module ThroughAssociationScope
protected
- def construct_scope
- { :create => construct_owner_attributes(@reflection),
- :find => { :conditions => construct_conditions,
- :joins => construct_joins,
- :include => @reflection.options[:include] || @reflection.source_reflection.options[:include],
- :select => construct_select,
- :order => @reflection.options[:order],
- :limit => @reflection.options[:limit],
- :readonly => @reflection.options[:readonly],
- } }
+ def construct_find_scope
+ {
+ :conditions => construct_conditions,
+ :joins => construct_joins,
+ :include => @reflection.options[:include] || @reflection.source_reflection.options[:include],
+ :select => construct_select,
+ :order => @reflection.options[:order],
+ :limit => @reflection.options[:limit],
+ :readonly => @reflection.options[:readonly]
+ }
+ end
+
+ def construct_create_scope
+ construct_owner_attributes(@reflection)
end
# Build SQL conditions from attributes, qualified by table name.
View
4 activerecord/lib/active_record/autosave_association.rb
@@ -322,8 +322,8 @@ def save_collection_association(reflection)
end
end
- # reconstruct the SQL queries now that we know the owner's id
- association.send(:construct_sql) if association.respond_to?(:construct_sql)
+ # reconstruct the scope now that we know the owner's id
+ association.send(:construct_scope) if association.respond_to?(:construct_scope)
end
end

0 comments on commit b82fab2

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