Make add_limit_offset! only add locking hints (for tally) when the :lock option is present. Added tests to make sure tally SQL is augmented correctly and tests to make sure that add_lock! is doing what it needs for deep sub selects in paginated results.

Author: metaskills

CHANGELOG

@@ -1,6 +1,10 @@
 
 MASTER
 
+* Make add_limit_offset! only add locking hints (for tally) when the :lock option is present. Added tests 
+  to make sure tally SQL is augmented correctly and tests to make sure that add_lock! is doing what it needs 
+  for deep sub selects in paginated results. [Ken Collins]
+
 * Add auto reconnect support utilizing a new #with_auto_reconnect block. By default each query run through 
   the adapter will automatically reconnect at standard intervals, logging attempts along the way, till success 
   or the original exception bubbles up. See docs for more details. Resolves ticket #18 [Ken Collins] 

lib/active_record/connection_adapters/sqlserver_adapter.rb

@@ -437,7 +437,7 @@ def add_limit_offset!(sql, options)
         # The business of adding limit/offset
         if options[:limit] and options[:offset]
           tally_sql = "SELECT count(*) as TotalRows from (#{sql.sub(/\bSELECT(\s+DISTINCT)?\b/i, "SELECT#{$1} TOP 1000000000")}) tally"
-          add_lock! tally_sql, :lock => 'WITH (NOLOCK)'
+          add_lock! tally_sql, options
           total_rows = select_value(tally_sql).to_i
           if (options[:limit] + options[:offset]) >= total_rows
             options[:limit] = (total_rows - options[:offset] >= 0) ? (total_rows - options[:offset]) : 0

test/cases/offset_and_limit_test_sqlserver.rb

@@ -66,9 +66,16 @@ def setup
 
     should 'alter SQL to limit number of records returned offset by specified amount' do
       options = { :limit => 3, :offset => 5 }
-      expected_sql = %&SELECT * FROM (SELECT TOP 3 * FROM (SELECT TOP 8 * FROM books) AS tmp1) AS tmp2&
+      expected_sql = "SELECT * FROM (SELECT TOP 3 * FROM (SELECT TOP 8 * FROM books) AS tmp1) AS tmp2"
       assert_equal(expected_sql, @connection.add_limit_offset!(@select_sql, options))
     end
+    
+    should 'add locks to deepest sub select in limit offset sql that has a limited tally' do
+      options = { :limit => 3, :offset => 5, :lock => 'WITH (NOLOCK)' }
+      expected_sql = "SELECT * FROM (SELECT TOP 3 * FROM (SELECT TOP 8 * FROM books WITH (NOLOCK)) AS tmp1) AS tmp2"
+      @connection.add_limit_offset! @select_sql, options
+      assert_equal expected_sql, @connection.add_lock!(@select_sql,options)
+    end
 
     # Not really sure what an offset sql injection might look like
     should 'not allow sql injection via offset' do
@@ -82,6 +89,18 @@ def setup
       assert_equal expected_sql, @connection.add_limit_offset!(@subquery_select_sql,options)
     end
 
+    should 'add lock hints to tally sql if :lock option is present' do
+      assert_sql %r|SELECT TOP 1000000000 \* FROM \[people\] WITH \(NOLOCK\)| do
+        Person.all :limit => 5, :offset => 1, :lock => 'WITH (NOLOCK)'
+      end
+    end
+    
+    should 'not add lock hints to tally sql if there is no :lock option' do
+      assert_sql %r|\(SELECT TOP 1000000000 \* FROM \[people\] \)| do
+        Person.all :limit => 5, :offset => 1
+      end
+    end
+    
   end