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

Fixed up delete methods on nodes, ways and relations to return the new version number and added some more tests.

Browse source
This commit is contained in:
Matt Amos 2008-10-14 14:27:12 +00:00
parent 2c16177174
commit b56f57ec43
10 changed files with 100 additions and 36 deletions
Download patch file Download diff file Expand all files Collapse all files

14
app/controllers/node_controller.rb
View file

@ -78,21 +78,17 @@ class NodeController < ApplicationController
end
end
# Delete a node. Doesn't actually delete it, but retains its history in a wiki-like way.
# FIXME remove all the fricking SQL
# Delete a node. Doesn't actually delete it, but retains its history
# in a wiki-like way. We therefore treat it like an update, so the delete
# method returns the new version number.
def delete
begin
node = Node.find(params[:id])
new_node = Node.from_xml(request.raw_post)
# FIXME we no longer care about the user, (or maybe we want to check
# that the user of the changeset is the same user as is making this
# little change?) we really care about the
# changeset which must be open, and that the version that we have been
# given is the one that is currently stored in the database
if new_node and new_node.id == node.id
node.delete_with_history(new_node, @user)
render :nothing => true
node.delete_with_history!(new_node, @user)
render :text => node.version.to_s, :content_type => "text/plain"
else
render :nothing => true, :status => :bad_request
end

4
app/controllers/relation_controller.rb
View file

@ -67,8 +67,8 @@ class RelationController < ApplicationController
relation = Relation.find(params[:id])
new_relation = Relation.from_xml(request.raw_post)
if new_relation and new_relation.id == relation.id
relation.delete_with_history(new_relation, @user)
render :nothing => true
relation.delete_with_history!(new_relation, @user)
render :text => relation.version.to_s, :content_type => "text/plain"
else
render :nothing => true, :status => :bad_request
end

7
app/controllers/way_controller.rb
View file

@ -67,11 +67,10 @@ class WayController < ApplicationController
begin
way = Way.find(params[:id])
new_way = Way.from_xml(request.raw_post)
if new_way and new_way.id == way.id
way.delete_with_history(new_way, @user)
# if we get here, all is fine, otherwise something will catch below.
render :nothing => true
if new_way and new_way.id == way.id
way.delete_with_history!(new_way, @user)
render :text => way.version.to_s, :content_type => "text/plain"
else
render :nothing => true, :status => :bad_request
end

2
app/models/node.rb
View file

@ -133,7 +133,7 @@ class Node < ActiveRecord::Base
end
# Should probably be renamed delete_from to come in line with update
def delete_with_history(new_node, user)
def delete_with_history!(new_node, user)
if self.visible
check_consistency(self, new_node, user)
if WayNode.find(:first, :joins => "INNER JOIN current_ways ON current_ways.id = current_way_nodes.id", :conditions => [ "current_ways.visible = 1 AND current_way_nodes.node_id = ?", self.id ])

8
app/models/relation.rb
View file

@ -224,14 +224,12 @@ class Relation < ActiveRecord::Base
end
end
def delete_with_history(new_relation, user)
def delete_with_history!(new_relation, user)
if self.visible
check_consistency(self, new_relation, user)
if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='relation' and member_id=?", self.id ])
if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='relation' and member_id=? ", self.id ])
raise OSM::APIPreconditionFailedError.new
else
#self.user_id = user.id
# FIXME we need to deal with changeset here, which is probably already dealt with
self.changeset_id = new_relation.changeset_id
self.tags = []
self.members = []
@ -248,8 +246,6 @@ class Relation < ActiveRecord::Base
if !new_relation.preconditions_ok?
raise OSM::APIPreconditionFailedError.new
end
# FIXME need to deal with changeset etc
#self.user_id = user.id
self.changeset_id = new_relation.changeset_id
self.tags = new_relation.tags
self.members = new_relation.members

11
app/models/way.rb
View file

@ -245,19 +245,12 @@ class Way < ActiveRecord::Base
return true
end
def delete_with_history(new_way, user)
def delete_with_history!(new_way, user)
check_consistency(self, new_way, user)
if self.visible
# FIXME
# this should actually delete the relations,
# not just throw a PreconditionFailed if it's a member of a relation!!
# WHY?? The editor should decide whether the node is in the relation or not!
# FIXME: this should probably renamed to delete_with_history
if RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id",
:conditions => [ "visible = 1 AND member_type='way' and member_id=?", self.id])
:conditions => [ "visible = 1 AND member_type='way' and member_id=? ", self.id])
raise OSM::APIPreconditionFailedError
# end FIXME
else
self.changeset_id = new_way.changeset_id
self.tags = []

15
lib/osm.rb
View file

@ -68,6 +68,21 @@ module OSM
end
end
# raised when a two tags have a duplicate key string in an element.
# this is now forbidden by the API.
class APIDuplicateTagsError < APIError
def initialize(type, id, tag_key)
@type, @id, @tag_key = type, id, tag_key
end
attr_reader :type, :id, :tag_key
def render_opts
{ :text => "Element #{@type}/#{@id} has duplicate tags with key #{@tag_key}.",
:status => :bad_request }
end
end
# Helper methods for going to/from mercator and lat/lng.
class Mercator
include Math

