Using an around_handler for catching and rendering errors in most of the API controller methods. This simplifies the code and makes errors and error messages a bit more consistent. Also added a utility method for checking the HTTP method.

app/controllers/application.rb

@@ -101,6 +101,31 @@ class ApplicationController < ActionController::Base
     render :text => message, :status => status
   end
 
+  def api_call_handle_error
+    begin
+      yield
+    rescue ActiveRecord::RecordNotFound => ex
+      render :nothing => true, :status => :not_found
+    rescue LibXML::XML::Error, ArgumentError => ex
+      report_error ex.message, :bad_request
+    rescue ActiveRecord::RecordInvalid => ex
+      message = "#{ex.record.class} #{ex.record.id}: "
+      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]
+    end
+  end
+
+  ##
+  # asserts that the request method is the +method+ given as a parameter
+  # or raises a suitable error. +method+ should be a symbol, e.g: :put or :get.
+  def assert_method(method)
+    ok = request.send((method.to_s.downcase + "?").to_sym)
+    raise OSM::APIBadMethodError.new(method) unless ok
+  end
+
 private 
 
   # extract authorisation credentials from headers, returns user = nil if none

app/controllers/changeset_controller.rb

@@ -11,6 +11,7 @@ class ChangesetController < ApplicationController
   before_filter :check_api_writable, :only => [:create, :update, :delete, :upload, :include]
   before_filter :check_api_readable, :except => [:create, :update, :delete, :upload, :download, :query]
   after_filter :compress_output
+  around_filter :api_call_handle_error
 
   filter_parameter_logging "<osmChange version"
 
@@ -22,7 +23,8 @@ class ChangesetController < ApplicationController
 
   # Create a changeset from XML.
   def create
-    if request.put?
+    assert_method :put
+
     cs = Changeset.from_xml(request.raw_post, true)
 
     if cs
@@ -30,10 +32,7 @@ class ChangesetController < ApplicationController
       cs.save_with_tags!
       render :text => cs.id.to_s, :content_type => "text/plain"
     else
-        render :nothing => true, :status => :bad_request
+      raise OSM::APIBadXMLError.new(Changeset, request.raw_post);
-      end
-    else
-      render :nothing => true, :status => :method_not_allowed
     end
   end
 
@@ -41,22 +40,15 @@ class ChangesetController < ApplicationController
   # Return XML giving the basic info about the changeset. Does not 
   # return anything about the nodes, ways and relations in the changeset.
   def read
-    begin
     changeset = Changeset.find(params[:id])
     render :text => changeset.to_xml.to_s, :content_type => "text/xml"
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    end
   end
   
   ##
   # marks a changeset as closed. this may be called multiple times
   # on the same changeset, so is idempotent.
   def close 
-    unless request.put?
+    assert_method :put
-      render :nothing => true, :status => :method_not_allowed
-      return
-    end
     
     changeset = Changeset.find(params[:id])    
     check_changeset_consistency(changeset, @user)
@@ -68,10 +60,6 @@ class ChangesetController < ApplicationController
 
     changeset.save!
     render :nothing => true
-  rescue ActiveRecord::RecordNotFound
-    render :nothing => true, :status => :not_found
-  rescue OSM::APIError => ex
-    render ex.render_opts
   end
 
   ##
@@ -82,7 +70,8 @@ class ChangesetController < ApplicationController
   def expand_bbox
     # only allow POST requests, because although this method is
     # idempotent, there is no "document" to PUT really...
-    if request.post?
+    assert_method :post
+
     cs = Changeset.find(params[:id])
     check_changeset_consistency(cs, @user)
     
@@ -112,17 +101,6 @@ class ChangesetController < ApplicationController
     # will include the bigger bounding box.
     cs.save!
     render :text => cs.to_xml.to_s, :content_type => "text/xml"
