Made relations ordered. Added some tests for this. Otherwise interface is unchanged.

diff --git a/app/controllers/relation_controller.rb b/app/controllers/relation_controller.rb
index da5129467ff1e72caba2f35da5815804eb285008..cdd1d34d6fb459434bab7b381286634ebf1d1291 100644 (file)
--- a/app/controllers/relation_controller.rb
+++ b/app/controllers/relation_controller.rb
@@ -180,7 +180,7 @@ class RelationController < ApplicationController
   end
 
   def relations_for_object(objtype)
   end
 
   def relations_for_object(objtype)

-    relationids = RelationMember.find(:all, :conditions => ['member_type=? and member_id=?', objtype, params[:id]]).collect { |ws| ws.id }.uniq
+    relationids = RelationMember.find(:all, :conditions => ['member_type=? and member_id=?', objtype, params[:id]]).collect { |ws| ws.id[0] }.uniq

 
     doc = OSM::API.new.get_xml_doc
 
 
     doc = OSM::API.new.get_xml_doc
 

diff --git a/app/models/node.rb b/app/models/node.rb
index 391b50dcda335b4cb7ddb1e54cce2ccad1775b51..faba4ed66d5e37817fc4821c4947d76aecbc03cc 100644 (file)
--- a/app/models/node.rb
+++ b/app/models/node.rb
@@ -267,6 +267,13 @@ class Node < ActiveRecord::Base
     @tags[k] = v
   end
 
     @tags[k] = v
   end
 

+  ##
+  # are the preconditions OK? this is mainly here to keep the duck
+  # typing interface the same between nodes, ways and relations.
+  def preconditions_ok?
+    in_world?
+  end
+

   ##
   # dummy method to make the interfaces of node, way and relation
   # more consistent.
   ##
   # dummy method to make the interfaces of node, way and relation
   # more consistent.

diff --git a/app/models/old_relation.rb b/app/models/old_relation.rb
index 491b444a677844f4c9d9db24874abac672226c6a..ffddc7945396b72180ce5571f459c4ca8076ce25 100644 (file)
--- a/app/models/old_relation.rb
+++ b/app/models/old_relation.rb
@@ -36,14 +36,12 @@ class OldRelation < ActiveRecord::Base
       tag.save!
     end
 
       tag.save!
     end
 

-    i = 1
-    self.members.each do |m|
+    self.members.each_with_index do |m,i|

       member = OldRelationMember.new
       member = OldRelationMember.new

-      member.id = self.id
+      member.id = [self.id, self.version, i]

       member.member_type = m[0]
       member.member_id = m[1]
       member.member_role = m[2]
       member.member_type = m[0]
       member.member_id = m[1]
       member.member_role = m[2]

-      member.version = self.version

       member.save!
     end
   end
       member.save!
     end
   end


@@ -51,7 +49,7 @@ class OldRelation < ActiveRecord::Base
   def members
     unless @members
         @members = Array.new
   def members
     unless @members
         @members = Array.new

-        OldRelationMember.find(:all, :conditions => ["id = ? AND version = ?", self.id, self.version]).each do |m|
+        OldRelationMember.find(:all, :conditions => ["id = ? AND version = ?", self.id, self.version], :order => "sequence_id").each do |m|

             @members += [[m.type,m.id,m.role]]
         end
     end
             @members += [[m.type,m.id,m.role]]
         end
     end

diff --git a/app/models/old_relation_member.rb b/app/models/old_relation_member.rb
index d8b68585428da54515eb579d9a31ca4f04f04576..f0294d33952bd015f43c48d860ff30721b7a8b4e 100644 (file)
--- a/app/models/old_relation_member.rb
+++ b/app/models/old_relation_member.rb
@@ -1,3 +1,6 @@
 class OldRelationMember < ActiveRecord::Base
   set_table_name 'relation_members'
 class OldRelationMember < ActiveRecord::Base
   set_table_name 'relation_members'

