From: Shaun McDonald Date: Thu, 9 Oct 2008 16:22:05 +0000 (+0000) Subject: moving the conistency checks for updates and deletes to library, hopefully got the... X-Git-Tag: live~7601^2~286 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/3d24694addd628cc55a3d2a24d736e61cbd55273 moving the conistency checks for updates and deletes to library, hopefully got the updates and deletes working now. --- diff --git a/app/controllers/node_controller.rb b/app/controllers/node_controller.rb index 8e8c8446d..29bf672e2 100644 --- a/app/controllers/node_controller.rb +++ b/app/controllers/node_controller.rb @@ -76,7 +76,7 @@ class NodeController < ApplicationController end rescue OSM::APIVersionMismatchError => ex render :text => "Version mismatch: Provided " + ex.provided.to_s + - ", server had: " + ex.latest.to_s, :status => :bad_request + ", server had: " + ex.latest.to_s, :status => :bad_request rescue ActiveRecord::RecordNotFound render :nothing => true, :status => :not_found end @@ -87,14 +87,19 @@ class NodeController < ApplicationController def delete begin node = Node.find(params[:id]) + new_node = Node.from_xml(request.raw_post) # FIXME we no longer care about the user, (or maybe we want to check # that the user of the changeset is the same user as is making this # little change?) we really care about the # changeset which must be open, and that the version that we have been # given is the one that is currently stored in the database - node.delete_with_history(@user) - - render :nothing => true + + if new_node and new_node.id == node.id + node.delete_with_history(new_node, @user) + render :nothing => true, :status => :success + else + render :nothing => true, :status => :bad_request + end rescue ActiveRecord::RecordNotFound render :nothing => true, :status => :not_found rescue OSM::APIError => ex diff --git a/app/controllers/relation_controller.rb b/app/controllers/relation_controller.rb index d87905059..b77d41ead 100644 --- a/app/controllers/relation_controller.rb +++ b/app/controllers/relation_controller.rb @@ -68,7 +68,13 @@ class RelationController < ApplicationController #XXX check if member somewhere! begin relation = Relation.find(params[:id]) - relation.delete_with_history(@user) + new_relation = Relation.from_xml(request.raw_post) + if new_relation and new_relation.id == relation.id + relation.delete_with_history(new_relation, @user) + render :nothing => true, :status => :success + else + render :nothing => true, :status => :bad_request + end rescue OSM::APIError => ex render ex.render_opts rescue ActiveRecord::RecordNotFound diff --git a/app/controllers/way_controller.rb b/app/controllers/way_controller.rb index 17f166ddc..6f4704c77 100644 --- a/app/controllers/way_controller.rb +++ b/app/controllers/way_controller.rb @@ -68,10 +68,15 @@ class WayController < ApplicationController def delete begin way = Way.find(params[:id]) - way.delete_with_history(@user) + new_way = Way.from_xml(request.raw_post) + if new_way and new_way.id == way.id + way.delete_with_history(@user) - # if we get here, all is fine, otherwise something will catch below. - render :nothing => true + # if we get here, all is fine, otherwise something will catch below. + render :nothing => true + else + render :nothing => true, :status => :bad_request + end rescue OSM::APIError => ex render ex.render_opts rescue ActiveRecord::RecordNotFound diff --git a/app/models/node.rb b/app/models/node.rb index a25a19f70..39e1228ac 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -131,14 +131,16 @@ class Node < ActiveRecord::Base end end - def delete_with_history(user) + # Should probably be renamed delete_from to come in line with update + def delete_with_history(new_node, user) if self.visible + 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 = 1 AND current_way_nodes.node_id = ?", self.id ]) raise OSM::APIPreconditionFailedError.new elsif RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='node' and member_id=?", self.id]) raise OSM::APIPreconditionFailedError.new else - self.user_id = user.id + self.changeset_id = new_node.changeset_id self.visible = 0 save_with_history! end @@ -148,15 +150,9 @@ class Node < ActiveRecord::Base end def update_from(new_node, user) - if new_node.version != version - raise OSM::APIVersionMismatchError.new(new_node.version, version) - elsif new_node.changeset.user_id != user.id - raise OSM::APIUserChangesetMismatchError.new - elsif not new_node.changeset.open? - raise OSM::APIChangesetAlreadyClosedError.new - end + check_consistency(self, new_node, user) - # FIXME logic need looked at + # FIXME logic needs to be double checked self.changeset_id = new_node.changeset_id self.latitude = new_node.latitude self.longitude = new_node.longitude diff --git a/app/models/relation.rb b/app/models/relation.rb index 93f0001da..c8ee89d37 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -217,13 +217,15 @@ class Relation < ActiveRecord::Base end end - def delete_with_history(user) + def delete_with_history(new_relation, user) if self.visible + check_consistency(self, new_relation, user) if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='relation' and member_id=?", self.id ]) raise OSM::APIPreconditionFailedError.new else #self.user_id = user.id # FIXME we need to deal with changeset here, which is probably already dealt with + self.changeset_id = new_relation.changeset_id self.tags = [] self.members = [] self.visible = false @@ -235,23 +237,17 @@ class Relation < ActiveRecord::Base end def update_from(new_relation, user) + check_consistency(self, new_relation, user) if !new_relation.preconditions_ok? raise OSM::APIPreconditionFailedError.new - elsif new_relation.version != version - raise OSM::APIVersionMismatchError.new(new_relation.version, version) - elsif new_relation.changeset.user_id != user.id - raise OSM::APIUserChangesetMismatchError.new - elsif not new_relation.changeset.open? - raise OSM::APIChangesetAlreadyClosedError.new - else - # FIXME need to deal with changeset etc - #self.user_id = user.id - self.changeset_id = new_relation.changeset_id - self.tags = new_relation.tags - self.members = new_relation.members - self.visible = true - save_with_history! end + # FIXME need to deal with changeset etc + #self.user_id = user.id + 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 preconditions_ok? diff --git a/app/models/way.rb b/app/models/way.rb index 9ddb603a8..05b412b29 100644 --- a/app/models/way.rb +++ b/app/models/way.rb @@ -206,17 +206,15 @@ class Way < ActiveRecord::Base end def update_from(new_way, user) + check_consistency(self, new_way, user) if !new_way.preconditions_ok? raise OSM::APIPreconditionFailedError.new - elsif new_way.version != version - raise OSM::APIVersionMismatchError.new(new_way.version, version) - else - self.user_id = user.id - self.tags = new_way.tags - self.nds = new_way.nds - self.visible = true - save_with_history! end + self.changeset_id = changeset_id + self.tags = new_way.tags + self.nds = new_way.nds + self.visible = true + save_with_history! end def preconditions_ok? @@ -230,11 +228,13 @@ class Way < ActiveRecord::Base return true end - def delete_with_history(user) + def delete_with_history(new_way, user) + check_consistency(self, new_way, user) if self.visible - # FIXME - # this should actually delete the relations, - # not just throw a PreconditionFailed if it's a member of a relation!! + # FIXME + # this should actually delete the relations, + # not just throw a PreconditionFailed if it's a member of a relation!! + # WHY?? The editor should decide whether the node is in the relation or not! # FIXME: this should probably renamed to delete_with_history if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", @@ -242,7 +242,7 @@ class Way < ActiveRecord::Base raise OSM::APIPreconditionFailedError # end FIXME else - self.user_id = user.id + self.changeset_id = new_way.changeset_id self.tags = [] self.nds = [] self.visible = false @@ -265,6 +265,7 @@ class Way < ActiveRecord::Base n.save_with_history! end + # FIXME needs more information passed in so that the changeset can be updated self.user_id = user.id self.delete_with_history(user) diff --git a/lib/geo_record.rb b/lib/geo_record.rb index 2740eab0c..3eab72b2d 100644 --- a/lib/geo_record.rb +++ b/lib/geo_record.rb @@ -42,6 +42,19 @@ module GeoRecord return self.longitude.to_f / SCALE end + # Generic checks that are run for the updates and deletes of + # node, ways and relations. This code is here to avoid duplication, + # and allow the extention of the checks without having to modify the + # code in 6 places. This will throw an exception if there is an inconsistency + def check_consistency(old, new, user) + if new.version != old.version + raise OSM::APIVersionMismatchError.new(new.version, old.version) + elsif new.changeset.user_id != user.id + raise OSM::APIUserChangesetMismatchError.new + elsif not new.changeset.is_open? + raise OSM::APIChangesetAlreadyClosedError.new + end + end private def lat2y(a)