From c54471ee33f79b7119eb284b03a845813a058876 Mon Sep 17 00:00:00 2001 From: Matt Amos Date: Sun, 8 Feb 2009 08:28:01 +0000 Subject: [PATCH] Fix for #1565. Added a couple of tests and fixed another issue with re-used placeholder IDs. --- lib/diff_reader.rb | 77 +++++++++++--- test/functional/changeset_controller_test.rb | 101 +++++++++++++++++++ 2 files changed, 162 insertions(+), 16 deletions(-) diff --git a/lib/diff_reader.rb b/lib/diff_reader.rb index 6a053e4ad..3b13e9462 100644 --- a/lib/diff_reader.rb +++ b/lib/diff_reader.rb @@ -22,6 +22,18 @@ class DiffReader @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. # @@ -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 - # 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 + read_or_die end ## @@ -75,14 +97,18 @@ class DiffReader # 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} + # 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" - # loop at the top level, within the element (although we - # don't actually check this...) + # loop at the top level, within the element 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? + # 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) @@ -104,7 +135,7 @@ class DiffReader 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 @@ -122,15 +153,22 @@ class DiffReader 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 - 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 - 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 @@ -144,7 +182,12 @@ class DiffReader 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? @@ -152,7 +195,9 @@ class DiffReader 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 diff --git a/test/functional/changeset_controller_test.rb b/test/functional/changeset_controller_test.rb index 524fad91b..4669df07d 100644 --- 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}" + # 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 @@ -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}" + + # 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 ## @@ -429,6 +439,97 @@ EOF 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 + + # 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 + + # 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 + + # 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. -- 2.39.5