From: Matt Amos <zerebubuth@gmail.com>
Date: Wed, 20 May 2009 17:39:59 +0000 (+0000)
Subject: Using an around_handler for catching and rendering errors in most of the API controll... 
X-Git-Tag: live~7407
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/3d0ca940d26bdc23aa791178b01b816185c5a086

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.
---

diff --git a/app/controllers/application.rb b/app/controllers/application.rb
index 5e50a503e..082c5cf65 100644
--- a/app/controllers/application.rb
+++ b/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
diff --git a/app/controllers/changeset_controller.rb b/app/controllers/changeset_controller.rb
index ea28675c8..778bb73ae 100644
--- a/app/controllers/changeset_controller.rb
+++ b/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,18 +23,16 @@ class ChangesetController < ApplicationController
 
   # Create a changeset from XML.
   def create
-    if request.put?
-      cs = Changeset.from_xml(request.raw_post, true)
+    assert_method :put
 
-      if cs
-        cs.user_id = @user.id
-        cs.save_with_tags!
-        render :text => cs.id.to_s, :content_type => "text/plain"
-      else
-        render :nothing => true, :status => :bad_request
-      end
+    cs = Changeset.from_xml(request.raw_post, true)
+
+    if cs
+      cs.user_id = @user.id
+      cs.save_with_tags!
+      render :text => cs.id.to_s, :content_type => "text/plain"
     else
-      render :nothing => true, :status => :method_not_allowed
+      raise OSM::APIBadXMLError.new(Changeset, request.raw_post);
     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
+    changeset = Changeset.find(params[:id])
+    render :text => changeset.to_xml.to_s, :content_type => "text/xml"
   end
   
   ##
   # marks a changeset as closed. this may be called multiple times
   # on the same changeset, so is idempotent.
   def close 
-    unless request.put?
-      render :nothing => true, :status => :method_not_allowed
-      return
-    end
+    assert_method :put
     
     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,47 +70,37 @@ 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?
-      cs = Changeset.find(params[:id])
-      check_changeset_consistency(cs, @user)
-
-      # keep an array of lons and lats
-      lon = Array.new
-      lat = Array.new
-
-      # the request is in pseudo-osm format... this is kind-of an
-      # abuse, maybe should change to some other format?
-      doc = XML::Parser.string(request.raw_post).parse
-      doc.find("//osm/node").each do |n|
-        lon << n['lon'].to_f * GeoRecord::SCALE
-        lat << n['lat'].to_f * GeoRecord::SCALE
-      end
-
-      # add the existing bounding box to the lon-lat array
-      lon << cs.min_lon unless cs.min_lon.nil?
-      lat << cs.min_lat unless cs.min_lat.nil?
-      lon << cs.max_lon unless cs.max_lon.nil?
-      lat << cs.max_lat unless cs.max_lat.nil?
-
-      # collapse the arrays to minimum and maximum
-      cs.min_lon, cs.min_lat, cs.max_lon, cs.max_lat = 
-        lon.min, lat.min, lon.max, lat.max
-
-      # save the larger bounding box and return the changeset, which
-      # will include the bigger bounding box.
-      cs.save!
-      render :text => cs.to_xml.to_s, :content_type => "text/xml"
+    assert_method :post
 
-    else
-      render :nothing => true, :status => :method_not_allowed
+    cs = Changeset.find(params[:id])
+    check_changeset_consistency(cs, @user)
+    
+    # keep an array of lons and lats
+    lon = Array.new
+    lat = Array.new
+    
+    # the request is in pseudo-osm format... this is kind-of an
+    # abuse, maybe should change to some other format?
+    doc = XML::Parser.string(request.raw_post).parse
+    doc.find("//osm/node").each do |n|
+      lon << n['lon'].to_f * GeoRecord::SCALE
+      lat << n['lat'].to_f * GeoRecord::SCALE
     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
