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

@@ -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'])

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

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 = "<osm><node id='#{id}' lat='12.3' lon='12.3' changeset='33' version='23' /></osm>"
+      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) {

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 = "<osm><way version='12' changeset='23' /></osm>"
+    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 = "<osm><way id='123' version='23' /></osm>"
+    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 = "<osm><way id='123' changeset='23' /></osm>"
+    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 = "<osm><way id='#{id}' changeset='33' version='23' /></osm>"
+      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