Skip to content

Commit

Permalink
Deprecate limit strings with commas
Browse files Browse the repository at this point in the history
Some backends allow `LIMIT 1,2` as a shorthand for `LIMIT 1 OFFSET 2`.
Supporting this in Active Record massively complicates using bind
parameters for limit and offset, and it's trivially easy to build an
invalid SQL query by also calling `offset` on the same `Relation`.

This is a niche syntax that is only supported by a few adapters, and can
be trivially worked around by calling offset explicitly.
  • Loading branch information
sgrif committed Dec 14, 2015
1 parent 9a17ce8 commit 4358b0d
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 3 deletions.
6 changes: 6 additions & 0 deletions activerecord/lib/active_record/relation/query_methods.rb
Expand Up @@ -677,6 +677,12 @@ def limit(value)
end

def limit!(value) # :nodoc:
if ::String === value && value.include?(",")
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.
WARNING
end
self.limit_value = value
self
end
Expand Down
10 changes: 7 additions & 3 deletions activerecord/test/cases/base_test.rb
Expand Up @@ -112,7 +112,9 @@ def test_primary_key_with_no_id

unless current_adapter?(:PostgreSQLAdapter, :OracleAdapter, :SQLServerAdapter, :FbAdapter)
def test_limit_with_comma
assert Topic.limit("1,2").to_a
assert_deprecated do
assert Topic.limit("1,2").to_a
end
end
end

Expand All @@ -138,8 +140,10 @@ def test_limit_should_sanitize_sql_injection_for_limit_without_commas
end

def test_limit_should_sanitize_sql_injection_for_limit_with_commas
assert_raises(ArgumentError) do
Topic.limit("1, 7 procedure help()").to_a
assert_deprecated do
assert_raises(ArgumentError) do
Topic.limit("1, 7 procedure help()").to_a
end
end
end

Expand Down

0 comments on commit 4358b0d

Please sign in to comment.