From e79fd0763854f8cf41aefd7364c6bdf476280811 Mon Sep 17 00:00:00 2001 From: Matt Amos Date: Mon, 10 Nov 2008 15:41:05 +0000 Subject: [PATCH] Made relations ordered. Added some tests for this. Otherwise interface is unchanged. --- app/controllers/relation_controller.rb | 2 +- app/models/node.rb | 7 ++ app/models/old_relation.rb | 8 +- app/models/old_relation_member.rb | 3 + app/models/relation.rb | 100 +++++++------------- app/models/relation_member.rb | 1 + db/migrate/022_order_relation_members.rb | 33 +++++++ test/functional/relation_controller_test.rb | 100 ++++++++++++++++++++ 8 files changed, 183 insertions(+), 71 deletions(-) create mode 100644 db/migrate/022_order_relation_members.rb diff --git a/app/controllers/relation_controller.rb b/app/controllers/relation_controller.rb index da5129467..cdd1d34d6 100644 --- 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) - 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 diff --git a/app/models/node.rb b/app/models/node.rb index 391b50dcd..faba4ed66 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -267,6 +267,13 @@ class Node < ActiveRecord::Base @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. diff --git a/app/models/old_relation.rb b/app/models/old_relation.rb index 491b444a6..ffddc7945 100644 --- a/app/models/old_relation.rb +++ b/app/models/old_relation.rb @@ -36,14 +36,12 @@ class OldRelation < ActiveRecord::Base tag.save! end - i = 1 - self.members.each do |m| + self.members.each_with_index do |m,i| 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.version = self.version member.save! end end @@ -51,7 +49,7 @@ class OldRelation < ActiveRecord::Base 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 diff --git a/app/models/old_relation_member.rb b/app/models/old_relation_member.rb index d8b685854..f0294d339 100644 --- 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' + + set_primary_keys :id, :version, :sequence_id + belongs_to :relation, :foreign_key=> :id end diff --git a/app/models/relation.rb b/app/models/relation.rb index 1bbb1d8e9..b94aef9ae 100644 --- 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 :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 @@ -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 - 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 - # 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| + + # 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.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 @@ -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. - # 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} - nodes = Hash.new - ways = Hash.new - relations = Hash.new + elements = { :node => Hash.new, :way => Hash.new, :relation => Hash.new } 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 - else - relations[m[1]] = true end - else - return false + hash[m[1]] = true end 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 diff --git a/app/models/relation_member.rb b/app/models/relation_member.rb index 9ff4f46f3..08bb988ee 100644 --- 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' + set_primary_keys :id, :sequence_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 index 000000000..e3820d4e6 --- /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 4b265b503..d44490036 100644 --- a/test/functional/relation_controller_test.rb +++ b/test/functional/relation_controller_test.rb @@ -335,6 +335,106 @@ class RelationControllerTest < ActionController::TestCase end end + ## + # check that relations are ordered + def test_relation_member_ordering + basic_authorization("test@openstreetmap.org", "test"); + + doc_str = < + + + + + + + +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 + 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+. -- 2.39.5