+    
+    # add the existing bounding box to the lon-lat array
+    lon << cs.min_lon unless cs.min_lon.nil?
+    lat << cs.min_lat unless cs.min_lat.nil?
+    lon << cs.max_lon unless cs.max_lon.nil?
+    lat << cs.max_lat unless cs.max_lat.nil?
+    
+    # collapse the arrays to minimum and maximum
+    cs.min_lon, cs.min_lat, cs.max_lon, cs.max_lat = 
+      lon.min, lat.min, lon.max, lat.max
+    
+    # save the larger bounding box and return the changeset, which
+    # will include the bigger bounding box.
+    cs.save!
+    render :text => cs.to_xml.to_s, :content_type => "text/xml"
   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?
-      render :nothing => true, :status => :method_not_allowed
-      return
-    end
+    assert_method :post
 
     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,11 +234,8 @@ class ChangesetController < ApplicationController
   # after succesful update, returns the XML of the changeset.
   def update
     # request *must* be a PUT.
-    unless request.put?
-      render :nothing => true, :status => :method_not_allowed
-      return
-    end
-    
+    assert_method :put
+
     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
 
   
diff --git a/app/controllers/node_controller.rb b/app/controllers/node_controller.rb
index 6e96d31ca..3b288e29b 100644
--- a/app/controllers/node_controller.rb
+++ b/app/controllers/node_controller.rb
@@ -9,58 +9,43 @@ 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
-      if request.put?
-        node = Node.from_xml(request.raw_post, true)
+    assert_method :put
 
-        if node
-          node.create_with_history @user
-          render :text => node.id.to_s, :content_type => "text/plain"
-        else
-          render :nothing => true, :status => :bad_request
-        end
-      else
-        render :nothing => true, :status => :method_not_allowed
-      end
-    rescue OSM::APIError => ex
-      render ex.render_opts
+    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
+      raise OSM::APIBadXMLError.new(:node, request.raw_post)
     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
-        render :text => node.to_xml.to_s, :content_type => "text/xml"
-       else
-        render :text => "", :status => :gone
-      end
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
+    node = Node.find(params[:id])
+    if node.visible?
+      response.headers['Last-Modified'] = node.timestamp.rfc822
+      render :text => node.to_xml.to_s, :content_type => "text/xml"
+    else
+      render :text => "", :status => :gone
     end
   end
   
   # Update a node from given XML
   def update
-    begin
-      node = Node.find(params[:id])
-      new_node = Node.from_xml(request.raw_post)
-
-      if new_node and new_node.id == node.id
-        node.update_from(new_node, @user)
-        render :text => node.version.to_s, :content_type => "text/plain"
-      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
+    node = Node.find(params[:id])
+    new_node = Node.from_xml(request.raw_post)
+    
+    if new_node and new_node.id == node.id
+      node.update_from(new_node, @user)
+      render :text => node.version.to_s, :content_type => "text/plain"
+    else
+      render :nothing => true, :status => :bad_request
     end
   end
 
@@ -68,20 +53,14 @@ class NodeController < ApplicationController
   # 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)
-      
-      if new_node and new_node.id == node.id
-        node.delete_with_history!(new_node, @user)
-        render :text => node.version.to_s, :content_type => "text/plain"
-      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
+    node = Node.find(params[:id])
+    new_node = Node.from_xml(request.raw_post)
+    
+    if new_node and new_node.id == node.id
+      node.delete_with_history!(new_node, @user)
+      render :text => node.version.to_s, :content_type => "text/plain"
+    else
+      render :nothing => true, :status => :bad_request
     end
   end
 
diff --git a/app/controllers/old_node_controller.rb b/app/controllers/old_node_controller.rb
index 2590fd24a..e57623dae 100644
--- a/app/controllers/old_node_controller.rb
+++ b/app/controllers/old_node_controller.rb
@@ -4,42 +4,33 @@ 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
-
-      node.old_nodes.each do |old_node|
-        doc.root << old_node.to_xml_node
-      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
+    node = Node.find(params[:id])
+    
+    doc = OSM::API.new.get_xml_doc
+    
+    node.old_nodes.each do |old_node|
+      doc.root << old_node.to_xml_node
     end
+    
+    render :text => doc.to_s, :content_type => "text/xml"
   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...)
