Refactoring the amf controller, and all models so that save_with_history! is now private. Everyone should use create_with_history, delete_with_history, and update_from instead. Also gets the amf controller to do version checks in the node/way/relation models. Needs checked. This breaks Potlatch until #1376 is fixed.

app/controllers/amf_controller.rb

@@ -3,7 +3,7 @@
 # OSM database takes place using this controller. Messages are 
 # encoded in the Actionscript Message Format (AMF).
 #
-# Helper functions are in /lib/potlatch.
+# Helper functions are in /lib/potlatch.rb
 #
 # Author::	editions Systeme D / Richard Fairhurst 2004-2008
 # Licence::	public domain.
@@ -130,8 +130,8 @@ class AmfController < ApplicationController
   # Start new changeset
   
   def startchangeset(usertoken, cstags, closeid, closecomment)
-    uid = getuserid(usertoken)
+    user = getuserid(usertoken)
-    if !uid then return -1,"You are not logged in, so Potlatch can't write any changes to the database." end
+    if !user then return -1,"You are not logged in, so Potlatch can't write any changes to the database." end
 
     # close previous changeset and add comment
     if closeid
@@ -392,20 +392,19 @@ class AmfController < ApplicationController
   # 1. original relation id (unchanged),
   # 2. new relation id.
 
-  def putrelation(renumberednodes, renumberedways, usertoken, changeset, relid, tags, members, visible) #:doc:
+  def putrelation(renumberednodes, renumberedways, usertoken, changeset, version, relid, tags, members, visible) #:doc:
-    uid = getuserid(usertoken)
+    user = getuserid(usertoken)
-    if !uid then return -1,"You are not logged in, so the relation could not be saved." end
+    if !user then return -1,"You are not logged in, so the relation could not be saved." end
 
     relid = relid.to_i
     visible = (visible.to_i != 0)
 
     # create a new relation, or find the existing one
-    if relid <= 0
+    if relid > 0
-      rel = Relation.new
+      relation = Relation.find(relid)
-      rel.version = 0
-    else
-      rel = Relation.find(relid)
     end
+    # We always need a new node, based on the data that has been sent to us
+    new_relation = Relation.new
 
     # check the members are all positive, and correctly type
     typedmembers = []
@@ -421,22 +420,38 @@ class AmfController < ApplicationController
     end
 
     # assign new contents
-    rel.members = typedmembers
+    new_relation.members = typedmembers
-    rel.tags = tags
+    new_relation.tags = tags
-    rel.visible = visible
+    new_relation.visible = visible
-    rel.changeset_id = changeset
+    new_relation.changeset_id = changeset
+    new_relation.version = version
 
-    # check it then save it
-    # BUG: the following is commented out because it always fails on my
-    #  install. I think it's a Rails bug.
 
-    #if !rel.preconditions_ok?
+    if id <= 0
-    #  return -2, "Relation preconditions failed"
+      # We're creating the node
-    #else
+      new_relation.create_with_history(user)
-    rel.save_with_history!
+    elsif visible
-    #end
+      # We're updating the node
+      relation.update_from(new_relation, user)
+    else
+      # We're deleting the node
+      relation.delete_with_history!(new_relation, user)
+    end
       
-    [0, relid, rel.id]
+    if id <= 0
+      return [0, relid, new_relation.id, new_relation.version]
+    else
+      return [0, relid, relation.id, relation.version]
+    end
+  rescue OSM::APIChangesetAlreadyClosedError => ex
+    return [-1, "The changeset #{ex.changeset.id} was closed at #{ex.changeset.closed_at}"]
+  rescue OSM::APIVersionMismatchError => ex
+    return [-1, "You have taken too long to edit, please reload the area"]
+  rescue OSM::APIAlreadyDeletedError => ex
+    return [-1, "The object has already been deleted"]
+  rescue OSM::APIError => ex
+    # Some error that we don't specifically catch
+    return [-2, "Something really bad happened :-()"]
   end
 
   # Save a way to the database, including all nodes. Any nodes in the previous