+
+  set_primary_keys :id, :version, :sequence_id
+  belongs_to :relation, :foreign_key=> :id

 end
 end

diff --git a/app/models/relation.rb b/app/models/relation.rb
index 1bbb1d8e93fc9218224e18872dc87d069486e6b6..b94aef9ae9bb3946e5c4f4caad0971d37e0f7364 100644 (file)
--- a/app/models/relation.rb
+++ b/app/models/relation.rb
@@ -9,7 +9,7 @@ class Relation < ActiveRecord::Base
 
   has_many :old_relations, :foreign_key => 'id', :order => 'version'
 
 
   has_many :old_relations, :foreign_key => 'id', :order => 'version'
 

-  has_many :relation_members, :foreign_key => 'id'
+  has_many :relation_members, :foreign_key => 'id', :order => 'sequence_id'

   has_many :relation_tags, :foreign_key => 'id'
 
   has_many :containing_relation_members, :class_name => "RelationMember", :as => :member
   has_many :relation_tags, :foreign_key => 'id'
 
   has_many :containing_relation_members, :class_name => "RelationMember", :as => :member


@@ -243,33 +243,33 @@ class Relation < ActiveRecord::Base
       # 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
       # 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
+      members = Hash.new
+      self.members.each do |m|
+        # should be: h[[m.id, m.type]] = m.role, but someone prefers arrays
+        members[[m[1], m[0]]] = m[2]
+      end

       relation_members.each do |old_member|
         key = [old_member.member_id.to_s, old_member.member_type]
         if members.has_key? key
       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
           members.delete key

-

         else
           changed_members << 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
         end
       end
       # any remaining members must be new additions
       changed_members += members.keys

-      members.each do |k,v|
+
+      # update the members. first delete all the old members, as the new
+      # members may be in a different order and i don't feel like implementing
+      # a longest common subsequence algorithm to optimise this.
+      members = self.members
+      RelationMember.delete_all(:id => self.id)
+      members.each_with_index do |m,i|

         mem = RelationMember.new
         mem = RelationMember.new

-        mem.id = self.id
-        mem.member_type = k[1];
-        mem.member_id = k[0];
-        mem.member_role = v;
+        mem.id = [self.id, i]
+        mem.member_type = m[0]
+        mem.member_id = m[1]
+        mem.member_role = m[2]

         mem.save!
       end
 
         mem.save!
       end
 


@@ -364,67 +364,37 @@ class Relation < ActiveRecord::Base
   def preconditions_ok?
     # These are hastables that store an id in the index of all 
     # the nodes/way/relations that have already been added.
   def preconditions_ok?
     # These are hastables that store an id in the index of all 
     # the nodes/way/relations that have already been added.

-    # Once we know the id of the node/way/relation exists
-    # we check to see if it is already existing in the hashtable
-    # if it does, then we return false. Otherwise
-    # we add it to the relevant hash table, with the value true..
+    # If the member is valid and visible then we add it to the 
+    # relevant hash table, with the value true as a cache.

     # Thus if you have nodes with the ids of 50 and 1 already in the
     # relation, then the hash table nodes would contain:
     # => {50=>true, 1=>true}
     # Thus if you have nodes with the ids of 50 and 1 already in the
     # relation, then the hash table nodes would contain:
     # => {50=>true, 1=>true}

-    nodes = Hash.new
-    ways = Hash.new
-    relations = Hash.new
+    elements = { :node => Hash.new, :way => Hash.new, :relation => Hash.new }

     self.members.each do |m|
     self.members.each do |m|

