Merge pull request #11152 from senny/remove_implicit_join_references

remove deprecated implicit join references

activerecord/CHANGELOG.md

@@ -1,3 +1,15 @@
+*   Remove implicit join references that were deprecated in 4.0.
+
+    Example:
+
+        # before with implicit joins
+        Comment.where('posts.author_id' => 7)
+
+        # after
+        Comment.references(:posts).where('posts.author_id' => 7)
+
+    *Yves Senn*
+
 *   Apply default scope when joining associations. For example:
 
         class Post < ActiveRecord::Base

activerecord/lib/active_record/core.rb

@@ -69,13 +69,10 @@ module ActiveRecord
       mattr_accessor :timestamped_migrations, instance_writer: false
       self.timestamped_migrations = true
 
-      ##
-      # :singleton-method:
-      # Disable implicit join references. This feature was deprecated with Rails 4.
-      # If you don't make use of implicit references but still see deprecation warnings
-      # you can disable the feature entirely. This will be the default with Rails 4.1.
-      mattr_accessor :disable_implicit_join_references, instance_writer: false
-      self.disable_implicit_join_references = false
+      def self.disable_implicit_join_references=(value)
+        ActiveSupport::Deprecation.warn("Implicit join references were removed with Rails 4.1." \
+                                        "Make sure to remove this configuration because it does nothing.")
+      end
 
       class_attribute :default_connection_handler, instance_writer: false
 

activerecord/lib/active_record/relation.rb

@@ -596,9 +596,7 @@ module ActiveRecord
 
     def references_eager_loaded_tables?
       joined_tables = arel.join_sources.map do |join|
-        if join.is_a?(Arel::Nodes::StringJoin)
-          tables_in_string(join.left)
-        else
+        unless join.is_a?(Arel::Nodes::StringJoin)
           [join.left.table_name, join.left.table_alias]
         end
       end
@@ -607,41 +605,8 @@ module ActiveRecord
 
       # always convert table names to downcase as in Oracle quoted table names are in uppercase
       joined_tables = joined_tables.flatten.compact.map { |t| t.downcase }.uniq
-      string_tables = tables_in_string(to_sql)
 
-      if (references_values - joined_tables).any?
-        true
-      elsif !ActiveRecord::Base.disable_implicit_join_references &&
-            (string_tables - joined_tables).any?
-        ActiveSupport::Deprecation.warn(
-          "It looks like you are eager loading table(s) (one of: #{string_tables.join(', ')}) " \
-          "that are referenced in a string SQL snippet. For example: \n" \
-          "\n" \
-          "    Post.includes(:comments).where(\"comments.title = 'foo'\")\n" \
-          "\n" \
-          "Currently, Active Record recognizes the table in the string, and knows to JOIN the " \
-          "comments table to the query, rather than loading comments in a separate query. " \
-          "However, doing this without writing a full-blown SQL parser is inherently flawed. " \
-          "Since we don't want to write an SQL parser, we are removing this functionality. " \
-          "From now on, you must explicitly tell Active Record when you are referencing a table " \
-          "from a string:\n" \
-          "\n" \
-          "    Post.includes(:comments).where(\"comments.title = 'foo'\").references(:comments)\n" \
-          "\n" \
-          "If you don't rely on implicit join references you can disable the feature entirely " \
-          "by setting `config.active_record.disable_implicit_join_references = true`."
-        )
-        true
-      else
-        false
-      end
-    end
-
-    def tables_in_string(string)
-      return [] if string.blank?
-      # always convert table names to downcase as in Oracle quoted table names are in uppercase
-      # ignore raw_sql_ that is used by Oracle adapter as alias for limit/offset subqueries
-      string.scan(/([a-zA-Z_][.\w]+).?\./).flatten.map{ |s| s.downcase }.uniq - ['raw_sql_']
+      (references_values - joined_tables).any?
     end
   end
 end

activerecord/test/cases/associations/eager_test.rb

@@ -346,9 +346,7 @@ class EagerAssociationTest < ActiveRecord::TestCase
 
   def test_eager_association_loading_with_belongs_to_and_conditions_string_with_unquoted_table_name
     assert_nothing_raised do
