Locking now done.

Default lock is now "WITH (UPDLOCK)". Fixes #368

Author: metaskills

CHANGELOG.md

@@ -37,7 +37,7 @@
 
 #### Fixed
 
-* n/a
+* Default lock is now "WITH (UPDLOCK)". Fixes #368
 
 #### Security
 

lib/arel/visitors/sqlserver.rb

@@ -16,6 +16,12 @@ def visit_Arel_Nodes_BindParam o, collector
         collector.add_bind(o) { |i| "@#{i-1}" }
       end
 
+      def visit_Arel_Nodes_Lock o, collector
+        o.expr = Arel.sql('WITH (UPDLOCK)') if o.expr.to_s =~ /FOR UPDATE/
+        collector << SPACE
+        visit o.expr, collector
+      end
+
       def visit_Arel_Nodes_Offset o, collector
         collector << OFFSET
         visit o.expr, collector
@@ -29,6 +35,7 @@ def visit_Arel_Nodes_Limit o, collector
       end
 
       def visit_Arel_Nodes_SelectStatement o, collector
+        @select_statement = o
         if o.with
           collector = visit o.with, collector
           collector << SPACE
@@ -38,12 +45,41 @@ def visit_Arel_Nodes_SelectStatement o, collector
         }
         collector = visit_Orders_And_Let_Fetch_Happen o, collector
         collector = visit_Make_Fetch_Happen o, collector
-        collector = visit o.lock, collector if o.lock
+        collector
+      ensure
+        @select_statement = nil
+      end
+
+      def visit_Arel_Nodes_JoinSource o, collector
+        if o.left
+          collector = visit o.left, collector
+          collector = visit_Arel_Nodes_SelectStatement_SQLServer_Lock collector
+        end
+        if o.right.any?
+          collector << " " if o.left
+          collector = inject_join o.right, collector, ' '
+        end
         collector
       end
 
+      def visit_Arel_Nodes_OuterJoin o, collector
+        collector << "LEFT OUTER JOIN "
+        collector = visit o.left, collector
+        collector = visit_Arel_Nodes_SelectStatement_SQLServer_Lock collector, space: true
+        collector << " "
+        visit o.right, collector
+      end
+
       # SQLServer ToSql/Visitor (Additions)
 
+      def visit_Arel_Nodes_SelectStatement_SQLServer_Lock collector, options = {}
+        if select_statement_lock?
+          collector = visit @select_statement.lock, collector
+          collector << SPACE if options[:space]
+        end
+        collector
+      end
+
       def visit_Orders_And_Let_Fetch_Happen o, collector
         if (o.limit || o.offset) && o.orders.empty?
           table = table_From_Statement o
@@ -71,6 +107,10 @@ def visit_Make_Fetch_Happen o, collector
 
       # SQLServer Helpers
 
+      def select_statement_lock?
+        @select_statement && @select_statement.lock
+      end
+
       def table_From_Statement o
         core = o.cores.first
         if Arel::Table === core.from

test/cases/pessimistic_locking_test_sqlserver.rb

@@ -4,42 +4,49 @@
 
 class PessimisticLockingTestSQLServer < ActiveRecord::TestCase
 
-  self.use_transactional_fixtures = false
   fixtures :people, :readers
 
-  setup do
-    Person.columns; Reader.columns # Avoid introspection queries during tests.
+  before do
+    Person.columns
+    Reader.columns
   end
 
-  context 'For simple finds with default lock option' do
+  it 'uses with updlock by default' do
+    assert_sql %r|SELECT \[people\]\.\* FROM \[people\] WITH \(UPDLOCK\)| do
+      Person.lock(true).to_a.must_equal Person.all.to_a
+    end
+  end
 
-    should 'lock with simple find' do
+  describe 'For simple finds with default lock option' do
+
+    it 'lock with simple find' do
       assert_nothing_raised do
         Person.transaction do
-          Person.lock(true).find(1)
+          Person.lock(true).find(1).must_equal Person.find(1)
         end
       end
     end
 
-    should 'lock with scoped find' do
+    it 'lock with scoped find' do
       assert_nothing_raised do
         Person.transaction do
           Person.lock(true).scoping do
-            Person.find(1)
+            Person.find(1).must_equal Person.find(1)
           end
         end
       end
     end
 
-    should 'lock with eager find' do
+    it 'lock with eager find' do
        assert_nothing_raised do
         Person.transaction do
-          Person.lock(true).includes(:readers).find(1)
+          person = Person.lock(true).includes(:readers).find(1)
+          person.must_equal Person.find(1)
         end
       end
     end
 
-    should 'reload with lock when #lock! called' do
+    it 'reload with lock when #lock! called' do
       assert_nothing_raised do
         Person.transaction do
           person = Person.find 1
@@ -50,29 +57,34 @@ class PessimisticLockingTestSQLServer < ActiveRecord::TestCase
       end
     end
 
-    should 'simply add lock to find all' do
-      assert_sql %r|SELECT \[people\]\.\* FROM \[people\] WITH \(NOLOCK\)| do
-        Person.lock('WITH (NOLOCK)').load
+    it 'can add a custom lock directive' do
+      assert_sql %r|SELECT \[people\]\.\* FROM \[people\] WITH\(HOLDLOCK, ROWLOCK\)| do
+        Person.lock('WITH(HOLDLOCK, ROWLOCK)').load
       end
     end
 
   end
 
-  context 'For paginated finds' do
+  describe 'For paginated finds' do
 
-    setup do
+    before do
+      Person.delete_all
       20.times { |n| Person.create!(first_name: "Thing_#{n}") }
     end
 
-    should 'cope with eager loading un-locked paginated' do
-      eager_ids_sql = /SELECT TOP \(5\).*FROM \[people\] WITH \(NOLOCK\)/
-      loader_sql = /FROM \[people\] WITH \(NOLOCK\).*WHERE \[people\]\.\[id\] IN/
-      assert_sql(eager_ids_sql,loader_sql) do
-        Person.lock('WITH (NOLOCK)').limit(5).offset(10).includes(:readers).references(:readers).load
+    it 'copes with eager loading un-locked paginated' do
+      eager_ids_sql = /SELECT\s+DISTINCT \[people\].\[id\] FROM \[people\] WITH \(UPDLOCK\) LEFT OUTER JOIN \[readers\] WITH \(UPDLOCK\)\s+ON \[readers\].\[person_id\] = \[people\].\[id\]\s+ORDER BY \[people\].\[id\] ASC OFFSET 10 ROWS FETCH NEXT 5 ROWS ONLY/
+      loader_sql = /SELECT.*FROM \[people\] WITH \(UPDLOCK\).*WHERE \[people\]\.\[id\] IN/
+      assert_sql(eager_ids_sql, loader_sql) do
+        people = Person.lock(true).limit(5).offset(10).includes(:readers).references(:readers).to_a
+        people[0].first_name.must_equal 'Thing_10'
+        people[1].first_name.must_equal 'Thing_11'
+        people[2].first_name.must_equal 'Thing_12'
+        people[3].first_name.must_equal 'Thing_13'
+        people[4].first_name.must_equal 'Thing_14'
       end
     end
 
   end
 
-
 end