-      if (m[0] == "node")
-        n = Node.find(:first, :conditions => ["id = ?", m[1]])
-        unless n and n.visible 
-          return false
-        end
-        if nodes[m[1]]
-          return false
-        else
-          nodes[m[1]] = true
-        end
-      elsif (m[0] == "way")
-        w = Way.find(:first, :conditions => ["id = ?", m[1]])
-        unless w and w.visible and w.preconditions_ok?
-          return false
-        end
-        if ways[m[1]]
-          return false
-        else
-          ways[m[1]] = true
-        end
-      elsif (m[0] == "relation")
-        e = Relation.find(:first, :conditions => ["id = ?", m[1]])
-        unless e and e.visible and e.preconditions_ok?
-          return false
-        end
-        if relations[m[1]]
+      # find the hash for the element type or die
+      hash = elements[m[0].to_sym] or return false
+
+      # unless its in the cache already
+      unless hash.key? m[1]
+        # use reflection to look up the appropriate class
+        model = Kernel.const_get(m[0].capitalize)
+
+        # get the element with that ID
+        element = model.find(m[1])
+
+        # and check that it is OK to use.
+        unless element and element.visible? and element.preconditions_ok?

           return false
           return false

-        else
-          relations[m[1]] = true

         end
         end

-      else
-        return false
+        hash[m[1]] = true

       end
     end
       end
     end

+

     return true
   rescue
     return false
   end
 
     return true
   rescue
     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
   # Temporary method to match interface to nodes
   def tags_as_hash
     return self.tags

diff --git a/app/models/relation_member.rb b/app/models/relation_member.rb
index 9ff4f46f3b8b0b6ae4fb1b02ba49ccab25a71b66..08bb988eec243117165e6e58de560558287ebbbf 100644 (file)
--- a/app/models/relation_member.rb
+++ b/app/models/relation_member.rb
@@ -1,6 +1,7 @@
 class RelationMember < ActiveRecord::Base
   set_table_name 'current_relation_members'
   
 class RelationMember < ActiveRecord::Base
   set_table_name 'current_relation_members'
   

+  set_primary_keys :id, :sequence_id

   belongs_to :member, :polymorphic => true, :foreign_type => :member_class
   belongs_to :relation, :foreign_key => :id
 
   belongs_to :member, :polymorphic => true, :foreign_type => :member_class
   belongs_to :relation, :foreign_key => :id
 

diff --git a/db/migrate/022_order_relation_members.rb b/db/migrate/022_order_relation_members.rb
new file mode 100644 (file)
index 0000000..e3820d4
--- /dev/null
+++ b/db/migrate/022_order_relation_members.rb
@@ -0,0 +1,33 @@
+class OrderRelationMembers < ActiveRecord::Migration
+  def self.up
+    # add sequence column. rails won't let us define an ordering here,
+    # as defaults must be constant.
+    add_column(:relation_members, :sequence_id, :integer,
+               :default => 0, :null => false)
+
+    # update the sequence column with default (partial) ordering by 
+    # element ID. the sequence ID is a smaller int type, so we can't
+    # just copy the member_id.
+    ActiveRecord::Base.connection().execute("update relation_members set sequence_id = mod(member_id, 16384)")
+
+    # need to update the primary key to include the sequence number, 
+    # otherwise the primary key will barf when we have repeated members.
+    # mysql barfs on this anyway, so we need a single command. this may
+    # not work in postgres... needs testing.
+    ActiveRecord::Base.connection().execute("alter table relation_members drop primary key, add primary key (id, version, member_type, member_id, member_role, sequence_id)")
+
+    # do the same for the current tables
+    add_column(:current_relation_members, :sequence_id, :integer,
+               :default => 0, :null => false)
+    ActiveRecord::Base.connection().execute("update current_relation_members set sequence_id = mod(member_id, 16384)")
+    ActiveRecord::Base.connection().execute("alter table current_relation_members drop primary key, add primary key (id, member_type, member_id, member_role, sequence_id)")
+  end
+
+  def self.down
+    ActiveRecord::Base.connection().execute("alter table current_relation_members drop primary key, add primary key (id, member_type, member_id, member_role)")
+    remove_column :relation_members, :sequence_id
+
+    ActiveRecord::Base.connection().execute("alter table relation_members drop primary key, add primary key (id, version, member_type, member_id, member_role)")
+    remove_column :current_relation_members, :sequence_id
+  end
+end