-      ActiveSupport::Deprecation.silence do
-        Comment.all.merge!(:includes => :post, :where => ['posts.id = ?',4]).to_a
-      end
+      Comment.includes(:post).references(:posts).where('posts.id = ?', 4)
     end
   end
 
@@ -367,9 +365,7 @@ class EagerAssociationTest < ActiveRecord::TestCase
   def test_eager_association_loading_with_belongs_to_and_conditions_string_with_quoted_table_name
     quoted_posts_id= Comment.connection.quote_table_name('posts') + '.' + Comment.connection.quote_column_name('id')
     assert_nothing_raised do
-      ActiveSupport::Deprecation.silence do
-        Comment.all.merge!(:includes => :post, :where => ["#{quoted_posts_id} = ?",4]).to_a
-      end
+      Comment.includes(:post).references(:posts).where("#{quoted_posts_id} = ?", 4)
     end
   end
 
@@ -382,9 +378,7 @@ class EagerAssociationTest < ActiveRecord::TestCase
   def test_eager_association_loading_with_belongs_to_and_order_string_with_quoted_table_name
     quoted_posts_id= Comment.connection.quote_table_name('posts') + '.' + Comment.connection.quote_column_name('id')
     assert_nothing_raised do
-      ActiveSupport::Deprecation.silence do
-        Comment.all.merge!(:includes => :post, :order => quoted_posts_id).to_a
-      end
+      Comment.includes(:post).references(:posts).order(quoted_posts_id)
     end
   end
 
@@ -548,14 +542,10 @@ class EagerAssociationTest < ActiveRecord::TestCase
   end
 
   def test_eager_with_has_many_and_limit_and_conditions_array_on_the_eagers
-    posts = ActiveSupport::Deprecation.silence do
-      Post.all.merge!(:includes => [ :author, :comments ], :limit => 2, :where => [ "authors.name = ?", 'David' ]).to_a
-    end
+    posts = Post.includes(:author, :comments).limit(2).references(:author).where("authors.name = ?", 'David')
     assert_equal 2, posts.size
 
-    count = ActiveSupport::Deprecation.silence do
-      Post.includes(:author, :comments).limit(2).references(:author).where("authors.name = ?", 'David').count
-    end
+    count = Post.includes(:author, :comments).limit(2).references(:author).where("authors.name = ?", 'David').count
     assert_equal posts.size, count
   end
 

activerecord/test/cases/relations_test.rb

@@ -1208,33 +1208,12 @@ class RelationTest < ActiveRecord::TestCase
     assert_equal "id", Post.all.primary_key
   end
 
-  def test_eager_loading_with_conditions_on_joins
-    scope = Post.includes(:comments)
-
-    # This references the comments table, and so it should cause the comments to be eager
-    # loaded via a JOIN, rather than by subsequent queries.
-    scope = scope.joins(
-      Post.arel_table.create_join(
-        Post.arel_table,
-        Post.arel_table.create_on(Comment.arel_table[:id].eq(3))
-      )
-    )
-
+  def test_disable_implicit_join_references_is_deprecated
     assert_deprecated do
-      assert scope.eager_loading?
+      ActiveRecord::Base.disable_implicit_join_references = true
     end
   end
 
-  def test_turn_off_eager_loading_with_conditions_on_joins
-    original_value = ActiveRecord::Base.disable_implicit_join_references
-    ActiveRecord::Base.disable_implicit_join_references = true
-
-    scope = Topic.where(author_email_address: 'my.example@gmail.com').includes(:replies)
-    assert_not scope.eager_loading?
-  ensure
-    ActiveRecord::Base.disable_implicit_join_references = original_value
-  end
-
   def test_ordering_with_extra_spaces
     assert_equal authors(:david), Author.order('id DESC , name DESC').last
   end

activerecord/test/cases/scoping/named_scoping_test.rb

@@ -60,11 +60,6 @@ class NamedScopingTest < ActiveRecord::TestCase
     assert Topic.approved.respond_to?(:length)
   end
 
-  def test_respond_to_respects_include_private_parameter
-    assert !Topic.approved.respond_to?(:tables_in_string)
-    assert Topic.approved.respond_to?(:tables_in_string, true)
-  end
-
   def test_scopes_with_options_limit_finds_to_those_matching_the_criteria_specified
     assert !Topic.all.merge!(:where => {:approved => true}).to_a.empty?