Implemented changeset bounding box handling for relations, based on the conversation...
authorMatt Amos <zerebubuth@gmail.com>
Thu, 6 Nov 2008 17:56:24 +0000 (17:56 +0000)
committerMatt Amos <zerebubuth@gmail.com>
Thu, 6 Nov 2008 17:56:24 +0000 (17:56 +0000)
app/models/relation.rb
test/functional/relation_controller_test.rb

index 9836ef4f1e29d9d65053f2cbacb833e4a7cb2bb2..cc7977833679b617ccea4807a2ff16c4b4d58a0d 100644 (file)
@@ -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
index ed595136561bdb9c255dd0b3b03cab77be548487..5f23702db4c3891de137252bf70409a1977c07d6 100644 (file)
@@ -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 "<osm><changeset/></osm>"
+      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)