@@ -453,8 +468,8 @@ class AmfController < ApplicationController
 
     # -- Initialise and carry out checks
 	
-    uid = getuserid(usertoken)
+    user = getuserid(usertoken)
-    if !uid then return -1,"You are not logged in, so the way could not be saved." end
+    if !user then return -1,"You are not logged in, so the way could not be saved." end
 
     originalway = originalway.to_i
 
@@ -467,14 +482,13 @@ class AmfController < ApplicationController
 
     # -- Get unique nodes
 
-    if originalway < 0
+    if originalway <= 0
-      way = Way.new
-      way.version = 0	# otherwise +=1 breaks
       uniques = []
     else
       way = Way.find(originalway)
       uniques = way.unshared_node_ids
     end
+    new_way = Way.new
 
     # -- Compare nodes and save changes to any that have changed
 
@@ -484,16 +498,19 @@ class AmfController < ApplicationController
       lon = n[0].to_f
       lat = n[1].to_f
       id = n[2].to_i
+      version = n[3].to_i # FIXME which index does the version come in on????
       savenode = false
+      # We always need a new node if we are saving it
+      new_node = Node.new
+
 
       if renumberednodes[id]
         id = renumberednodes[id]
-      elsif id < 0
+      elsif id <= 0
         # Create new node
-        node = Node.new
-        node.version = 0	# otherwise +=1 breaks
         savenode = true
       else
+        # Don't modify this node, make any changes you want to the new_node above
         node = Node.find(id)
         nodetags=node.tags
         nodetags.delete('created_by')
@@ -504,12 +521,19 @@ class AmfController < ApplicationController
       end
 
       if savenode
-        node.changeset_id = changeset
+        new_node.changeset_id = changeset
-        node.lat = lat
+        new_node.lat = lat
-        node.lon = lon
+        new_node.lon = lon
-        node.tags = n[4]
+        new_node.tags = n[4]
-        node.visible = true
+        new_node.visible = true
-        node.save_with_history!
+        new_node.version = version
+        if id <= 0
+          # We're creating the node
+          new_node.create_with_history(user)
+        else
+          # We're updating the node (no delete here)
+          node.update_from(new_node, user)
+        end
 
         if id != node.id
           renumberednodes[id] = node.id
@@ -527,19 +551,21 @@ class AmfController < ApplicationController
       deleteitemrelations(n, 'node')
 
       node = Node.find(n)
-      node.changeset_id = changeset
+      new_node = Node.new
-      node.visible = false
+      new_node.changeset_id = changeset
-      node.save_with_history!
+      new_node.version = version
+      node.delete_with_history!(new_node, user)
     end
 
     # -- Save revised way
 
     if way.tags!=attributes or way.nds!=nodes or !way.visible?
-      way.tags = attributes
+      new_way = Way.new
-      way.nds = nodes
+      new_way.tags = attributes
-      way.changeset_id = changeset
+      new_way.nds = nodes
-      way.visible = true
+      new_way.changeset_id = changeset
-      way.save_with_history!
+      new_way.version = version
+      way.update_from(new_way, user)
     end
 
     [0, originalway, way.id, renumberednodes, way.version]
@@ -553,9 +579,9 @@ class AmfController < ApplicationController
   # 2. new node id,
   # 3. version.
 
-  def putpoi(usertoken, changeset, id, lon, lat, tags, visible) #:doc:
+  def putpoi(usertoken, changeset, version, id, lon, lat, tags, visible) #:doc:
-    uid = getuserid(usertoken)
+    user = getuser(usertoken)
-    if !uid then return -1,"You are not logged in, so the point could not be saved." end
+    if !user then return -1,"You are not logged in, so the point could not be saved." end
 
     id = id.to_i
     visible = (visible.to_i == 1)