-
-    else
-      render :nothing => true, :status => :method_not_allowed
-    end
-
-  rescue LibXML::XML::Error, ArgumentError => ex
-    raise OSM::APIBadXMLError.new("osm", xml, ex.message)
-  rescue ActiveRecord::RecordNotFound
-    render :nothing => true, :status => :not_found
-  rescue OSM::APIError => ex
-    render ex.render_opts
   end
 
   ##
@@ -142,10 +120,7 @@ class ChangesetController < ApplicationController
     # not idempotent, as several uploads with placeholder IDs will have
     # different side-effects.
     # see http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.1.2
-    unless request.post?
+    assert_method :post
-      render :nothing => true, :status => :method_not_allowed
-      return
-    end
 
     changeset = Changeset.find(params[:id])
     check_changeset_consistency(changeset, @user)
@@ -155,11 +130,6 @@ class ChangesetController < ApplicationController
       result = diff_reader.commit
       render :text => result.to_s, :content_type => "text/xml"
     end
-    
-  rescue ActiveRecord::RecordNotFound
-    render :nothing => true, :status => :not_found
-  rescue OSM::APIError => ex
-    render ex.render_opts
   end
 
   ##
@@ -227,11 +197,6 @@ class ChangesetController < ApplicationController
     end
 
     render :text => result.to_s, :content_type => "text/xml"
-            
-  rescue ActiveRecord::RecordNotFound
-    render :nothing => true, :status => :not_found
-  rescue OSM::APIError => ex
-    render ex.render_opts
   end
 
   ##
@@ -257,11 +222,6 @@ class ChangesetController < ApplicationController
     end
 
     render :text => results.to_s, :content_type => "text/xml"
-
-  rescue ActiveRecord::RecordNotFound
-    render :nothing => true, :status => :not_found
-  rescue OSM::APIError => ex
-    render ex.render_opts
   end
   
   ##
@@ -274,10 +234,7 @@ class ChangesetController < ApplicationController
   # after succesful update, returns the XML of the changeset.
   def update
     # request *must* be a PUT.
-    unless request.put?
+    assert_method :put
-      render :nothing => true, :status => :method_not_allowed
-      return
-    end
 
     changeset = Changeset.find(params[:id])
     new_changeset = Changeset.from_xml(request.raw_post)
@@ -290,11 +247,6 @@ class ChangesetController < ApplicationController
       
       render :nothing => true, :status => :bad_request
     end
-      
-  rescue ActiveRecord::RecordNotFound
-    render :nothing => true, :status => :not_found
-  rescue OSM::APIError => ex
-    render ex.render_opts
   end
 
   

app/controllers/node_controller.rb

@@ -9,30 +9,24 @@ class NodeController < ApplicationController
   before_filter :check_api_writable, :only => [:create, :update, :delete]
   before_filter :check_api_readable, :except => [:create, :update, :delete]
   after_filter :compress_output
+  around_filter :api_call_handle_error
 
   # Create a node from XML.
   def create
-    begin
+    assert_method :put
-      if request.put?
+
     node = Node.from_xml(request.raw_post, true)
 
     if node
       node.create_with_history @user
       render :text => node.id.to_s, :content_type => "text/plain"
     else
-          render :nothing => true, :status => :bad_request
+      raise OSM::APIBadXMLError.new(:node, request.raw_post)
-        end
-      else
-        render :nothing => true, :status => :method_not_allowed
-      end
-    rescue OSM::APIError => ex
-      render ex.render_opts
     end
   end
 
   # Dump the details on a node given in params[:id]
   def read
-    begin
     node = Node.find(params[:id])
     if node.visible?
       response.headers['Last-Modified'] = node.timestamp.rfc822
@@ -40,14 +34,10 @@ class NodeController < ApplicationController
     else
       render :text => "", :status => :gone
     end
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    end
   end
   
   # Update a node from given XML
   def update
-    begin
     node = Node.find(params[:id])
     new_node = Node.from_xml(request.raw_post)
     
@@ -57,18 +47,12 @@ class NodeController < ApplicationController
     else
       render :nothing => true, :status => :bad_request
     end
