From d26dd00854c783bcb1249168bb3f4adf9f99be6c Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Tue, 27 Jan 2015 10:30:38 -0700 Subject: `WhereClause#predicates` does not need to be public The only place it was accessed was in tests. Many of them have another way that they can test their behavior, that doesn't involve reaching into internals as far as they did. `AssociationScopeTest` is testing a situation where the where clause would have one bind param per predicate, so it can just ignore the predicates entirely. The where chain test was primarly duplicating the logic tested on `WhereClause` directly, so I instead just make sure it calls the appropriate method which is fully tested in isolation. --- .../cases/associations/association_scope_test.rb | 5 - .../associations/has_many_associations_test.rb | 8 +- .../associations/has_one_associations_test.rb | 8 +- activerecord/test/cases/associations_test.rb | 2 +- .../test/cases/relation/where_chain_test.rb | 122 ++++----------------- .../test/cases/relation/where_clause_test.rb | 7 +- activerecord/test/cases/relation_test.rb | 4 +- .../test/cases/scoping/default_scoping_test.rb | 8 +- .../test/cases/scoping/named_scoping_test.rb | 4 +- .../test/cases/scoping/relation_scoping_test.rb | 2 +- 10 files changed, 45 insertions(+), 125 deletions(-) (limited to 'activerecord/test') diff --git a/activerecord/test/cases/associations/association_scope_test.rb b/activerecord/test/cases/associations/association_scope_test.rb index dd26a85e44..fc5dbd0890 100644 --- a/activerecord/test/cases/associations/association_scope_test.rb +++ b/activerecord/test/cases/associations/association_scope_test.rb @@ -8,12 +8,7 @@ module ActiveRecord test 'does not duplicate conditions' do scope = AssociationScope.scope(Author.new.association(:welcome_posts), Author.connection) - wheres = scope.where_clause.predicates.map(&:right) binds = scope.where_clause.binds.map(&:last) - wheres.reject! { |node| - Arel::Nodes::BindParam === node - } - assert_equal wheres.uniq, wheres assert_equal binds.uniq, binds end end diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 5ef84562ad..19f4384e83 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -316,16 +316,16 @@ class HasManyAssociationsTest < ActiveRecord::TestCase # would be convenient), because this would cause that scope to be applied to any callbacks etc. def test_build_and_create_should_not_happen_within_scope car = cars(:honda) - scoped_count = car.foo_bulbs.where_clause.predicates.count + scope = car.foo_bulbs.where_values_hash bulb = car.foo_bulbs.build - assert_not_equal scoped_count, bulb.scope_after_initialize.where_clause.predicates.count + assert_not_equal scope, bulb.scope_after_initialize.where_values_hash bulb = car.foo_bulbs.create - assert_not_equal scoped_count, bulb.scope_after_initialize.where_clause.predicates.count + assert_not_equal scope, bulb.scope_after_initialize.where_values_hash bulb = car.foo_bulbs.create! - assert_not_equal scoped_count, bulb.scope_after_initialize.where_clause.predicates.count + assert_not_equal scope, bulb.scope_after_initialize.where_values_hash end def test_no_sql_should_be_fired_if_association_already_loaded diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index c02509db46..3b6a4038cd 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -237,16 +237,16 @@ class HasOneAssociationsTest < ActiveRecord::TestCase def test_build_and_create_should_not_happen_within_scope pirate = pirates(:blackbeard) - scoped_count = pirate.association(:foo_bulb).scope.where_clause.predicates.count + scope = pirate.association(:foo_bulb).scope.where_values_hash bulb = pirate.build_foo_bulb - assert_not_equal scoped_count, bulb.scope_after_initialize.where_clause.predicates.count + assert_not_equal scope, bulb.scope_after_initialize.where_values_hash bulb = pirate.create_foo_bulb - assert_not_equal scoped_count, bulb.scope_after_initialize.where_clause.predicates.count + assert_not_equal scope, bulb.scope_after_initialize.where_values_hash bulb = pirate.create_foo_bulb! - assert_not_equal scoped_count, bulb.scope_after_initialize.where_clause.predicates.count + assert_not_equal scope, bulb.scope_after_initialize.where_values_hash end def test_create_association diff --git a/activerecord/test/cases/associations_test.rb b/activerecord/test/cases/associations_test.rb index c88dc58950..de358114ab 100644 --- a/activerecord/test/cases/associations_test.rb +++ b/activerecord/test/cases/associations_test.rb @@ -238,7 +238,7 @@ class AssociationProxyTest < ActiveRecord::TestCase end def test_scoped_allows_conditions - assert developers(:david).projects.merge!(where: 'foo').where_clause.predicates.include?('foo') + assert developers(:david).projects.merge(where: 'foo').to_sql.include?('foo') end test "getting a scope from an association" do diff --git a/activerecord/test/cases/relation/where_chain_test.rb b/activerecord/test/cases/relation/where_chain_test.rb index 00c9a001c0..27bbd80f79 100644 --- a/activerecord/test/cases/relation/where_chain_test.rb +++ b/activerecord/test/cases/relation/where_chain_test.rb @@ -11,22 +11,11 @@ module ActiveRecord @name = 'title' end - def test_not_eq + def test_not_inverts_where_clause relation = Post.where.not(title: 'hello') + expected_where_clause = Post.where(title: 'hello').where_clause.invert - assert_equal 1, relation.where_clause.predicates.length - - value = relation.where_clause.predicates.first - bind = relation.where_clause.binds.first - - assert_bound_ast value, Post.arel_table[@name], Arel::Nodes::NotEqual - assert_equal 'hello', bind.last - end - - def test_not_null - expected = Post.arel_table[@name].not_eq(nil) - relation = Post.where.not(title: nil) - assert_equal([expected], relation.where_clause.predicates) + assert_equal expected_where_clause, relation.where_clause end def test_not_with_nil @@ -35,146 +24,81 @@ module ActiveRecord end end - def test_not_in - expected = Post.arel_table[@name].not_in(%w[hello goodbye]) - relation = Post.where.not(title: %w[hello goodbye]) - assert_equal([expected], relation.where_clause.predicates) - end - def test_association_not_eq - expected = Comment.arel_table[@name].not_eq(Arel::Nodes::BindParam.new) + expected = Arel::Nodes::Grouping.new(Comment.arel_table[@name].not_eq(Arel::Nodes::BindParam.new)) relation = Post.joins(:comments).where.not(comments: {title: 'hello'}) - assert_equal(expected.to_sql, relation.where_clause.predicates.first.to_sql) + assert_equal(expected.to_sql, relation.where_clause.ast.to_sql) end def test_not_eq_with_preceding_where relation = Post.where(title: 'hello').where.not(title: 'world') + expected_where_clause = + Post.where(title: 'hello').where_clause + + Post.where(title: 'world').where_clause.invert - value = relation.where_clause.predicates.first - bind = relation.where_clause.binds.first - assert_bound_ast value, Post.arel_table[@name], Arel::Nodes::Equality - assert_equal 'hello', bind.last - - value = relation.where_clause.predicates.last - bind = relation.where_clause.binds.last - assert_bound_ast value, Post.arel_table[@name], Arel::Nodes::NotEqual - assert_equal 'world', bind.last + assert_equal expected_where_clause, relation.where_clause end def test_not_eq_with_succeeding_where relation = Post.where.not(title: 'hello').where(title: 'world') + expected_where_clause = + Post.where(title: 'hello').where_clause.invert + + Post.where(title: 'world').where_clause - value = relation.where_clause.predicates.first - bind = relation.where_clause.binds.first - assert_bound_ast value, Post.arel_table[@name], Arel::Nodes::NotEqual - assert_equal 'hello', bind.last - - value = relation.where_clause.predicates.last - bind = relation.where_clause.binds.last - assert_bound_ast value, Post.arel_table[@name], Arel::Nodes::Equality - assert_equal 'world', bind.last - end - - def test_not_eq_with_string_parameter - expected = Arel::Nodes::Not.new("title = 'hello'") - relation = Post.where.not("title = 'hello'") - assert_equal([expected], relation.where_clause.predicates) - end - - def test_not_eq_with_array_parameter - expected = Arel::Nodes::Not.new("title = 'hello'") - relation = Post.where.not(['title = ?', 'hello']) - assert_equal([expected], relation.where_clause.predicates) + assert_equal expected_where_clause, relation.where_clause end def test_chaining_multiple relation = Post.where.not(author_id: [1, 2]).where.not(title: 'ruby on rails') + expected_where_clause = + Post.where(author_id: [1, 2]).where_clause.invert + + Post.where(title: 'ruby on rails').where_clause.invert - expected = Post.arel_table['author_id'].not_in([1, 2]) - assert_equal(expected, relation.where_clause.predicates[0]) - - value = relation.where_clause.predicates[1] - bind = relation.where_clause.binds.first - - assert_bound_ast value, Post.arel_table[@name], Arel::Nodes::NotEqual - assert_equal 'ruby on rails', bind.last + assert_equal expected_where_clause, relation.where_clause end def test_rewhere_with_one_condition relation = Post.where(title: 'hello').where(title: 'world').rewhere(title: 'alone') + expected = Post.where(title: 'alone') - assert_equal 1, relation.where_clause.predicates.size - value = relation.where_clause.predicates.first - bind = relation.where_clause.binds.first - assert_bound_ast value, Post.arel_table[@name], Arel::Nodes::Equality - assert_equal 'alone', bind.last + assert_equal expected.where_clause, relation.where_clause end def test_rewhere_with_multiple_overwriting_conditions relation = Post.where(title: 'hello').where(body: 'world').rewhere(title: 'alone', body: 'again') + expected = Post.where(title: 'alone', body: 'again') - assert_equal 2, relation.where_clause.predicates.size - - value = relation.where_clause.predicates.first - bind = relation.where_clause.binds.first - assert_bound_ast value, Post.arel_table['title'], Arel::Nodes::Equality - assert_equal 'alone', bind.last - - value = relation.where_clause.predicates[1] - bind = relation.where_clause.binds[1] - assert_bound_ast value, Post.arel_table['body'], Arel::Nodes::Equality - assert_equal 'again', bind.last - end - - def assert_bound_ast value, table, type - assert_equal table, value.left - assert_kind_of type, value - assert_kind_of Arel::Nodes::BindParam, value.right + assert_equal expected.where_clause, relation.where_clause end def test_rewhere_with_one_overwriting_condition_and_one_unrelated relation = Post.where(title: 'hello').where(body: 'world').rewhere(title: 'alone') + expected = Post.where(body: 'world', title: 'alone') - assert_equal 2, relation.where_clause.predicates.size - - value = relation.where_clause.predicates.first - bind = relation.where_clause.binds.first - - assert_bound_ast value, Post.arel_table['body'], Arel::Nodes::Equality - assert_equal 'world', bind.last - - value = relation.where_clause.predicates.second - bind = relation.where_clause.binds.second - - assert_bound_ast value, Post.arel_table['title'], Arel::Nodes::Equality - assert_equal 'alone', bind.last + assert_equal expected.where_clause, relation.where_clause end def test_rewhere_with_range relation = Post.where(comments_count: 1..3).rewhere(comments_count: 3..5) - assert_equal 1, relation.where_clause.predicates.size assert_equal Post.where(comments_count: 3..5), relation end def test_rewhere_with_infinite_upper_bound_range relation = Post.where(comments_count: 1..Float::INFINITY).rewhere(comments_count: 3..5) - assert_equal 1, relation.where_clause.predicates.size assert_equal Post.where(comments_count: 3..5), relation end def test_rewhere_with_infinite_lower_bound_range relation = Post.where(comments_count: -Float::INFINITY..1).rewhere(comments_count: 3..5) - assert_equal 1, relation.where_clause.predicates.size assert_equal Post.where(comments_count: 3..5), relation end def test_rewhere_with_infinite_range relation = Post.where(comments_count: -Float::INFINITY..Float::INFINITY).rewhere(comments_count: 3..5) - assert_equal 1, relation.where_clause.predicates.size assert_equal Post.where(comments_count: 3..5), relation end end diff --git a/activerecord/test/cases/relation/where_clause_test.rb b/activerecord/test/cases/relation/where_clause_test.rb index 511d595f71..f3644154b6 100644 --- a/activerecord/test/cases/relation/where_clause_test.rb +++ b/activerecord/test/cases/relation/where_clause_test.rb @@ -112,11 +112,12 @@ class ActiveRecord::Relation end test "ast groups its predicates with AND" do - where_clause = WhereClause.new([ + predicates = [ table["id"].in([1, 2, 3]), table["name"].eq(bind_param), - ], []) - expected = Arel::Nodes::And.new(where_clause.predicates) + ] + where_clause = WhereClause.new(predicates, []) + expected = Arel::Nodes::And.new(predicates) assert_equal expected, where_clause.ast end diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb index d75c5435c6..194faa9473 100644 --- a/activerecord/test/cases/relation_test.rb +++ b/activerecord/test/cases/relation_test.rb @@ -156,7 +156,7 @@ module ActiveRecord relation = Relation.new(FakeKlass, :b, nil) relation = relation.merge where: :lol, readonly: true - assert_equal [:lol], relation.where_clause.predicates + assert_equal Relation::WhereClause.new([:lol], []), relation.where_clause assert_equal true, relation.readonly_value end @@ -191,7 +191,7 @@ module ActiveRecord relation = Relation.new(klass, :b, nil) relation.merge!(where: ['foo = ?', 'bar']) - assert_equal ['foo = bar'], relation.where_clause.predicates + assert_equal Relation::WhereClause.new(['foo = bar'], []), relation.where_clause end def test_merging_readonly_false diff --git a/activerecord/test/cases/scoping/default_scoping_test.rb b/activerecord/test/cases/scoping/default_scoping_test.rb index d0835ba3e5..4137b20c4a 100644 --- a/activerecord/test/cases/scoping/default_scoping_test.rb +++ b/activerecord/test/cases/scoping/default_scoping_test.rb @@ -426,19 +426,19 @@ class DefaultScopingTest < ActiveRecord::TestCase test "additional conditions are ANDed with the default scope" do scope = DeveloperCalledJamis.where(name: "David") - assert_equal 2, scope.where_clause.predicates.length + assert_equal 2, scope.where_clause.ast.children.length assert_equal [], scope.to_a end test "additional conditions in a scope are ANDed with the default scope" do scope = DeveloperCalledJamis.david - assert_equal 2, scope.where_clause.predicates.length + assert_equal 2, scope.where_clause.ast.children.length assert_equal [], scope.to_a end test "a scope can remove the condition from the default scope" do scope = DeveloperCalledJamis.david2 - assert_equal 1, scope.where_clause.predicates.length - assert_equal Developer.where(name: "David").map(&:id), scope.map(&:id) + assert_equal 1, scope.where_clause.ast.children.length + assert_equal Developer.where(name: "David"), scope end end diff --git a/activerecord/test/cases/scoping/named_scoping_test.rb b/activerecord/test/cases/scoping/named_scoping_test.rb index 57c9ff6a8d..8cd94ebcc2 100644 --- a/activerecord/test/cases/scoping/named_scoping_test.rb +++ b/activerecord/test/cases/scoping/named_scoping_test.rb @@ -380,8 +380,8 @@ class NamedScopingTest < ActiveRecord::TestCase end def test_should_not_duplicates_where_values - where_values = Topic.where("1=1").scope_with_lambda.where_clause.predicates - assert_equal ["1=1"], where_values + relation = Topic.where("1=1") + assert_equal relation.where_clause, relation.scope_with_lambda.where_clause end def test_chaining_with_duplicate_joins diff --git a/activerecord/test/cases/scoping/relation_scoping_test.rb b/activerecord/test/cases/scoping/relation_scoping_test.rb index 73760e628b..02b32abebf 100644 --- a/activerecord/test/cases/scoping/relation_scoping_test.rb +++ b/activerecord/test/cases/scoping/relation_scoping_test.rb @@ -184,7 +184,7 @@ class RelationScopingTest < ActiveRecord::TestCase rescue end - assert !Developer.all.where_clause.predicates.include?("name = 'Jamis'") + assert_not Developer.all.to_sql.include?("name = 'Jamis'"), "scope was not restored" end def test_default_scope_filters_on_joins -- cgit v1.2.3