From: Matt Amos Date: Fri, 27 Mar 2009 12:22:23 +0000 (+0000) Subject: Fixed a couple of bugs with changeset bounding box calculation when modifying or... X-Git-Tag: live~7573^2~45 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/be75a1504f3c9a49b6cbf4e83c35339f45390628?hp=663416ae0bdca040f23cf867c43b94d0c1e2b45b Fixed a couple of bugs with changeset bounding box calculation when modifying or deleting. --- diff --git a/app/models/node.rb b/app/models/node.rb index d3e0a7e8d..05aae0896 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -150,17 +150,20 @@ class Node < ActiveRecord::Base def update_from(new_node, user) check_consistency(self, new_node, user) - # update changeset with *old* position first + # update changeset first + self.changeset_id = new_node.changeset_id + self.changeset = new_node.changeset + + # update changeset bbox with *old* position first changeset.update_bbox!(bbox); # FIXME logic needs to be double checked - self.changeset_id = new_node.changeset_id self.latitude = new_node.latitude self.longitude = new_node.longitude self.tags = new_node.tags self.visible = true - # update changeset with *new* position + # update changeset bbox with *new* position changeset.update_bbox!(bbox); save_with_history! diff --git a/app/models/relation.rb b/app/models/relation.rb index c3769b14d..4b5d9e32b 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -253,6 +253,7 @@ class Relation < ActiveRecord::Base raise OSM::APIPreconditionFailedError.new end self.changeset_id = new_relation.changeset_id + self.changeset = new_relation.changeset self.tags = new_relation.tags self.members = new_relation.members self.visible = true @@ -372,6 +373,10 @@ class Relation < ActiveRecord::Base tag.id = self.id tag.save! end + + # reload, so that all of the members are accessible in their + # new state. + self.reload # same pattern as before, but this time we're collecting the # changed members in an array, as the bounding box updates for diff --git a/app/models/way.rb b/app/models/way.rb index 88bbaf6c5..94a6fa754 100644 --- a/app/models/way.rb +++ b/app/models/way.rb @@ -202,7 +202,9 @@ class Way < ActiveRecord::Base if !new_way.preconditions_ok? raise OSM::APIPreconditionFailedError.new end + self.changeset_id = new_way.changeset_id + self.changeset = new_way.changeset self.tags = new_way.tags self.nds = new_way.nds self.visible = true @@ -248,6 +250,8 @@ class Way < ActiveRecord::Base 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.changeset = new_way.changeset + self.tags = [] self.nds = [] self.visible = false @@ -294,11 +298,12 @@ class Way < ActiveRecord::Base 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 + # update the bounding box, 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? + # bbox. we use a copy of the changeset so that it isn't reloaded + # later in the save. + cs = self.changeset + cs.update_bbox!(bbox) unless nodes.empty? Way.transaction do self.version += 1 @@ -330,14 +335,18 @@ class Way < ActiveRecord::Base old_way.timestamp = t old_way.save_with_dependencies! + # reload the way so that the nodes array points to the correct + # new set of nodes. + self.reload + # 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? + cs.update_bbox!(bbox) unless nodes.empty? # tell the changeset we updated one element only - changeset.add_changes! 1 + cs.add_changes! 1 - changeset.save! + cs.save! end end diff --git a/test/functional/changeset_controller_test.rb b/test/functional/changeset_controller_test.rb index 591241aa0..4c98fb36d 100644 --- a/test/functional/changeset_controller_test.rb +++ b/test/functional/changeset_controller_test.rb @@ -553,6 +553,84 @@ EOF "shouldn't be able to re-use placeholder IDs" end + ## + # test what happens if a diff is uploaded containing only a node + # move. + def test_upload_node_move + basic_authorization "test@openstreetmap.org", "test" + + content "" + + "" + + "" + put :create + assert_response :success + changeset_id = @response.body.to_i + + old_node = current_nodes(:visible_node) + + diff = XML::Document.new + diff.root = XML::Node.new "osmChange" + modify = XML::Node.new "modify" + xml_old_node = old_node.to_xml_node + xml_old_node["lat"] = (2.0).to_s + xml_old_node["lon"] = (2.0).to_s + xml_old_node["changeset"] = changeset_id.to_s + modify << xml_old_node + diff.root << modify + + # upload it + content diff + post :upload, :id => changeset_id + assert_response :success, + "diff should have uploaded OK" + + # check the bbox + changeset = Changeset.find(changeset_id) + assert_equal 1*SCALE, changeset.min_lon, "min_lon should be 1 degree" + assert_equal 2*SCALE, changeset.max_lon, "max_lon should be 2 degrees" + assert_equal 1*SCALE, changeset.min_lat, "min_lat should be 1 degree" + assert_equal 2*SCALE, changeset.max_lat, "max_lat should be 2 degrees" + end + + ## + # test what happens if a diff is uploaded adding a node to a way. + def test_upload_way_extend + basic_authorization "test@openstreetmap.org", "test" + + content "" + + "" + + "" + put :create + assert_response :success + changeset_id = @response.body.to_i + + old_way = current_ways(:visible_way) + + diff = XML::Document.new + diff.root = XML::Node.new "osmChange" + modify = XML::Node.new "modify" + xml_old_way = old_way.to_xml_node + nd_ref = XML::Node.new "nd" + nd_ref["ref"] = current_nodes(:visible_node).id.to_s + xml_old_way << nd_ref + xml_old_way["changeset"] = changeset_id.to_s + modify << xml_old_way + diff.root << modify + + # upload it + content diff + post :upload, :id => changeset_id + assert_response :success, + "diff should have uploaded OK" + + # check the bbox + changeset = Changeset.find(changeset_id) + assert_equal 1*SCALE, changeset.min_lon, "min_lon should be 1 degree" + assert_equal 3*SCALE, changeset.max_lon, "max_lon should be 3 degrees" + assert_equal 1*SCALE, changeset.min_lat, "min_lat should be 1 degree" + assert_equal 3*SCALE, changeset.max_lat, "max_lat should be 3 degrees" + end + ## # test for more issues in #1568 def test_upload_empty_invalid @@ -773,7 +851,7 @@ EOF assert_select "osm>changeset[min_lat=1.0]", 1 assert_select "osm>changeset[max_lat=2.0]", 1 - # add (delete) a way to it + # add (delete) a way to it, which contains a point at (3,3) with_controller(WayController.new) do content update_changeset(current_ways(:visible_way).to_xml, changeset_id) @@ -784,6 +862,7 @@ EOF # get the bounding box back from the changeset get :read, :id => changeset_id assert_response :success, "Couldn't read back changeset for the third time." + # note that the 3.1 here is because of the bbox overexpansion assert_select "osm>changeset[min_lon=1.0]", 1 assert_select "osm>changeset[max_lon=3.1]", 1 assert_select "osm>changeset[min_lat=1.0]", 1