From: Matt Amos Date: Thu, 6 Nov 2008 17:56:24 +0000 (+0000) Subject: Implemented changeset bounding box handling for relations, based on the conversation... X-Git-Tag: live~8182^2~220 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/d43a327a950591278f56041ced57986582dbcfb2 Implemented changeset bounding box handling for relations, based on the conversation on the mailing list. Its a bit complex... --- diff --git a/app/models/relation.rb b/app/models/relation.rb index 9836ef4f1..cc7977833 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -192,13 +192,42 @@ class Relation < ActiveRecord::Base def save_with_history! Relation.transaction do + # have to be a little bit clever here - to detect if any tags + # changed then we have to monitor their before and after state. + tags_changed = false + t = Time.now self.version += 1 self.timestamp = t self.save! tags = self.tags - RelationTag.delete_all(['id = ?', self.id]) + self.relation_tags.each do |old_tag| + key = old_tag.k + # if we can match the tags we currently have to the list + # of old tags, then we never set the tags_changed flag. but + # if any are different then set the flag and do the DB + # update. + if tags.has_key? key + # rails 2.1 dirty handling should take care of making this + # somewhat efficient... hopefully... + old_tag.v = tags[key] + tags_changed |= old_tag.changed? + old_tag.save! + + # remove from the map, so that we can expect an empty map + # at the end if there are no new tags + tags.delete key + + else + # this means a tag was deleted + tags_changed = true + RelationTag.delete_all ['id = ? and k = ?', self.id, old_tag.k] + end + end + # if there are left-over tags then they are new and will have to + # be added. + tags_changed |= (not tags.empty?) tags.each do |k,v| tag = RelationTag.new tag.k = k @@ -207,14 +236,37 @@ class Relation < ActiveRecord::Base tag.save! end - members = self.members - RelationMember.delete_all(['id = ?', self.id]) - members.each do |n| + # same pattern as before, but this time we're collecting the + # changed members in an array, as the bounding box updates for + # elements are per-element, not blanked on/off like for tags. + changed_members = Array.new + members = self.members_as_hash + relation_members.each do |old_member| + key = [old_member.member_id.to_s, old_member.member_type] + if members.has_key? key + # i'd love to rely on rails' dirty handling here, but the + # relation members are always dirty because of the member_class + # handling. + if members[key] != old_member.member_role + old_member.member_role = members[key] + changed_members << key + old_member.save! + end + members.delete key + + else + changed_members << key + RelationMember.delete_all ['id = ? and member_id = ? and member_type = ?', self.id, old_member.member_id, old_member.member_type] + end + end + # any remaining members must be new additions + changed_members += members.keys + members.each do |k,v| mem = RelationMember.new mem.id = self.id - mem.member_type = n[0]; - mem.member_id = n[1]; - mem.member_role = n[2]; + mem.member_type = k[1]; + mem.member_id = k[0]; + mem.member_role = v; mem.save! end @@ -223,10 +275,50 @@ class Relation < ActiveRecord::Base old_relation.save_with_dependencies! # update the bbox of the changeset and save it too. - # FIXME: what is the bounding box of a relation? + # discussion on the mailing list gave the following definition for + # the bounding box update procedure of a relation: + # + # adding or removing nodes or ways from a relation causes them to be + # added to the changeset bounding box. adding a relation member or + # changing tag values causes all node and way members to be added to the + # bounding box. this is similar to how the map call does things and is + # reasonable on the assumption that adding or removing members doesn't + # materially change the rest of the relation. + any_relations = + changed_members.collect { |id,type| type == "relation" }. + inject(false) { |b,s| b or s } + + if tags_changed or any_relations + # add all non-relation bounding boxes to the changeset + # FIXME: check for tag changes along with element deletions and + # make sure that the deleted element's bounding box is hit. + self.members.each do |type, id, role| + if type != "relation" + update_changeset_element(type, id) + end + end + else + # add only changed members to the changeset + changed_members.each do |id, type| + update_changeset_element(type, id) + end + end + + # save the (maybe updated) changeset bounding box + changeset.save! 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) if self.visible check_consistency(self, new_relation, user) @@ -234,7 +326,7 @@ class Relation < ActiveRecord::Base raise OSM::APIPreconditionFailedError.new else self.changeset_id = new_relation.changeset_id - self.tags = [] + self.tags = {} self.members = [] self.visible = false save_with_history! @@ -319,6 +411,17 @@ class Relation < ActiveRecord::Base return false end + ## + # members in a hash table [id,type] => role + def members_as_hash + h = Hash.new + members.each do |m| + # should be: h[[m.id, m.type]] = m.role, but someone prefers arrays + h[[m[1], m[0]]] = m[2] + end + return h + end + # Temporary method to match interface to nodes def tags_as_hash return self.tags diff --git a/test/functional/relation_controller_test.rb b/test/functional/relation_controller_test.rb index ed5951365..5f23702db 100644 --- a/test/functional/relation_controller_test.rb +++ b/test/functional/relation_controller_test.rb @@ -265,6 +265,107 @@ class RelationControllerTest < ActionController::TestCase assert_response :not_found end + ## + # when a relation's tag is modified then it should put the bounding + # box of all its members into the changeset. + def test_tag_modify_bounding_box + # in current fixtures, relation 5 contains nodes 3 and 5 (node 3 + # indirectly via way 3), so the bbox should be [3,3,5,5]. + check_changeset_modify([3,3,5,5]) do |changeset_id| + # add a tag to an existing relation + relation_xml = current_relations(:visible_relation).to_xml + relation_element = relation_xml.find("//osm/relation").first + new_tag = XML::Node.new("tag") + new_tag['k'] = "some_new_tag" + new_tag['v'] = "some_new_value" + relation_element << new_tag + + # update changeset ID to point to new changeset + update_changeset(relation_xml, changeset_id) + + # upload the change + content relation_xml + put :update, :id => current_relations(:visible_relation).id + assert_response :success, "can't update relation for tag/bbox test" + end + end + + ## + # add a member to a relation and check the bounding box is only that + # element. + def test_add_member_bounding_box + check_changeset_modify([4,4,4,4]) do |changeset_id| + # add node 4 (4,4) to an existing relation + relation_xml = current_relations(:visible_relation).to_xml + relation_element = relation_xml.find("//osm/relation").first + new_member = XML::Node.new("member") + new_member['ref'] = current_nodes(:used_node_2).id.to_s + new_member['type'] = "node" + new_member['role'] = "some_role" + relation_element << new_member + + # update changeset ID to point to new changeset + update_changeset(relation_xml, changeset_id) + + # upload the change + content relation_xml + put :update, :id => current_relations(:visible_relation).id + assert_response :success, "can't update relation for add node/bbox test" + end + end + + ## + # remove a member from a relation and check the bounding box is + # only that element. + def test_remove_member_bounding_box + check_changeset_modify([5,5,5,5]) do |changeset_id| + # remove node 5 (5,5) from an existing relation + relation_xml = current_relations(:visible_relation).to_xml + relation_xml. + find("//osm/relation/member[@type='node'][@ref='5']"). + first.remove! + + # update changeset ID to point to new changeset + update_changeset(relation_xml, changeset_id) + + # upload the change + content relation_xml + put :update, :id => current_relations(:visible_relation).id + assert_response :success, "can't update relation for remove node/bbox test" + end + end + + ## + # create a changeset and yield to the caller to set it up, then assert + # that the changeset bounding box is +bbox+. + def check_changeset_modify(bbox) + basic_authorization("test@openstreetmap.org", "test"); + + # create a new changeset for this operation, so we are assured + # that the bounding box will be newly-generated. + changeset_id = with_controller(ChangesetController.new) do + content "" + put :create + assert_response :success, "couldn't create changeset for modify test" + @response.body.to_i + end + + # go back to the block to do the actual modifies + yield changeset_id + + # now download the changeset to check its bounding box + with_controller(ChangesetController.new) do + get :read, :id => changeset_id + assert_response :success, "can't re-read changeset for modify test" + assert_select "osm>changeset", 1 + assert_select "osm>changeset[id=#{changeset_id}]", 1 + assert_select "osm>changeset[min_lon=#{bbox[0]}]", 1 + assert_select "osm>changeset[min_lat=#{bbox[1]}]", 1 + assert_select "osm>changeset[max_lon=#{bbox[2]}]", 1 + assert_select "osm>changeset[max_lat=#{bbox[3]}]", 1 + end + end + ## # update the changeset_id of a node element def update_changeset(xml, changeset_id)