Moved transaction boundary to cover used-by tests on deletion so that the database...

diff --git a/app/models/node.rb b/app/models/node.rb
index f2fe341aecc2bf6fcc3b82701201fb11eb48f03b..e1ad818ddec0c53991690a3c52727d7a09af930f 100644 (file)
--- a/app/models/node.rb
+++ b/app/models/node.rb
@@ -155,7 +155,14 @@ class Node < ActiveRecord::Base
 
   # Should probably be renamed delete_from to come in line with update
   def delete_with_history!(new_node, user)
 
   # Should probably be renamed delete_from to come in line with update
   def delete_with_history!(new_node, user)

-    if self.visible
+    unless self.visible
+      raise OSM::APIAlreadyDeletedError.new
+    end
+
+    # need to start the transaction here, so that the database can 
+    # provide repeatable reads for the used-by checks. this means it
+    # shouldn't be possible to get race conditions.
+    Node.transaction do

       check_consistency(self, new_node, user)
       if WayNode.find(:first, :joins => "INNER JOIN current_ways ON current_ways.id = current_way_nodes.id", :conditions => [ "current_ways.visible = ? AND current_way_nodes.node_id = ?", true, self.id ])
         raise OSM::APIPreconditionFailedError.new
       check_consistency(self, new_node, user)
       if WayNode.find(:first, :joins => "INNER JOIN current_ways ON current_ways.id = current_way_nodes.id", :conditions => [ "current_ways.visible = ? AND current_way_nodes.node_id = ?", true, self.id ])
         raise OSM::APIPreconditionFailedError.new


@@ -164,14 +171,12 @@ class Node < ActiveRecord::Base
       else
         self.changeset_id = new_node.changeset_id
         self.visible = false
       else
         self.changeset_id = new_node.changeset_id
         self.visible = false

-
+        

         # update the changeset with the deleted position
         changeset.update_bbox!(bbox)
         # update the changeset with the deleted position
         changeset.update_bbox!(bbox)

-
+        

         save_with_history!
       end
         save_with_history!
       end

-    else
-      raise OSM::APIAlreadyDeletedError.new

     end
   end
 
     end
   end
 

diff --git a/app/models/relation.rb b/app/models/relation.rb
index 0159c60b875d791ccb7b690ea12a263d436f78d2..0bdacb5ec70f1bb488dcb841f20bdaaa49819b37 100644 (file)
--- a/app/models/relation.rb
+++ b/app/models/relation.rb
@@ -347,7 +347,14 @@ class Relation < ActiveRecord::Base
   end    
 
   def delete_with_history!(new_relation, user)
   end    
 
   def delete_with_history!(new_relation, user)

-    if self.visible
+    unless self.visible
+      raise OSM::APIAlreadyDeletedError.new
+    end
+
+    # need to start the transaction here, so that the database can 
+    # provide repeatable reads for the used-by checks. this means it
+    # shouldn't be possible to get race conditions.
+    Relation.transaction do

       check_consistency(self, new_relation, user)
       # This will check to see if this relation is used by another relation
       if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = ? AND member_type='relation' and member_id=? ", true, self.id ])
       check_consistency(self, new_relation, user)
       # This will check to see if this relation is used by another relation
       if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = ? AND member_type='relation' and member_id=? ", true, self.id ])


@@ -358,8 +365,6 @@ class Relation < ActiveRecord::Base
       self.members = []
       self.visible = false
       save_with_history!
       self.members = []
       self.visible = false
       save_with_history!

-    else
-      raise OSM::APIAlreadyDeletedError.new

     end
   end
 
     end
   end
 

diff --git a/app/models/way.rb b/app/models/way.rb
index e2e1ae302c17bd465c4561f9dcc7d2d9e0958946..2071fd559c33ddd136da6608a71606f4698362b7 100644 (file)
--- a/app/models/way.rb
+++ b/app/models/way.rb
@@ -283,8 +283,15 @@ class Way < ActiveRecord::Base
   end
 
   def delete_with_history!(new_way, user)
   end
 
   def delete_with_history!(new_way, user)

-    check_consistency(self, new_way, user)
-    if self.visible
+    unless self.visible
+      raise OSM::APIAlreadyDeletedError
+    end
+    
+    # need to start the transaction here, so that the database can 
+    # provide repeatable reads for the used-by checks. this means it
+    # shouldn't be possible to get race conditions.
+    Way.transaction do
+      check_consistency(self, new_way, user)

       if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id",
                              :conditions => [ "visible = ? AND member_type='way' and member_id=? ", true, self.id])
         raise OSM::APIPreconditionFailedError
       if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id",
                              :conditions => [ "visible = ? AND member_type='way' and member_id=? ", true, self.id])
         raise OSM::APIPreconditionFailedError


@@ -295,8 +302,6 @@ class Way < ActiveRecord::Base
         self.visible = false
         self.save_with_history!
       end
         self.visible = false
         self.save_with_history!
       end

-    else
-      raise OSM::APIAlreadyDeletedError

     end
   end
 
     end
   end