@@ -567,19 +593,32 @@ class AmfController < ApplicationController
         unless node.ways.empty? then return -1,"The point has since become part of a way, so you cannot save it as a POI." end
         deleteitemrelations(id, 'node')
       end
+    end
+    # We always need a new node, based on the data that has been sent to us
+    new_node = Node.new
+
+    new_node.changeset_id = changeset
+    new_node.version = version
+    new_node.lat = lat
+    new_node.lon = lon
+    new_node.tags = tags
+    new_node.visible = visible
+    if id <= 0 
+      # We're creating the node
+      new_node.create_with_history(user)
+    elsif visible
+      # We're updating the node
+      node.update_from(new_node, user)
     else
-      node = Node.new
+      # We're deleting the node
-      node.version = 0
+      node.delete_with_history!(new_node, user)
     end
 
-    node.changeset_id = changeset
+    if id <= 0
-    node.lat = lat
+      return [0, id, new_node.id, new_node.version]
-    node.lon = lon
+    else
-    node.tags = tags
+      return [0, id, node.id, node.version]
-    node.visible = visible
+    end
-    node.save_with_history!
-
-    [0, id, node.id, node.version]
   end
 
   # Read POI from database
@@ -652,14 +691,20 @@ class AmfController < ApplicationController
 
   # Authenticate token
   # (can also be of form user:pass)
+  # When we are writing to the api, we need the actual user model, 
+  # not just the id, hence this abstraction
 
-  def getuserid(token) #:doc:
+  def getuser(token) #:doc:
     if (token =~ /^(.+)\:(.+)$/) then
       user = User.authenticate(:username => $1, :password => $2)
     else
       user = User.authenticate(:token => token)
     end
+    return user
+  end
   
+  def getuserid(token)
+    user = getuser(token)
     return user ? user.id : nil;
   end
 

app/models/changeset.rb

@@ -50,7 +50,9 @@ class Changeset < ActiveRecord::Base
   end
 
   def set_closed_time_now
-    self.closed_at = DateTime.now
+    unless is_open?
+      self.closed_at = DateTime.now
+    end
   end
   
   def self.from_xml(xml, create=false)

app/models/node.rb

@@ -88,7 +88,7 @@ class Node < ActiveRecord::Base
     raise OSM::APIBadUserInput.new("The node is outside this world") unless node.in_world?
 
     # version must be present unless creating
-    return nil unless create or not pt['version'].nil?
+    raise OSM::APIBadXMLError.new("node", pt, "Version is required when updating") unless create or not pt['version'].nil?
     node.version = create ? 0 : pt['version'].to_i
 
     unless create
@@ -119,42 +119,12 @@ class Node < ActiveRecord::Base
   end
 
   ##
-  # the bounding box around a node
+  # the bounding box around a node, which is used for determining the changeset's
+  # bounding box
   def bbox
     [ longitude, latitude, longitude, latitude ]
   end
 
-  def save_with_history!
-    t = Time.now
-    Node.transaction do
-      self.version += 1
-      self.timestamp = t
-      self.save!
-
-      # Create a NodeTag
-      tags = self.tags
-      NodeTag.delete_all(['id = ?', self.id])
-      tags.each do |k,v|
-        tag = NodeTag.new
-        tag.k = k 
-        tag.v = v 
-        tag.id = self.id
-        tag.save!
-      end 
-
-      # Create an OldNode
-      old_node = OldNode.from_node(self)
-      old_node.timestamp = t
-      old_node.save_with_dependencies!
-
-      # tell the changeset we updated one element only
-      changeset.add_changes! 1
-
-      # save the changeset in case of bounding box updates
-      changeset.save!
-    end
-  end
-
   # Should probably be renamed delete_from to come in line with update
   def delete_with_history!(new_node, user)
     unless self.visible
@@ -295,4 +265,37 @@ class Node < ActiveRecord::Base
     # nodes don't refer to anything, so there is nothing to do here
   end
   
