From 19d2ff83db5232a816dee201800baf3924705b31 Mon Sep 17 00:00:00 2001 From: Emilio Tagua Date: Wed, 29 Apr 2009 19:39:53 -0300 Subject: Calculations now use Arel to construct the query. Implemented other methods in AR::Base with Arel support. --- activerecord/lib/active_record/associations.rb | 28 +-- activerecord/lib/active_record/base.rb | 31 ++-- activerecord/lib/active_record/calculations.rb | 197 +++++++++------------ activerecord/lib/active_record/test_case.rb | 2 +- .../has_many_through_associations_test.rb | 44 ++--- activerecord/test/cases/calculations_test.rb | 2 +- activerecord/test/cases/inheritance_test.rb | 4 +- activerecord/test/cases/named_scope_test.rb | 2 +- arel | 2 +- 9 files changed, 150 insertions(+), 162 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 2115878e32..2dd1197192 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -510,14 +510,14 @@ module ActiveRecord # # Since only one table is loaded at a time, conditions or orders cannot reference tables other than the main one. If this is the case # Active Record falls back to the previously used LEFT OUTER JOIN based strategy. For example - # + # # Post.find(:all, :include => [ :author, :comments ], :conditions => ['comments.approved = ?', true]) # # will result in a single SQL query with joins along the lines of: LEFT OUTER JOIN comments ON comments.post_id = posts.id and # LEFT OUTER JOIN authors ON authors.id = posts.author_id. Note that using conditions like this can have unintended consequences. # In the above example posts with no approved comments are not returned at all, because the conditions apply to the SQL statement as a whole # and not just to the association. You must disambiguate column references for this fallback to happen, for example - # :order => "author.name DESC" will work but :order => "name DESC" will not. + # :order => "author.name DESC" will work but :order => "name DESC" will not. # # If you do want eagerload only some members of an association it is usually more natural to :include an association # which has conditions defined on it: @@ -551,10 +551,10 @@ module ActiveRecord # # Address.find(:all, :include => :addressable) # - # will execute one query to load the addresses and load the addressables with one query per addressable type. + # will execute one query to load the addresses and load the addressables with one query per addressable type. # For example if all the addressables are either of class Person or Company then a total of 3 queries will be executed. The list of # addressable types to load is determined on the back of the addresses loaded. This is not supported if Active Record has to fallback - # to the previous implementation of eager loading and will raise ActiveRecord::EagerLoadPolymorphicError. The reason is that the parent + # to the previous implementation of eager loading and will raise ActiveRecord::EagerLoadPolymorphicError. The reason is that the parent # model's type is a column value so its corresponding table name cannot be put in the +FROM+/+JOIN+ clauses of that query. # # == Table Aliasing @@ -869,7 +869,7 @@ module ActiveRecord # but not include the joined columns. Do not forget to include the primary and foreign keys, otherwise it will raise an error. # [:through] # Specifies a Join Model through which to perform the query. Options for :class_name and :foreign_key - # are ignored, as the association uses the source reflection. You can only use a :through query through a + # are ignored, as the association uses the source reflection. You can only use a :through query through a # has_one or belongs_to association on the join model. # [:source] # Specifies the source association name used by has_one :through queries. Only use it if the name cannot be @@ -1123,8 +1123,8 @@ module ActiveRecord # the association will use "project_id" as the default :association_foreign_key. # [:conditions] # Specify the conditions that the associated object must meet in order to be included as a +WHERE+ - # SQL fragment, such as authorized = 1. Record creations from the association are scoped if a hash is used. - # has_many :posts, :conditions => {:published => true} will create published posts with @blog.posts.create + # SQL fragment, such as authorized = 1. Record creations from the association are scoped if a hash is used. + # has_many :posts, :conditions => {:published => true} will create published posts with @blog.posts.create # or @blog.posts.build. # [:order] # Specify the order in which the associated objects are returned as an ORDER BY SQL fragment, @@ -1335,12 +1335,12 @@ module ActiveRecord "#{reflection.class_name}.send(:attr_readonly,\"#{cache_column}\".intern) if defined?(#{reflection.class_name}) && #{reflection.class_name}.respond_to?(:attr_readonly)" ) end - + def add_touch_callbacks(reflection, touch_attribute) method_name = "belongs_to_touch_after_save_or_destroy_for_#{reflection.name}".to_sym define_method(method_name) do association = send(reflection.name) - + if touch_attribute == true association.touch unless association.nil? else @@ -1552,7 +1552,7 @@ module ActiveRecord options[:extend] = create_extension_modules(association_id, extension, options[:extend]) reflection = create_reflection(:has_and_belongs_to_many, association_id, options, self) - + if reflection.association_foreign_key == reflection.primary_key_name raise HasAndBelongsToManyAssociationForeignKeyNeeded.new(reflection) end @@ -1607,6 +1607,14 @@ module ActiveRecord end end + def construct_limited_ids_condition(where, options, join_dependency) + unless (id_list = select_limited_ids_list(options, join_dependency)).empty? + "#{where.blank? ? 'WHERE ' : ' AND '} #{connection.quote_table_name table_name}.#{primary_key} IN (#{id_list}) " + else + throw :invalid_query + end + end + def select_limited_ids_list(options, join_dependency) pk = columns_hash[primary_key] diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 8bf92de6d4..8882a00dd5 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -904,9 +904,7 @@ module ActiveRecord #:nodoc: # Both calls delete the affected posts all at once with a single DELETE statement. If you need to destroy dependent # associations or call your before_* or +after_destroy+ callbacks, use the +destroy_all+ method instead. def delete_all(conditions = nil) - sql = "DELETE FROM #{quoted_table_name} " - add_conditions!(sql, conditions, scope(:find)) - connection.delete(sql, "#{name} Delete all") + Arel(table_name).where(construct_conditions(conditions, scope(:find))).delete end # Returns the result of an SQL statement that should only include a COUNT(*) in the SELECT part. @@ -1687,19 +1685,27 @@ module ActiveRecord #:nodoc: end end - def construct_finder_sql(options) + def arel_table(table) + Arel(table) + end + + def construct_finder_arel(options) scope = scope(:find) # TODO add lock to Arel - Arel(table_name). - join(construct_join(options[:joins], scope)). - where(construct_conditions(options[:conditions], scope)). + arel_table(options[:from] || table_name). + join(options[:merged_joins] || construct_join(options[:joins], scope)). + where(options[:merged_conditions] || construct_conditions(options[:conditions], scope)). project(options[:select] || (scope && scope[:select]) || default_select(options[:joins] || (scope && scope[:joins]))). group(construct_group(options[:group], options[:having], scope)). order(construct_order(options[:order], scope)). take(construct_limit(options, scope)). skip(construct_offset(options, scope) - ).to_sql + ) + end + + def construct_finder_sql(options) + construct_finder_arel(options).to_sql end def construct_join(joins, scope = :auto) @@ -1715,6 +1721,8 @@ module ActiveRecord #:nodoc: end when String " #{merged_joins} " + else + "" end end @@ -2644,11 +2652,8 @@ module ActiveRecord #:nodoc: # be made (since they can't be persisted). def destroy unless new_record? - connection.delete( - "DELETE FROM #{self.class.quoted_table_name} " + - "WHERE #{connection.quote_column_name(self.class.primary_key)} = #{quoted_id}", - "#{self.class.name} Destroy" - ) + table = Arel(self.class.table_name) + table.where(table[self.class.primary_key].eq(quoted_id)).delete end freeze diff --git a/activerecord/lib/active_record/calculations.rb b/activerecord/lib/active_record/calculations.rb index f077818d3b..9dbfcdf175 100644 --- a/activerecord/lib/active_record/calculations.rb +++ b/activerecord/lib/active_record/calculations.rb @@ -54,7 +54,7 @@ module ActiveRecord # # Person.average('age') # => 35.8 def average(column_name, options = {}) - calculate(:avg, column_name, options) + calculate(:average, column_name, options) end # Calculates the minimum value on a given column. The value is returned @@ -63,7 +63,7 @@ module ActiveRecord # # Person.minimum('age') # => 7 def minimum(column_name, options = {}) - calculate(:min, column_name, options) + calculate(:minimum, column_name, options) end # Calculates the maximum value on a given column. The value is returned @@ -72,7 +72,7 @@ module ActiveRecord # # Person.maximum('age') # => 93 def maximum(column_name, options = {}) - calculate(:max, column_name, options) + calculate(:maximum, column_name, options) end # Calculates the sum of values on a given column. The value is returned @@ -124,19 +124,96 @@ module ActiveRecord # Person.sum("2 * age") def calculate(operation, column_name, options = {}) validate_calculation_options(operation, options) - column_name = options[:select] if options[:select] - column_name = '*' if column_name == :all - column = column_for column_name + + scope = scope(:find) + + merged_includes = merge_includes(scope ? scope[:include] : [], options[:include]) + joins = construct_join(options[:joins], scope) + + if merged_includes.any? + join_dependency = ActiveRecord::Associations::ClassMethods::JoinDependency.new(self, merged_includes, joins) + joins << join_dependency.join_associations.collect{|join| join.association_join }.join + end + + if operation == :count + if merged_includes.any? + distinct = true + column_name = options[:select] || primary_key + end + + distinct = nil if column_name.to_s =~ /\s*DISTINCT\s+/i + distinct ||= options[:distinct] + else + distinct = nil + end + catch :invalid_query do + conditions = construct_conditions(options[:conditions], scope) + conditions << construct_limited_ids_condition(conditions, options, join_dependency) if join_dependency && !using_limitable_reflections?(join_dependency.reflections) && ((scope && scope[:limit]) || options[:limit]) + if options[:group] - return execute_grouped_calculation(operation, column_name, column, options) + return execute_grouped_calculation(operation, column_name, options.merge(:merged_conditions => conditions, :merged_joins => joins, :distinct => distinct)) else - return execute_simple_calculation(operation, column_name, column, options) + return execute_simple_calculation(operation, column_name, options.merge(:merged_conditions => conditions, :merged_joins => joins, :distinct => distinct)) end end 0 end + def execute_simple_calculation(operation, column_name, options) #:nodoc: + table = options[:from] || table_name + value = if operation == :count + if column_name == :all && options[:select].blank? + column_name = "*" + elsif !options[:select].blank? + column_name = options[:select] + end + construct_finder_arel(options.merge(:select => Arel::Attribute.new(Arel(table), column_name).count(options[:distinct]))).select_value + else + construct_finder_arel(options.merge(:select => Arel::Attribute.new(Arel(table), column_name).send(operation))).select_value + end + + type_cast_calculated_value(value, column_for(column_name), operation) + end + + def execute_grouped_calculation(operation, column_name, options) #:nodoc: + group_attr = options[:group].to_s + association = reflect_on_association(group_attr.to_sym) + associated = association && association.macro == :belongs_to # only count belongs_to associations + group_field = associated ? association.primary_key_name : group_attr + group_alias = column_alias_for(group_field) + group_column = column_for group_field + + options[:group] = connection.adapter_name == 'FrontBase' ? group_alias : group_field + + aggregate_alias = column_alias_for(operation, column_name) + if operation == :count && column_name == :all + options[:select] = "COUNT(*) AS count_all, #{group_field} AS #{group_alias}" + else + arel_column = Arel::Attribute.new(Arel(table_name), column_name).send(operation) + options[:select] = "#{arel_column.as(aggregate_alias).to_sql}, #{group_field} AS #{group_alias}" + end + + + sql = construct_finder_arel(options) + + calculated_data = connection.select_all(sql.to_sql) + + if association + key_ids = calculated_data.collect { |row| row[group_alias] } + key_records = association.klass.base_class.find(key_ids) + key_records = key_records.inject({}) { |hsh, r| hsh.merge(r.id => r) } + end + + calculated_data.inject(ActiveSupport::OrderedHash.new) do |all, row| + key = type_cast_calculated_value(row[group_alias], group_column) + key = key_records[key] if associated + value = row[aggregate_alias] + all[key] = type_cast_calculated_value(value, column_for(column_name), operation) + all + end + end + protected def construct_count_options_from_args(*args) options = {} @@ -167,108 +244,6 @@ module ActiveRecord [column_name || :all, options] end - def construct_calculation_sql(operation, column_name, options) #:nodoc: - operation = operation.to_s.downcase - options = options.symbolize_keys - - scope = scope(:find) - merged_includes = merge_includes(scope ? scope[:include] : [], options[:include]) - aggregate_alias = column_alias_for(operation, column_name) - column_name = "#{connection.quote_table_name(table_name)}.#{column_name}" if column_names.include?(column_name.to_s) - - if operation == 'count' - if merged_includes.any? - options[:distinct] = true - column_name = options[:select] || [connection.quote_table_name(table_name), primary_key] * '.' - end - - if options[:distinct] - use_workaround = !connection.supports_count_distinct? - end - end - - if options[:distinct] && column_name.to_s !~ /\s*DISTINCT\s+/i - distinct = 'DISTINCT ' - end - sql = "SELECT #{operation}(#{distinct}#{column_name}) AS #{aggregate_alias}" - - # A (slower) workaround if we're using a backend, like sqlite, that doesn't support COUNT DISTINCT. - sql = "SELECT COUNT(*) AS #{aggregate_alias}" if use_workaround - - sql << ", #{options[:group_field]} AS #{options[:group_alias]}" if options[:group] - if options[:from] - sql << " FROM #{options[:from]} " - else - sql << " FROM (SELECT #{distinct}#{column_name}" if use_workaround - sql << " FROM #{connection.quote_table_name(table_name)} " - end - - joins = "" - add_joins!(joins, options[:joins], scope) - - if merged_includes.any? - join_dependency = ActiveRecord::Associations::ClassMethods::JoinDependency.new(self, merged_includes, joins) - sql << join_dependency.join_associations.collect{|join| join.association_join }.join - end - - sql << joins unless joins.blank? - - add_conditions!(sql, options[:conditions], scope) - add_limited_ids_condition!(sql, options, join_dependency) if join_dependency && !using_limitable_reflections?(join_dependency.reflections) && ((scope && scope[:limit]) || options[:limit]) - - if options[:group] - group_key = connection.adapter_name == 'FrontBase' ? :group_alias : :group_field - sql << " GROUP BY #{options[group_key]} " - end - - if options[:group] && options[:having] - having = sanitize_sql_for_conditions(options[:having]) - - # FrontBase requires identifiers in the HAVING clause and chokes on function calls - if connection.adapter_name == 'FrontBase' - having.downcase! - having.gsub!(/#{operation}\s*\(\s*#{column_name}\s*\)/, aggregate_alias) - end - - sql << " HAVING #{having} " - end - - sql << " ORDER BY #{options[:order]} " if options[:order] - add_limit!(sql, options, scope) - sql << ") #{aggregate_alias}_subquery" if use_workaround - sql - end - - def execute_simple_calculation(operation, column_name, column, options) #:nodoc: - value = connection.select_value(construct_calculation_sql(operation, column_name, options)) - type_cast_calculated_value(value, column, operation) - end - - def execute_grouped_calculation(operation, column_name, column, options) #:nodoc: - group_attr = options[:group].to_s - association = reflect_on_association(group_attr.to_sym) - associated = association && association.macro == :belongs_to # only count belongs_to associations - group_field = associated ? association.primary_key_name : group_attr - group_alias = column_alias_for(group_field) - group_column = column_for group_field - sql = construct_calculation_sql(operation, column_name, options.merge(:group_field => group_field, :group_alias => group_alias)) - calculated_data = connection.select_all(sql) - aggregate_alias = column_alias_for(operation, column_name) - - if association - key_ids = calculated_data.collect { |row| row[group_alias] } - key_records = association.klass.base_class.find(key_ids) - key_records = key_records.inject({}) { |hsh, r| hsh.merge(r.id => r) } - end - - calculated_data.inject(ActiveSupport::OrderedHash.new) do |all, row| - key = type_cast_calculated_value(row[group_alias], group_column) - key = key_records[key] if associated - value = row[aggregate_alias] - all[key] = type_cast_calculated_value(value, column, operation) - all - end - end private def validate_calculation_options(operation, options = {}) @@ -304,7 +279,7 @@ module ActiveRecord case operation when 'count' then value.to_i when 'sum' then type_cast_using_column(value || '0', column) - when 'avg' then value && (value.is_a?(Fixnum) ? value.to_f : value).to_d + when 'average' then value && (value.is_a?(Fixnum) ? value.to_f : value).to_d else type_cast_using_column(value, column) end end diff --git a/activerecord/lib/active_record/test_case.rb b/activerecord/lib/active_record/test_case.rb index b790eb4343..2dfe2c09ea 100644 --- a/activerecord/lib/active_record/test_case.rb +++ b/activerecord/lib/active_record/test_case.rb @@ -20,7 +20,7 @@ module ActiveRecord patterns_to_match.each do |pattern| failed_patterns << pattern unless $queries_executed.any?{ |sql| pattern === sql } end - assert failed_patterns.empty?, "Query pattern(s) #{failed_patterns.map(&:inspect).join(', ')} not found in #{$queries_executed}" + assert failed_patterns.empty?, "Query pattern(s) #{failed_patterns.map(&:inspect).join(', ')} not found.#{$queries_executed.size == 0 ? '' : "\nQueries:\n#{$queries_executed.join("\n")}"}" end def assert_queries(num = 1) diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb index 97efca7891..b075db24e7 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -23,49 +23,49 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase assert_queries(1) do posts(:thinking).people << people(:david) end - + assert_queries(1) do assert posts(:thinking).people.include?(people(:david)) end - + assert posts(:thinking).reload.people(true).include?(people(:david)) end def test_associating_new assert_queries(1) { posts(:thinking) } new_person = nil # so block binding catches it - + assert_queries(0) do new_person = Person.new :first_name => 'bob' end - + # Associating new records always saves them # Thus, 1 query for the new person record, 1 query for the new join table record assert_queries(2) do posts(:thinking).people << new_person end - + assert_queries(1) do assert posts(:thinking).people.include?(new_person) end - + assert posts(:thinking).reload.people(true).include?(new_person) end def test_associate_new_by_building assert_queries(1) { posts(:thinking) } - + assert_queries(0) do posts(:thinking).people.build(:first_name=>"Bob") posts(:thinking).people.new(:first_name=>"Ted") end - + # Should only need to load the association once assert_queries(1) do assert posts(:thinking).people.collect(&:first_name).include?("Bob") assert posts(:thinking).people.collect(&:first_name).include?("Ted") end - + # 2 queries for each new record (1 to save the record itself, 1 for the join model) # * 2 new records = 4 # + 1 query to save the actual post = 5 @@ -73,22 +73,22 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase posts(:thinking).body += '-changed' posts(:thinking).save end - + assert posts(:thinking).reload.people(true).collect(&:first_name).include?("Bob") assert posts(:thinking).reload.people(true).collect(&:first_name).include?("Ted") end def test_delete_association assert_queries(2){posts(:welcome);people(:michael); } - + assert_queries(1) do posts(:welcome).people.delete(people(:michael)) end - + assert_queries(1) do assert posts(:welcome).people.empty? end - + assert posts(:welcome).reload.people(true).empty? end @@ -112,36 +112,36 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase def test_replace_association assert_queries(4){posts(:welcome);people(:david);people(:michael); posts(:welcome).people(true)} - + # 1 query to delete the existing reader (michael) # 1 query to associate the new reader (david) assert_queries(2) do posts(:welcome).people = [people(:david)] end - + assert_queries(0){ assert posts(:welcome).people.include?(people(:david)) assert !posts(:welcome).people.include?(people(:michael)) } - + assert posts(:welcome).reload.people(true).include?(people(:david)) assert !posts(:welcome).reload.people(true).include?(people(:michael)) end def test_associate_with_create assert_queries(1) { posts(:thinking) } - + # 1 query for the new record, 1 for the join table record # No need to update the actual collection yet! assert_queries(2) do posts(:thinking).people.create(:first_name=>"Jeb") end - + # *Now* we actually need the collection so it's loaded assert_queries(1) do assert posts(:thinking).people.collect(&:first_name).include?("Jeb") end - + assert posts(:thinking).reload.people(true).collect(&:first_name).include?("Jeb") end @@ -159,15 +159,15 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase def test_clear_associations assert_queries(2) { posts(:welcome);posts(:welcome).people(true) } - + assert_queries(1) do posts(:welcome).people.clear end - + assert_queries(0) do assert posts(:welcome).people.empty? end - + assert posts(:welcome).reload.people(true).empty? end diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 56dcdea110..622e3f3a1c 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -25,7 +25,7 @@ class CalculationsTest < ActiveRecord::TestCase def test_should_return_nil_as_average assert_nil NumericData.average(:bank_balance) end - + def test_type_cast_calculated_value_should_convert_db_averages_of_fixnum_class_to_decimal assert_equal 0, NumericData.send(:type_cast_calculated_value, 0, nil, 'avg') assert_equal 53.0, NumericData.send(:type_cast_calculated_value, 53, nil, 'avg') diff --git a/activerecord/test/cases/inheritance_test.rb b/activerecord/test/cases/inheritance_test.rb index eae5a60829..1943af2b9c 100644 --- a/activerecord/test/cases/inheritance_test.rb +++ b/activerecord/test/cases/inheritance_test.rb @@ -30,7 +30,7 @@ class InheritanceTest < ActiveRecord::TestCase ensure ActiveRecord::Base.store_full_sti_class = old end - + def test_should_store_full_class_name_with_store_full_sti_class_option_enabled old = ActiveRecord::Base.store_full_sti_class ActiveRecord::Base.store_full_sti_class = true @@ -39,7 +39,7 @@ class InheritanceTest < ActiveRecord::TestCase ensure ActiveRecord::Base.store_full_sti_class = old end - + def test_different_namespace_subclass_should_load_correctly_with_store_full_sti_class_option old = ActiveRecord::Base.store_full_sti_class ActiveRecord::Base.store_full_sti_class = true diff --git a/activerecord/test/cases/named_scope_test.rb b/activerecord/test/cases/named_scope_test.rb index ae6a54a5bd..4929f09812 100644 --- a/activerecord/test/cases/named_scope_test.rb +++ b/activerecord/test/cases/named_scope_test.rb @@ -338,7 +338,7 @@ class NamedScopeTest < ActiveRecord::TestCase end end -class DynamicScopeMatchTest < ActiveRecord::TestCase +class DynamicScopeMatchTest < ActiveRecord::TestCase def test_scoped_by_no_match assert_nil ActiveRecord::DynamicScopeMatch.match("not_scoped_at_all") end diff --git a/arel b/arel index 45646ec54c..b4b73d9520 160000 --- a/arel +++ b/arel @@ -1 +1 @@ -Subproject commit 45646ec54c4c4a3c7340b79deea9e3cf76554f0b +Subproject commit b4b73d9520a2ff27021b530ccd3dcd96973ce5fe -- cgit v1.2.3