From: Shaun McDonald Date: Thu, 4 Dec 2008 17:30:17 +0000 (+0000) Subject: Refactoring the amf controller, and all models so that save_with_history! is now... X-Git-Tag: live~8182^2~114 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/e9527b363da441337ac8c44c9d3d3feada3eaa9c 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. --- diff --git a/app/controllers/amf_controller.rb b/app/controllers/amf_controller.rb index 43bec41a1..e9bcb281b 100644 --- a/app/controllers/amf_controller.rb +++ b/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) - if !uid then return -1,"You are not logged in, so Potlatch can't write any changes to the database." end + user = getuserid(usertoken) + 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: - uid = getuserid(usertoken) - if !uid then return -1,"You are not logged in, so the relation could not be saved." end + def putrelation(renumberednodes, renumberedways, usertoken, changeset, version, relid, tags, members, visible) #:doc: + user = getuserid(usertoken) + 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 - rel = Relation.new - rel.version = 0 - else - rel = Relation.find(relid) + if relid > 0 + relation = 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 - rel.tags = tags - rel.visible = visible - rel.changeset_id = changeset - - # 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? - # return -2, "Relation preconditions failed" - #else - rel.save_with_history! - #end - - [0, relid, rel.id] + new_relation.members = typedmembers + new_relation.tags = tags + new_relation.visible = visible + new_relation.changeset_id = changeset + new_relation.version = version + + + if id <= 0 + # We're creating the node + new_relation.create_with_history(user) + elsif visible + # 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 + + 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) - if !uid then return -1,"You are not logged in, so the way could not be saved." end + user = getuserid(usertoken) + 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 - way = Way.new - way.version = 0 # otherwise +=1 breaks + if originalway <= 0 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 - node.lat = lat - node.lon = lon - node.tags = n[4] - node.visible = true - node.save_with_history! + new_node.changeset_id = changeset + new_node.lat = lat + new_node.lon = lon + new_node.tags = n[4] + new_node.visible = true + 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 - node.visible = false - node.save_with_history! + new_node = Node.new + new_node.changeset_id = changeset + 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 - way.nds = nodes - way.changeset_id = changeset - way.visible = true - way.save_with_history! + new_way = Way.new + new_way.tags = attributes + new_way.nds = nodes + new_way.changeset_id = changeset + 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: - uid = getuserid(usertoken) - if !uid then return -1,"You are not logged in, so the point could not be saved." end + def putpoi(usertoken, changeset, version, id, lon, lat, tags, visible) #:doc: + user = getuser(usertoken) + 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 - node.version = 0 + # We're deleting the node + node.delete_with_history!(new_node, user) end - node.changeset_id = changeset - node.lat = lat - node.lon = lon - node.tags = tags - node.visible = visible - node.save_with_history! - - [0, id, node.id, node.version] + if id <= 0 + return [0, id, new_node.id, new_node.version] + else + return [0, id, node.id, node.version] + end 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 diff --git a/app/models/changeset.rb b/app/models/changeset.rb index 9dc60de48..6bc553a78 100644 --- a/app/models/changeset.rb +++ b/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) diff --git a/app/models/node.rb b/app/models/node.rb index e90c32950..5f4f7af6a 100644 --- a/app/models/node.rb +++ b/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 @@ -294,5 +264,38 @@ class Node < ActiveRecord::Base def fix_placeholders!(id_map) # 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 diff --git a/app/models/relation.rb b/app/models/relation.rb index ba27e9d7d..94ef9d42a 100644 --- a/app/models/relation.rb +++ b/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 diff --git a/app/models/way.rb b/app/models/way.rb index d6aa12af1..ac4461f8e 100644 --- a/app/models/way.rb +++ b/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 diff --git a/test/unit/node_test.rb b/test/unit/node_test.rb index a2c8d7fb6..13dea88da 100644 --- a/test/unit/node_test.rb +++ b/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.save_with_history! + assert node_template.delete_with_history!(old_node_template, users(:normal_user)) node = Node.find(node_template.id) assert_not_nil node