+  private
+
+  def save_with_history!
+    t = Time.now
+    Node.transaction do
+      self.version += 1
+      self.timestamp = t
+      self.save!
+
+      # Create a NodeTag
+      tags = self.tags
+      NodeTag.delete_all(['id = ?', self.id])
+      tags.each do |k,v|
+        tag = NodeTag.new
+        tag.k = k 
+        tag.v = v 
+        tag.id = self.id
+        tag.save!
+      end 
+
+      # Create an OldNode
+      old_node = OldNode.from_node(self)
+      old_node.timestamp = t
+      old_node.save_with_dependencies!
+
+      # tell the changeset we updated one element only
+      changeset.add_changes! 1
+
+      # save the changeset in case of bounding box updates
+      changeset.save!
+    end
+  end
+  
 end

app/models/relation.rb

@@ -214,6 +214,118 @@ class Relation < ActiveRecord::Base
     @tags[k] = v
   end
 
+  ##
+  # updates the changeset bounding box to contain the bounding box of 
+  # the element with given +type+ and +id+. this only works with nodes
+  # and ways at the moment, as they're the only elements to respond to
+  # the :bbox call.
+  def update_changeset_element(type, id)
+    element = Kernel.const_get(type.capitalize).find(id)
+    changeset.update_bbox! element.bbox
+  end    
+
+  def delete_with_history!(new_relation, user)
+    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 ])
+        raise OSM::APIPreconditionFailedError.new("The relation #{new_relation.id} is a used in another relation")
+      end
+      self.changeset_id = new_relation.changeset_id
+      self.tags = {}
+      self.members = []
+      self.visible = false
+      save_with_history!
+    end
+  end
+
+  def update_from(new_relation, user)
+    check_consistency(self, new_relation, user)
+    if !new_relation.preconditions_ok?
+      raise OSM::APIPreconditionFailedError.new
+    end
+    self.changeset_id = new_relation.changeset_id
+    self.tags = new_relation.tags
+    self.members = new_relation.members
+    self.visible = true
+    save_with_history!
+  end
+  
+  def create_with_history(user)
+    check_create_consistency(self, user)
+    if !self.preconditions_ok?
+      raise OSM::APIPreconditionFailedError.new
+    end
+    self.version = 0
+    self.visible = true
+    save_with_history!
+  end
+
+  def preconditions_ok?
+    # These are hastables that store an id in the index of all 
+    # the nodes/way/relations that have already been added.
+    # If the member is valid and visible then we add it to the 
+    # relevant hash table, with the value true as a cache.
+    # Thus if you have nodes with the ids of 50 and 1 already in the
+    # relation, then the hash table nodes would contain:
+    # => {50=>true, 1=>true}
+    elements = { :node => Hash.new, :way => Hash.new, :relation => Hash.new }
+    self.members.each do |m|
+      # find the hash for the element type or die
+      hash = elements[m[0].to_sym] or return false
+
+      # unless its in the cache already
+      unless hash.key? m[1]
+        # use reflection to look up the appropriate class
+        model = Kernel.const_get(m[0].capitalize)
+
+        # get the element with that ID
+        element = model.find(m[1])
+
+        # and check that it is OK to use.
+        unless element and element.visible? and element.preconditions_ok?
+          return false
+        end
+        hash[m[1]] = true
+      end
+    end
+
+    return true
+  rescue
+    return false
+  end
+
+  # Temporary method to match interface to nodes
+  def tags_as_hash
+    return self.tags
+  end
+
+  ##
+  # if any members are referenced by placeholder IDs (i.e: negative) then
+  # this calling this method will fix them using the map from placeholders 
+  # to IDs +id_map+. 
+  def fix_placeholders!(id_map)
+    self.members.map! do |type, id, role|
+      old_id = id.to_i
+      if old_id < 0
+        new_id = id_map[type.to_sym][old_id]
+        raise "invalid placeholder" if new_id.nil?
+        [type, new_id, role]
+      else
+        [type, id, role]
+      end
+    end
+  end
+
+  private
+  
   def save_with_history!
     Relation.transaction do
       # have to be a little bit clever here - to detect if any tags
