Make the role in relations optional, with a test to make sure it is. Also start movin...

diff --git a/app/controllers/relation_controller.rb b/app/controllers/relation_controller.rb
index cdd1d34d6fb459434bab7b381286634ebf1d1291..575cca419941efc42635b75899e91ce0079ee9b9 100644 (file)
--- a/app/controllers/relation_controller.rb
+++ b/app/controllers/relation_controller.rb
@@ -12,12 +12,14 @@ class RelationController < ApplicationController
       if request.put?
         relation = Relation.from_xml(request.raw_post, true)
 
       if request.put?
         relation = Relation.from_xml(request.raw_post, true)
 

-        if relation
+        # We assume that an exception has been thrown if there was an error 
+        # generating the relation
+        #if relation

           relation.create_with_history @user
           render :text => relation.id.to_s, :content_type => "text/plain"
           relation.create_with_history @user
           render :text => relation.id.to_s, :content_type => "text/plain"

-        else
-          render :nothing => true, :status => :bad_request
-        end
+        #else
+         # render :text => "Couldn't get turn the input into a relation.", :status => :bad_request
+        #end

       else
         render :nothing => true, :status => :method_not_allowed
       end
       else
         render :nothing => true, :status => :method_not_allowed
       end

diff --git a/app/models/relation.rb b/app/models/relation.rb
index 2607e7f2fd3c0f17ecb314730248839f494e7f52..2a2ec3dcae843dd10bc8f8c9c7ecdc2583ccea3f 100644 (file)
--- a/app/models/relation.rb
+++ b/app/models/relation.rb
@@ -15,6 +15,8 @@ class Relation < ActiveRecord::Base
   has_many :containing_relation_members, :class_name => "RelationMember", :as => :member
   has_many :containing_relations, :class_name => "Relation", :through => :containing_relation_members, :source => :relation, :extend => ObjectFinder
 
   has_many :containing_relation_members, :class_name => "RelationMember", :as => :member
   has_many :containing_relations, :class_name => "Relation", :through => :containing_relation_members, :source => :relation, :extend => ObjectFinder
 

+  TYPES = ["node", "way", "relation"]
+

   def self.from_xml(xml, create=false)
     begin
       p = XML::Parser.new
   def self.from_xml(xml, create=false)
     begin
       p = XML::Parser.new


@@ -22,10 +24,11 @@ class Relation < ActiveRecord::Base
       doc = p.parse
 
       doc.find('//osm/relation').each do |pt|
       doc = p.parse
 
       doc.find('//osm/relation').each do |pt|

-       return Relation.from_xml_node(pt, create)
+        return Relation.from_xml_node(pt, create)

       end
       end

-    rescue
-      return nil
+    rescue LibXML::XML::Error => ex
+      #return nil
+      raise OSM::APIBadXMLError.new("relation", xml, ex.message)

     end
   end
 
     end
   end
 


@@ -53,8 +56,17 @@ class Relation < ActiveRecord::Base
     end
 
     pt.find('member').each do |member|
     end
 
     pt.find('member').each do |member|

+      #member_type = 
+      logger.debug "each member"
+      raise OSM::APIBadXMLError.new("relation", pt, "The #{member['type']} is not allowed only, #{TYPES.inspect} allowed") unless TYPES.include? member['type']
+      logger.debug "after raise"
+      #member_ref = member['ref']
+      #member_role
+      member['role'] ||= "" # Allow  the upload to not include this, in which case we default to an empty string.
+      logger.debug member['role']

       relation.add_member(member['type'], member['ref'], member['role'])
     end
       relation.add_member(member['type'], member['ref'], member['role'])
     end

+    raise OSM::APIBadUserInput.new("Some bad xml in relation") if relation.nil?

 
     return relation
   end
 
     return relation
   end

diff --git a/config/environment.rb b/config/environment.rb
index 2e5da44e317af0b14397b6592a89de9222b29eeb..ffed548bd4360f2715b9a83b82dda5fdc171ebd3 100644 (file)
--- a/config/environment.rb
+++ b/config/environment.rb
@@ -46,7 +46,7 @@ Rails::Initializer.run do |config|
   # config.gem "hpricot", :version => '0.6', :source => "http://code.whytheluckystiff.net"
   # config.gem "aws-s3", :lib => "aws/s3"
   config.gem 'composite_primary_keys', :version => '1.1.0'
   # config.gem "hpricot", :version => '0.6', :source => "http://code.whytheluckystiff.net"
   # config.gem "aws-s3", :lib => "aws/s3"
   config.gem 'composite_primary_keys', :version => '1.1.0'

