Don't cache `find_by` statements on STI subclasses

Caching `find_by` statements on STI subclasses is unsafe, since
`type IN (?,?,?,?)` part is dynamic, and we don't have SQL statements
cache invalidation when a STI subclass is created or removed for now.

3 files changed, 22 insertions, 5 deletions

diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb
index c67980173f..18cfac1f2f 100644
--- a/activerecord/lib/active_record/core.rb
+++ b/activerecord/lib/active_record/core.rb
@@ -182,7 +182,8 @@ module ActiveRecord
       end
 
       def find_by(*args) # :nodoc:
-        return super if scope_attributes? || reflect_on_all_aggregations.any?
+        return super if scope_attributes? || reflect_on_all_aggregations.any? ||
+                        columns_hash.key?(inheritance_column) && !base_class?
 
         hash = args.first
 
diff --git a/activerecord/test/cases/bind_parameter_test.rb b/activerecord/test/cases/bind_parameter_test.rb
index 8c3fe0437c..b89f054821 100644
--- a/activerecord/test/cases/bind_parameter_test.rb
+++ b/activerecord/test/cases/bind_parameter_test.rb
@@ -2,6 +2,7 @@
 
 require "cases/helper"
 require "models/topic"
+require "models/reply"
 require "models/author"
 require "models/post"
 
@@ -53,12 +54,17 @@ if ActiveRecord::Base.connection.prepared_statements
         @connection.disable_query_cache!
       end
 
-      def test_statement_cache_with_find
+      def test_statement_cache_with_find_by
         @connection.clear_cache!
 
-        topics = Topic.where(id: 1).limit(1)
-        assert_equal 1, Topic.find(1).id
-        assert_includes statement_cache, to_sql_key(topics.arel)
+        assert_equal 1, Topic.find_by!(id: 1).id
+        assert_equal 2, Reply.find_by!(id: 2).id
+
+        topic_sql = cached_statement(Topic, [:id])
+        assert_includes statement_cache, to_sql_key(topic_sql)
+
+        e = assert_raise { cached_statement(Reply, [:id]) }
+        assert_equal "Reply has no cached statement by [:id]", e.message
       end
 
       def test_statement_cache_with_in_clause
@@ -139,6 +145,13 @@ if ActiveRecord::Base.connection.prepared_statements
           @connection.respond_to?(:sql_key, true) ? @connection.send(:sql_key, sql) : sql
         end
 
+        def cached_statement(klass, key)
+          cache = klass.send(:cached_find_by_statement, key) do
+            raise "#{klass} has no cached statement by #{key.inspect}"
+          end
+          cache.send(:query_builder).instance_variable_get(:@sql)
+        end
+
         def statement_cache
           @connection.instance_variable_get(:@statements).send(:cache)
         end
diff --git a/activerecord/test/cases/inheritance_test.rb b/activerecord/test/cases/inheritance_test.rb
index 19655a2d38..629167e9ed 100644
--- a/activerecord/test/cases/inheritance_test.rb
+++ b/activerecord/test/cases/inheritance_test.rb
@@ -514,10 +514,12 @@ class InheritanceComputeTypeTest < ActiveRecord::TestCase
 
       # Should fail without FirmOnTheFly in the type condition.
       assert_raise(ActiveRecord::RecordNotFound) { Firm.find(foo.id) }
+      assert_raise(ActiveRecord::RecordNotFound) { Firm.find_by!(id: foo.id) }
 
       # Nest FirmOnTheFly in the test case where Dependencies won't see it.
       self.class.const_set :FirmOnTheFly, Class.new(Firm)
       assert_raise(ActiveRecord::SubclassNotFound) { Firm.find(foo.id) }
+      assert_raise(ActiveRecord::SubclassNotFound) { Firm.find_by!(id: foo.id) }
 
       # Nest FirmOnTheFly in Firm where Dependencies will see it.
       # This is analogous to nesting models in a migration.
@@ -526,6 +528,7 @@ class InheritanceComputeTypeTest < ActiveRecord::TestCase
       # And instantiate will find the existing constant rather than trying
       # to require firm_on_the_fly.
       assert_nothing_raised { assert_kind_of Firm::FirmOnTheFly, Firm.find(foo.id) }
+      assert_nothing_raised { assert_kind_of Firm::FirmOnTheFly, Firm.find_by!(id: foo.id) }
     end
   end