-    rescue OSM::APIError => ex
-      render ex.render_opts
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    end
   end
 
   # Delete a node. Doesn't actually delete it, but retains its history 
   # in a wiki-like way. We therefore treat it like an update, so the delete
   # method returns the new version number.
   def delete
-    begin
     node = Node.find(params[:id])
     new_node = Node.from_xml(request.raw_post)
     
@@ -78,11 +62,6 @@ class NodeController < ApplicationController
     else
       render :nothing => true, :status => :bad_request
     end
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    rescue OSM::APIError => ex
-      render ex.render_opts
-    end
   end
 
   # Dump the details on many nodes whose ids are given in the "nodes" parameter.

app/controllers/old_node_controller.rb

@@ -4,9 +4,9 @@ class OldNodeController < ApplicationController
   session :off
   before_filter :check_api_readable
   after_filter :compress_output
+  around_filter :api_call_handle_error
 
   def history
-    begin
     node = Node.find(params[:id])
     
     doc = OSM::API.new.get_xml_doc
@@ -16,15 +16,9 @@ class OldNodeController < ApplicationController
     end
     
     render :text => doc.to_s, :content_type => "text/xml"
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    rescue
-      render :nothing => true, :status => :internal_server_error
-    end
   end
   
   def version
-    begin
     old_node = OldNode.find(:first, :conditions => {:id => params[:id], :version => params[:version]} )
     if old_node.nil?
       # (RecordNotFound is not raised with find :first...)
@@ -38,8 +32,5 @@ class OldNodeController < ApplicationController
     doc.root << old_node.to_xml_node
 
     render :text => doc.to_s, :content_type => "text/xml"
-    rescue
-      render :nothing => true, :status => :internal_server_error
-    end
   end
 end

app/controllers/old_relation_controller.rb

@@ -4,9 +4,9 @@ class OldRelationController < ApplicationController
   session :off
   before_filter :check_api_readable
   after_filter :compress_output
+  around_filter :api_call_handle_error
 
   def history
-    begin
     relation = Relation.find(params[:id])
     doc = OSM::API.new.get_xml_doc
     
@@ -15,15 +15,9 @@ class OldRelationController < ApplicationController
     end
     
     render :text => doc.to_s, :content_type => "text/xml"
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    rescue
-      render :nothing => true, :status => :internal_server_error
-    end
   end
   
   def version
-    begin
     old_relation = OldRelation.find(:first, :conditions => {:id => params[:id], :version => params[:version]} )
     if old_relation.nil?
       # (RecordNotFound is not raised with find :first...)
@@ -37,8 +31,5 @@ class OldRelationController < ApplicationController
     doc.root << old_relation.to_xml_node
     
     render :text => doc.to_s, :content_type => "text/xml"
-    rescue
-      render :nothing => true, :status => :internal_server_error
-    end
   end
 end

app/controllers/old_way_controller.rb

@@ -4,9 +4,9 @@ class OldWayController < ApplicationController
   session :off
   before_filter :check_api_readable
   after_filter :compress_output
+  around_filter :api_call_handle_error
 
   def history
-    begin
     way = Way.find(params[:id])
     
     doc = OSM::API.new.get_xml_doc
@@ -16,15 +16,9 @@ class OldWayController < ApplicationController
     end
     
     render :text => doc.to_s, :content_type => "text/xml"
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    rescue
-      render :nothing => true, :status => :internal_server_error
-    end
   end
   
   def version
-    begin
     old_way = OldWay.find(:first, :conditions => {:id => params[:id], :version => params[:version]} )
     if old_way.nil?
       # (RecordNotFound is not raised with find :first...)
@@ -38,8 +32,5 @@ class OldWayController < ApplicationController
     doc.root << old_way.to_xml_node
     
     render :text => doc.to_s, :content_type => "text/xml"
-    rescue
-      render :nothing => true, :status => :internal_server_error
-    end
   end
 end

app/controllers/relation_controller.rb