diff --git a/test/functional/relation_controller_test.rb b/test/functional/relation_controller_test.rb
index 4b265b5036c3e403d6ca216da2e7ec242eb59d01..d44490036b1684b3adad76bb95e754bfd9c1a721 100644 (file)
--- a/test/functional/relation_controller_test.rb
+++ b/test/functional/relation_controller_test.rb
@@ -335,6 +335,106 @@ class RelationControllerTest < ActionController::TestCase
     end
   end
   
     end
   end
   

+  ##
+  # check that relations are ordered
+  def test_relation_member_ordering
+    basic_authorization("test@openstreetmap.org", "test");  
+
+    doc_str = <<OSM
+<osm>
+ <relation changeset='1'>
+  <member ref='1' type='node' role='first'/>
+  <member ref='3' type='node' role='second'/>
+  <member ref='1' type='way' role='third'/>
+  <member ref='3' type='way' role='fourth'/>
+ </relation>
+</osm>
+OSM
+    doc = XML::Parser.string(doc_str).parse
+
+    content doc
+    put :create
+    assert_response :success, "can't create a relation: #{@response.body}"
+    relation_id = @response.body.to_i
+
+    # get it back and check the ordering
+    get :read, :id => relation_id
+    assert_response :success, "can't read back the relation: #{@response.body}"
+    check_ordering(doc, @response.body)
+
+    # insert a member at the front
+    new_member = XML::Node.new "member"
+    new_member['ref'] = 5.to_s
+    new_member['type'] = 'node'
+    new_member['role'] = 'new first'
+    doc.find("//osm/relation").first.child.prev = new_member
+    # update the version, should be 1?
+    doc.find("//osm/relation").first['id'] = relation_id.to_s
+    doc.find("//osm/relation").first['version'] = 1.to_s
+
+    # upload the next version of the relation
+    content doc
+    put :update, :id => relation_id
+    assert_response :success, "can't update relation: #{@response.body}"
+    new_version = @response.body.to_i
+
+    # get it back again and check the ordering again
+    get :read, :id => relation_id
+    assert_response :success, "can't read back the relation: #{@response.body}"
+    check_ordering(doc, @response.body)
+  end
+
+  ## 
+  # check that relations can contain duplicate members
+  def test_relation_member_duplicates
+    basic_authorization("test@openstreetmap.org", "test");  
+
+    doc_str = <<OSM
+<osm>
+ <relation changeset='1'>
+  <member ref='1' type='node' role='forward'/>
+  <member ref='3' type='node' role='forward'/>
+  <member ref='1' type='node' role='forward'/>
+  <member ref='3' type='node' role='forward'/>
+ </relation>
+</osm>
+OSM
+    doc = XML::Parser.string(doc_str).parse
+
+    content doc
+    put :create
+    assert_response :success, "can't create a relation: #{@response.body}"
+    relation_id = @response.body.to_i
+
+    # get it back and check the ordering
+    get :read, :id => relation_id
+    assert_response :success, "can't read back the relation: #{@response.body}"
+    check_ordering(doc, @response.body)
+  end
+
+  # ============================================================
+  # utility functions
+  # ============================================================
+
+  ##
+  # checks that the XML document and the string arguments have
+  # members in the same order.
+  def check_ordering(doc, xml)
+    new_doc = XML::Parser.string(xml).parse
+
+    doc_members = doc.find("//osm/relation/member").collect do |m|
+      [m['ref'].to_i, m['type'].to_sym, m['role']]
+    end
+
+    new_members = new_doc.find("//osm/relation/member").collect do |m|
+      [m['ref'].to_i, m['type'].to_sym, m['role']]
+    end
+
+    doc_members.zip(new_members).each do |d, n|
+      assert_equal d, n, "members are not equal - ordering is wrong? (#{doc}, #{xml})"
+    end
+  end
+

   ##
   # create a changeset and yield to the caller to set it up, then assert
   # that the changeset bounding box is +bbox+.
   ##
   # create a changeset and yield to the caller to set it up, then assert
   # that the changeset bounding box is +bbox+.