From ec75ff34517c98d8feb6ad81ae79c44e611b92e7 Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Fri, 10 May 2013 11:37:58 +0200 Subject: Reject blank order_values within #columns_for_distinct, as the orders aren't used at all on non-postgres adapters. --- .../active_record/connection_adapters/postgresql/schema_statements.rb | 2 +- activerecord/lib/active_record/relation/finder_methods.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb index 8feee23df0..a651b6c32e 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb @@ -470,7 +470,7 @@ module ActiveRecord # PostgreSQL requires the ORDER BY columns in the select list for distinct queries, and # requires that the ORDER BY include the distinct column. def columns_for_distinct(columns, orders) #:nodoc: - order_columns = orders.map{ |s| + order_columns = orders.reject(&:blank?).map{ |s| # Convert Arel node to string s = s.to_sql unless s.is_a?(String) # Remove any ASC/DESC modifiers diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index ff825e52c1..a51db614cd 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -244,8 +244,8 @@ module ActiveRecord end def construct_limited_ids_condition(relation) - orders = relation.order_values.map { |val| val.presence }.compact - values = @klass.connection.columns_for_distinct("#{quoted_table_name}.#{quoted_primary_key}", orders) + values = @klass.connection.columns_for_distinct( + "#{quoted_table_name}.#{quoted_primary_key}", relation.order_values) relation = relation.dup.select(values).distinct! -- cgit v1.2.3 From cd04a99ba4b5227fb103b6d4e7504c770833e612 Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Fri, 10 May 2013 11:39:08 +0200 Subject: Move the except(:select) inside the construct_limited_ids_condition method to pair it closely with its motivation. --- activerecord/lib/active_record/relation/finder_methods.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index a51db614cd..531343782e 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -234,7 +234,7 @@ module ActiveRecord limitable_reflections = using_limitable_reflections?(join_dependency.reflections) if !limitable_reflections && relation.limit_value - limited_id_condition = construct_limited_ids_condition(relation.except(:select)) + limited_id_condition = construct_limited_ids_condition(relation) relation = relation.where(limited_id_condition) end @@ -247,7 +247,7 @@ module ActiveRecord values = @klass.connection.columns_for_distinct( "#{quoted_table_name}.#{quoted_primary_key}", relation.order_values) - relation = relation.dup.select(values).distinct! + relation = relation.except(:select).select(values).distinct! id_rows = @klass.connection.select_all(relation.arel, 'SQL', relation.bind_values) ids_array = id_rows.map {|row| row[primary_key]} -- cgit v1.2.3 From 1dcb1ccc9d3d4f41e8f1a76ff3465f708189dd2f Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Fri, 10 May 2013 11:42:24 +0200 Subject: Simplify conditions within apply_join_dependency --- activerecord/lib/active_record/relation/finder_methods.rb | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 531343782e..9a774b5abc 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -231,16 +231,12 @@ module ActiveRecord relation = association.join_relation(relation) end - limitable_reflections = using_limitable_reflections?(join_dependency.reflections) - - if !limitable_reflections && relation.limit_value - limited_id_condition = construct_limited_ids_condition(relation) - relation = relation.where(limited_id_condition) + if using_limitable_reflections?(join_dependency.reflections) + relation + else + relation = relation.where(construct_limited_ids_condition(relation)) if relation.limit_value + relation.except(:limit, :offset) end - - relation = relation.except(:limit, :offset) unless limitable_reflections - - relation end def construct_limited_ids_condition(relation) -- cgit v1.2.3 From 88219cc88aadf75fe57a1ecacbe92a7acef64145 Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Fri, 10 May 2013 11:47:45 +0200 Subject: Pull the excepts into apply_join_dependency, for the sake of DRY. --- activerecord/lib/active_record/relation/finder_methods.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 9a774b5abc..ed5fe2c683 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -217,16 +217,17 @@ module ActiveRecord def construct_relation_for_association_calculations including = (eager_load_values + includes_values).uniq join_dependency = ActiveRecord::Associations::JoinDependency.new(@klass, including, arel.froms.first) - relation = except(:includes, :eager_load, :preload) - apply_join_dependency(relation, join_dependency) + apply_join_dependency(self, join_dependency) end def construct_relation_for_association_find(join_dependency) - relation = except(:includes, :eager_load, :preload, :select).select(join_dependency.columns) + relation = except(:select).select(join_dependency.columns) apply_join_dependency(relation, join_dependency) end def apply_join_dependency(relation, join_dependency) + relation = relation.except(:includes, :eager_load, :preload) + join_dependency.join_associations.each do |association| relation = association.join_relation(relation) end -- cgit v1.2.3 From 75fe7434a81d0aaef8ec2ada8f88b5b20721c25a Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Fri, 10 May 2013 11:53:23 +0200 Subject: DRY-up join dependency creation by extracting construct_join_depdency --- activerecord/lib/active_record/relation/finder_methods.rb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index ed5fe2c683..d2cc2c7d88 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -160,7 +160,7 @@ module ActiveRecord conditions = conditions.id if Base === conditions return false if !conditions - join_dependency = construct_join_dependency_for_association_find + join_dependency = construct_join_dependency relation = construct_relation_for_association_find(join_dependency) relation = relation.except(:select, :order).select("1 AS one").limit(1) @@ -201,7 +201,7 @@ module ActiveRecord protected def find_with_associations - join_dependency = construct_join_dependency_for_association_find + join_dependency = construct_join_dependency relation = construct_relation_for_association_find(join_dependency) rows = connection.select_all(relation, 'SQL', relation.bind_values.dup) join_dependency.instantiate(rows) @@ -209,15 +209,13 @@ module ActiveRecord [] end - def construct_join_dependency_for_association_find + def construct_join_dependency(joins = []) including = (eager_load_values + includes_values).uniq - ActiveRecord::Associations::JoinDependency.new(@klass, including, []) + ActiveRecord::Associations::JoinDependency.new(@klass, including, joins) end def construct_relation_for_association_calculations - including = (eager_load_values + includes_values).uniq - join_dependency = ActiveRecord::Associations::JoinDependency.new(@klass, including, arel.froms.first) - apply_join_dependency(self, join_dependency) + apply_join_dependency(self, construct_join_dependency(arel.froms.first)) end def construct_relation_for_association_find(join_dependency) -- cgit v1.2.3 From 35c198ca9bc1ec530fc29b686978511f23cee076 Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Fri, 10 May 2013 17:34:51 +0200 Subject: In #apply_join_dependency, we can apply the #where in-place because relation is always a new object. Thanks to the #except we call at the top of the method. --- activerecord/lib/active_record/relation/finder_methods.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index d2cc2c7d88..332548ea3e 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -233,7 +233,7 @@ module ActiveRecord if using_limitable_reflections?(join_dependency.reflections) relation else - relation = relation.where(construct_limited_ids_condition(relation)) if relation.limit_value + relation.where!(construct_limited_ids_condition(relation)) if relation.limit_value relation.except(:limit, :offset) end end -- cgit v1.2.3 From fba18f19948f084023fd8744025f57da00163265 Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Fri, 10 May 2013 17:41:29 +0200 Subject: Extract JoinDependency#join_relation to DRY the repeated application of the #join_associations. --- activerecord/lib/active_record/associations/join_dependency.rb | 7 +++++++ activerecord/lib/active_record/relation/finder_methods.rb | 5 +---- activerecord/lib/active_record/relation/merger.rb | 4 +--- 3 files changed, 9 insertions(+), 7 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/join_dependency.rb b/activerecord/lib/active_record/associations/join_dependency.rb index 28e081c03c..5b2f2d1902 100644 --- a/activerecord/lib/active_record/associations/join_dependency.rb +++ b/activerecord/lib/active_record/associations/join_dependency.rb @@ -55,6 +55,13 @@ module ActiveRecord join_parts.first end + def join_relation(relation) + join_associations.each do |association| + relation = association.join_relation(relation) + end + relation + end + def columns join_parts.collect { |join_part| table = join_part.aliased_table diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 332548ea3e..ba222aac93 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -225,10 +225,7 @@ module ActiveRecord def apply_join_dependency(relation, join_dependency) relation = relation.except(:includes, :eager_load, :preload) - - join_dependency.join_associations.each do |association| - relation = association.join_relation(relation) - end + relation = join_dependency.join_relation(relation) if using_limitable_reflections?(join_dependency.reflections) relation diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index bda7a76330..eea43aff0e 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -94,9 +94,7 @@ module ActiveRecord []) relation.joins! rest - join_dependency.join_associations.each do |association| - @relation = association.join_relation(relation) - end + @relation = join_dependency.join_relation(relation) end end -- cgit v1.2.3