Added a bunch more tests on the API 0.6. Fixed node/way/relation from_xml code to...

diff --git a/app/controllers/node_controller.rb b/app/controllers/node_controller.rb
index 309b930b7f89af7344e160b25d5af9dd0599f144..f1023e78fd2de7669193b4ddb0911cbf9b6f3e63 100644 (file)
--- a/app/controllers/node_controller.rb
+++ b/app/controllers/node_controller.rb
@@ -71,9 +71,8 @@ class NodeController < ApplicationController
       else
         render :nothing => true, :status => :bad_request
       end
-    rescue OSM::APIVersionMismatchError => ex
-      render :text => "Version mismatch: Provided " + ex.provided.to_s +
-        ", server had: " + ex.latest.to_s, :status => :bad_request
+    rescue OSM::APIError => ex
+      render ex.render_opts
     rescue ActiveRecord::RecordNotFound
       render :nothing => true, :status => :not_found
     end

diff --git a/app/models/node.rb b/app/models/node.rb
index d6a5143db1c3ec94263ef701177bc25201a28d89..3b59ac80ea558960ade0cb97437da82f639d88d5 100644 (file)
--- a/app/models/node.rb
+++ b/app/models/node.rb
@@ -75,7 +75,7 @@ class Node < ActiveRecord::Base
   def self.from_xml_node(pt, create=false)
     node = Node.new
     
-    node.version = pt['version']
+    node.version = pt['version'].to_i
     node.lat = pt['lat'].to_f
     node.lon = pt['lon'].to_f
     node.changeset_id = pt['changeset'].to_i
@@ -138,7 +138,7 @@ class Node < ActiveRecord::Base
       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 ])
         raise OSM::APIPreconditionFailedError.new
-      elsif RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='node' and member_id=?", self.id])
+      elsif RelationMember.find(:first, :joins => "INNER JOIN current_relations ON current_relations.id=current_relation_members.id", :conditions => [ "visible = 1 AND member_type='node' and member_id=? ", self.id])
         raise OSM::APIPreconditionFailedError.new
       else
         self.changeset_id = new_node.changeset_id
@@ -227,6 +227,11 @@ class Node < ActiveRecord::Base
 
   def add_tag_key_val(k,v)
     @tags = Hash.new unless @tags
+
+    # duplicate tags are now forbidden, so we can't allow values
+    # in the hash to be overwritten.
+    raise OSM::APIDuplicateTagsError.new if @tags.include? k
+
     @tags[k] = v
   end
 

diff --git a/app/models/relation.rb b/app/models/relation.rb
index 195090e4b1f2755593773bc1fafb4755c0f37e6d..436c1e32ef8693bf068429743000b38cfe8f4546 100644 (file)
--- a/app/models/relation.rb
+++ b/app/models/relation.rb
@@ -182,6 +182,11 @@ class Relation < ActiveRecord::Base
 
   def add_tag_keyval(k, v)
     @tags = Hash.new unless @tags
+
+    # duplicate tags are now forbidden, so we can't allow values
+    # in the hash to be overwritten.
+    raise OSM::APIDuplicateTagsError.new if @tags.include? k
+
     @tags[k] = v
   end
 

diff --git a/app/models/way.rb b/app/models/way.rb
index f26d7658b14589eb0b81187c2cb42882db37b9a2..90458006e8cf15a64a85bd797a5bf6a4116d2130 100644 (file)
--- a/app/models/way.rb
+++ b/app/models/way.rb
@@ -169,6 +169,11 @@ class Way < ActiveRecord::Base
 
   def add_tag_keyval(k, v)
     @tags = Hash.new unless @tags
+
+    # duplicate tags are now forbidden, so we can't allow values
+    # in the hash to be overwritten.
+    raise OSM::APIDuplicateTagsError.new if @tags.include? k
+
     @tags[k] = v
   end
 

diff --git a/test/functional/node_controller_test.rb b/test/functional/node_controller_test.rb
index a8ef700a3aba3f364c33e07b2541656297052ae1..085cd3078f6d28d275f76d9f7f397eeea68df1ed 100644 (file)
--- a/test/functional/node_controller_test.rb
+++ b/test/functional/node_controller_test.rb
@@ -16,7 +16,6 @@ class NodeControllerTest < Test::Unit::TestCase
   def test_create
     # cannot read password from fixture as it is stored as MD5 digest
     basic_authorization(users(:normal_user).email, "test");
-    # FIXME we need to create a changeset first argh
     
     # create a node with random lat/lon
     lat = rand(100)-50 + rand
@@ -57,7 +56,6 @@ class NodeControllerTest < Test::Unit::TestCase
   # this tests deletion restrictions - basic deletion is tested in the unit
   # tests for node!
   def test_delete
-
     # first try to delete node without auth
     delete :delete, :id => current_nodes(:visible_node).id
     assert_response :unauthorized
@@ -65,7 +63,18 @@ class NodeControllerTest < Test::Unit::TestCase
     # now set auth
     basic_authorization(users(:normal_user).email, "test");  
 
-    # delete now takes a payload
+    # try to delete with an invalid (closed) changeset
+    content update_changeset(current_nodes(:visible_node).to_xml,
+                             changesets(:normal_user_closed_change).id)
+    delete :delete, :id => current_nodes(:visible_node).id
+    assert_response :conflict
+
+    # try to delete with an invalid (non-existent) changeset
+    content update_changeset(current_nodes(:visible_node).to_xml,0)
+    delete :delete, :id => current_nodes(:visible_node).id
+    assert_response :conflict
+
+    # valid delete now takes a payload
     content(nodes(:visible_node).to_xml)
     delete :delete, :id => current_nodes(:visible_node).id
     assert_response :success
