From: Matt Amos Date: Wed, 29 Apr 2009 10:07:35 +0000 (+0000) Subject: Fixed 'raw' raises by converting them to the appropriate OSM::APIError type. Made... X-Git-Tag: live~8069 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/3e9b6845d34f9cd80f9eb1099f76f7801bfae76f 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. --- diff --git a/app/models/node.rb b/app/models/node.rb index bd51d6c50..2c25bbd14 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -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 diff --git a/app/models/relation.rb b/app/models/relation.rb index 37969ee76..08b77f449 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -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] diff --git a/app/models/way.rb b/app/models/way.rb index 52d280209..325ffae48 100644 --- a/app/models/way.rb +++ b/app/models/way.rb @@ -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 diff --git a/lib/diff_reader.rb b/lib/diff_reader.rb index 217e9309d..be48f8b4e 100644 --- a/lib/diff_reader.rb +++ b/lib/diff_reader.rb @@ -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) diff --git a/test/functional/changeset_controller_test.rb b/test/functional/changeset_controller_test.rb index 932d685bb..b50e1b54a 100644 --- a/test/functional/changeset_controller_test.rb +++ b/test/functional/changeset_controller_test.rb @@ -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 + + # 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 + + # 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 + + # 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 + + # 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.