Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Merge scoped :joins together instead of overwriting them. May expose …

…scoping bugs in your code!

[#501 state:resolved]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>
  • Loading branch information...
commit db22c89543f45d7f27847003af949afa21cb6fa1 1 parent 44af2ef
@pixeltrix pixeltrix authored jeremy committed
View
4 activerecord/lib/active_record/associations.rb
@@ -1599,7 +1599,7 @@ def construct_finder_sql_with_included_associations(options, join_dependency)
sql = "SELECT #{column_aliases(join_dependency)} FROM #{(scope && scope[:from]) || options[:from] || quoted_table_name} "
sql << join_dependency.join_associations.collect{|join| join.association_join }.join
- add_joins!(sql, options, scope)
+ add_joins!(sql, options[:joins], scope)
add_conditions!(sql, options[:conditions], scope)
add_limited_ids_condition!(sql, options, join_dependency) if !using_limitable_reflections?(join_dependency.reflections) && ((scope && scope[:limit]) || options[:limit])
@@ -1655,7 +1655,7 @@ def construct_finder_sql_for_association_limiting(options, join_dependency)
if is_distinct
sql << distinct_join_associations.collect { |assoc| assoc.association_join }.join
- add_joins!(sql, options, scope)
+ add_joins!(sql, options[:joins], scope)
end
add_conditions!(sql, options[:conditions], scope)
View
37 activerecord/lib/active_record/base.rb
@@ -1549,7 +1549,7 @@ def construct_finder_sql(options)
sql = "SELECT #{options[:select] || (scope && scope[:select]) || ((options[:joins] || (scope && scope[:joins])) && quoted_table_name + '.*') || '*'} "
sql << "FROM #{(scope && scope[:from]) || options[:from] || quoted_table_name} "
- add_joins!(sql, options, scope)
+ add_joins!(sql, options[:joins], scope)
add_conditions!(sql, options[:conditions], scope)
add_group!(sql, options[:group], scope)
@@ -1565,6 +1565,22 @@ def merge_includes(first, second)
(safe_to_array(first) + safe_to_array(second)).uniq
end
+ def merge_joins(first, second)
+ if first.is_a?(String) && second.is_a?(String)
+ "#{first} #{second}"
+ elsif first.is_a?(String) || second.is_a?(String)
+ if first.is_a?(String)
+ join_dependency = ActiveRecord::Associations::ClassMethods::InnerJoinDependency.new(self, second, nil)
+ "#{first} #{join_dependency.join_associations.collect { |assoc| assoc.association_join }.join}"
+ else
+ join_dependency = ActiveRecord::Associations::ClassMethods::InnerJoinDependency.new(self, first, nil)
+ "#{join_dependency.join_associations.collect { |assoc| assoc.association_join }.join} #{second}"
+ end
+ else
+ (safe_to_array(first) + safe_to_array(second)).uniq
+ end
+ end
+
# Object#to_a is deprecated, though it does have the desired behavior
def safe_to_array(o)
case o
@@ -1620,16 +1636,15 @@ def add_lock!(sql, options, scope = :auto)
end
# The optional scope argument is for the current <tt>:find</tt> scope.
- def add_joins!(sql, options, scope = :auto)
+ def add_joins!(sql, joins, scope = :auto)
scope = scope(:find) if :auto == scope
- [(scope && scope[:joins]), options[:joins]].each do |join|
- case join
- when Symbol, Hash, Array
- join_dependency = ActiveRecord::Associations::ClassMethods::InnerJoinDependency.new(self, join, nil)
- sql << " #{join_dependency.join_associations.collect { |assoc| assoc.association_join }.join} "
- else
- sql << " #{join} "
- end
+ merged_joins = scope && scope[:joins] && joins ? merge_joins(scope[:joins], joins) : (joins || scope && scope[:joins])
+ case merged_joins
+ when Symbol, Hash, Array
+ join_dependency = ActiveRecord::Associations::ClassMethods::InnerJoinDependency.new(self, merged_joins, nil)
+ sql << " #{join_dependency.join_associations.collect { |assoc| assoc.association_join }.join} "
+ when String
+ sql << " #{merged_joins} "
end
end
@@ -1879,6 +1894,8 @@ def with_scope(method_scoping = {}, action = :merge, &block)
hash[method][key] = merge_conditions(params[key], hash[method][key])
elsif key == :include && merge
hash[method][key] = merge_includes(hash[method][key], params[key]).uniq
+ elsif key == :joins && merge
+ hash[method][key] = merge_joins(params[key], hash[method][key])
else
hash[method][key] = hash[method][key] || params[key]
end
View
2  activerecord/lib/active_record/calculations.rb
@@ -188,7 +188,7 @@ def construct_calculation_sql(operation, column_name, options) #:nodoc:
end
joins = ""
- add_joins!(joins, options, scope)
+ add_joins!(joins, options[:joins], scope)
if merged_includes.any?
join_dependency = ActiveRecord::Associations::ClassMethods::JoinDependency.new(self, merged_includes, joins)
View
81 activerecord/test/cases/method_scoping_test.rb
@@ -1,4 +1,5 @@
require "cases/helper"
+require 'models/author'
require 'models/developer'
require 'models/project'
require 'models/comment'
@@ -6,7 +7,7 @@
require 'models/category'
class MethodScopingTest < ActiveRecord::TestCase
- fixtures :developers, :projects, :comments, :posts, :developers_projects
+ fixtures :authors, :developers, :projects, :comments, :posts, :developers_projects
def test_set_conditions
Developer.with_scope(:find => { :conditions => 'just a test...' }) do
@@ -97,6 +98,46 @@ def test_scoped_find_joins
assert_equal developers(:david).attributes, scoped_developers.first.attributes
end
+ def test_scoped_find_using_new_style_joins
+ scoped_developers = Developer.with_scope(:find => { :joins => :projects }) do
+ Developer.find(:all, :conditions => 'projects.id = 2')
+ end
+ assert scoped_developers.include?(developers(:david))
+ assert !scoped_developers.include?(developers(:jamis))
+ assert_equal 1, scoped_developers.size
+ assert_equal developers(:david).attributes, scoped_developers.first.attributes
+ end
+
+ def test_scoped_find_merges_old_style_joins
+ scoped_authors = Author.with_scope(:find => { :joins => 'INNER JOIN posts ON authors.id = posts.author_id ' }) do
+ Author.find(:all, :select => 'DISTINCT authors.*', :joins => 'INNER JOIN comments ON posts.id = comments.post_id', :conditions => 'comments.id = 1')
+ end
+ assert scoped_authors.include?(authors(:david))
+ assert !scoped_authors.include?(authors(:mary))
+ assert_equal 1, scoped_authors.size
+ assert_equal authors(:david).attributes, scoped_authors.first.attributes
+ end
+
+ def test_scoped_find_merges_new_style_joins
+ scoped_authors = Author.with_scope(:find => { :joins => :posts }) do
+ Author.find(:all, :select => 'DISTINCT authors.*', :joins => :comments, :conditions => 'comments.id = 1')
+ end
+ assert scoped_authors.include?(authors(:david))
+ assert !scoped_authors.include?(authors(:mary))
+ assert_equal 1, scoped_authors.size
+ assert_equal authors(:david).attributes, scoped_authors.first.attributes
+ end
+
+ def test_scoped_find_merges_new_and_old_style_joins
+ scoped_authors = Author.with_scope(:find => { :joins => :posts }) do
+ Author.find(:all, :select => 'DISTINCT authors.*', :joins => 'JOIN comments ON posts.id = comments.post_id', :conditions => 'comments.id = 1')
+ end
+ assert scoped_authors.include?(authors(:david))
+ assert !scoped_authors.include?(authors(:mary))
+ assert_equal 1, scoped_authors.size
+ assert_equal authors(:david).attributes, scoped_authors.first.attributes
+ end
+
def test_scoped_count_include
# with the include, will retrieve only developers for the given project
Developer.with_scope(:find => { :include => :projects }) do
@@ -152,7 +193,7 @@ def test_ensure_that_method_scoping_is_correctly_restored
end
class NestedScopingTest < ActiveRecord::TestCase
- fixtures :developers, :projects, :comments, :posts
+ fixtures :authors, :developers, :projects, :comments, :posts
def test_merge_options
Developer.with_scope(:find => { :conditions => 'salary = 80000' }) do
@@ -357,6 +398,42 @@ def test_ensure_that_method_scoping_is_correctly_restored
assert_equal scoped_methods, Developer.instance_eval('current_scoped_methods')
end
end
+
+ def test_nested_scoped_find_merges_old_style_joins
+ scoped_authors = Author.with_scope(:find => { :joins => 'INNER JOIN posts ON authors.id = posts.author_id' }) do
+ Author.with_scope(:find => { :joins => 'INNER JOIN comments ON posts.id = comments.post_id' }) do
+ Author.find(:all, :select => 'DISTINCT authors.*', :conditions => 'comments.id = 1')
+ end
+ end
+ assert scoped_authors.include?(authors(:david))
+ assert !scoped_authors.include?(authors(:mary))
+ assert_equal 1, scoped_authors.size
+ assert_equal authors(:david).attributes, scoped_authors.first.attributes
+ end
+
+ def test_nested_scoped_find_merges_new_style_joins
+ scoped_authors = Author.with_scope(:find => { :joins => :posts }) do
+ Author.with_scope(:find => { :joins => :comments }) do
+ Author.find(:all, :select => 'DISTINCT authors.*', :conditions => 'comments.id = 1')
+ end
+ end
+ assert scoped_authors.include?(authors(:david))
+ assert !scoped_authors.include?(authors(:mary))
+ assert_equal 1, scoped_authors.size
+ assert_equal authors(:david).attributes, scoped_authors.first.attributes
+ end
+
+ def test_nested_scoped_find_merges_new_and_old_style_joins
+ scoped_authors = Author.with_scope(:find => { :joins => :posts }) do
+ Author.with_scope(:find => { :joins => 'INNER JOIN comments ON posts.id = comments.post_id' }) do
+ Author.find(:all, :select => 'DISTINCT authors.*', :joins => '', :conditions => 'comments.id = 1')
+ end
+ end
+ assert scoped_authors.include?(authors(:david))
+ assert !scoped_authors.include?(authors(:mary))
+ assert_equal 1, scoped_authors.size
+ assert_equal authors(:david).attributes, scoped_authors.first.attributes
+ end
end
class HasManyScopingTest< ActiveRecord::TestCase
Please sign in to comment.
Something went wrong with that request. Please try again.