From e4d7b3ee6631e1c5aee23ebe52adb32c716f5cbd Mon Sep 17 00:00:00 2001 From: Shaun McDonald Date: Wed, 10 Jun 2009 15:42:12 +0000 Subject: [PATCH] Throw errors in the way xml parsing if there is a problem. Bring the way parsing in line with the node parsing. Properly deal with the 0 id case. Some Way.from_xml tests. --- app/models/node.rb | 13 +++++---- app/models/way.rb | 29 ++++++++++---------- test/unit/node_test.rb | 18 ++++++++++-- test/unit/way_test.rb | 62 +++++++++++++++++++++++++++++++++++++++++- 4 files changed, 98 insertions(+), 24 deletions(-) diff --git a/app/models/node.rb b/app/models/node.rb index 6ef1b9e6f..eeb6e6da5 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -82,7 +82,7 @@ class Node < ActiveRecord::Base raise OSM::APIBadXMLError.new("node", pt, "lon missing") if pt['lon'].nil? node.lat = pt['lat'].to_f node.lon = pt['lon'].to_f - raise OSM::APIBadXMLError.new("node", pt, "changeset id missing") if pt['changeset'].nil? + raise OSM::APIBadXMLError.new("node", pt, "Changeset id is missing") if pt['changeset'].nil? node.changeset_id = pt['changeset'].to_i raise OSM::APIBadUserInput.new("The node is outside this world") unless node.in_world? @@ -93,15 +93,16 @@ class Node < ActiveRecord::Base unless create raise OSM::APIBadXMLError.new("node", pt, "ID is required when updating.") if pt['id'].nil? - if pt['id'] != '0' - node.id = pt['id'].to_i - end + node.id = pt['id'].to_i + # .to_i will return 0 if there is no number that can be parsed. + # We want to make sure that there is no id with zero anyway + raise OSM::APIBadUserInput.new("ID of node cannot be zero when updating.") if node.id == 0 end # We don't care about the time, as it is explicitly set on create/update/delete # We don't care about the visibility as it is implicit based on the action - - tags = [] + # and set manually before the actual delete + node.visible = true pt.find('tag').each do |tag| node.add_tag_key_val(tag['k'],tag['v']) diff --git a/app/models/way.rb b/app/models/way.rb index 4f988dc9d..f9d36be93 100644 --- a/app/models/way.rb +++ b/app/models/way.rb @@ -25,6 +25,7 @@ class Way < ActiveRecord::Base validates_numericality_of :id, :on => :update, :integer_only => true validates_associated :changeset + # Read in xml as text and return it's Way object representation def self.from_xml(xml, create=false) begin p = XML::Parser.string(xml) @@ -41,26 +42,24 @@ class Way < ActiveRecord::Base def self.from_xml_node(pt, create=false) way = Way.new - if !create and pt['id'] != '0' - way.id = pt['id'].to_i - end - + raise OSM::APIBadXMLError.new("way", pt, "Version is required when updating") unless create or not pt['version'].nil? way.version = pt['version'] - raise OSM::APIBadXMLError.new("node", pt, "Changeset is required") if pt['changeset'].nil? + raise OSM::APIBadXMLError.new("way", pt, "Changeset id is missing") if pt['changeset'].nil? way.changeset_id = pt['changeset'] - # This next section isn't required for the create, update, or delete of ways - if create - way.timestamp = Time.now.getutc - way.visible = true - else - if pt['timestamp'] - way.timestamp = Time.parse(pt['timestamp']) - end - # if visible isn't present then it defaults to true - way.visible = (pt['visible'] or true) + unless create + raise OSM::APIBadXMLError.new("way", pt, "ID is required when updating") if pt['id'].nil? + way.id = pt['id'].to_i + # .to_i will return 0 if there is no number that can be parsed. + # We want to make sure that there is no id with zero anyway + raise OSM::APIBadUserInput.new("ID of way cannot be zero when updating.") if way.id == 0 end + # We don't care about the timestamp nor the visibility as these are either + # set explicitly or implicit in the action. The visibility is set to true, + # and manually set to false before the actual delete. + way.visible = true + pt.find('tag').each do |tag| way.add_tag_keyval(tag['k'], tag['v']) end diff --git a/test/unit/node_test.rb b/test/unit/node_test.rb index 21a62cc5f..11b745f5e 100644 --- a/test/unit/node_test.rb +++ b/test/unit/node_test.rb @@ -214,11 +214,11 @@ class NodeTest < ActiveSupport::TestCase message_create = assert_raise(OSM::APIBadXMLError) { Node.from_xml(nocs, true) } - assert_match /changeset id missing/, message_create.message + assert_match /Changeset id is missing/, message_create.message message_update = assert_raise(OSM::APIBadXMLError) { Node.from_xml(nocs, false) } - assert_match /changeset id missing/, message_update.message + assert_match /Changeset id is missing/, message_update.message end def test_from_xml_no_version @@ -244,6 +244,20 @@ class NodeTest < ActiveSupport::TestCase assert_match /Fatal error: Attribute lat redefined at/, message_update.message end + def test_from_xml_id_zero + id_list = ["", "0", "00", "0.0", "a"] + id_list.each do |id| + zero_id = "" + assert_nothing_raised(OSM::APIBadUserInput) { + Node.from_xml(zero_id, true) + } + message_update = assert_raise(OSM::APIBadUserInput) { + Node.from_xml(zero_id, false) + } + assert_match /ID of node cannot be zero when updating/, message_update.message + end + end + def test_from_xml_no_text no_text = "" message_create = assert_raise(OSM::APIBadXMLError) { diff --git a/test/unit/way_test.rb b/test/unit/way_test.rb index c173454e4..afbe36252 100644 --- a/test/unit/way_test.rb +++ b/test/unit/way_test.rb @@ -26,7 +26,7 @@ class WayTest < ActiveSupport::TestCase way = Way.find(current_ways(:visible_way).id) assert way.valid? # it already has 1 node - 1.upto((APP_CONFIG['max_number_of_way_nodes'])/2) { + 1.upto((APP_CONFIG['max_number_of_way_nodes']) / 2) { way.add_nd_num(current_nodes(:used_node_1).id) way.add_nd_num(current_nodes(:used_node_2).id) } @@ -36,4 +36,64 @@ class WayTest < ActiveSupport::TestCase way.add_nd_num(current_nodes(:visible_node).id) assert way.valid? end + + def test_from_xml_no_id + noid = "" + assert_nothing_raised(OSM::APIBadXMLError) { + Way.from_xml(noid, true) + } + message = assert_raise(OSM::APIBadXMLError) { + Way.from_xml(noid, false) + } + assert_match /ID is required when updating/, message.message + end + + def test_from_xml_no_changeset_id + nocs = "" + message_create = assert_raise(OSM::APIBadXMLError) { + Way.from_xml(nocs, true) + } + assert_match /Changeset id is missing/, message_create.message + message_update = assert_raise(OSM::APIBadXMLError) { + Way.from_xml(nocs, false) + } + assert_match /Changeset id is missing/, message_update.message + end + + def test_from_xml_no_version + no_version = "" + assert_nothing_raised(OSM::APIBadXMLError) { + Way.from_xml(no_version, true) + } + message_update = assert_raise(OSM::APIBadXMLError) { + Way.from_xml(no_version, false) + } + assert_match /Version is required when updating/, message_update.message + end + + def test_from_xml_id_zero + id_list = ["", "0", "00", "0.0", "a"] + id_list.each do |id| + zero_id = "" + assert_nothing_raised(OSM::APIBadUserInput) { + Way.from_xml(zero_id, true) + } + message_update = assert_raise(OSM::APIBadUserInput) { + Way.from_xml(zero_id, false) + } + assert_match /ID of way cannot be zero when updating/, message_update.message + end + end + + def test_from_xml_no_text + no_text = "" + message_create = assert_raise(OSM::APIBadXMLError) { + Way.from_xml(no_text, true) + } + assert_match /Must specify a string with one or more characters/, message_create.message + message_update = assert_raise(OSM::APIBadXMLError) { + Way.from_xml(no_text, false) + } + assert_match /Must specify a string with one or more characters/, message_create.message + end end -- 2.43.2