Rationalise API error classes by getting of the render_opts method which

was never used except to extract the data it contained. Instead each class
now has a status method that returns the HTTP status code to use and a
to_s method to return a textual description of the error.

diff --git a/app/controllers/application.rb b/app/controllers/application.rb
index 6c6ffacb25396f80113b60e68a6c38a3477292a7..64eb2180f6d7f0807b1a4aef5987110185a98f0d 100644 (file)
--- a/app/controllers/application.rb
+++ b/app/controllers/application.rb
@@ -113,8 +113,7 @@ class ApplicationController < ActionController::Base
       ex.record.errors.each { |attr,msg| message << "#{attr}: #{msg} (#{ex.record[attr].inspect})" }
       report_error message, :bad_request
     rescue OSM::APIError => ex
-      render_opts = ex.render_opts
-      report_error render_opts[:text], render_opts[:status]
+      report_error ex.message, ex.status
     rescue Exception => ex
       report_error "#{ex.class}: #{ex.message}", :internal_server_error
     end

diff --git a/lib/osm.rb b/lib/osm.rb
index 885d1e35821961d998e85bf78e75eff081a9ff57..02aa1a105306cbe3b3afa214a04c0ae151339a44 100644 (file)
--- a/lib/osm.rb
+++ b/lib/osm.rb
@@ -10,8 +10,8 @@ module OSM
 
   # The base class for API Errors.
   class APIError < RuntimeError
-    def render_opts
-      { :text => "Generic API Error", :status => :internal_server_error, :content_type => "text/plain" }
+    def status
+      :internal_server_error
     end
 
     def to_s
@@ -21,8 +21,12 @@ module OSM
 
   # Raised when an API object is not found.
   class APINotFoundError < APIError
-    def render_opts
-      { :text => "The API wasn't found", :status => :not_found, :content_type => "text/plain" }
+    def status
+      :not_found
+    end
+
+    def to_s
+      "Object not found"
     end
   end
 
@@ -31,9 +35,9 @@ module OSM
     def initialize(message = "")
       @message = message
     end
-    
-    def render_opts
-      { :text => "Precondition failed: #{@message}", :status => :precondition_failed, :content_type => "text/plain" }
+
+    def status
+      :precondition_failed    
     end
 
     def to_s
@@ -48,16 +52,24 @@ module OSM
     end
     
     attr_reader :object, :object_id
+
+    def status
+      :gone
+    end
     
-    def render_opts
-      { :text => "The #{object} with the id #{object_id} has already been deleted", :status => :gone, :content_type => "text/plain" }
+    def to_s
+      "The #{object} with the id #{object_id} has already been deleted"
     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, :content_type => "text/plain" }
+    def status
+      :conflict
+    end
+
+    def to_s
+      "The user doesn't own that changeset"
     end
   end
 
@@ -68,20 +80,24 @@ module OSM
     end
 
     attr_reader :changeset
-    
-    def render_opts
-      { :text => "The changeset #{@changeset.id} was closed at #{@changeset.closed_at}.", :status => :conflict, :content_type => "text/plain" }
+
+    def status
+      :conflict
+    end
+
+    def to_s
+      "The changeset #{@changeset.id} was closed at #{@changeset.closed_at}"
     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, :content_type => "text/plain" }
+    def status
+      :conflict
     end
-    
+
     def to_s
-       "You need to supply a changeset to be able to make a change"
+      "You need to supply a changeset to be able to make a change"
     end
   end
 
@@ -91,10 +107,13 @@ module OSM
     def initialize(provided, allowed)
       @provided, @allowed = provided, allowed
     end
-    
-    def render_opts
-      { :text => "Changeset mismatch: Provided #{@provided} but only " +
-      "#{@allowed} is allowed.", :status => :conflict, :content_type => "text/plain" }
+
+    def status
+      :conflict
+    end
+
+    def rto_s
+      "Changeset mismatch: Provided #{@provided} but only #{@allowed} is allowed"
     end
   end
   
@@ -104,10 +123,13 @@ module OSM
     def initialize(provided)
       @provided = provided
     end
+
+    def status
+      :bad_request
+    end
     
-    def render_opts
-      { :text => "Unknown action #{@provided}, choices are create, modify, delete.",
-      :status => :bad_request, :content_type => "text/plain" }
+    def to_s
+      "Unknown action #{@provided}, choices are create, modify, delete"
     end
   end
 
@@ -118,9 +140,12 @@ module OSM
       @model, @xml, @message = model, xml, message
     end
 
-    def render_opts
-      { :text => "Cannot parse valid #{@model} from xml string #{@xml}. #{@message}",
-      :status => :bad_request, :content_type => "text/plain" }
+    def status
+      :bad_request
+    end
+
+    def to_s
+      "Cannot parse valid #{@model} from xml string #{@xml}. #{@message}"
     end
   end
 
@@ -132,13 +157,12 @@ module OSM
 
     attr_reader :provided, :latest, :id, :type
 
-    def render_opts
-      { :text => "Version mismatch: Provided #{provided}, server had: #{latest} of #{type} #{id}",
-        :status => :conflict, :content_type => "text/plain" }
+    def status
+      :conflict
     end
-    
+
     def to_s
-       "Version mismatch: Provided #{provided}, server had: #{latest} of #{type} #{id}"
+      "Version mismatch: Provided #{provided}, server had: #{latest} of #{type} #{id}"
     end
   end
 