-        render :nothing => true, :status => :not_found
-        return
-      end
-      
-      response.headers['Last-Modified'] = old_node.timestamp.rfc822
-
-      doc = OSM::API.new.get_xml_doc
-      doc.root << old_node.to_xml_node
-
-      render :text => doc.to_s, :content_type => "text/xml"
-    rescue
-      render :nothing => true, :status => :internal_server_error
+    old_node = OldNode.find(:first, :conditions => {:id => params[:id], :version => params[:version]} )
+    if old_node.nil?
+      # (RecordNotFound is not raised with find :first...)
+      render :nothing => true, :status => :not_found
+      return
     end
+    
+    response.headers['Last-Modified'] = old_node.timestamp.rfc822
+    
+    doc = OSM::API.new.get_xml_doc
+    doc.root << old_node.to_xml_node
+
+    render :text => doc.to_s, :content_type => "text/xml"
   end
 end
diff --git a/app/controllers/old_relation_controller.rb b/app/controllers/old_relation_controller.rb
index 65de83278..b98c9da49 100644
--- a/app/controllers/old_relation_controller.rb
+++ b/app/controllers/old_relation_controller.rb
@@ -4,41 +4,32 @@ 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
-
-      relation.old_relations.each do |old_relation|
-        doc.root << old_relation.to_xml_node
-      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
+    relation = Relation.find(params[:id])
+    doc = OSM::API.new.get_xml_doc
+    
+    relation.old_relations.each do |old_relation|
+      doc.root << old_relation.to_xml_node
     end
+    
+    render :text => doc.to_s, :content_type => "text/xml"
   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...)
-        render :nothing => true, :status => :not_found
-        return
-      end
-      
-      response.headers['Last-Modified'] = old_relation.timestamp.rfc822
-
-      doc = OSM::API.new.get_xml_doc
-      doc.root << old_relation.to_xml_node
-
-      render :text => doc.to_s, :content_type => "text/xml"
-    rescue
-      render :nothing => true, :status => :internal_server_error
+    old_relation = OldRelation.find(:first, :conditions => {:id => params[:id], :version => params[:version]} )
+    if old_relation.nil?
+      # (RecordNotFound is not raised with find :first...)
+      render :nothing => true, :status => :not_found
+      return
     end
+    
+    response.headers['Last-Modified'] = old_relation.timestamp.rfc822
+    
+    doc = OSM::API.new.get_xml_doc
+    doc.root << old_relation.to_xml_node
+    
+    render :text => doc.to_s, :content_type => "text/xml"
   end
 end
diff --git a/app/controllers/old_way_controller.rb b/app/controllers/old_way_controller.rb
index 2f1103216..6131c9c3f 100644
--- a/app/controllers/old_way_controller.rb
+++ b/app/controllers/old_way_controller.rb
@@ -4,42 +4,33 @@ 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])
+    way = Way.find(params[:id])
     
-      doc = OSM::API.new.get_xml_doc
-
-      way.old_ways.each do |old_way|
-        doc.root << old_way.to_xml_node
-      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
+    doc = OSM::API.new.get_xml_doc
+    
+    way.old_ways.each do |old_way|
+      doc.root << old_way.to_xml_node
     end
+    
+    render :text => doc.to_s, :content_type => "text/xml"
   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...)
-        render :nothing => true, :status => :not_found
-        return
-      end
-      
-      response.headers['Last-Modified'] = old_way.timestamp.rfc822
-      
-      doc = OSM::API.new.get_xml_doc
-      doc.root << old_way.to_xml_node
-      
-      render :text => doc.to_s, :content_type => "text/xml"
-    rescue
-      render :nothing => true, :status => :internal_server_error
+    old_way = OldWay.find(:first, :conditions => {:id => params[:id], :version => params[:version]} )
+    if old_way.nil?
+      # (RecordNotFound is not raised with find :first...)
+      render :nothing => true, :status => :not_found
+      return
     end
+    
+    response.headers['Last-Modified'] = old_way.timestamp.rfc822
+    
+    doc = OSM::API.new.get_xml_doc
+    doc.root << old_way.to_xml_node
+    
+    render :text => doc.to_s, :content_type => "text/xml"
   end
 end
