From def54d44356759faedf4bc1f9e4365291086ef8f Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Thu, 21 Oct 2010 17:35:07 +0100 Subject: [PATCH] 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. --- .../associations/association_collection.rb | 66 +++++++++----------- .../associations/association_proxy.rb | 19 ++++++ .../associations/belongs_to_association.rb | 20 +++--- .../belongs_to_polymorphic_association.rb | 22 +++--- .../has_and_belongs_to_many_association.rb | 37 +++++------ .../associations/has_many_association.rb | 56 +++++++---------- .../associations/has_many_through_association.rb | 16 +----- .../associations/has_one_association.rb | 39 +++++------- .../associations/has_one_through_association.rb | 2 +- .../associations/through_association_scope.rb | 24 ++++--- .../lib/active_record/autosave_association.rb | 4 +- 11 files changed, 146 insertions(+), 159 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index cb2d9e0..896e18a 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -19,11 +19,6 @@ module ActiveRecord # 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 @@ module ActiveRecord end def scoped - with_scope(construct_scope) { @reflection.klass.scoped } + with_scope(@scope) { @reflection.klass.scoped } end def find(*args) @@ -58,9 +53,7 @@ module ActiveRecord 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 @@ module ActiveRecord 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 @@ module ActiveRecord 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 @@ module ActiveRecord 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 @@ module ActiveRecord 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 @@ module ActiveRecord 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 @@ module ActiveRecord 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 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb index f333f4d..0c12c37 100644 --- a/activerecord/lib/active_record/associations/association_proxy.rb +++ b/activerecord/lib/active_record/associations/association_proxy.rb @@ -61,6 +61,7 @@ module ActiveRecord 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 @@ module ActiveRecord @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) diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb index 2eb56e5..34b6cd5 100644 --- a/activerecord/lib/active_record/associations/belongs_to_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_association.rb @@ -50,19 +50,21 @@ module ActiveRecord "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? diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb index e429806..a0df860 100644 --- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb @@ -44,20 +44,20 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb index eb65234..1fc9aba 100644 --- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb +++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb @@ -24,7 +24,7 @@ module ActiveRecord 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 @@ module ActiveRecord ).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 diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index 978fc74..830a829 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -6,14 +6,10 @@ module ActiveRecord # If the association has a :through 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 @@ module ActiveRecord 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 @@ module ActiveRecord 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) diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index 97883d8..437c8b1 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -82,21 +82,7 @@ module ActiveRecord 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? diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index a6e6bfa..1790138 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/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 @@ module ActiveRecord 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 @@ module ActiveRecord # 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 diff --git a/activerecord/lib/active_record/associations/has_one_through_association.rb b/activerecord/lib/active_record/associations/has_one_through_association.rb index fba0a2b..7f28abf 100644 --- a/activerecord/lib/active_record/associations/has_one_through_association.rb +++ b/activerecord/lib/active_record/associations/has_one_through_association.rb @@ -33,7 +33,7 @@ module ActiveRecord private def find_target - with_scope(construct_scope) { @reflection.klass.find(:first) } + with_scope(@scope) { @reflection.klass.find(:first) } end end end diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index cabb33c..bd8e304 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -5,16 +5,20 @@ module ActiveRecord 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. diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 21a9a1f..f3f89fe 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -313,8 +313,8 @@ module ActiveRecord 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 -- 1.7.1