42
test/functional/node_controller_test.rb
View file

@ -79,6 +79,11 @@ class NodeControllerTest < Test::Unit::TestCase
delete :delete, :id => current_nodes(:visible_node).id
assert_response :success
# valid delete should return the new version number, which should
# be greater than the old version number
assert @response.body.to_i > current_nodes(:visible_node).version,
"delete request should return a new version number for node"
# this won't work since the node is already deleted
content(nodes(:invisible_node).to_xml)
delete :delete, :id => current_nodes(:invisible_node).id
@ -92,12 +97,14 @@ class NodeControllerTest < Test::Unit::TestCase
# in a way...
content(nodes(:used_node_1).to_xml)
delete :delete, :id => current_nodes(:used_node_1).id
assert_response :precondition_failed
assert_response :precondition_failed,
"shouldn't be able to delete a node used in a way (#{@response.body})"
# in a relation...
content(nodes(:node_used_by_relationship).to_xml)
delete :delete, :id => current_nodes(:node_used_by_relationship).id
assert_response :precondition_failed
assert_response :precondition_failed,
"shouldn't be able to delete a node used in a relation (#{@response.body})"
end
##
@ -197,7 +204,36 @@ class NodeControllerTest < Test::Unit::TestCase
put :update, :id => current_nodes(:visible_node).id
assert_response :bad_request,
"adding duplicate tags to a node should fail with 'bad request'"
end
end
# test whether string injection is possible
def test_string_injection
basic_authorization(users(:normal_user).email, "test")
changeset_id = changesets(:normal_user_first_change).id
# try and put something into a string that the API might
# use unquoted and therefore allow code injection...
content "<osm><node lat='0' lon='0' changeset='#{changeset_id}'>" +
'<tag k="#{@user.inspect}" v="0"/>' +
'</node></osm>'
put :create
assert_response :success
nodeid = @response.body
# find the node in the database
checknode = Node.find(nodeid)
assert_not_nil checknode, "node not found in data base after upload"
# and grab it using the api
get :read, :id => nodeid
assert_response :success
apinode = Node.from_xml(@response.body)
assert_not_nil apinode, "downloaded node is nil, but shouldn't be"
# check the tags are not corrupted
assert_equal checknode.tags, apinode.tags
assert apinode.tags.include?('#{@user.inspect}')
end
def basic_authorization(user, pass)
@request.env["HTTP_AUTHORIZATION"] = "Basic %s" % Base64.encode64("#{user}:#{pass}")

18
test/functional/relation_controller_test.rb
View file

@ -210,16 +210,34 @@ class RelationControllerTest < Test::Unit::TestCase
delete :delete, :id => current_relations(:visible_relation).id
assert_response :bad_request
# this won't work because the relation is in-use by another relation
content(relations(:used_relation).to_xml)
delete :delete, :id => current_relations(:used_relation).id
assert_response :precondition_failed,
"shouldn't be able to delete a relation used in a relation (#{@response.body})"
# this should work when we provide the appropriate payload...
content(relations(:visible_relation).to_xml)
delete :delete, :id => current_relations(:visible_relation).id
assert_response :success
# valid delete should return the new version number, which should
# be greater than the old version number
assert @response.body.to_i > current_relations(:visible_relation).version,
"delete request should return a new version number for relation"
# this won't work since the relation is already deleted
content(relations(:invisible_relation).to_xml)
delete :delete, :id => current_relations(:invisible_relation).id
assert_response :gone
# this works now because the relation which was using this one
# has been deleted.
content(relations(:used_relation).to_xml)
delete :delete, :id => current_relations(:used_relation).id
assert_response :success,
"should be able to delete a relation used in an old relation (#{@response.body})"
# this won't work since the relation never existed
delete :delete, :id => 0
assert_response :not_found

15
test/functional/way_controller_test.rb
View file

@ -165,17 +165,28 @@ class WayControllerTest < Test::Unit::TestCase
delete :delete, :id => current_ways(:visible_way).id
assert_response :bad_request
# Now try and get a changeset
changeset_id = changesets(:normal_user_first_change).id
# Now try with a valid changeset
content current_ways(:visible_way).to_xml
delete :delete, :id => current_ways(:visible_way).id
assert_response :success
# check the returned value - should be the new version number
# valid delete should return the new version number, which should
# be greater than the old version number
assert @response.body.to_i > current_ways(:visible_way).version,
"delete request should return a new version number for way"
# this won't work since the way is already deleted
content current_ways(:invisible_way).to_xml
delete :delete, :id => current_ways(:invisible_way).id
assert_response :gone
# this shouldn't work as the way is used in a relation
content current_ways(:used_way).to_xml
delete :delete, :id => current_ways(:used_way).id
assert_response :precondition_failed,
"shouldn't be able to delete a way used in a relation (#{@response.body})"
# this won't work since the way never existed
delete :delete, :id => 0
assert_response :not_found