cst1/openstreetmap-website
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

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

Browse source
This commit is contained in:
Matt Amos 2009-02-08 08:28:01 +00:00
parent 76708eefcf
commit c54471ee33
2 changed files with 162 additions and 16 deletions
Download patch file Download diff file Expand all files Collapse all files

77
lib/diff_reader.rb
View file

@ -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 <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
@ -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

101
test/functional/changeset_controller_test.rb
View file

@ -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
<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.