Fix bug allowing created elements to reference deleted ones

The bug allows a newly-created element to refer to a deleted one
if the transactions for both overlap. Precisely, the issue is that
the check that an element exists does not prevent a concurrent
transaction from altering that row.

Because "deleting" an element in the OSM database does not remove
the row, we cannot rely on FK constraints to ensure the correct
behaviour. Instead, this fix relies on manually locking referenced
elements.

Note that this "fix" is suboptimal, as it does not allow any
updates to the referenced elements. Updates which do not delete
the row could safely be done, but will be prevented.

Also, it's not clear what the negative performance impact of this
change will be.

diff --git a/app/models/relation.rb b/app/models/relation.rb
index 3d3c317aa74722677266db9ca729a115c2aaeb59..cb9621822ba63cefd1349318f28c8947765ec829 100644 (file)
--- a/app/models/relation.rb
+++ b/app/models/relation.rb
@@ -246,8 +246,10 @@ class Relation < ActiveRecord::Base
 
       # use reflection to look up the appropriate class
       model = Kernel.const_get(m[0].capitalize)
 
       # use reflection to look up the appropriate class
       model = Kernel.const_get(m[0].capitalize)

-      # get the element with that ID
-      element = model.find_by(:id => m[1])
+      # get the element with that ID. and, if found, lock the element to
+      # ensure it can't be deleted until after the current transaction
+      # commits.
+      element = model.lock("for share").find_by(:id => m[1])

 
       # and check that it is OK to use.
       unless element && element.visible? && element.preconditions_ok?
 
       # and check that it is OK to use.
       unless element && element.visible? && element.preconditions_ok?

diff --git a/app/models/way.rb b/app/models/way.rb
index 34e568e4ab15d407ff052e4fb3526f88209ce48e..6d49735f1406bee653d02783eae58ab076b7e5fc 100644 (file)
--- a/app/models/way.rb
+++ b/app/models/way.rb
@@ -199,7 +199,9 @@ class Way < ActiveRecord::Base
     new_nds = (nds - old_nodes).sort.uniq
 
     unless new_nds.empty?
     new_nds = (nds - old_nodes).sort.uniq
 
     unless new_nds.empty?

-      db_nds = Node.where(:id => new_nds, :visible => true)
+      # NOTE: nodes are locked here to ensure they can't be deleted before
+      # the current transaction commits.
+      db_nds = Node.where(:id => new_nds, :visible => true).lock("for share")

 
       if db_nds.length < new_nds.length
         missing = new_nds - db_nds.collect(&:id)
 
       if db_nds.length < new_nds.length
         missing = new_nds - db_nds.collect(&:id)