Fix for #1565. Added a couple of tests and fixed another issue with re-used placehold...

diff --git a/lib/diff_reader.rb b/lib/diff_reader.rb
index 6a053e4adc19dc8a010d6744da1a125b47314e25..3b13e94626cc301f941a103a8df812a1b7fc831a 100644 (file)
--- a/lib/diff_reader.rb
+++ b/lib/diff_reader.rb
@@ -22,6 +22,18 @@ class DiffReader
     @changeset = changeset
   end
 
     @changeset = changeset
   end
 

+  ##
+  # Reads the next element from the XML document. Checks the return value
+  # and throws an exception if an error occurred.
+  def read_or_die
+    # NOTE: XML::Reader#read returns 0 for EOF and -1 for error.
+    # we allow an EOF because we are expecting this to always happen
+    # at the end of a document.
+    if @reader.read < 0
+      raise APIBadUserInput.new("Unexpected end of XML document.")
+    end
+  end
+

   ##
   # An element-block mapping for using the LibXML reader interface. 
   #
   ##
   # An element-block mapping for using the LibXML reader interface. 
   #


@@ -29,15 +41,25 @@ class DiffReader
   # elements, it would be better to DRY and do this in a block. This
   # could also help with error handling...?
   def with_element
   # elements, it would be better to DRY and do this in a block. This
   # could also help with error handling...?
   def with_element

-    # skip the first element, which is our opening element of the block
-    @reader.read
-    # loop over all elements. 
-    # NOTE: XML::Reader#read returns 0 for EOF and -1 for error.
-    while @reader.read == 1
-      break if @reader.node_type == 15 # end element
-      next unless @reader.node_type == 1 # element
-      yield @reader.name
+    # if the start element is empty then don't do any processing, as
+    # there won't be any child elements to process!
+    unless @reader.empty_element?
+      # read the first element
+      read_or_die
+
+      begin
+        # because we read elements in DOM-style to reuse their DOM
+        # parsing code, we don't always read an element on each pass
+        # as the call to @reader.next in the innermost loop will take
+        # care of that for us.
+        if @reader.node_type == 1 # element
+          yield @reader.name
+        else
+          read_or_die
+        end
+      end while @reader.node_type != 15 # end element

     end
     end

+    read_or_die

   end
 
   ##
   end
 
   ##


@@ -75,14 +97,18 @@ class DiffReader
   # an exception subclassing OSM::APIError will be thrown.
   def commit
 
   # an exception subclassing OSM::APIError will be thrown.
   def commit
 

+    # data structure used for mapping placeholder IDs to real IDs

     node_ids, way_ids, rel_ids = {}, {}, {}
     ids = { :node => node_ids, :way => way_ids, :relation => rel_ids}
 
     node_ids, way_ids, rel_ids = {}, {}, {}
     ids = { :node => node_ids, :way => way_ids, :relation => rel_ids}
 

+    # take the first element and check that it is an osmChange element
+    @reader.read
+    raise APIBadUserInput.new("Document element should be 'osmChange'.") if @reader.name != 'osmChange'
+

     result = OSM::API.new.get_xml_doc
     result.root.name = "diffResult"
 
     result = OSM::API.new.get_xml_doc
     result.root.name = "diffResult"
 

-    # loop at the top level, within the <osmChange> element (although we
-    # don't actually check this...)
+    # loop at the top level, within the <osmChange> element

     with_element do |action_name|
       if action_name == 'create'
         # create a new element. this code is agnostic of the element type
     with_element do |action_name|
       if action_name == 'create'
         # create a new element. this code is agnostic of the element type


@@ -96,6 +122,11 @@ class DiffReader
           placeholder_id = xml['id'].to_i
           raise OSM::APIBadXMLError.new(model, xml) if placeholder_id.nil?
 
           placeholder_id = xml['id'].to_i
           raise OSM::APIBadXMLError.new(model, xml) if placeholder_id.nil?
 

+          # check if the placeholder ID has been given before and throw
+          # an exception if it has - we can't create the same element twice.
+          model_sym = model.to_s.downcase.to_sym
+          raise OSM::APIBadUserInput.new("Placeholder IDs must be unique for created elements.") if ids[model_sym].include? placeholder_id
+

           # some elements may have placeholders for other elements in the
           # diff, so we must fix these before saving the element.
           new.fix_placeholders!(ids)
           # some elements may have placeholders for other elements in the
           # diff, so we must fix these before saving the element.
           new.fix_placeholders!(ids)