-  config.gem 'libxml-ruby', :version => '>= 0.8.3', :lib => 'libxml'
+  config.gem 'libxml-ruby', :version => '0.9.4', :lib => 'libxml'

   config.gem 'rmagick', :lib => 'RMagick'
   config.gem 'mysql'
 
   config.gem 'rmagick', :lib => 'RMagick'
   config.gem 'mysql'
 

diff --git a/config/initializers/libxml.rb b/config/initializers/libxml.rb
index 3b5919f0fca8c0a65c7a0f48f75d099f275bc4fa..ae636a9a31039a260539fd2d195fa49e9ce56684 100644 (file)
--- a/config/initializers/libxml.rb
+++ b/config/initializers/libxml.rb
@@ -1,5 +1,9 @@
 # This is required otherwise libxml writes out memory errors to
 # the standard output and exits uncleanly 
 # This is required otherwise libxml writes out memory errors to
 # the standard output and exits uncleanly 

-LibXML::XML::Parser.register_error_handler do |message|
+# Changed method due to deprecation of the old register_error_handler
+# http://libxml.rubyforge.org/rdoc/classes/LibXML/XML/Parser.html#M000076
+# So set_handler is used instead
+# http://libxml.rubyforge.org/rdoc/classes/LibXML/XML/Error.html#M000334
+LibXML::XML::Error.set_handler do |message|

   raise message
 end
   raise message
 end

diff --git a/lib/osm.rb b/lib/osm.rb
index 09ded2bd231806c43ae116057bf04c014389a5bc..f6646503d0e37e0d68e04efbaf01755be4c6195e 100644 (file)
--- a/lib/osm.rb
+++ b/lib/osm.rb
@@ -11,35 +11,39 @@ module OSM
   # The base class for API Errors.
   class APIError < RuntimeError
     def render_opts
   # The base class for API Errors.
   class APIError < RuntimeError
     def render_opts

-      { :text => "", :status => :internal_server_error }
+      { :text => "Generic API Error", :status => :internal_server_error, :content_type => "text/plain" }

     end
   end
 
   # Raised when an API object is not found.
   class APINotFoundError < APIError
     def render_opts
     end
   end
 
   # Raised when an API object is not found.
   class APINotFoundError < APIError
     def render_opts

-      { :nothing => true, :status => :not_found }
+      { :text => "The API wasn't found", :status => :not_found, :content_type => "text/plain" }

     end
   end
 
   # Raised when a precondition to an API action fails sanity check.
   class APIPreconditionFailedError < APIError
     end
   end
 
   # Raised when a precondition to an API action fails sanity check.
   class APIPreconditionFailedError < APIError

+    def initialize(message = "")
+      @message = message
+    end
+    

     def render_opts
     def render_opts

-      { :text => "", :status => :precondition_failed }
+      { :text => "Precondition failed: #{@message}", :status => :precondition_failed, :content_type => "text/plain" }

     end
   end
 
   # Raised when to delete an already-deleted object.
   class APIAlreadyDeletedError < APIError
     def render_opts
     end
   end
 
   # Raised when to delete an already-deleted object.
   class APIAlreadyDeletedError < APIError
     def render_opts

-      { :text => "", :status => :gone }
+      { :text => "The object has already been deleted", :status => :gone, :content_type => "text/plain" }

     end
   end
 
   # Raised when the user logged in isn't the same as the changeset
   class APIUserChangesetMismatchError < APIError
     def render_opts
     end
   end
 
   # Raised when the user logged in isn't the same as the changeset
   class APIUserChangesetMismatchError < APIError
     def render_opts

-      { :text => "The user doesn't own that changeset", :status => :conflict }
+      { :text => "The user doesn't own that changeset", :status => :conflict, :content_type => "text/plain" }

     end
   end
 
     end
   end
 