@@ -151,9 +175,12 @@ module OSM
 
     attr_reader :type, :id, :tag_key
 
-    def render_opts
-      { :text => "Element #{@type}/#{@id} has duplicate tags with key #{@tag_key}.",
-        :status => :bad_request, :content_type => "text/plain" }
+    def status
+      :bad_request
+    end
+
+    def to_s
+      "Element #{@type}/#{@id} has duplicate tags with key #{@tag_key}"
     end
   end
   
@@ -165,10 +192,13 @@ module OSM
     end
     
     attr_reader :provided, :max
+
+    def status
+      :bad_request
+    end
     
-    def render_opts
-      { :text => "You tried to add #{provided} nodes to the way, however only #{max} are allowed",
-        :status => :bad_request, :content_type => "text/plain" }
+    def to_s
+      "You tried to add #{provided} nodes to the way, however only #{max} are allowed"
     end
   end
 
@@ -179,8 +209,12 @@ module OSM
       @message = message
     end
 
-    def render_opts
-      { :text => @message, :content_type => "text/plain", :status => :bad_request }
+    def status
+      :bad_request
+    end
+
+    def to_s
+      @message
     end
   end
 
@@ -191,16 +225,20 @@ module OSM
       @supported_method = supported_method
     end
 
-    def render_opts
-      { :text => "Only method #{@supported_method} is supported on this URI.", :status => :method_not_allowed }
+    def status
+      :method_not_allowed
+    end
+
+    def to_s
+      "Only method #{@supported_method} is supported on this URI"
     end
   end
 
   ##
   # raised when an API call takes too long
   class APITimeoutError < APIError
-    def render_opts
-      { :text => "Request timed out", :status => :request_timeout }
+    def status
+      :request_timeout
     end
 
     def to_s

diff --git a/test/functional/changeset_controller_test.rb b/test/functional/changeset_controller_test.rb
index e1cb3a7d484e3139ce8752509dd7cc1f6cba2c82..d60fe2fbc9621344512f373f479916ee85b3942d 100644 (file)
--- a/test/functional/changeset_controller_test.rb
+++ b/test/functional/changeset_controller_test.rb
@@ -670,7 +670,7 @@ EOF
   content diff
   post :upload, :id => cs_id
   assert_response :bad_request, "Shouldn't be able to upload a diff with the action ping"
-  assert_equal @response.body, "Unknown action ping, choices are create, modify, delete."
+  assert_equal @response.body, "Unknown action ping, choices are create, modify, delete"
   end
 
   ##

diff --git a/test/functional/node_controller_test.rb b/test/functional/node_controller_test.rb
index 266682fd058f607fd193f1aa69a0174bc6524776..42311c38fdbbd4a3fea8a10d62f943189812b8ca 100644 (file)
--- a/test/functional/node_controller_test.rb
+++ b/test/functional/node_controller_test.rb
@@ -370,7 +370,7 @@ class NodeControllerTest < ActionController::TestCase
     put :update, :id => current_nodes(:public_visible_node).id
     assert_response :bad_request, 
       "adding duplicate tags to a node should fail with 'bad request'"
-    assert_equal "Element node/#{current_nodes(:public_visible_node).id} has duplicate tags with key #{current_node_tags(:t1).k}.", @response.body
+    assert_equal "Element node/#{current_nodes(:public_visible_node).id} has duplicate tags with key #{current_node_tags(:t1).k}", @response.body
   end
 
   # test whether string injection is possible

diff --git a/test/functional/way_controller_test.rb b/test/functional/way_controller_test.rb
index cf123972aef35537d6231b86bb15a30ed3c94510..607f2dc6fe39017b934d79b265ac00a49d758a1d 100644 (file)
--- a/test/functional/way_controller_test.rb
+++ b/test/functional/way_controller_test.rb
@@ -349,7 +349,7 @@ class WayControllerTest < ActionController::TestCase
     put :update, :id => current_ways(:visible_way).id
     assert_response :bad_request, 
        "adding a duplicate tag to a way should fail with 'bad request'"
-    assert_equal "Element way/#{current_ways(:visible_way).id} has duplicate tags with key #{current_way_tags(:t1).k}.", @response.body
+    assert_equal "Element way/#{current_ways(:visible_way).id} has duplicate tags with key #{current_way_tags(:t1).k}", @response.body
   end
 
   ##
@@ -396,7 +396,7 @@ class WayControllerTest < ActionController::TestCase
     put :update, :id => current_ways(:visible_way).id
     assert_response :bad_request, 
        "adding new duplicate tags to a way should fail with 'bad request'"
-    assert_equal "Element way/#{current_ways(:visible_way).id} has duplicate tags with key i_am_a_duplicate.", @response.body
+    assert_equal "Element way/#{current_ways(:visible_way).id} has duplicate tags with key i_am_a_duplicate", @response.body
     
   end
 
@@ -436,7 +436,7 @@ class WayControllerTest < ActionController::TestCase
     put :create
     assert_response :bad_request, 
     "adding new duplicate tags to a way should fail with 'bad request'"
-    assert_equal "Element way/ has duplicate tags with key addr:housenumber.", @response.body
+    assert_equal "Element way/ has duplicate tags with key addr:housenumber", @response.body
   end
 
   ##