@@ -336,114 +448,4 @@ class Relation < ActiveRecord::Base
     end
   end
 
-  ##
-  # updates the changeset bounding box to contain the bounding box of 
-  # the element with given +type+ and +id+. this only works with nodes
-  # and ways at the moment, as they're the only elements to respond to
-  # the :bbox call.
-  def update_changeset_element(type, id)
-    element = Kernel.const_get(type.capitalize).find(id)
-    changeset.update_bbox! element.bbox
-  end    
-
-  def delete_with_history!(new_relation, user)
-    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 ])
-        raise OSM::APIPreconditionFailedError.new("The relation #{new_relation.id} is a used in another relation")
-      end
-      self.changeset_id = new_relation.changeset_id
-      self.tags = {}
-      self.members = []
-      self.visible = false
-      save_with_history!
-    end
-  end
-
-  def update_from(new_relation, user)
-    check_consistency(self, new_relation, user)
-    if !new_relation.preconditions_ok?
-      raise OSM::APIPreconditionFailedError.new
-    end
-    self.changeset_id = new_relation.changeset_id
-    self.tags = new_relation.tags
-    self.members = new_relation.members
-    self.visible = true
-    save_with_history!
-  end
-  
-  def create_with_history(user)
-    check_create_consistency(self, user)
-    if !self.preconditions_ok?
-      raise OSM::APIPreconditionFailedError.new
-    end
-    self.version = 0
-    self.visible = true
-    save_with_history!
-  end
-
-  def preconditions_ok?
-    # These are hastables that store an id in the index of all 
-    # the nodes/way/relations that have already been added.
-    # If the member is valid and visible then we add it to the 
-    # relevant hash table, with the value true as a cache.
-    # Thus if you have nodes with the ids of 50 and 1 already in the
-    # relation, then the hash table nodes would contain:
-    # => {50=>true, 1=>true}
-    elements = { :node => Hash.new, :way => Hash.new, :relation => Hash.new }
-    self.members.each do |m|
-      # find the hash for the element type or die
-      hash = elements[m[0].to_sym] or return false
-
-      # unless its in the cache already
-      unless hash.key? m[1]
-        # use reflection to look up the appropriate class
-        model = Kernel.const_get(m[0].capitalize)
-
-        # get the element with that ID
-        element = model.find(m[1])
-
-        # and check that it is OK to use.
-        unless element and element.visible? and element.preconditions_ok?
-          return false
-        end
-        hash[m[1]] = true
-      end
-    end
-
-    return true
-  rescue
-    return false
-  end
-
-  # Temporary method to match interface to nodes
-  def tags_as_hash
-    return self.tags
-  end
-
-  ##
-  # if any members are referenced by placeholder IDs (i.e: negative) then
-  # this calling this method will fix them using the map from placeholders 
-  # to IDs +id_map+. 
-  def fix_placeholders!(id_map)
-    self.members.map! do |type, id, role|
-      old_id = id.to_i
-      if old_id < 0
-        new_id = id_map[type.to_sym][old_id]
-        raise "invalid placeholder" if new_id.nil?
-        [type, new_id, role]
-      else
-        [type, id, role]
-      end
-    end
-  end
-
 end

app/models/way.rb

@@ -196,56 +196,6 @@ class Way < ActiveRecord::Base
     [ lons.min, lats.min, lons.max, lats.max ]
   end
 
