From 574f255629a45cd67babcfb9bb8e163e091a53b8 Mon Sep 17 00:00:00 2001
From: Sean Griffin <sean@seantheprogrammer.com>
Date: Mon, 14 Dec 2015 08:34:14 -0700
Subject: [PATCH] Use a bind param for `LIMIT` and `OFFSET`

We currently generate an unbounded number of prepared statements when
`limit` or `offset` are called with a dynamic argument. This changes
`LIMIT` and `OFFSET` to use bind params, eliminating the problem.

`Type::Value#hash` needed to be implemented, as it turns out we busted
the query cache if the type object used wasn't exactly the same object.

This drops support for passing an `Arel::Nodes::SqlLiteral` to `limit`.
Doing this relied on AR internals, and was never officially supported
usage.

Fixes #22250.
---
 activemodel/lib/active_model/type/value.rb    |  5 +++
 activerecord/CHANGELOG.md                     |  9 +++++
 .../active_record/relation/query_methods.rb   | 34 ++++++++++++++++---
 activerecord/test/cases/base_test.rb          |  6 ----
 activerecord/test/cases/finder_test.rb        |  6 ++--
 5 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/activemodel/lib/active_model/type/value.rb b/activemodel/lib/active_model/type/value.rb
index 5fea0561a6..9d1f267b41 100644
--- a/activemodel/lib/active_model/type/value.rb
+++ b/activemodel/lib/active_model/type/value.rb
@@ -90,6 +90,11 @@ module ActiveModel
           scale == other.scale &&
           limit == other.limit
       end
+      alias eql? ==
+
+      def hash
+        [self.class, precision, scale, limit].hash
+      end
 
       def assert_valid_value(*)
       end
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 56a3232ee9..8cedfd5277 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,12 @@
+*   Use bind params for `limit` and `offset`. This will generate significantly
+    fewer prepared statements for common tasks like pagination. To support this
+    change, passing a string containing a comma to `limit` has been deprecated,
+    and passing an Arel node to `limit` is no longer supported.
+
+    Fixes #22250
+
+    *Sean Griffin*
+
 *   Introduce after_{create,update,delete}_commit callbacks.
 
     Before:
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index f7115c7a91..66f00c31e2 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -97,7 +97,22 @@ module ActiveRecord
     end
 
     def bound_attributes
-      from_clause.binds + arel.bind_values + where_clause.binds + having_clause.binds
+      result = from_clause.binds + arel.bind_values + where_clause.binds + having_clause.binds
+      if limit_value && !string_containing_comma?(limit_value)
+        result << Attribute.with_cast_value(
+          "LIMIT".freeze,
+          connection.sanitize_limit(limit_value),
+          Type::Value.new,
+        )
+      end
+      if offset_value
+        result << Attribute.with_cast_value(
+          "OFFSET".freeze,
+          offset_value.to_i,
+          Type::Value.new,
+        )
+      end
+      result
     end
 
     def create_with_value # :nodoc:
@@ -677,7 +692,8 @@ module ActiveRecord
     end
 
     def limit!(value) # :nodoc:
-      if ::String === value && value.include?(",")
+      if string_containing_comma?(value)
+        # Remove `string_containing_comma?` when removing this deprecation
         ActiveSupport::Deprecation.warn(<<-WARNING)
           Passing a string to limit in the form "1,2" is deprecated and will be
           removed in Rails 5.1. Please call `offset` explicitly instead.
@@ -933,8 +949,14 @@ module ActiveRecord
 
       arel.where(where_clause.ast) unless where_clause.empty?
       arel.having(having_clause.ast) unless having_clause.empty?
-      arel.take(connection.sanitize_limit(limit_value)) if limit_value
-      arel.skip(offset_value.to_i) if offset_value
+      if limit_value
+        if string_containing_comma?(limit_value)
+          arel.take(connection.sanitize_limit(limit_value))
+        else
+          arel.take(Arel::Nodes::BindParam.new)
+        end
+      end
+      arel.skip(Arel::Nodes::BindParam.new) if offset_value
       arel.group(*arel_columns(group_values.uniq.reject(&:blank?))) unless group_values.empty?
 
       build_order(arel)
@@ -1183,5 +1205,9 @@ module ActiveRecord
     def new_from_clause
       Relation::FromClause.empty
     end
+
+    def string_containing_comma?(value)
+      ::String === value && value.include?(",")
+    end
   end
 end
diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index b449280fb4..dc555caaff 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -147,12 +147,6 @@ class BasicsTest < ActiveRecord::TestCase
     end
   end
 
-  unless current_adapter?(:MysqlAdapter, :Mysql2Adapter)
-    def test_limit_should_allow_sql_literal
-      assert_equal 1, Topic.limit(Arel.sql('2-1')).to_a.length
-    end
-  end
-
   def test_select_symbol
     topic_ids = Topic.select(:id).map(&:id).sort
     assert_equal Topic.pluck(:id).sort, topic_ids
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index 91214da048..73f5312eba 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -434,9 +434,9 @@ class FinderTest < ActiveRecord::TestCase
   end
 
   def test_take_and_first_and_last_with_integer_should_use_sql_limit
-    assert_sql(/LIMIT 3|ROWNUM <= 3/) { Topic.take(3).entries }
-    assert_sql(/LIMIT 2|ROWNUM <= 2/) { Topic.first(2).entries }
-    assert_sql(/LIMIT 5|ROWNUM <= 5/) { Topic.last(5).entries }
+    assert_sql(/LIMIT|ROWNUM <=/) { Topic.take(3).entries }
+    assert_sql(/LIMIT|ROWNUM <=/) { Topic.first(2).entries }
+    assert_sql(/LIMIT|ROWNUM <=/) { Topic.last(5).entries }
   end
 
   def test_last_with_integer_and_order_should_keep_the_order