diff --git a/app/controllers/relation_controller.rb b/app/controllers/relation_controller.rb
index 2171f6cc4..f3c4c9f79 100644
--- a/app/controllers/relation_controller.rb
+++ b/app/controllers/relation_controller.rb
@@ -7,77 +7,55 @@ 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
-      if request.put?
-        relation = Relation.from_xml(request.raw_post, true)
-
-        # 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 :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
+    assert_method :put
+
+    relation = Relation.from_xml(request.raw_post, true)
+    
+    # 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 :text => "Couldn't get turn the input into a relation.", :status => :bad_request
+    #end
   end
 
   def read
-    begin
-      relation = Relation.find(params[:id])
-      response.headers['Last-Modified'] = relation.timestamp.rfc822
-      if relation.visible
-        render :text => relation.to_xml.to_s, :content_type => "text/xml"
-      else
-        render :text => "", :status => :gone
-      end
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-    rescue
-      render :nothing => true, :status => :internal_server_error
+    relation = Relation.find(params[:id])
+    response.headers['Last-Modified'] = relation.timestamp.rfc822
+    if relation.visible
+      render :text => relation.to_xml.to_s, :content_type => "text/xml"
+    else
+      render :text => "", :status => :gone
     end
   end
 
   def update
     logger.debug request.raw_post
-    begin
-      relation = Relation.find(params[:id])
-      new_relation = Relation.from_xml(request.raw_post)
-
-      if new_relation and new_relation.id == relation.id
-        relation.update_from new_relation, @user
-        render :text => relation.version.to_s, :content_type => "text/plain"
-      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
+
+    relation = Relation.find(params[:id])
+    new_relation = Relation.from_xml(request.raw_post)
+    
+    if new_relation and new_relation.id == relation.id
+      relation.update_from new_relation, @user
+      render :text => relation.version.to_s, :content_type => "text/plain"
+    else
+      render :nothing => true, :status => :bad_request
     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
-        relation.delete_with_history!(new_relation, @user)
-        render :text => relation.version.to_s, :content_type => "text/plain"
-      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
+    relation = Relation.find(params[:id])
+    new_relation = Relation.from_xml(request.raw_post)
+    if new_relation and new_relation.id == relation.id
+      relation.delete_with_history!(new_relation, @user)
+      render :text => relation.version.to_s, :content_type => "text/plain"
+    else
+      render :nothing => true, :status => :bad_request
     end
   end
 
@@ -90,71 +68,63 @@ class RelationController < ApplicationController
   # members, plus all nodes part of member ways
   # -----------------------------------------------------------------
   def full
-    begin
-      relation = Relation.find(params[:id])
-
-      if relation.visible
-
-        # first find the ids of nodes, ways and relations referenced by this
-        # relation - note that we exclude this relation just in case.
-
-        node_ids = relation.members.select { |m| m[0] == 'Node' }.map { |m| m[1] }
-        way_ids = relation.members.select { |m| m[0] == 'Way' }.map { |m| m[1] }
-        relation_ids = relation.members.select { |m| m[0] == 'Relation' and m[1] != relation.id }.map { |m| m[1] }
-
-        # next load the relations and the ways.
-
-        relations = Relation.find(relation_ids, :include => [:relation_tags])
-        ways = Way.find(way_ids, :include => [:way_nodes, :way_tags])
-
-        # now additionally collect nodes referenced by ways. Note how we 
-        # recursively evaluate ways but NOT relations.
-
-        way_node_ids = ways.collect { |way|
-           way.way_nodes.collect { |way_node| way_node.node_id }
-        }
-        node_ids += way_node_ids.flatten
-        nodes = Node.find(node_ids.uniq, :include => :node_tags)
+    relation = Relation.find(params[:id])
     
