From 6928950def1bea9d564778e734822d4f5b8bac61 Mon Sep 17 00:00:00 2001
From: Daniel Colson <danieljamescolson@gmail.com>
Date: Wed, 24 Jan 2018 14:06:30 -0500
Subject: Avoid passing unnecessary arguments to relation

Most of the time the table and predicate_builder
passed to Relation.new are exactly the
arel_table and predicate builder of the
given klass. This uses klass.arel_table
and klass.predicate_builder as the defaults,
so we don't have to pass them in most cases.

This does change the signaure of both Relation and
AssocationRelation. Are we ok with that?
---
 .../lib/active_record/association_relation.rb      |  4 +-
 .../lib/active_record/associations/association.rb  |  2 +-
 .../active_record/associations/collection_proxy.rb |  2 +-
 activerecord/lib/active_record/core.rb             |  2 +-
 activerecord/lib/active_record/reflection.rb       |  6 ++-
 activerecord/lib/active_record/relation.rb         |  2 +-
 activerecord/lib/active_record/relation/merger.rb  |  6 ++-
 .../lib/active_record/relation/spawn_methods.rb    |  2 +-
 activerecord/test/cases/batches_test.rb            |  6 ++-
 .../test/cases/collection_cache_key_test.rb        |  6 ++-
 activerecord/test/cases/relation/mutation_test.rb  |  2 +-
 activerecord/test/cases/relation_test.rb           | 50 +++++++++++-----------
 activerecord/test/cases/relations_test.rb          |  6 ++-
 activerecord/test/models/post.rb                   |  8 ++++
 14 files changed, 66 insertions(+), 38 deletions(-)

(limited to 'activerecord')

diff --git a/activerecord/lib/active_record/association_relation.rb b/activerecord/lib/active_record/association_relation.rb
index 2b0b2864bc..403667fb70 100644
--- a/activerecord/lib/active_record/association_relation.rb
+++ b/activerecord/lib/active_record/association_relation.rb
@@ -2,8 +2,8 @@
 
 module ActiveRecord
   class AssociationRelation < Relation
-    def initialize(klass, table, predicate_builder, association)
-      super(klass, table, predicate_builder)
+    def initialize(klass, association)
+      super(klass)
       @association = association
     end
 
diff --git a/activerecord/lib/active_record/associations/association.rb b/activerecord/lib/active_record/associations/association.rb
index ca1f9f1650..7667c6ed8b 100644
--- a/activerecord/lib/active_record/associations/association.rb
+++ b/activerecord/lib/active_record/associations/association.rb
@@ -124,7 +124,7 @@ module ActiveRecord
       # Can be overridden (i.e. in ThroughAssociation) to merge in other scopes (i.e. the
       # through association's scope)
       def target_scope
-        AssociationRelation.create(klass, klass.arel_table, klass.predicate_builder, self).merge!(klass.all)
+        AssociationRelation.create(klass, self).merge!(klass.all)
       end
 
       def extensions
diff --git a/activerecord/lib/active_record/associations/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb
index 8b4a48a38c..9a30198b95 100644
--- a/activerecord/lib/active_record/associations/collection_proxy.rb
+++ b/activerecord/lib/active_record/associations/collection_proxy.rb
@@ -32,7 +32,7 @@ module ActiveRecord
     class CollectionProxy < Relation
       def initialize(klass, association) #:nodoc:
         @association = association
-        super klass, klass.arel_table, klass.predicate_builder
+        super klass
 
         extensions = association.extensions
         extend(*extensions) if extensions.any?
diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb
index 88810cb328..2c65f618dc 100644
--- a/activerecord/lib/active_record/core.rb
+++ b/activerecord/lib/active_record/core.rb
@@ -281,7 +281,7 @@ module ActiveRecord
         end
 
         def relation
-          relation = Relation.create(self, arel_table, predicate_builder)
+          relation = Relation.create(self)
 
           if finder_needs_type_condition? && !ignore_default_scope?
             relation.where!(type_condition)
diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
index c28e31a3da..e640d75d2f 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -291,7 +291,11 @@ module ActiveRecord
       end
 
       def build_scope(table, predicate_builder = predicate_builder(table))
