Change the way that the changeset parsing is done, so that it is in line with the nodes/ways/relations and gives much nicer error messages. Adding some tests too to back up those changes.

app/controllers/changeset_controller.rb

@@ -28,13 +28,10 @@ class ChangesetController < ApplicationController
 
     cs = Changeset.from_xml(request.raw_post, true)
 
-    if cs
-      cs.user_id = @user.id
-      cs.save_with_tags!
-      render :text => cs.id.to_s, :content_type => "text/plain"
-    else
-      raise OSM::APIBadXMLError.new(Changeset, request.raw_post);
-    end
+    # Assume that Node.from_xml has thrown an exception if there is an error parsing the xml
+    cs.user_id = @user.id
+    cs.save_with_tags!
+    render :text => cs.id.to_s, :content_type => "text/plain"
   end
 
   ##

app/models/changeset.rb

@@ -56,26 +56,38 @@ class Changeset < ActiveRecord::Base
       p = XML::Parser.string(xml)
       doc = p.parse
 
-      cs = Changeset.new
-
       doc.find('//osm/changeset').each do |pt|
-        if create
-          cs.created_at = Time.now.getutc
-          # initial close time is 1h ahead, but will be increased on each
-          # modification.
-          cs.closed_at = cs.created_at + IDLE_TIMEOUT
-          # initially we have no changes in a changeset
-          cs.num_changes = 0
-        end
-
-        pt.find('tag').each do |tag|
-          cs.add_tag_keyval(tag['k'], tag['v'])
-        end
+        return Changeset.from_xml_node(pt, create)
       end
-    rescue Exception => ex
-      cs = nil
+      raise OSM::APIBadXMLError.new("changeset", xml, "XML doesn't contain an osm/changeset element.")
+    rescue LibXML::XML::Error, ArgumentError => ex
+      raise OSM::APIBadXMLError.new("changeset", xml, ex.message)
+    end
+  end
+  
+  def self.from_xml_node(pt, create=false)
+    cs = Changeset.new
+    if create
+      cs.created_at = Time.now.getutc
+      # initial close time is 1h ahead, but will be increased on each
+      # modification.
+      cs.closed_at = cs.created_at + IDLE_TIMEOUT
+      # initially we have no changes in a changeset
+      cs.num_changes = 0
+    else
+      raise OSM::APIBadXMLError.new("changeset", pt, "ID is required when updating.") if pt['id'].nil?
+      cs.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 changeset cannot be zero when updating.") if cs.id == 0
     end
 
+    pt.find('tag').each do |tag|
+      raise OSM::APIBadXMLError.new("changeset", pt, "tag is missing key") if tag['k'].nil?
+      raise OSM::APIBadXMLError.new("changeset", pt, "tag is missing value") if tag['v'].nil?
+      cs.add_tag_keyval(tag['k'], tag['v'])
+    end
+    
     return cs
   end
 
@@ -150,6 +162,11 @@ class Changeset < ActiveRecord::Base
 
   def add_tag_keyval(k, v)
     @tags = Hash.new unless @tags
+
+    # duplicate tags are now forbidden, so we can't allow values
+    # in the hash to be overwritten.
+    raise OSM::APIDuplicateTagsError.new("changeset", self.id, k) if @tags.include? k
+
     @tags[k] = v
   end
 

test/unit/changeset_test.rb

@@ -7,4 +7,77 @@ class ChangesetTest < ActiveSupport::TestCase
     assert_equal 7, Changeset.count
   end
   
+  def test_xml_from_id_zero
+    id_list = ["", "0", "00", "0.0", "a"]
+    id_list.each do |id|
+      zero_id = "<osm><changeset id='#{id}' /></osm>"
+      assert_nothing_raised(OSM::APIBadUserInput) {
+        Changeset.from_xml(zero_id, true)
+      }
+      message_update = assert_raise(OSM::APIBadUserInput) {
+        Changeset.from_xml(zero_id, false)
+      }
+      assert_match /ID of changeset cannot be zero when updating/, message_update.message
+    end
+  end
+  
+  def test_from_xml_no_text
+    no_text = ""
+    message_create = assert_raise(OSM::APIBadXMLError) {
+      Changeset.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) {
+      Changeset.from_xml(no_text, false)
+    }
+    assert_match /Must specify a string with one or more characters/, message_update.message
+  end
+  
+  def test_from_xml_no_changeset
+    nocs = "<osm></osm>"
+    message_create = assert_raise(OSM::APIBadXMLError) {
+      Changeset.from_xml(nocs, true)
+    }
+    assert_match /XML doesn't contain an osm\/changeset element/, message_create.message
+    message_update = assert_raise(OSM::APIBadXMLError) {
+      Changeset.from_xml(nocs, false)
+    }
+    assert_match /XML doesn't contain an osm\/changeset element/, message_update.message
+  end
+  
+  def test_from_xml_no_k_v
+    nokv = "<osm><changeset><tag /></changeset></osm>"
+    message_create = assert_raise(OSM::APIBadXMLError) {
+      Changeset.from_xml(nokv, true)
+    }
+    assert_match /tag is missing key/, message_create.message
+    message_update = assert_raise(OSM::APIBadXMLError) {
+      Changeset.from_xml(nokv, false)
+    }
+    assert_match /tag is missing key/, message_create.message
+  end
+  
+  def test_from_xml_no_v
+    no_v = "<osm><changeset id='1'><tag k='key' /></changeset></osm>"
+    message_create = assert_raise(OSM::APIBadXMLError) {
+      Changeset.from_xml(no_v, true)
+    }
+    assert_match /tag is missing value/, message_create.message
+    message_update = assert_raise(OSM::APIBadXMLError) {
+      Changeset.from_xml(no_v, false)
+    }
+    assert_match /tag is missing value/, message_update.message
+  end
+  
+  def test_from_xml_duplicate_k
+    dupk = "<osm><changeset id='1'><tag k='dup' v='test' /><tag k='dup' v='value' /></changeset></osm>"
+    message_create = assert_raise(OSM::APIDuplicateTagsError) {
+      Changeset.from_xml(dupk, true)
+    }
+    assert_equal "Element changeset/ has duplicate tags with key dup", message_create.message
+    message_update = assert_raise(OSM::APIDuplicateTagsError) {
+      Changeset.from_xml(dupk, false)
+    }
+    assert_equal "Element changeset/1 has duplicate tags with key dup", message_update.message
+  end
 end

test/unit/node_test.rb

@@ -270,6 +270,18 @@ class NodeTest < ActiveSupport::TestCase
     assert_match /Must specify a string with one or more characters/, message_update.message
   end
   
+  def test_from_xml_no_node
+    no_node = "<osm></osm>"
+    message_create = assert_raise(OSM::APIBadXMLError) {
+      Node.from_xml(no_node, true)
+    }
+    assert_match /XML doesn't contain an osm\/node element/, message_create.message
+    message_update = assert_raise(OSM::APIBadXMLError) {
+      Node.from_xml(no_node, false)
+    }
+    assert_match /XML doesn't contain an osm\/node element/, message_update.message
+  end
+  
   def test_from_xml_no_k_v
     nokv = "<osm><node id='23' lat='12.3' lon='23.4' changeset='12' version='23'><tag /></node></osm>"
     message_create = assert_raise(OSM::APIBadXMLError) {