-  def save_with_history!
-    t = Time.now
-
-    # update the bounding box, but don't save it as the controller knows the 
-    # lifetime of the change better. note that this has to be done both before 
-    # and after the save, so that nodes from both versions are included in the 
-    # bbox.
-    changeset.update_bbox!(bbox) unless nodes.empty?
-
-    Way.transaction do
-      self.version += 1
-      self.timestamp = t
-      self.save!
-
-      tags = self.tags
-      WayTag.delete_all(['id = ?', self.id])
-      tags.each do |k,v|
-        tag = WayTag.new
-        tag.k = k
-        tag.v = v
-        tag.id = self.id
-        tag.save!
-      end
-
-      nds = self.nds
-      WayNode.delete_all(['id = ?', self.id])
-      sequence = 1
-      nds.each do |n|
-        nd = WayNode.new
-        nd.id = [self.id, sequence]
-        nd.node_id = n
-        nd.save!
-        sequence += 1
-      end
-
-      old_way = OldWay.from_way(self)
-      old_way.timestamp = t
-      old_way.save_with_dependencies!
-
-      # update and commit the bounding box, now that way nodes 
-      # have been updated and we're in a transaction.
-      changeset.update_bbox!(bbox) unless nodes.empty?
-
-      # tell the changeset we updated one element only
-      changeset.add_changes! 1
-
-      changeset.save!
-    end
-  end
-
   def update_from(new_way, user)
     check_consistency(self, new_way, user)
     if !new_way.preconditions_ok?
@@ -300,7 +250,7 @@ class Way < ActiveRecord::Base
         self.tags = []
         self.nds = []
         self.visible = false
-        self.save_with_history!
+        save_with_history!
       end
     end
   end
@@ -358,4 +308,56 @@ class Way < ActiveRecord::Base
     end
   end
 
+  private
+  
+  def save_with_history!
+    t = Time.now
+
+    # update the bounding box, but don't save it as the controller knows the 
+    # lifetime of the change better. note that this has to be done both before 
+    # and after the save, so that nodes from both versions are included in the 
+    # bbox.
+    changeset.update_bbox!(bbox) unless nodes.empty?
+
+    Way.transaction do
+      self.version += 1
+      self.timestamp = t
+      self.save!
+
+      tags = self.tags
+      WayTag.delete_all(['id = ?', self.id])
+      tags.each do |k,v|
+        tag = WayTag.new
+        tag.k = k
+        tag.v = v
+        tag.id = self.id
+        tag.save!
+      end
+
+      nds = self.nds
+      WayNode.delete_all(['id = ?', self.id])
+      sequence = 1
+      nds.each do |n|
+        nd = WayNode.new
+        nd.id = [self.id, sequence]
+        nd.node_id = n
+        nd.save!
+        sequence += 1
+      end
+
+      old_way = OldWay.from_way(self)
+      old_way.timestamp = t
+      old_way.save_with_dependencies!
+
+      # update and commit the bounding box, now that way nodes 
+      # have been updated and we're in a transaction.
+      changeset.update_bbox!(bbox) unless nodes.empty?
+
+      # tell the changeset we updated one element only
+      changeset.add_changes! 1
+
+      changeset.save!
+    end
+  end
+
 end

test/unit/node_test.rb

@@ -82,7 +82,7 @@ class NodeTest < Test::Unit::TestCase
                              :changeset_id => changesets(:normal_user_first_change).id,
                              :visible => 1, 
                              :version => 1)
-    assert node_template.save_with_history!
+    assert node_template.create_with_history(users(:normal_user))
 
     node = Node.find(node_template.id)
     assert_not_nil node
@@ -114,7 +114,7 @@ class NodeTest < Test::Unit::TestCase
     node_template.latitude = 12.3456
     node_template.longitude = 65.4321
     #node_template.tags = "updated=yes"
-    assert node_template.save_with_history!
+    assert node_template.update_from(old_node_template, users(:normal_user))
 
     node = Node.find(node_template.id)
     assert_not_nil node
@@ -145,8 +145,7 @@ class NodeTest < Test::Unit::TestCase
     old_node_template = OldNode.find(:first, :conditions => [ "id = ?", node_template.id ])
     assert_not_nil old_node_template
 
-    node_template.visible = 0
+    assert node_template.delete_with_history!(old_node_template, users(:normal_user))
-    assert node_template.save_with_history!
 
     node = Node.find(node_template.id)
     assert_not_nil node