From 64a5e21f53be132cd5fe3265230c6cdc00ccabaa Mon Sep 17 00:00:00 2001 From: Shaun McDonald Date: Mon, 7 Jul 2008 14:20:27 +0000 Subject: [PATCH] Fixing the resync that I had done wrong at the end of last week on the nodes. All unit tests now do work. More assertions added. Using a scaling factor as a constant, so that the nodes are stored in the db correctly. Using the same scaling factor in the georecord library, to make code more readable, and reduce errors. Removing duplicate code that is in the GeoRecord include library. --- app/models/node.rb | 149 ++++++++++++++++++++++---------- app/models/old_node.rb | 73 ++++++++++++---- lib/geo_record.rb | 13 ++- test/fixtures/current_nodes.yml | 32 +++---- test/fixtures/nodes.yml | 20 ++--- test/unit/node_test.rb | 23 +++-- 6 files changed, 211 insertions(+), 99 deletions(-) diff --git a/app/models/node.rb b/app/models/node.rb index abfa44d67..7f9b939db 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -1,6 +1,5 @@ class Node < ActiveRecord::Base require 'xml/libxml' - include GeoRecord set_table_name 'current_nodes' @@ -10,17 +9,14 @@ class Node < ActiveRecord::Base validates_numericality_of :latitude, :longitude validate :validate_position - belongs_to :user - has_many :old_nodes, :foreign_key => :id - has_many :way_nodes - has_many :ways, :through => :way_nodes - + has_many :node_tags, :foreign_key => :id + belongs_to :user + has_many :containing_relation_members, :class_name => "RelationMember", :as => :member has_many :containing_relations, :class_name => "Relation", :through => :containing_relation_members, :source => :relation, :extend => ObjectFinder - - # Sanity check the latitude and longitude and add an error if it's broken + def validate_position errors.add_to_base("Node is not in the world") unless in_world? end @@ -57,64 +53,109 @@ class Node < ActiveRecord::Base p = XML::Parser.new p.string = xml doc = p.parse - - node = Node.new doc.find('//osm/node').each do |pt| - node.lat = pt['lat'].to_f - node.lon = pt['lon'].to_f + return Node.from_xml_node(pt, create) + end + rescue + return nil + end + end - return nil unless node.in_world? + def self.from_xml_node(pt, create=false) + node = Node.new + + node.version = pt['version'] + node.lat = pt['lat'].to_f + node.lon = pt['lon'].to_f - unless create - if pt['id'] != '0' - node.id = pt['id'].to_i - end - end + return nil unless node.in_world? - node.visible = pt['visible'] and pt['visible'] == 'true' + unless create + if pt['id'] != '0' + node.id = pt['id'].to_i + end + end - if create - node.timestamp = Time.now - else - if pt['timestamp'] - node.timestamp = Time.parse(pt['timestamp']) - end - end + node.visible = pt['visible'] and pt['visible'] == 'true' - tags = [] + if create + node.timestamp = Time.now + else + if pt['timestamp'] + node.timestamp = Time.parse(pt['timestamp']) + end + end - pt.find('tag').each do |tag| - tags << [tag['k'],tag['v']] - end + tags = [] - node.tags = Tags.join(tags) - end - rescue - node = nil + pt.find('tag').each do |tag| + node.add_tag_key_val(tag['k'],tag['v']) end return node end - # Save this node with the appropriate OldNode object to represent it's history. def save_with_history! + t = Time.now Node.transaction do - self.timestamp = Time.now + self.version += 1 + self.timestamp = t self.save! + + # Create a NodeTag + tags = self.tags + NodeTag.delete_all(['id = ?', self.id]) + tags.each do |k,v| + tag = NodeTag.new + tag.k = k + tag.v = v + tag.id = self.id + tag.save! + end + + # Create an OldNode old_node = OldNode.from_node(self) - old_node.save! + old_node.timestamp = t + old_node.save_with_dependencies! + end + end + + def delete_with_history(user) + if self.visible + if WayNode.find(:first, :joins => "INNER JOIN current_ways ON current_ways.id = current_way_nodes.id", :conditions => [ "current_ways.visible = 1 AND current_way_nodes.node_id = ?", self.id ]) + raise OSM::APIPreconditionFailedError.new + elsif RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='node' and member_id=?", self.id]) + raise OSM::APIPreconditionFailedError.new + else + self.user_id = user.id + self.visible = 0 + save_with_history! + end + else + raise OSM::APIAlreadyDeletedError.new + end + end + + def update_from(new_node, user) + if new_node.version != version + raise OSM::APIVersionMismatchError.new(new_node.version, version) end + + self.user_id = user.id + self.latitude = new_node.latitude + self.longitude = new_node.longitude + self.tags = new_node.tags + self.visible = true + save_with_history! end - # Turn this Node in to a complete OSM XML object with wrapper def to_xml doc = OSM::API.new.get_xml_doc doc.root << to_xml_node() return doc end - # Turn this Node in to an XML Node without the wrapper. def to_xml_node(user_display_name_cache = nil) el1 = XML::Node.new 'node' el1['id'] = self.id.to_s @@ -133,7 +174,7 @@ class Node < ActiveRecord::Base el1['user'] = user_display_name_cache[self.user_id] unless user_display_name_cache[self.user_id].nil? - Tags.split(self.tags) do |k,v| + self.tags.each do |k,v| el2 = XML::Node.new('tag') el2['k'] = k.to_s el2['v'] = v.to_s @@ -142,15 +183,33 @@ class Node < ActiveRecord::Base el1['visible'] = self.visible.to_s el1['timestamp'] = self.timestamp.xmlschema + el1['version'] = self.version.to_s return el1 end - # Return the node's tags as a Hash of keys and their values def tags_as_hash - hash = {} - Tags.split(self.tags) do |k,v| - hash[k] = v + return tags + end + + def tags + unless @tags + @tags = {} + self.node_tags.each do |tag| + @tags[tag.k] = tag.v + end end - hash + @tags + end + + def tags=(t) + @tags = t + end + + def add_tag_key_val(k,v) + @tags = Hash.new unless @tags + @tags[k] = v end + + + end diff --git a/app/models/old_node.rb b/app/models/old_node.rb index 76eab8427..018284528 100644 --- a/app/models/old_node.rb +++ b/app/models/old_node.rb @@ -1,6 +1,5 @@ -class OldNode < ActiveRecord::Base - include GeoRecord - +class OldNode < ActiveRecord::Base + include GeoRecord set_table_name 'nodes' validates_presence_of :user_id, :timestamp @@ -29,8 +28,15 @@ class OldNode < ActiveRecord::Base old_node.timestamp = node.timestamp old_node.user_id = node.user_id old_node.id = node.id + old_node.version = node.version return old_node end + + def to_xml + doc = OSM::API.new.get_xml_doc + doc.root << to_xml_node() + return doc + end def to_xml_node el1 = XML::Node.new 'node' @@ -39,7 +45,7 @@ class OldNode < ActiveRecord::Base el1['lon'] = self.lon.to_s el1['user'] = self.user.display_name if self.user.data_public? - Tags.split(self.tags) do |k,v| + self.tags.each do |k,v| el2 = XML::Node.new('tag') el2['k'] = k.to_s el2['v'] = v.to_s @@ -48,24 +54,57 @@ class OldNode < ActiveRecord::Base el1['visible'] = self.visible.to_s el1['timestamp'] = self.timestamp.xmlschema + el1['version'] = self.version.to_s return el1 end - - def tags_as_hash - hash = {} - Tags.split(self.tags) do |k,v| - hash[k] = v + + def save_with_dependencies! + save! + #not sure whats going on here + clear_aggregation_cache + clear_association_cache + #ok from here + @attributes.update(OldNode.find(:first, :conditions => ['id = ? AND timestamp = ?', self.id, self.timestamp]).instance_variable_get('@attributes')) + + self.tags.each do |k,v| + tag = OldNodeTag.new + tag.k = k + tag.v = v + tag.id = self.id + tag.version = self.version + tag.save! end - hash end - # Pretend we're not in any ways - def ways - return [] + def tags + unless @tags + @tags = Hash.new + OldNodeTag.find(:all, :conditions => ["id = ? AND version = ?", self.id, self.version]).each do |tag| + @tags[tag.k] = tag.v + end + end + @tags = Hash.new unless @tags + @tags end - # Pretend we're not in any relations - def containing_relation_members - return [] - end + def tags=(t) + @tags = t + end + def tags_as_hash + hash = {} + Tags.split(self.tags) do |k,v| + hash[k] = v + end + hash + end + + # Pretend we're not in any ways + def ways + return [] + end + + # Pretend we're not in any relations + def containing_relation_members + return [] + end end diff --git a/lib/geo_record.rb b/lib/geo_record.rb index 025bbe4a8..286ce69e1 100644 --- a/lib/geo_record.rb +++ b/lib/geo_record.rb @@ -1,4 +1,9 @@ module GeoRecord + # This scaling factor is used to convert between the float lat/lon that is + # returned by the API, and the integer lat/lon equivalent that is stored in + # the database. + SCALE = 10000000 + def self.included(base) base.extend(ClassMethods) end @@ -20,21 +25,21 @@ module GeoRecord end def lat=(l) - self.latitude = (l * 10000000).round + self.latitude = (l * SCALE).round end def lon=(l) - self.longitude = (l * 10000000).round + self.longitude = (l * SCALE).round end # Return WGS84 latitude def lat - return self.latitude.to_f / 10000000 + return self.latitude.to_f / SCALE end # Return WGS84 longitude def lon - return self.longitude.to_f / 10000000 + return self.longitude.to_f / SCALE end # Potlatch projections diff --git a/test/fixtures/current_nodes.yml b/test/fixtures/current_nodes.yml index 8fd3b781f..7af9bbda1 100644 --- a/test/fixtures/current_nodes.yml +++ b/test/fixtures/current_nodes.yml @@ -10,32 +10,32 @@ visible_node: invisible_node: id: 2 - latitude: 2 - longitude: 2 + latitude: <%= 2*SCALE %> + longitude: <%= 2*SCALE %> user_id: 1 visible: 0 timestamp: 2007-01-01 00:00:00 used_node_1: id: 3 - latitude: 3 - longitude: 3 + latitude: <%= 3*SCALE %> + longitude: <%= 3*SCALE %> user_id: 1 visible: 1 timestamp: 2007-01-01 00:00:00 used_node_2: id: 4 - latitude: 4 - longitude: 4 + latitude: <%= 4*SCALE %> + longitude: <%= 4*SCALE %> user_id: 1 visible: 1 timestamp: 2007-01-01 00:00:00 node_used_by_relationship: id: 5 - latitude: 5 - longitude: 5 + latitude: <%= 5*SCALE %> + longitude: <%= 5*SCALE %> user_id: 1 visible: 1 timestamp: 2007-01-01 00:00:00 @@ -49,29 +49,29 @@ node_too_far_north: node_too_far_south: id: 7 - latitude: -90 - longitude: 7 + latitude: <%= -91*SCALE %> + longitude: <%= 7*SCALE %> user_id: 1 timestamp: 2007-01-01 00:00:00 node_too_far_west: id: 8 - latitude: 8 - longitude: -181 + latitude: <%= 8*SCALE %> + longitude: <%= -181*SCALE %> user_id: 1 timestamp: 2007-01-01 00:00:00 node_too_far_east: id: 9 - latitude: 9 - longitude: 180 + latitude: <%= 9*SCALE %> + longitude: <%= 181*SCALE %> user_id: 1 timestamp: 2007-01-01 00:00:00 node_totally_wrong: id: 10 - latitude: 1000 - longitude: 1000 + latitude: <%= 1000*SCALE %> + longitude: <%= 1000*SCALE %> user_id: 1 timestamp: 2007-01-01 00:00:00 diff --git a/test/fixtures/nodes.yml b/test/fixtures/nodes.yml index b10ce2fe7..202a7d931 100644 --- a/test/fixtures/nodes.yml +++ b/test/fixtures/nodes.yml @@ -1,40 +1,40 @@ # Read about fixtures at http://ar.rubyonrails.org/classes/Fixtures.html visible_node: id: 1 - latitude: 1 - longitude: 1 + latitude: <%= 1*SCALE %> + longitude: <%= 1*SCALE %> user_id: 1 visible: 1 timestamp: 2007-01-01 00:00:00 invisible_node: id: 2 - latitude: 2 - longitude: 2 + latitude: <%= 2*SCALE %> + longitude: <%= 2*SCALE %> user_id: 1 visible: 0 timestamp: 2007-01-01 00:00:00 used_node_1: id: 3 - latitude: 3 - longitude: 3 + latitude: <%= 3*SCALE %> + longitude: <%= 3*SCALE %> user_id: 1 visible: 1 timestamp: 2007-01-01 00:00:00 used_node_2: id: 4 - latitude: 4 - longitude: 4 + latitude: <%= 4*SCALE %> + longitude: <%= 4*SCALE %> user_id: 1 visible: 1 timestamp: 2007-01-01 00:00:00 node_used_by_relationship: id: 5 - latitude: 5 - longitude: 5 + latitude: <%= 5*SCALE %> + longitude: <%= 5*SCALE %> user_id: 1 visible: 1 timestamp: 2007-01-01 00:00:00 diff --git a/test/unit/node_test.rb b/test/unit/node_test.rb index d56fed50a..460c13a02 100644 --- a/test/unit/node_test.rb +++ b/test/unit/node_test.rb @@ -8,29 +8,38 @@ class NodeTest < Test::Unit::TestCase set_fixture_class :currenr_node_tags => :NodeTag def test_node_too_far_north - noden = current_nodes(:node_too_far_north) - assert_equal noden.lat, current_nodes(:node_too_far_north).latitude/SCALE - assert_equal false, noden.valid? + node = current_nodes(:node_too_far_north) + assert_equal node.lat, current_nodes(:node_too_far_north).latitude/SCALE + assert_equal node.lon, current_nodes(:node_too_far_north).longitude/SCALE + assert_equal false, node.valid? end def test_node_too_far_south node = current_nodes(:node_too_far_south) - assert_valid node + assert_equal node.lat, current_nodes(:node_too_far_south).latitude/SCALE + assert_equal node.lon, current_nodes(:node_too_far_south).longitude/SCALE + assert_equal false, node.valid? end def test_node_too_far_west node = current_nodes(:node_too_far_west) - assert_valid node + assert_equal node.lat, current_nodes(:node_too_far_west).latitude/SCALE + assert_equal node.lon, current_nodes(:node_too_far_west).longitude/SCALE + assert_equal false, node.valid? end def test_node_too_far_east node = current_nodes(:node_too_far_east) - assert_valid node + assert_equal node.lat, current_nodes(:node_too_far_east).latitude/SCALE + assert_equal node.lon, current_nodes(:node_too_far_east).longitude/SCALE + assert_equal false, node.valid? end def test_totally_wrong node = current_nodes(:node_totally_wrong) - assert_valid node + #assert_equal node.lat, current_nodes(:node_totally_wrong).latitude/SCALE + #assert_equal node.lon, current_nodes(:node_totally_wrong).longitude/SCALE + assert_equal false, node.valid? end def test_create -- 2.39.5