@@ -52,14 +56,14 @@ module OSM
     attr_reader :changeset
     
     def render_opts
     attr_reader :changeset
     
     def render_opts

-      { :text => "The changeset #{@changeset.id} was closed at #{@changeset.closed_at}.", :status => :conflict }
+      { :text => "The changeset #{@changeset.id} was closed at #{@changeset.closed_at}.", :status => :conflict, :content_type => "text/plain" }

     end
   end
   
   # Raised when a change is expecting a changeset, but the changeset doesn't exist
   class APIChangesetMissingError < APIError
     def render_opts
     end
   end
   
   # Raised when a change is expecting a changeset, but the changeset doesn't exist
   class APIChangesetMissingError < APIError
     def render_opts

-      { :text => "You need to supply a changeset to be able to make a change", :status => :conflict }
+      { :text => "You need to supply a changeset to be able to make a change", :status => :conflict, :content_type => "text/plain" }

     end
   end
 
     end
   end
 


@@ -72,7 +76,7 @@ module OSM
     
     def render_opts
       { :text => "Changeset mismatch: Provided #{@provided} but only " +
     
     def render_opts
       { :text => "Changeset mismatch: Provided #{@provided} but only " +

-        "#{@allowed} is allowed.", :status => :conflict }
+      "#{@allowed} is allowed.", :status => :conflict, :content_type => "text/plain" }

     end
   end
   
     end
   end
   


@@ -85,20 +89,20 @@ module OSM
     
     def render_opts
       { :text => "Unknown action #{@provided}, choices are create, modify, delete.",
     
     def render_opts
       { :text => "Unknown action #{@provided}, choices are create, modify, delete.",

-      :status => :bad_request }
+      :status => :bad_request, :content_type => "text/plain" }

     end
   end
 
   # Raised when bad XML is encountered which stops things parsing as
   # they should.
   class APIBadXMLError < APIError
     end
   end
 
   # Raised when bad XML is encountered which stops things parsing as
   # they should.
   class APIBadXMLError < APIError

-    def initialize(model, xml)
-      @model, @xml = model, xml
+    def initialize(model, xml, message="")
+      @model, @xml, @message = model, xml, message

     end
 
     def render_opts
     end
 
     def render_opts

-      { :text => "Cannot parse valid #{@model} from xml string #{@xml}",
-        :status => :bad_request }
+      { :text => "Cannot parse valid #{@model} from xml string #{@xml}. #{@message}",
+      :status => :bad_request, :content_type => "text/plain" }

     end
   end
 
     end
   end
 


@@ -113,7 +117,7 @@ module OSM
     def render_opts
       { :text => "Version mismatch: Provided " + provided.to_s +
         ", server had: " + latest.to_s + " of " + type + " " + id.to_s, 
     def render_opts
       { :text => "Version mismatch: Provided " + provided.to_s +
         ", server had: " + latest.to_s + " of " + type + " " + id.to_s, 

-        :status => :conflict }
+        :status => :conflict, :content_type => "text/plain" }

     end
   end
 
     end
   end
 


@@ -128,7 +132,7 @@ module OSM
 
     def render_opts
       { :text => "Element #{@type}/#{@id} has duplicate tags with key #{@tag_key}.",
 
     def render_opts
       { :text => "Element #{@type}/#{@id} has duplicate tags with key #{@tag_key}.",

-        :status => :bad_request }
+        :status => :bad_request, :content_type => "text/plain" }

     end
   end
   
     end
   end
   


@@ -143,7 +147,7 @@ module OSM
     
     def render_opts
       { :text => "You tried to add #{provided} nodes to the way, however only #{max} are allowed",
     
     def render_opts
       { :text => "You tried to add #{provided} nodes to the way, however only #{max} are allowed",

-      :status => :bad_request }
+        :status => :bad_request, :content_type => "text/plain" }

     end
   end
 
     end
   end
 


@@ -155,7 +159,7 @@ module OSM
     end
 
     def render_opts
     end
 
     def render_opts

-      { :text => message, :mime_type => "text/plain", :status => :bad_request }
+      { :text => @message, :content_type => "text/plain", :status => :bad_request }

     end
   end
 
     end
   end
 