@@ -79,12 +88,116 @@ class NodeControllerTest < Test::Unit::TestCase
     delete :delete, :id => 0
     assert_response :not_found
 
-    # this won't work since the node is in use
+    ## these test whether nodes which are in-use can be deleted:
+    # in a way...
     content(nodes(:used_node_1).to_xml)
     delete :delete, :id => current_nodes(:used_node_1).id
     assert_response :precondition_failed
+
+    # 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
+  end
+
+  ##
+  # tests whether the API works and prevents incorrect use while trying
+  # to update nodes.
+  def test_update
+    # try and update a node without authorisation
+    # first try to delete node without auth
+    content current_nodes(:visible_node).to_xml
+    put :update, :id => current_nodes(:visible_node).id
+    assert_response :unauthorized
+    
+    # setup auth
+    basic_authorization(users(:normal_user).email, "test")
+
+    ## trying to break changesets
+
+    # try and update in someone else's changeset
+    content update_changeset(current_nodes(:visible_node).to_xml,
+                             changesets(:second_user_first_change).id)
+    put :update, :id => current_nodes(:visible_node).id
+    assert_response :conflict, "update with other user's changeset should be rejected"
+
+    # try and update in a closed changeset
+    content update_changeset(current_nodes(:visible_node).to_xml,
+                             changesets(:normal_user_closed_change).id)
+    put :update, :id => current_nodes(:visible_node).id
+    assert_response :conflict, "update with closed changeset should be rejected"
+
+    # try and update in a non-existant changeset
+    content update_changeset(current_nodes(:visible_node).to_xml, 0)
+    put :update, :id => current_nodes(:visible_node).id
+    assert_response :conflict, "update with changeset=0 should be rejected"
+
+    ## try and submit invalid updates
+    content xml_attr_rewrite(current_nodes(:visible_node).to_xml, 'lat', 91.0);
+    put :update, :id => current_nodes(:visible_node).id
+    assert_response :bad_request, "node at lat=91 should be rejected"
+
+    content xml_attr_rewrite(current_nodes(:visible_node).to_xml, 'lat', -91.0);
+    put :update, :id => current_nodes(:visible_node).id
+    assert_response :bad_request, "node at lat=-91 should be rejected"
+    
+    content xml_attr_rewrite(current_nodes(:visible_node).to_xml, 'lon', 181.0);
+    put :update, :id => current_nodes(:visible_node).id
+    assert_response :bad_request, "node at lon=181 should be rejected"
+
+    content xml_attr_rewrite(current_nodes(:visible_node).to_xml, 'lon', -181.0);
+    put :update, :id => current_nodes(:visible_node).id
+    assert_response :bad_request, "node at lon=-181 should be rejected"
+
+    ## next, attack the versioning
+    current_node_version = current_nodes(:visible_node).version
+
+    # try and submit a version behind
+    content xml_attr_rewrite(current_nodes(:visible_node).to_xml, 
+                             'version', current_node_version - 1);
+    put :update, :id => current_nodes(:visible_node).id
+    assert_response :conflict, "should have failed on old version number"
+    
+    # try and submit a version ahead
+    content xml_attr_rewrite(current_nodes(:visible_node).to_xml, 
+                             'version', current_node_version + 1);
+    put :update, :id => current_nodes(:visible_node).id
+    assert_response :conflict, "should have failed on skipped version number"
+
+    # try and submit total crap in the version field
+    content xml_attr_rewrite(current_nodes(:visible_node).to_xml, 
+                             'version', 'p1r4t3s!');
+    put :update, :id => current_nodes(:visible_node).id
+    assert_response :conflict, 
+       "should not be able to put 'p1r4at3s!' in the version field"
+    
+    ## finally, produce a good request which should work
+    content current_nodes(:visible_node).to_xml
+    put :update, :id => current_nodes(:visible_node).id
+    assert_response :success, "a valid update request failed"
   end
 
+  ##
+  # test adding tags to a node
+  def test_duplicate_tags
+    # setup auth
+    basic_authorization(users(:normal_user).email, "test")
+
+    # add an identical tag to the node
+    tag_xml = XML::Node.new("tag")
+    tag_xml['k'] = current_node_tags(:t1).k
+    tag_xml['v'] = current_node_tags(:t1).v
+
+    # add the tag into the existing xml
+    node_xml = current_nodes(:visible_node).to_xml
+    node_xml.find("//osm/node").first << tag_xml
+
+    # try and upload it
+    content node_xml
+    put :update, :id => current_nodes(:visible_node).id
+    assert_response :bad_request, 
+       "adding duplicate tags to a node should fail with 'bad request'"
+  end    
 
   def basic_authorization(user, pass)
     @request.env["HTTP_AUTHORIZATION"] = "Basic %s" % Base64.encode64("#{user}:#{pass}")
@@ -93,4 +206,25 @@ class NodeControllerTest < Test::Unit::TestCase
   def content(c)
     @request.env["RAW_POST_DATA"] = c.to_s
   end
+
+  ##
+  # update the changeset_id of a node element
+  def update_changeset(xml, changeset_id)
+    xml_attr_rewrite(xml, 'changeset', changeset_id)
+  end
+
+  ##
+  # update an attribute in the node element
+  def xml_attr_rewrite(xml, name, value)
+    xml.find("//osm/node").first[name] = value.to_s
+    return xml
+  end
+
+  ##
+  # parse some xml
+  def xml_parse(xml)
+    parser = XML::Parser.new
+    parser.string = xml
+    parser.parse
+  end
 end