From: Shaun McDonald Date: Tue, 9 Dec 2008 18:44:11 +0000 (+0000) Subject: Some improvements for the XML parsing, as there are somethings that we don't need... X-Git-Tag: live~7573^2~108 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/b7335940e21bfe4fc9ac688130687398e677cfae?ds=sidebyside Some improvements for the XML parsing, as there are somethings that we don't need, like timestamp and visibility, thus hopefully bringing a speedup in the xml parsing. Some more AMF controller fixes, plenty more to go though. --- diff --git a/app/controllers/amf_controller.rb b/app/controllers/amf_controller.rb index 9c9f85e1d..39de7e671 100644 --- a/app/controllers/amf_controller.rb +++ b/app/controllers/amf_controller.rb @@ -446,7 +446,10 @@ class AmfController < ApplicationController 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"] + # Really need to check to see whether this is a server load issue, and the + # last version was in the same changeset, or belongs to the same user, then + # we can return something different + return [-3, "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 @@ -570,6 +573,18 @@ class AmfController < ApplicationController end [0, originalway, way.id, renumberednodes, way.version] + rescue OSM::APIChangesetAlreadyClosedError => ex + return [-1, "The changeset #{ex.changeset.id} was closed at #{ex.changeset.closed_at}"] + rescue OSM::APIVersionMismatchError => ex + # Really need to check to see whether this is a server load issue, and the + # last version was in the same changeset, or belongs to the same user, then + # we can return something different + return [-3, "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 POI to the database. @@ -621,6 +636,18 @@ class AmfController < ApplicationController else return [0, id, node.id, node.version] end + rescue OSM::APIChangesetAlreadyClosedError => ex + return [-1, "The changeset #{ex.changeset.id} was closed at #{ex.changeset.closed_at}"] + rescue OSM::APIVersionMismatchError => ex + # Really need to check to see whether this is a server load issue, and the + # last version was in the same changeset, or belongs to the same user, then + # we can return something different + return [-3, "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 # Read POI from database @@ -645,21 +672,23 @@ class AmfController < ApplicationController # Delete way and all constituent nodes. Also removes from any relations. # Returns 0 (success), unchanged way id. - def deleteway(usertoken, changeset_id, way_id) #:doc: - if !getuserid(usertoken) then return -1,"You are not logged in, so the way could not be deleted." end - - way_id = way_id.to_i + def deleteway(usertoken, changeset_id, way_id, version_id) #:doc: + user = getuser(usertoken) + if user then return -1,"You are not logged in, so the way could not be deleted." end + # Need a transaction so that if one item fails to delete, the whole delete fails. + Way.transaction do + way_id = way_id.to_i + + # FIXME: would be good not to make two history entries when removing + # two nodes from the same relation + old_way = Way.find(way_id) + old_way.unshared_node_ids.each do |n| + deleteitemrelations(n, 'node') + end + deleteitemrelations(way_id, 'way') - # FIXME: would be good not to make two history entries when removing - # two nodes from the same relation - way = Way.find(way_id) - way.unshared_node_ids.each do |n| - deleteitemrelations(n, 'node') + way.delete_with_relations_and_nodes_and_history(changeset_id.to_i) end - deleteitemrelations(way_id, 'way') - - way.delete_with_relations_and_nodes_and_history(changeset_id.to_i) - [0, way_id] end @@ -668,7 +697,7 @@ class AmfController < ApplicationController # Support functions # Remove a node or way from all relations - + # FIXME needs version, changeset, and user def deleteitemrelations(objid, type) #:doc: relations = RelationMember.find(:all, :conditions => ['member_type = ? and member_id = ?', type, objid], @@ -676,7 +705,12 @@ class AmfController < ApplicationController relations.each do |rel| rel.members.delete_if { |x| x[0] == type and x[1] == objid } - rel.save_with_history! + # FIXME need to create the new node/way based on the type. + new_rel = Relation.new + new_rel.version = version + new_rel.members = members + new_rel.changeset = changeset + rel.delete_with_history(new_rel, user) end end diff --git a/app/models/node.rb b/app/models/node.rb index ce2bd5c74..f2ad3a78a 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -83,6 +83,7 @@ class Node < ActiveRecord::Base raise OSM::APIBadXMLError.new("node", pt, "lon missing") if pt['lon'].nil? node.lat = pt['lat'].to_f node.lon = pt['lon'].to_f + raise OSM::APIBadXMLError.new("node", pt, "changeset id missing") if pt['changeset'].nil? node.changeset_id = pt['changeset'].to_i raise OSM::APIBadUserInput.new("The node is outside this world") unless node.in_world? @@ -99,15 +100,10 @@ class Node < ActiveRecord::Base # visible if it says it is, or as the default if the attribute # is missing. - node.visible = pt['visible'].nil? or pt['visible'] == 'true' + # Don't need to set the visibility, when it is set explicitly in the create/update/delete + #node.visible = pt['visible'].nil? or pt['visible'] == 'true' - if create - node.timestamp = Time.now - else - if pt['timestamp'] - node.timestamp = Time.parse(pt['timestamp']) - end - end + # We don't care about the time, as it is explicitly set on create/update/delete tags = [] diff --git a/app/models/relation.rb b/app/models/relation.rb index 94ef9d42a..6be106159 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -49,6 +49,8 @@ class Relation < ActiveRecord::Base raise OSM::APIBadXMLError.new("relation", pt, "You are missing the required changeset in the relation") if pt['changeset'].nil? relation.changeset_id = pt['changeset'] + # The follow block does not need to be executed because they are dealt with + # in create_with_history, update_from and delete_with_history if create relation.timestamp = Time.now relation.visible = true diff --git a/app/models/way.rb b/app/models/way.rb index ac4461f8e..92b69ed7d 100644 --- a/app/models/way.rb +++ b/app/models/way.rb @@ -47,8 +47,10 @@ class Way < ActiveRecord::Base end way.version = pt['version'] + raise OSM::APIBadXMLError.new("node", pt, "Changeset is required") if pt['changeset'].nil? way.changeset_id = pt['changeset'] + # This next section isn't required for the create, update, or delete of ways if create way.timestamp = Time.now way.visible = true @@ -244,7 +246,7 @@ class Way < ActiveRecord::Base 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 + raise OSM::APIPreconditionFailedError.new("You need to make sure that this way is not a member of a relation.") else self.changeset_id = new_way.changeset_id self.tags = [] @@ -259,12 +261,15 @@ class Way < ActiveRecord::Base # FIXME: merge the potlatch code to delete the relations # and refactor to use delete_with_history! + # This really needs the ids and versions of the nodes/relations to be passed in too + # so that we can do the version checking before the delete def delete_with_relations_and_nodes_and_history(changeset_id) # delete the nodes not used by other ways self.unshared_node_ids.each do |node_id| n = Node.find(node_id) n.changeset_id = changeset_id n.visible = false + # FIXME next line is bad n.save_with_history! end @@ -272,6 +277,7 @@ class Way < ActiveRecord::Base self.tags = [] self.nds = [] self.visible = false + # FIXME next line is bad self.save_with_history! end