@@ -7,10 +7,11 @@ class RelationController < ApplicationController
   before_filter :check_api_writable, :only => [:create, :update, :delete]
   before_filter :check_api_readable, :except => [:create, :update, :delete]
   after_filter :compress_output
+  around_filter :api_call_handle_error
 
   def create
-    begin
+    assert_method :put
-      if request.put?
+
     relation = Relation.from_xml(request.raw_post, true)
     
     # We assume that an exception has been thrown if there was an error 
@@ -21,16 +22,9 @@ class RelationController < ApplicationController
     #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
-    rescue OSM::APIError => ex
-      render ex.render_opts
-    end
   end
 
   def read
-    begin
     relation = Relation.find(params[:id])
     response.headers['Last-Modified'] = relation.timestamp.rfc822
     if relation.visible
@@ -38,16 +32,11 @@ class RelationController < ApplicationController
     else
       render :text => "", :status => :gone
     end
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    rescue
-      render :nothing => true, :status => :internal_server_error
-    end
   end
 
   def update
     logger.debug request.raw_post
-    begin
+
     relation = Relation.find(params[:id])
     new_relation = Relation.from_xml(request.raw_post)
     
@@ -57,15 +46,9 @@ class RelationController < ApplicationController
     else
       render :nothing => true, :status => :bad_request
     end
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    rescue OSM::APIError => ex
-      render ex.render_opts
-    end
   end
 
   def delete
-    begin
     relation = Relation.find(params[:id])
     new_relation = Relation.from_xml(request.raw_post)
     if new_relation and new_relation.id == relation.id
@@ -74,11 +57,6 @@ class RelationController < ApplicationController
     else
       render :nothing => true, :status => :bad_request
     end
-    rescue OSM::APIError => ex
-      render ex.render_opts
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    end
   end
 
   # -----------------------------------------------------------------
@@ -90,7 +68,6 @@ class RelationController < ApplicationController
   # members, plus all nodes part of member ways
   # -----------------------------------------------------------------
   def full
-    begin
     relation = Relation.find(params[:id])
     
     if relation.visible
@@ -149,13 +126,6 @@ class RelationController < ApplicationController
     else
       render :nothing => true, :status => :gone
     end
-
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-
-    rescue
-      render :nothing => true, :status => :internal_server_error
-    end
   end
 
   def relations
@@ -172,16 +142,16 @@ class RelationController < ApplicationController
     else
       render :text => "You need to supply a comma separated list of ids.", :status => :bad_request
     end
-  rescue ActiveRecord::RecordNotFound
-    render :text => "Could not find one of the relations", :status => :not_found
   end
 
   def relations_for_way
     relations_for_object("Way")
   end
+
   def relations_for_node
     relations_for_object("Node")
   end
+
   def relations_for_relation
     relations_for_object("Relation")
   end

app/controllers/way_controller.rb

@@ -7,10 +7,11 @@ class WayController < ApplicationController
   before_filter :check_api_writable, :only => [:create, :update, :delete]
   before_filter :check_api_readable, :except => [:create, :update, :delete]
   after_filter :compress_output
+  around_filter :api_call_handle_error
 
   def create
-    begin
+    assert_method :put
-      if request.put?
+
     way = Way.from_xml(request.raw_post, true)
     
     if way
@@ -19,17 +20,9 @@ class WayController < ApplicationController
     else
       render :nothing => true, :status => :bad_request
     end
-      else
-        render :nothing => true, :status => :method_not_allowed
-      end
-    rescue OSM::APIError => ex
-      logger.warn request.raw_post
-      render ex.render_opts
-    end
   end
 
   def read
-    begin
     way = Way.find(params[:id])
     
     response.headers['Last-Modified'] = way.timestamp.rfc822
@@ -39,15 +32,9 @@ class WayController < ApplicationController
     else
       render :text => "", :status => :gone
     end
-    rescue OSM::APIError => ex
-      render ex.render_opts
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    end
   end
 
   def update
-    begin
     way = Way.find(params[:id])
     new_way = Way.from_xml(request.raw_post)
     