-        # create XML.
-        doc = OSM::API.new.get_xml_doc
-        visible_nodes = {}
-        visible_members = { "Node" => {}, "Way" => {}, "Relation" => {} }
-        changeset_cache = {}
-        user_display_name_cache = {}
-
-        nodes.each do |node|
-          if node.visible? # should be unnecessary if data is consistent.
-            doc.root << node.to_xml_node(changeset_cache, user_display_name_cache)
-            visible_nodes[node.id] = node
-            visible_members["Node"][node.id] = true
-          end
+    if relation.visible
+      
+      # first find the ids of nodes, ways and relations referenced by this
+      # relation - note that we exclude this relation just in case.
+      
+      node_ids = relation.members.select { |m| m[0] == 'Node' }.map { |m| m[1] }
+      way_ids = relation.members.select { |m| m[0] == 'Way' }.map { |m| m[1] }
+      relation_ids = relation.members.select { |m| m[0] == 'Relation' and m[1] != relation.id }.map { |m| m[1] }
+      
+      # next load the relations and the ways.
+      
+      relations = Relation.find(relation_ids, :include => [:relation_tags])
+      ways = Way.find(way_ids, :include => [:way_nodes, :way_tags])
+      
+      # now additionally collect nodes referenced by ways. Note how we 
+      # recursively evaluate ways but NOT relations.
+      
+      way_node_ids = ways.collect { |way|
+        way.way_nodes.collect { |way_node| way_node.node_id }
+      }
+      node_ids += way_node_ids.flatten
+      nodes = Node.find(node_ids.uniq, :include => :node_tags)
+      
+      # create XML.
+      doc = OSM::API.new.get_xml_doc
+      visible_nodes = {}
+      visible_members = { "Node" => {}, "Way" => {}, "Relation" => {} }
+      changeset_cache = {}
+      user_display_name_cache = {}
+      
+      nodes.each do |node|
+        if node.visible? # should be unnecessary if data is consistent.
+          doc.root << node.to_xml_node(changeset_cache, user_display_name_cache)
+          visible_nodes[node.id] = node
+          visible_members["Node"][node.id] = true
         end
-        ways.each do |way|
-          if way.visible? # should be unnecessary if data is consistent.
-            doc.root << way.to_xml_node(visible_nodes, changeset_cache, user_display_name_cache)
-            visible_members["Way"][way.id] = true
-          end
+      end
+      ways.each do |way|
+        if way.visible? # should be unnecessary if data is consistent.
+          doc.root << way.to_xml_node(visible_nodes, changeset_cache, user_display_name_cache)
+          visible_members["Way"][way.id] = true
         end
-        relations.each do |rel|
-          if rel.visible? # should be unnecessary if data is consistent.
-            doc.root << rel.to_xml_node(nil, changeset_cache, user_display_name_cache)
-            visible_members["Relation"][rel.id] = true
-          end
+      end
+      relations.each do |rel|
+        if rel.visible? # should be unnecessary if data is consistent.
+          doc.root << rel.to_xml_node(nil, changeset_cache, user_display_name_cache)
+          visible_members["Relation"][rel.id] = true
         end
-        # finally add self and output
-        doc.root << relation.to_xml_node(visible_members, changeset_cache, user_display_name_cache)
-        render :text => doc.to_s, :content_type => "text/xml"
-
-      else
-        render :nothing => true, :status => :gone
       end
-
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
-
-    rescue
-      render :nothing => true, :status => :internal_server_error
+      # finally add self and output
+      doc.root << relation.to_xml_node(visible_members, changeset_cache, user_display_name_cache)
+      render :text => doc.to_s, :content_type => "text/xml"
+      
+    else
+      render :nothing => true, :status => :gone
     end
   end
 
@@ -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
diff --git a/app/controllers/way_controller.rb b/app/controllers/way_controller.rb
index ab83d4ec3..d41340754 100644
--- a/app/controllers/way_controller.rb
+++ b/app/controllers/way_controller.rb
@@ -7,104 +7,75 @@ 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
-      if request.put?
-        way = Way.from_xml(request.raw_post, true)
+    assert_method :put
 
