From: Shaun McDonald Date: Mon, 24 Nov 2008 18:55:24 +0000 (+0000) Subject: Make the role in relations optional, with a test to make sure it is. Also start movin... X-Git-Tag: live~7573^2~131 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/0ff1214f86ac1347bb257abfda70581cc78903dd?hp=152cb13a02dfcb2966b8a9b29382f84a99a23a69 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. --- diff --git a/app/controllers/relation_controller.rb b/app/controllers/relation_controller.rb index cdd1d34d6..575cca419 100644 --- 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 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 diff --git a/app/models/relation.rb b/app/models/relation.rb index 2607e7f2f..2a2ec3dca 100644 --- 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 + 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 diff --git a/config/environment.rb b/config/environment.rb index 2e5da44e3..ffed548bd 100644 --- 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 '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' diff --git a/config/initializers/libxml.rb b/config/initializers/libxml.rb index 3b5919f0f..ae636a9a3 100644 --- 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 -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 diff --git a/lib/osm.rb b/lib/osm.rb index 09ded2bd2..f6646503d 100644 --- 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 - { :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 diff --git a/test/functional/relation_controller_test.rb b/test/functional/relation_controller_test.rb index d44490036..b8d15e529 100644 --- 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 + ### # 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 "" + - "" + + "" + + "" + 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 "" + + ""+ "" 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 "" + + "" + + "" + 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. # -------------------------------------