@@ -104,7 +135,7 @@ class DiffReader
           new.create_with_history(@changeset.user)
           
           # save placeholder => allocated ID map
           new.create_with_history(@changeset.user)
           
           # save placeholder => allocated ID map

-          ids[model.to_s.downcase.to_sym][placeholder_id] = new.id
+          ids[model_sym][placeholder_id] = new.id

 
           # add the result to the document we're building for return.
           xml_result = XML::Node.new model.to_s.downcase
 
           # add the result to the document we're building for return.
           xml_result = XML::Node.new model.to_s.downcase


@@ -122,15 +153,22 @@ class DiffReader
           new = model.from_xml_node(xml, false)
           check(model, xml, new)
 
           new = model.from_xml_node(xml, false)
           check(model, xml, new)
 

+          # if the ID is a placeholder then map it to the real ID
+          model_sym = model.to_s.downcase.to_sym
+          is_placeholder = ids[model_sym].include? new.id
+          id = is_placeholder ? ids[model_sym][new.id] : new.id
+

           # and the old one from the database
           # and the old one from the database

-          old = model.find(new.id)
+          old = model.find(id)

 
           new.fix_placeholders!(ids)
           old.update_from(new, @changeset.user)
 
           xml_result = XML::Node.new model.to_s.downcase
 
           new.fix_placeholders!(ids)
           old.update_from(new, @changeset.user)
 
           xml_result = XML::Node.new model.to_s.downcase

-          xml_result["old_id"] = old.id.to_s
-          xml_result["new_id"] = new.id.to_s 
+          # oh, the irony... the "new" element actually contains the "old" ID
+          # a better name would have been client/server, but anyway...
+          xml_result["old_id"] = new.id.to_s
+          xml_result["new_id"] = id.to_s 

           # version is updated in "old" through the update, so we must not
           # return new.version here but old.version!
           xml_result["new_version"] = old.version.to_s
           # version is updated in "old" through the update, so we must not
           # return new.version here but old.version!
           xml_result["new_version"] = old.version.to_s


@@ -144,7 +182,12 @@ class DiffReader
           new = model.from_xml_node(xml, false)
           check(model, xml, new)
 
           new = model.from_xml_node(xml, false)
           check(model, xml, new)
 

-          old = model.find(new.id)
+          # if the ID is a placeholder then map it to the real ID
+          model_sym = model.to_s.downcase.to_sym
+          is_placeholder = ids[model_sym].include? new.id
+          id = is_placeholder ? ids[model_sym][new.id] : new.id
+
+          old = model.find(id)

 
           # can a delete have placeholders under any circumstances?
           # if a way is modified, then deleted is that a valid diff?
 
           # can a delete have placeholders under any circumstances?
           # if a way is modified, then deleted is that a valid diff?


@@ -152,7 +195,9 @@ class DiffReader
           old.delete_with_history!(new, @changeset.user)
 
           xml_result = XML::Node.new model.to_s.downcase
           old.delete_with_history!(new, @changeset.user)
 
           xml_result = XML::Node.new model.to_s.downcase

-          xml_result["old_id"] = old.id.to_s
+          # oh, the irony... the "new" element actually contains the "old" ID
+          # a better name would have been client/server, but anyway...
+          xml_result["old_id"] = new.id.to_s

           result.root << xml_result
         end
 
           result.root << xml_result
         end
 

diff --git a/test/functional/changeset_controller_test.rb b/test/functional/changeset_controller_test.rb
index 524fad91b922894f2c3f3c98240d35deac921936..4669df07d4ed96d0714d349ea41721b7c4392e3f 100644 (file)
--- a/test/functional/changeset_controller_test.rb
+++ b/test/functional/changeset_controller_test.rb
@@ -205,6 +205,11 @@ EOF
     assert_response :success, 
       "can't upload a deletion diff to changeset: #{@response.body}"
 
     assert_response :success, 
       "can't upload a deletion diff to changeset: #{@response.body}"
 

