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

Fixed 'raw' raises by converting them to the appropriate OSM::APIError type. Made the error messages for placeholder fixing more informative. Added tests for these.

Browse source
This commit is contained in:
Matt Amos 2009-04-29 10:07:35 +00:00
parent 9acc6d3288
commit 3e9b6845d3
5 changed files with 116 additions and 8 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
app/models/node.rb
View file

@ -267,7 +267,7 @@ class Node < ActiveRecord::Base
##
# dummy method to make the interfaces of node, way and relation
# more consistent.
def fix_placeholders!(id_map)
def fix_placeholders!(id_map, placeholder_id = nil)
# nodes don't refer to anything, so there is nothing to do here
end

4
app/models/relation.rb
View file

@ -318,12 +318,12 @@ class Relation < ActiveRecord::Base
# if any members are referenced by placeholder IDs (i.e: negative) then
# this calling this method will fix them using the map from placeholders
# to IDs +id_map+.
def fix_placeholders!(id_map)
def fix_placeholders!(id_map, placeholder_id = nil)
self.members.map! do |type, id, role|
old_id = id.to_i
if old_id < 0
new_id = id_map[type.downcase.to_sym][old_id]
raise "invalid placeholder" if new_id.nil?
raise OSM::APIBadUserInput.new("Placeholder #{type} not found for reference #{old_id} in relation #{self.id.nil? ? placeholder_id : self.id}.") if new_id.nil?
[type, new_id, role]
else
[type, id, role]

4
app/models/way.rb
View file

@ -287,11 +287,11 @@ class Way < ActiveRecord::Base
# if any referenced nodes are placeholder IDs (i.e: are negative) then
# this calling this method will fix them using the map from placeholders
# to IDs +id_map+.
def fix_placeholders!(id_map)
def fix_placeholders!(id_map, placeholder_id = nil)
self.nds.map! do |node_id|
if node_id < 0
new_id = id_map[:node][node_id]
raise "invalid placeholder for #{node_id.inspect}: #{new_id.inspect}" if new_id.nil?
raise OSM::APIBadUserInput.new("Placeholder node not found for reference #{node_id} in way #{self.id.nil? ? placeholder_id : self.id}") if new_id.nil?
new_id
else
node_id

6
lib/diff_reader.rb
View file

@ -72,8 +72,8 @@ class DiffReader
def with_model
with_element do |model_name|
model = MODELS[model_name]
raise "Unexpected element type #{model_name}, " +
"expected node, way, relation." if model.nil?
raise OSM::APIBadUserInput.new("Unexpected element type #{model_name}, " +
"expected node, way or relation.") if model.nil?
yield model, @reader.expand
@reader.next
end
@ -130,7 +130,7 @@ class DiffReader
# some elements may have placeholders for other elements in the
# diff, so we must fix these before saving the element.
new.fix_placeholders!(ids)
new.fix_placeholders!(ids, placeholder_id)
# create element given user
new.create_with_history(@changeset.user)

108
test/functional/changeset_controller_test.rb
View file

@ -573,6 +573,114 @@ EOF
"shouldn't be able to re-use placeholder IDs"
end
##
# test that uploading a way referencing invalid placeholders gives a
# proper error, not a 500.
def test_upload_placeholder_invalid_way
basic_authorization "test@example.com", "test"
diff = <<EOF
<osmChange>
<create>
<node id="-1" lon="0" lat="0" changeset="2" version="1"/>
<node id="-2" lon="1" lat="1" changeset="2" version="1"/>
<node id="-3" lon="2" lat="2" changeset="2" version="1"/>
<way id="-1" changeset="2" version="1">
<nd ref="-1"/>
<nd ref="-2"/>
<nd ref="-3"/>
<nd ref="-4"/>
</way>
</create>
</osmChange>
EOF
# upload it
content diff
post :upload, :id => 2
assert_response :bad_request,
"shouldn't be able to use invalid placeholder IDs"
assert_equal "Placeholder node not found for reference -4 in way -1", @response.body
# the same again, but this time use an existing way
diff = <<EOF
<osmChange>
<create>
<node id="-1" lon="0" lat="0" changeset="2" version="1"/>
<node id="-2" lon="1" lat="1" changeset="2" version="1"/>
<node id="-3" lon="2" lat="2" changeset="2" version="1"/>
<way id="1" changeset="2" version="1">
<nd ref="-1"/>
<nd ref="-2"/>
<nd ref="-3"/>
<nd ref="-4"/>
</way>
</create>
</osmChange>
EOF
# upload it
content diff
post :upload, :id => 2
assert_response :bad_request,
"shouldn't be able to use invalid placeholder IDs"
assert_equal "Placeholder node not found for reference -4 in way 1", @response.body
end
##
# test that uploading a relation referencing invalid placeholders gives a
# proper error, not a 500.
def test_upload_placeholder_invalid_relation
basic_authorization "test@example.com", "test"
diff = <<EOF
<osmChange>
<create>
<node id="-1" lon="0" lat="0" changeset="2" version="1"/>
<node id="-2" lon="1" lat="1" changeset="2" version="1"/>
<node id="-3" lon="2" lat="2" changeset="2" version="1"/>
<relation id="-1" changeset="2" version="1">
<member type="node" role="foo" ref="-1"/>
<member type="node" role="foo" ref="-2"/>
<member type="node" role="foo" ref="-3"/>
<member type="node" role="foo" ref="-4"/>
</relation>
</create>
</osmChange>
EOF
# upload it
content diff
post :upload, :id => 2
assert_response :bad_request,
"shouldn't be able to use invalid placeholder IDs"
assert_equal "Placeholder Node not found for reference -4 in relation -1.", @response.body
# the same again, but this time use an existing way
diff = <<EOF
<osmChange>
<create>
<node id="-1" lon="0" lat="0" changeset="2" version="1"/>
<node id="-2" lon="1" lat="1" changeset="2" version="1"/>
<node id="-3" lon="2" lat="2" changeset="2" version="1"/>
<relation id="1" changeset="2" version="1">
<member type="node" role="foo" ref="-1"/>
<member type="node" role="foo" ref="-2"/>
<member type="node" role="foo" ref="-3"/>
<member type="way" role="bar" ref="-1"/>
</relation>
</create>
</osmChange>
EOF
# upload it
content diff
post :upload, :id => 2
assert_response :bad_request,
"shouldn't be able to use invalid placeholder IDs"
assert_equal "Placeholder Way not found for reference -1 in relation 1.", @response.body
end
##
# test what happens if a diff is uploaded containing only a node
# move.