Make the role in relations optional, with a test to make sure it is. Also start moving the errors reading the xml to exceptions, thus making it possible to give meaningful error messages, when bad xml is sent (More work is required on this including doing the same for nodes and ways). With the latest gems update it seems that the lib xml handling was broken, using the newer method. Adding the content type for the exceptions.

app/controllers/relation_controller.rb

@@ -12,12 +12,14 @@ class RelationController < ApplicationController
       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"
-        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

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
 
+  TYPES = ["node", "way", "relation"]
+
   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|
-	return Relation.from_xml_node(pt, create)
+        return Relation.from_xml_node(pt, create)
       end
-    rescue
-      return nil
+    rescue LibXML::XML::Error => ex
+      #return nil
+      raise OSM::APIBadXMLError.new("relation", xml, ex.message)
     end
   end
 
@@ -53,8 +56,17 @@ class Relation < ActiveRecord::Base
     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
+    raise OSM::APIBadUserInput.new("Some bad xml in relation") if relation.nil?
 
     return relation
   end

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 '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/initializers/libxml.rb

@@ -1,5 +1,9 @@
 # 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

lib/osm.rb

@@ -11,35 +11,39 @@ module OSM
   # 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
-      { :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
+    def initialize(message = "")
+      @message = message
+    end
+    
     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
-      { :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
-      { :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
 
@@ -52,14 +56,14 @@ module OSM
     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
-      { :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
 
@@ -72,7 +76,7 @@ module OSM
     
     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
   
@@ -85,20 +89,20 @@ module OSM
     
     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
-    def initialize(model, xml)
-      @model, @xml = model, xml
+    def initialize(model, xml, message="")
+      @model, @xml, @message = model, xml, message
     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
 
@@ -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, 
-        :status => :conflict }
+        :status => :conflict, :content_type => "text/plain" }
     end
   end
 
@@ -128,7 +132,7 @@ module OSM
 
     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
   
@@ -143,7 +147,7 @@ module OSM
     
     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
 
@@ -155,7 +159,7 @@ module OSM
     end
 
     def render_opts
-      { :text => message, :mime_type => "text/plain", :status => :bad_request }
+      { :text => @message, :content_type => "text/plain", :status => :bad_request }
     end
   end
 

test/functional/relation_controller_test.rb

@@ -117,10 +117,45 @@ class RelationControllerTest < ActionController::TestCase
     assert_response :success
 
 
+    ###
     # 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}'>" +
-      "<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
@@ -147,6 +182,7 @@ class RelationControllerTest < ActionController::TestCase
     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
@@ -200,6 +236,27 @@ class RelationControllerTest < ActionController::TestCase
         "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.
   # -------------------------------------