@@ -57,17 +44,10 @@ class WayController < ApplicationController
     else
       render :nothing => true, :status => :bad_request
     end
-    rescue OSM::APIError => ex
-      logger.warn request.raw_post
-      render ex.render_opts
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    end
   end
 
   # This is the API call to delete a way
   def delete
-    begin
     way = Way.find(params[:id])
     new_way = Way.from_xml(request.raw_post)
     
@@ -77,15 +57,9 @@ class WayController < ApplicationController
     else
       render :nothing => true, :status => :bad_request
     end
-    rescue OSM::APIError => ex
-      render ex.render_opts
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    end
   end
 
   def full
-    begin
     way = Way.find(params[:id])
     
     if way.visible
@@ -103,9 +77,6 @@ class WayController < ApplicationController
     else
       render :text => "", :status => :gone
     end
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    end
   end
 
   def ways

app/models/node.rb

@@ -254,11 +254,6 @@ class Node < ActiveRecord::Base
     # in the hash to be overwritten.
     raise OSM::APIDuplicateTagsError.new("node", self.id, k) if @tags.include? k
 
-    # check tag size here, as we don't create a NodeTag object until
-    # just before we save...
-    raise OSM::APIBadUserInput.new("Node #{self.id} has a tag with too long a key, '#{k}'.") if k.length > 255
-    raise OSM::APIBadUserInput.new("Node #{self.id} has a tag with too long a value, '#{k}'='#{v}'.") if v.length > 255
-
     @tags[k] = v
   end
 

app/models/relation.rb

@@ -218,11 +218,6 @@ class Relation < ActiveRecord::Base
     # in the hash to be overwritten.
     raise OSM::APIDuplicateTagsError.new("relation", self.id, k) if @tags.include? k
 
-    # check tag size here, as we don't create a RelationTag object until
-    # just before we save...
-    raise OSM::APIBadUserInput.new("Relation #{self.id} has a tag with too long a key, '#{k}'.") if k.length > 255
-    raise OSM::APIBadUserInput.new("Relation #{self.id} has a tag with too long a value, '#{k}'='#{v}'.") if v.length > 255
-
     @tags[k] = v
   end
 

app/models/way.rb

@@ -191,11 +191,6 @@ class Way < ActiveRecord::Base
     # in the hash to be overwritten.
     raise OSM::APIDuplicateTagsError.new("way", self.id, k) if @tags.include? k
 
-    # check tag size here, as we don't create a WayTag object until
-    # just before we save...
-    raise OSM::APIBadUserInput.new("Way #{self.id} has a tag with too long a key, '#{k}'.") if k.length > 255
-    raise OSM::APIBadUserInput.new("Way #{self.id} has a tag with too long a value, '#{k}'='#{v}'.") if v.length > 255
-
     @tags[k] = v
   end
 

lib/osm.rb

@@ -185,6 +185,18 @@ module OSM
     end
   end
 
+  ##
+  # raised when an API call is made using a method not supported on that URI
+  class APIBadMethodError < APIError
+    def initialize(supported_method)
+      @supported_method = supported_method
+    end
+
+    def render_opts
+      { :text => "Only method #{@supported_method} is supported on this URI.", :status => :method_not_allowed }
+    end
+  end
+
   # Helper methods for going to/from mercator and lat/lng.
   class Mercator
     include Math

test/functional/node_controller_test.rb

@@ -95,7 +95,7 @@ class NodeControllerTest < ActionController::TestCase
     content("<osm><node lat='#{lat}' lon='#{lon}' changeset='#{changeset.id}'><tag k='foo' v='#{'x'*256}'/></node></osm>")
     put :create
     assert_response :bad_request, "node upload did not return bad_request status"
-    assert_equal "Node  has a tag with too long a value, 'foo'='#{'x'*256}'.", @response.body
+    assert_equal ["NodeTag ", " v: is too long (maximum is 255 characters) (\"#{'x'*256}\")"], @response.body.split(/[0-9]+:/)
 
   end