+    # check the response is well-formed
+    assert_select "diffResult>node", 1
+    assert_select "diffResult>way", 1
+    assert_select "diffResult>relation", 2
+

     # check that everything was deleted
     assert_equal false, Node.find(current_nodes(:node_used_by_relationship).id).visible
     assert_equal false, Way.find(current_ways(:used_way).id).visible
     # check that everything was deleted
     assert_equal false, Node.find(current_nodes(:node_used_by_relationship).id).visible
     assert_equal false, Way.find(current_ways(:used_way).id).visible


@@ -367,6 +372,11 @@ EOF
     post :upload, :id => 1
     assert_response :success, 
       "can't upload multiple versions of an element in a diff: #{@response.body}"
     post :upload, :id => 1
     assert_response :success, 
       "can't upload multiple versions of an element in a diff: #{@response.body}"

+    
+    # check the response is well-formed. its counter-intuitive, but the
+    # API will return multiple elements with the same ID and different
+    # version numbers for each change we made.
+    assert_select "diffResult>node", 8

   end
 
   ##
   end
 
   ##


@@ -429,6 +439,97 @@ EOF
   assert_equal @response.body, "Unknown action ping, choices are create, modify, delete."
   end
 
   assert_equal @response.body, "Unknown action ping, choices are create, modify, delete."
   end
 

+  ##
+  # upload a valid changeset which has a mixture of whitespace
+  # to check a bug reported by ivansanchez (#1565).
+  def test_upload_whitespace_valid
+    basic_authorization "test@openstreetmap.org", "test"
+
+    diff = <<EOF
+<osmChange>
+ <modify><node id='1' lon='0' lat='0' changeset='1' 
+  version='1'></node>
+  <node id='1' lon='1' lat='1' changeset='1' version='2'><tag k='k' v='v'/></node></modify>
+ <modify>
+  <relation id='1' changeset='1' version='1'><member 
+   type='way' role='some' ref='3'/><member 
+    type='node' role='some' ref='5'/>
+   <member type='relation' role='some' ref='3'/>
+  </relation>
+ </modify></osmChange>
+EOF
+
+    # upload it
+    content diff
+    post :upload, :id => 1
+    assert_response :success, 
+      "can't upload a valid diff with whitespace variations to changeset: #{@response.body}"
+
+    # check the response is well-formed
+    assert_select "diffResult>node", 2
+    assert_select "diffResult>relation", 1
+
+    # check that the changes made it into the database
+    assert_equal 1, Node.find(1).tags.size, "node 1 should now have one tag"
+    assert_equal 0, Relation.find(1).tags.size, "relation 1 should now have no tags"
+  end
+
+  ##
+  # upload a valid changeset which has a mixture of whitespace
+  # to check a bug reported by ivansanchez.
+  def test_upload_reuse_placeholder_valid
+    basic_authorization "test@openstreetmap.org", "test"
+
+    diff = <<EOF
+<osmChange>
+ <create>
+  <node id='-1' lon='0' lat='0' changeset='1'>
+   <tag k="foo" v="bar"/>
+  </node>
+ </create>
+ <modify>
+  <node id='-1' lon='1' lat='1' changeset='1' version='1'/>
+ </modify>
+ <delete>
+  <node id='-1' lon='2' lat='2' changeset='1' version='2'/>
+ </delete>
+</osmChange>
+EOF
+
+    # upload it
+    content diff
+    post :upload, :id => 1
+    assert_response :success, 
+      "can't upload a valid diff with re-used placeholders to changeset: #{@response.body}"
+
+    # check the response is well-formed
+    assert_select "diffResult>node", 3
+    assert_select "diffResult>node[old_id=-1]", 3
+  end
+
+  ##
+  # test what happens if a diff upload re-uses placeholder IDs in an
+  # illegal way.
+  def test_upload_placeholder_invalid
+    basic_authorization "test@openstreetmap.org", "test"
+
+    diff = <<EOF
+<osmChange>
+ <create>
+  <node id='-1' lon='0' lat='0' changeset='1' version='1'/>
+  <node id='-1' lon='1' lat='1' changeset='1' version='1'/>
+  <node id='-1' lon='2' lat='2' changeset='1' version='2'/>
+ </create>
+</osmChange>
+EOF
+
+    # upload it
+    content diff
+    post :upload, :id => 1
+    assert_response :bad_request, 
+      "shouldn't be able to re-use placeholder IDs"
+  end
+

   ##
   # when we make some simple changes we get the same changes back from the 
   # diff download.
   ##
   # when we make some simple changes we get the same changes back from the 
   # diff download.