-        if way
-          way.create_with_history @user
-          render :text => way.id.to_s, :content_type => "text/plain"
-        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
+    way = Way.from_xml(request.raw_post, true)
+    
+    if way
+      way.create_with_history @user
+      render :text => way.id.to_s, :content_type => "text/plain"
+    else
+      render :nothing => true, :status => :bad_request
     end
   end
 
   def read
-    begin
-      way = Way.find(params[:id])
-
-      response.headers['Last-Modified'] = way.timestamp.rfc822
-
-      if way.visible
-        render :text => way.to_xml.to_s, :content_type => "text/xml"
-      else
-        render :text => "", :status => :gone
-      end
-    rescue OSM::APIError => ex
-      render ex.render_opts
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
+    way = Way.find(params[:id])
+    
+    response.headers['Last-Modified'] = way.timestamp.rfc822
+    
+    if way.visible
+      render :text => way.to_xml.to_s, :content_type => "text/xml"
+    else
+      render :text => "", :status => :gone
     end
   end
 
   def update
-    begin
-      way = Way.find(params[:id])
-      new_way = Way.from_xml(request.raw_post)
-
-      if new_way and new_way.id == way.id
-        way.update_from(new_way, @user)
-        render :text => way.version.to_s, :content_type => "text/plain"
-      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
+    way = Way.find(params[:id])
+    new_way = Way.from_xml(request.raw_post)
+    
+    if new_way and new_way.id == way.id
+      way.update_from(new_way, @user)
+      render :text => way.version.to_s, :content_type => "text/plain"
+    else
+      render :nothing => true, :status => :bad_request
     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)
-
-      if new_way and new_way.id == way.id
-        way.delete_with_history!(new_way, @user)
-        render :text => way.version.to_s, :content_type => "text/plain"
-      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
+    way = Way.find(params[:id])
+    new_way = Way.from_xml(request.raw_post)
+    
+    if new_way and new_way.id == way.id
+      way.delete_with_history!(new_way, @user)
+      render :text => way.version.to_s, :content_type => "text/plain"
+    else
+      render :nothing => true, :status => :bad_request
     end
   end
 
   def full
-    begin
-      way = Way.find(params[:id])
-
-      if way.visible
-        nd_ids = way.nds + [-1]
-        nodes = Node.find(:all, :conditions => ["visible = ? AND id IN (#{nd_ids.join(',')})", true])
-
-        # Render
-        doc = OSM::API.new.get_xml_doc
-        nodes.each do |node|
-          doc.root << node.to_xml_node()
-        end
-        doc.root << way.to_xml_node()
-
-        render :text => doc.to_s, :content_type => "text/xml"
-      else
-        render :text => "", :status => :gone
+    way = Way.find(params[:id])
+    
+    if way.visible
+      nd_ids = way.nds + [-1]
+      nodes = Node.find(:all, :conditions => ["visible = ? AND id IN (#{nd_ids.join(',')})", true])
+      
+      # Render
+      doc = OSM::API.new.get_xml_doc
+      nodes.each do |node|
+        doc.root << node.to_xml_node()
       end
-    rescue ActiveRecord::RecordNotFound
-      render :nothing => true, :status => :not_found
+      doc.root << way.to_xml_node()
+      
+      render :text => doc.to_s, :content_type => "text/xml"
+    else
+      render :text => "", :status => :gone
     end
   end
 
diff --git a/app/models/node.rb b/app/models/node.rb
index 742609c0e..e37a10250 100644
--- a/app/models/node.rb
+++ b/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
 
diff --git a/app/models/relation.rb b/app/models/relation.rb
index 8078cf860..1553414e6 100644
--- a/app/models/relation.rb
+++ b/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
 
diff --git a/app/models/way.rb b/app/models/way.rb
index f608dccc8..935a7422c 100644
--- a/app/models/way.rb
+++ b/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
 
diff --git a/lib/osm.rb b/lib/osm.rb
index a392b88a8..274576206 100644
--- a/lib/osm.rb
+++ b/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
diff --git a/test/functional/node_controller_test.rb b/test/functional/node_controller_test.rb
index 609a1767f..266682fd0 100644
--- a/test/functional/node_controller_test.rb
+++ b/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