Skip to content

Commit acd697b

Browse files
committed
[Rails3] No longer ask the engine/adapter to perform crazy string manipulation for adding limit offset, but rather do all this in the Arel SQL compiler.
1 parent c828694 commit acd697b

File tree

3 files changed

+22
-105
lines changed

3 files changed

+22
-105
lines changed

RAILS3_NOTES

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ on a local branch of our remote tracking branch.
3434

3535
= SQL Server Todo
3636

37-
2364 tests, 9480 assertions, 9 failures, 48 errors
37+
2364 tests, 8447 assertions, 8 failures, 47 errors
3838

3939
* SQL Server Test Cases
4040
x eager_association_test_sqlserver.rb (2) Limit offset stuff.

lib/active_record/connection_adapters/sqlserver/database_statements.rb

Lines changed: 1 addition & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -43,110 +43,7 @@ def rollback_to_savepoint
4343
end
4444

4545
def add_limit_offset!(sql, options)
46-
# Easy locals.
47-
limit = options[:limit].try(:to_i)
48-
offset = options[:offset].try(:to_i)
49-
select_clauses = options[:select_clauses]
50-
order_clauses = options[:order_clauses]
51-
primary_key = options[:primary_key]
52-
table_name = options[:table_name]
53-
# Template strings
54-
limit_string = "TOP (#{limit}) " if limit.present?
55-
select_string = select_clauses.join(', ').gsub(%r|\[#{table_name}\]\.|,'[_rnt].')
56-
order_string = order_clauses.present? ? order_clauses.join(', ') : [quote_table_name(table_name),quote_column_name(primary_key)].join('.')
57-
window_template =
58-
'SELECT #{limit_string}#{select_string} ' +
59-
'FROM (SELECT ROW_NUMBER() OVER (ORDER BY #{order_string}) AS [rn], #{sql.strip}) AS [_rnt] ' +
60-
'WHERE [_rnt].[rn] > #{offset}'
61-
# Assembly
62-
sql.sub! /SELECT/i, ''
63-
sql.replace instance_eval("%|#{window_template}|")
64-
65-
# http://github.com/kenglishhi/2000-2005-adapter/commit/476ac1f6dbd517ed090ef350e6d5855a581f6cbf
66-
# if options[:limit] and options[:offset]
67-
# sql.sub! /ORDER BY.*$/i, ''
68-
# sql.sub! /SELECT/i, "SELECT ROW_NUMBER() OVER (ORDER BY #{options[:order]}) AS [row_number], "
69-
# sql.replace "SELECT TOP (#{options[:limit]}) * FROM (#{sql}) as [row_number_table] WHERE [row_number_table].[row_number] > #{options[:offset]}"
70-
# end
71-
72-
# options = {:limit => 10, :offset => 20, :order => '[users].[id] ASC'}
73-
# sql = %|SELECT [users].[id], [users].[name] FROM [users]|
74-
# >> sql.sub! /SELECT/i, "SELECT ROW_NUMBER() OVER (ORDER BY #{options[:order]}) AS [row_number], "
75-
# => "SELECT ROW_NUMBER() OVER (ORDER BY [users].[id] ASC) AS [row_number], [users].[id], [users].[name] FROM [users]"
76-
# >> sql.replace "SELECT TOP (#{options[:limit]}) * FROM (#{sql}) as [row_number_table] WHERE [row_number_table].[row_number] > #{options[:offset]}"
77-
# SELECT TOP (10) * FROM (
78-
# SELECT ROW_NUMBER() OVER (ORDER BY [users].[id] ASC) AS [row_number],
79-
# [users].[id], [users].[name] FROM [users]
80-
# ) as [row_number_table] WHERE [row_number_table].[row_number] > 20"
81-
82-
# SELECT * FROM
83-
# (SELECT ROW_NUMBER()
84-
# OVER (ORDER BY [users].[id]) AS [rn],
85-
# [users].[id], [users].[name]
86-
# FROM [users]) AS [_rnt]
87-
# WHERE [_rnt].[rn] > 4
88-
89-
# options[:offset] ||= 0
90-
# if options[:offset] > 0
91-
# options[:order] ||= if order_by = sql.match(/ORDER BY (.*$)/i)
92-
# order_by[1]
93-
# else
94-
# table_name = sql.match('FROM ([\[\]a-zA-Z0-9_\.]+)')[1]
95-
#
96-
# find_table_primary_key_columns(table_name)
97-
# end
98-
# sql.sub!(/ORDER BY.*$/i, '')
99-
# sql.sub!(/SELECT/i, "SELECT * FROM ( SELECT ROW_NUMBER() OVER( ORDER BY #{options[:order] } ) AS row_num, ")
100-
# sql << ") AS t WHERE row_num > #{options[:offset]}"
101-
# end
102-
# sql.sub!(/^SELECT/i, "SELECT TOP #{options[:limit]}") if options[:limit]
103-
# sql
104-
105-
106-
107-
# # The business of adding limit/offset
108-
# if options[:limit] and options[:offset]
109-
# tally_sql = "SELECT count(*) as TotalRows from (#{sql.sub(/\bSELECT(\s+DISTINCT)?\b/i, "SELECT#{$1} TOP 1000000000")}) tally"
110-
# add_lock! tally_sql, options
111-
# total_rows = select_value(tally_sql).to_i
112-
# if (options[:limit] + options[:offset]) >= total_rows
113-
# options[:limit] = (total_rows - options[:offset] >= 0) ? (total_rows - options[:offset]) : 0
114-
# end
115-
# # Make sure we do not need a special limit/offset for association limiting. http://gist.github.com/25118
116-
# add_limit_offset_for_association_limiting!(sql,options) and return if sql_for_association_limiting?(sql)
117-
# # Wrap the SQL query in a bunch of outer SQL queries that emulate proper LIMIT,OFFSET support.
118-
# sql.sub!(/^\s*SELECT(\s+DISTINCT)?/i, "SELECT * FROM (SELECT TOP #{options[:limit]} * FROM (SELECT#{$1} TOP #{options[:limit] + options[:offset]}")
119-
# sql << ") AS tmp1"
120-
# if options[:order]
121-
# order = options[:order].split(',').map do |field|
122-
# order_by_column, order_direction = field.split(" ")
123-
# order_by_column = quote_column_name(order_by_column)
124-
# # Investigate the SQL query to figure out if the order_by_column has been renamed.
125-
# if sql =~ /#{Regexp.escape(order_by_column)} AS (t\d+_r\d+)/
126-
# # Fx "[foo].[bar] AS t4_r2" was found in the SQL. Use the column alias (ie 't4_r2') for the subsequent orderings
127-
# order_by_column = $1
128-
# elsif order_by_column =~ /\w+\.\[?(\w+)\]?/
129-
# order_by_column = $1
130-
# else
131-
# # It doesn't appear that the column name has been renamed as part of the query. Use just the column
132-
# # name rather than the full identifier for the outer queries.
133-
# order_by_column = order_by_column.split('.').last
134-
# end
135-
# # Put the column name and eventual direction back together
136-
# [order_by_column, order_direction].join(' ').strip
137-
# end.join(', ')
138-
# sql << " ORDER BY #{change_order_direction(order)}) AS tmp2 ORDER BY #{order}"
139-
# else
140-
# sql << ") AS tmp2"
141-
# end
142-
# elsif options[:limit] && sql !~ /^\s*SELECT (@@|COUNT\()/i
143-
# if md = sql.match(/^(\s*SELECT)(\s+DISTINCT)?(.*)/im)
144-
# sql.replace "#{md[1]}#{md[2]} TOP #{options[:limit]}#{md[3]}"
145-
# else
146-
# # Account for building SQL fragments without SELECT yet. See #update_all and #limited_update_conditions.
147-
# sql.replace "TOP #{options[:limit]} #{sql}"
148-
# end
149-
# end
46+
raise NotImplementedError, 'This has been moved to the SQLServerCompiler in Arel.'
15047
end
15148

15249
def empty_insert_statement_value

test/cases/adapter_test_sqlserver.rb

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -621,3 +621,23 @@ def with_enable_default_unicode_types(setting)
621621

622622
end
623623

624+
625+
class AdapterTest < ActiveRecord::TestCase
626+
627+
COERCED_TESTS = [
628+
:test_add_limit_offset_should_sanitize_sql_injection_for_limit_without_comas,
629+
:test_add_limit_offset_should_sanitize_sql_injection_for_limit_with_comas
630+
]
631+
632+
include SqlserverCoercedTest
633+
634+
def test_coerced_test_add_limit_offset_should_sanitize_sql_injection_for_limit_without_comas
635+
true # Tested in our OffsetAndLimitTestSqlserver test case.
636+
end
637+
638+
def test_coerced_test_add_limit_offset_should_sanitize_sql_injection_for_limit_with_comas
639+
true # Tested in our OffsetAndLimitTestSqlserver test case.
640+
end
641+
642+
643+
end

0 commit comments

Comments
 (0)