diff --git a/test/functional/relation_controller_test.rb b/test/functional/relation_controller_test.rb
index d44490036b1684b3adad76bb95e754bfd9c1a721..b8d15e5294f43f0c17d20ef9708cc05e9c175bd9 100644 (file)
--- a/test/functional/relation_controller_test.rb
+++ b/test/functional/relation_controller_test.rb
@@ -117,10 +117,45 @@ class RelationControllerTest < ActionController::TestCase
     assert_response :success
 
 
     assert_response :success
 
 

+    ###

     # create an relation with a node as member
     # create an relation with a node as member

+    # This time try with a role attribute in the relation

     nid = current_nodes(:used_node_1).id
     content "<osm><relation changeset='#{changeset_id}'>" +
     nid = current_nodes(:used_node_1).id
     content "<osm><relation changeset='#{changeset_id}'>" +

-      "<member type='node' ref='#{nid}' role='some'/>" +
+      "<member  ref='#{nid}' type='node' role='some'/>" +
+      "<tag k='test' v='yes' /></relation></osm>"
+    put :create
+    # hope for success
+    assert_response :success, 
+        "relation upload did not return success status"
+    # read id of created relation and search for it
+    relationid = @response.body
+    checkrelation = Relation.find(relationid)
+    assert_not_nil checkrelation, 
+        "uploaded relation not found in data base after upload"
+    # compare values
+    assert_equal checkrelation.members.length, 1, 
+        "saved relation does not contain exactly one member"
+    assert_equal checkrelation.tags.length, 1, 
+        "saved relation does not contain exactly one tag"
+    assert_equal changeset_id, checkrelation.changeset.id,
+        "saved relation does not belong in the changeset it was assigned to"
+    assert_equal users(:normal_user).id, checkrelation.changeset.user_id, 
+        "saved relation does not belong to user that created it"
+    assert_equal true, checkrelation.visible, 
+        "saved relation is not visible"
+    # ok the relation is there but can we also retrieve it?
+    
+    get :read, :id => relationid
+    assert_response :success
+    
+    
+    ###
+    # create an relation with a node as member, this time test that we don't 
+    # need a role attribute to be included
+    nid = current_nodes(:used_node_1).id
+    content "<osm><relation changeset='#{changeset_id}'>" +
+      "<member  ref='#{nid}' type='node'/>"+

       "<tag k='test' v='yes' /></relation></osm>"
     put :create
     # hope for success
       "<tag k='test' v='yes' /></relation></osm>"
     put :create
     # hope for success


@@ -147,6 +182,7 @@ class RelationControllerTest < ActionController::TestCase
     get :read, :id => relationid
     assert_response :success
 
     get :read, :id => relationid
     assert_response :success
 

+    ###

     # create an relation with a way and a node as members
     nid = current_nodes(:used_node_1).id
     wid = current_ways(:used_way).id
     # create an relation with a way and a node as members
     nid = current_nodes(:used_node_1).id
     wid = current_ways(:used_way).id


@@ -200,6 +236,27 @@ class RelationControllerTest < ActionController::TestCase
         "relation upload with invalid node did not return 'precondition failed'"
   end
 
         "relation upload with invalid node did not return 'precondition failed'"
   end
 

+  # -------------------------------------
+  # Test creating a relation, with some invalid XML
+  # -------------------------------------
+  def test_create_invalid_xml
+    basic_authorization "test@openstreetmap.org", "test"
+    
+    # put the relation in a dummy fixture changeset that works
+    changeset_id = changesets(:normal_user_first_change).id
+    
+    # create some xml that should return an error
+    content "<osm><relation changeset='#{changeset_id}'>" +
+    "<member type='type' ref='#{current_nodes(:used_node_1).id}' role=''/>" +
+    "<tag k='tester' v='yep'/></relation></osm>"
+    put :create
+    # expect failure
+    assert_response :bad_request
+    assert_match(/Cannot parse valid relation from xml string/, @response.body)
+    assert_match(/The type is not allowed only, /, @response.body)
+  end
+  
+  

   # -------------------------------------
   # Test deleting relations.
   # -------------------------------------
   # -------------------------------------
   # Test deleting relations.
   # -------------------------------------