-        Relation.create(klass, table, predicate_builder)
+        Relation.create(
+          klass,
+          table: table,
+          predicate_builder: predicate_builder
+        )
       end
 
       def join_primary_key(*)
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index 10be583ef4..d71c430045 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -22,7 +22,7 @@ module ActiveRecord
     alias :loaded? :loaded
     alias :locked? :lock_value
 
-    def initialize(klass, table, predicate_builder, values = {})
+    def initialize(klass, table: klass.arel_table, predicate_builder: klass.predicate_builder, values: {})
       @klass  = klass
       @table  = table
       @values = values
diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb
index ebdd4144bb..25510d4a57 100644
--- a/activerecord/lib/active_record/relation/merger.rb
+++ b/activerecord/lib/active_record/relation/merger.rb
@@ -23,7 +23,11 @@ module ActiveRecord
       # build a relation to merge in rather than directly merging
       # the values.
       def other
-        other = Relation.create(relation.klass, relation.table, relation.predicate_builder)
+        other = Relation.create(
+          relation.klass,
+          table: relation.table,
+          predicate_builder: relation.predicate_builder
+        )
         hash.each { |k, v|
           if k == :joins
             if Hash === v
diff --git a/activerecord/lib/active_record/relation/spawn_methods.rb b/activerecord/lib/active_record/relation/spawn_methods.rb
index 617d8de8b2..562e04194c 100644
--- a/activerecord/lib/active_record/relation/spawn_methods.rb
+++ b/activerecord/lib/active_record/relation/spawn_methods.rb
@@ -69,7 +69,7 @@ module ActiveRecord
     private
 
       def relation_with(values)
-        result = Relation.create(klass, table, predicate_builder, values)
+        result = Relation.create(klass, values: values)
         result.extend(*extending_values) if extending_values.any?
         result
       end
diff --git a/activerecord/test/cases/batches_test.rb b/activerecord/test/cases/batches_test.rb
index be8aeed5ac..bbd0b1724a 100644
--- a/activerecord/test/cases/batches_test.rb
+++ b/activerecord/test/cases/batches_test.rb
@@ -592,7 +592,11 @@ class EachTest < ActiveRecord::TestCase
       table_metadata = ActiveRecord::TableMetadata.new(Post, table_alias)
       predicate_builder = ActiveRecord::PredicateBuilder.new(table_metadata)
 
-      posts = ActiveRecord::Relation.create(Post, table_alias, predicate_builder)
+      posts = ActiveRecord::Relation.create(
+        Post,
+        table: table_alias,
+        predicate_builder: predicate_builder
+      )
       posts.find_each {}
     end
   end
diff --git a/activerecord/test/cases/collection_cache_key_test.rb b/activerecord/test/cases/collection_cache_key_test.rb
index cfe95b2360..a5d908344a 100644
--- a/activerecord/test/cases/collection_cache_key_test.rb
+++ b/activerecord/test/cases/collection_cache_key_test.rb
@@ -60,7 +60,11 @@ module ActiveRecord
       table_metadata = ActiveRecord::TableMetadata.new(Developer, table_alias)
       predicate_builder = ActiveRecord::PredicateBuilder.new(table_metadata)
 
-      developers = ActiveRecord::Relation.create(Developer, table_alias, predicate_builder)
+      developers = ActiveRecord::Relation.create(
+        Developer,
+        table: table_alias,
+        predicate_builder: predicate_builder
+      )
       developers = developers.where(salary: 100000).order(updated_at: :desc)
       last_developer_timestamp = developers.first.updated_at
 
diff --git a/activerecord/test/cases/relation/mutation_test.rb b/activerecord/test/cases/relation/mutation_test.rb
index ad3700b73a..932a4eca2c 100644
--- a/activerecord/test/cases/relation/mutation_test.rb
+++ b/activerecord/test/cases/relation/mutation_test.rb
@@ -139,7 +139,7 @@ module ActiveRecord
 
     private
       def relation
-        @relation ||= Relation.new(FakeKlass, Post.arel_table, Post.predicate_builder)
+        @relation ||= Relation.new(FakeKlass)
       end
   end
 end
diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb
index dbf3389774..3d64bfb810 100644
--- a/activerecord/test/cases/relation_test.rb
+++ b/activerecord/test/cases/relation_test.rb
@@ -11,19 +11,19 @@ module ActiveRecord
     fixtures :posts, :comments, :authors, :author_addresses, :ratings
 
     def test_construction
-      relation = Relation.new(FakeKlass, :b, nil)
+      relation = Relation.new(FakeKlass, table: :b)
       assert_equal FakeKlass, relation.klass
       assert_equal :b, relation.table
       assert !relation.loaded, "relation is not loaded"
     end
 
     def test_responds_to_model_and_returns_klass
-      relation = Relation.new(FakeKlass, :b, nil)
+      relation = Relation.new(FakeKlass)
       assert_equal FakeKlass, relation.model
     end
 
     def test_initialize_single_values
-      relation = Relation.new(FakeKlass, :b, nil)
+      relation = Relation.new(FakeKlass)
       (Relation::SINGLE_VALUE_METHODS - [:create_with]).each do |method|
         assert_nil relation.send("#{method}_value"), method.to_s
       end
@@ -33,7 +33,7 @@ module ActiveRecord
     end
 
     def test_multi_value_initialize
-      relation = Relation.new(FakeKlass, :b, nil)
+      relation = Relation.new(FakeKlass)
       Relation::MULTI_VALUE_METHODS.each do |method|
         values = relation.send("#{method}_values")
         assert_equal [], values, method.to_s
@@ -42,29 +42,29 @@ module ActiveRecord
     end
 
     def test_extensions
-      relation = Relation.new(FakeKlass, :b, nil)
+      relation = Relation.new(FakeKlass)
       assert_equal [], relation.extensions
     end
 
     def test_empty_where_values_hash
-      relation = Relation.new(FakeKlass, :b, nil)
+      relation = Relation.new(FakeKlass)
       assert_equal({}, relation.where_values_hash)
     end
 
     def test_has_values
-      relation = Relation.new(Post, Post.arel_table, Post.predicate_builder)
+      relation = Relation.new(Post)
       relation.where!(id: 10)
       assert_equal({ "id" => 10 }, relation.where_values_hash)
     end
 
     def test_values_wrong_table
-      relation = Relation.new(Post, Post.arel_table, Post.predicate_builder)
+      relation = Relation.new(Post)
       relation.where! Comment.arel_table[:id].eq(10)
       assert_equal({}, relation.where_values_hash)
     end
 
     def test_tree_is_not_traversed
-      relation = Relation.new(Post, Post.arel_table, Post.predicate_builder)
+      relation = Relation.new(Post)
       left     = relation.table[:id].eq(10)
       right    = relation.table[:id].eq(10)
       combine  = left.or(right)
@@ -73,18 +73,18 @@ module ActiveRecord
     end
 
     def test_scope_for_create
-      relation = Relation.new(FakeKlass, :b, nil)
+      relation = Relation.new(FakeKlass)
       assert_equal({}, relation.scope_for_create)
     end
 
     def test_create_with_value
-      relation = Relation.new(Post, Post.arel_table, Post.predicate_builder)
+      relation = Relation.new(Post)
       relation.create_with_value = { hello: "world" }
       assert_equal({ "hello" => "world" }, relation.scope_for_create)
     end
 
     def test_create_with_value_with_wheres
-      relation = Relation.new(Post, Post.arel_table, Post.predicate_builder)
+      relation = Relation.new(Post)
       assert_equal({}, relation.scope_for_create)
 
       relation.where!(id: 10)
@@ -95,7 +95,7 @@ module ActiveRecord
     end
 
     def test_empty_scope
-      relation = Relation.new(Post, Post.arel_table, Post.predicate_builder)
+      relation = Relation.new(Post)
       assert relation.empty_scope?
 
       relation.merge!(relation)
@@ -109,31 +109,31 @@ module ActiveRecord
     end
 
     def test_empty_eager_loading?
-      relation = Relation.new(FakeKlass, :b, nil)
+      relation = Relation.new(FakeKlass)
       assert !relation.eager_loading?
     end
 
     def test_eager_load_values
-      relation = Relation.new(FakeKlass, :b, nil)
+      relation = Relation.new(FakeKlass)
       relation.eager_load! :b
       assert relation.eager_loading?
     end
 
     def test_references_values
-      relation = Relation.new(FakeKlass, :b, nil)
+      relation = Relation.new(FakeKlass)
       assert_equal [], relation.references_values
       relation = relation.references(:foo).references(:omg, :lol)
       assert_equal ["foo", "omg", "lol"], relation.references_values
     end
 
     def test_references_values_dont_duplicate
-      relation = Relation.new(FakeKlass, :b, nil)
+      relation = Relation.new(FakeKlass)
       relation = relation.references(:foo).references(:foo)
       assert_equal ["foo"], relation.references_values
     end
 
     test "merging a hash into a relation" do
-      relation = Relation.new(Post, Post.arel_table, Post.predicate_builder)
+      relation = Relation.new(Post)
       relation = relation.merge where: { name: :lol }, readonly: true
 
       assert_equal({ "name" => :lol }, relation.where_clause.to_h)
@@ -141,7 +141,7 @@ module ActiveRecord
     end
 
     test "merging an empty hash into a relation" do
-      assert_equal Relation::WhereClause.empty, Relation.new(FakeKlass, :b, nil).merge({}).where_clause
+      assert_equal Relation::WhereClause.empty, Relation.new(FakeKlass).merge({}).where_clause
     end
 
     test "merging a hash with unknown keys raises" do
@@ -149,7 +149,7 @@ module ActiveRecord
     end
 
     test "merging nil or false raises" do
-      relation = Relation.new(FakeKlass, :b, nil)
+      relation = Relation.new(FakeKlass)
 
       e = assert_raises(ArgumentError) do
         relation = relation.merge nil
@@ -165,7 +165,7 @@ module ActiveRecord
     end
 
     test "#values returns a dup of the values" do
-      relation = Relation.new(Post, Post.arel_table, Post.predicate_builder).where!(name: :foo)
+      relation = Relation.new(Post).where!(name: :foo)
       values   = relation.values
 
       values[:where] = nil
@@ -173,7 +173,7 @@ module ActiveRecord
     end
 
     test "relations can be created with a values hash" do
-      relation = Relation.new(FakeKlass, :b, nil, select: [:foo])
+      relation = Relation.new(FakeKlass, values: { select: [:foo] })
       assert_equal [:foo], relation.select_values
     end
 
@@ -185,13 +185,13 @@ module ActiveRecord
         end
       end
 
-      relation = Relation.new(klass, :b, nil)
+      relation = Relation.new(klass)
       relation.merge!(where: ["foo = ?", "bar"])
       assert_equal Relation::WhereClause.new(["foo = bar"]), relation.where_clause
     end
 
     def test_merging_readonly_false
-      relation = Relation.new(FakeKlass, :b, nil)
+      relation = Relation.new(FakeKlass)
       readonly_false_relation = relation.readonly(false)
       # test merging in both directions
       assert_equal false, relation.merge(readonly_false_relation).readonly_value
@@ -235,7 +235,7 @@ module ActiveRecord
 
     def test_merge_raises_with_invalid_argument
       assert_raises ArgumentError do
-        relation = Relation.new(FakeKlass, :b, nil)
+        relation = Relation.new(FakeKlass)
         relation.merge(true)
       end
     end
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 7785f8c99b..19489f3c71 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -1934,6 +1934,10 @@ class RelationTest < ActiveRecord::TestCase
       table_metadata = ActiveRecord::TableMetadata.new(Post, table_alias)
       predicate_builder = ActiveRecord::PredicateBuilder.new(table_metadata)
 
-      ActiveRecord::Relation.create(Post, table_alias, predicate_builder)
+      ActiveRecord::Relation.create(
+        Post,
+        table: table_alias,
+        predicate_builder: predicate_builder
+      )
     end
 end
diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb
index b552f66787..54eb5e6783 100644
--- a/activerecord/test/models/post.rb
+++ b/activerecord/test/models/post.rb
@@ -326,5 +326,13 @@ class FakeKlass
     def enforce_raw_sql_whitelist(*args)
       # noop
     end
+
+    def arel_table
+      Post.arel_table
+    end
+
+    def predicate_builder
+      Post.predicate_builder
+    end
   end
 end
-- 
cgit v1.2.3