Make the role in relations optional, with a test to make sure it is. Also start movin...
authorShaun McDonald <shaun@shaunmcdonald.me.uk>
Mon, 24 Nov 2008 18:55:24 +0000 (18:55 +0000)
committerShaun McDonald <shaun@shaunmcdonald.me.uk>
Mon, 24 Nov 2008 18:55:24 +0000 (18:55 +0000)
app/controllers/relation_controller.rb
app/models/relation.rb
config/environment.rb
config/initializers/libxml.rb
lib/osm.rb
test/functional/relation_controller_test.rb

index cdd1d34..575cca4 100644 (file)
@@ -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
index 2607e7f..2a2ec3d 100644 (file)
@@ -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
index 2e5da44..ffed548 100644 (file)
@@ -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'
 
index 3b5919f..ae636a9 100644 (file)
@@ -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
index 09ded2b..f664650 100644 (file)
@@ -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
 
index d444900..b8d15e5 100644 (file)
@@ -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.
   # -------------------------------------