Refactor potlatch error handling.

app/controllers/amf_controller.rb

@@ -45,7 +45,6 @@ class AmfController < ApplicationController
 
   session :off
   before_filter :check_api_writable
-  around_filter :api_call_timeout, :only => [:amf_read]
 
   # Main AMF handlers: process the raw AMF string (using AMF library) and
   # calls each action (private method) accordingly.
@@ -138,7 +137,7 @@ class AmfController < ApplicationController
             when 'putpoi';         r=putpoi(*args)
                     	             if r[2] != r[3] then renumberednodes[r[2]] = r[3] end
                                    results[index]=AMF.putdata(index,r)
-            when 'startchangeset';results[index]=AMF.putdata(index,startchangeset(*args))
+            when 'startchangeset'; results[index]=AMF.putdata(index,startchangeset(*args))
            end
           if results[index][0]==-3 then err=true end  # If a conflict is detected, don't execute any more writes
         end
@@ -152,10 +151,33 @@ class AmfController < ApplicationController
 
   private
 
+  def amf_handle_error(call)
+    yield
+  rescue OSM::APIVersionMismatchError => ex
+    return [-3, [ex.type.downcase, ex.latest, ex.id]]
+  rescue OSM::APIUserChangesetMismatchError => ex
+    return [-2, ex.to_s]
+  rescue OSM::APIBadBoundingBox => ex
+    return [-2, "Sorry - I can't get the map for that area. The server said: #{ex.to_s}"]
+  rescue OSM::APIError => ex
+    return [-1, ex.to_s]
+  rescue Exception => ex
+    return [-2, "An unusual error happened (in #{call}). The server said: #{ex.to_s}"]
+  end
+
+  def amf_handle_error_with_timeout(call)
+    amf_handle_error(call) do
+      Timeout::timeout(APP_CONFIG['api_timeout'], OSM::APITimeoutError) do
+        yield
+      end
+    end
+  end
+
   # Start new changeset
   # Returns success_code,success_message,changeset id
   
   def startchangeset(usertoken, cstags, closeid, closecomment, opennew)
+    amf_handle_error("'startchangeset'") do
       user = getuser(usertoken)
       if !user then return -1,"You are not logged in, so Potlatch can't write any changes to the database." end
 
@@ -164,7 +186,7 @@ class AmfController < ApplicationController
         cs = Changeset.find(closeid)
         cs.set_closed_time_now
         if cs.user_id!=user.id
-        return -2,"You cannot close that changeset because you're not the person who opened it.",nil
+          raise OSM::APIUserChangesetMismatchError.new
         elsif closecomment.empty?
           cs.save!
         else
@@ -188,6 +210,7 @@ class AmfController < ApplicationController
         return [0,'',nil]
       end
     end
+  end
 
   # Return presets (default tags, localisation etc.):
   # uses POTLATCH_PRESETS global, set up in OSM::Potlatch.
@@ -224,6 +247,7 @@ class AmfController < ApplicationController
   # used in any way, rel is any relation which refers to either a way
   # or node that we're returning.
   def whichways(xmin, ymin, xmax, ymax) #:doc:
+    amf_handle_error_with_timeout("'whichways'") do
       enlarge = [(xmax-xmin)/8,0.01].min
       xmin -= enlarge; ymin -= enlarge
       xmax += enlarge; ymax += enlarge
@@ -256,39 +280,34 @@ class AmfController < ApplicationController
       end
 
       [0, '', ways, points, relations]
-
+    end
-  rescue OSM::APITimeoutError => err
-    [-1,"Sorry, I can't get the map for that area - try zooming in further. The server said: #{err}"]
-  rescue Exception => err
-    [-2,"Sorry - I can't get the map for that area. The server said: #{err}",[],[],[] ]
   end
 
   # Find deleted ways in current bounding box (similar to whichways, but ways
   # with a deleted node only - not POIs or relations).
 
   def whichways_deleted(xmin, ymin, xmax, ymax) #:doc:
+    amf_handle_error_with_timeout("'whichways_deleted'") do
       enlarge = [(xmax-xmin)/8,0.01].min
       xmin -= enlarge; ymin -= enlarge
       xmax += enlarge; ymax += enlarge
 
       # check boundary is sane and area within defined
       # see /config/application.yml
-    begin
       check_boundaries(xmin, ymin, xmax, ymax)
-    rescue Exception => err
-      return [-2,"Sorry - I can't get the map for that area. The server said: #{err}",[]]
-    end
 
       nodes_in_area = Node.find_by_area(ymin, xmin, ymax, xmax, :conditions => ["current_ways.visible = ?", false], :include => :ways_via_history)
       way_ids = nodes_in_area.collect { |node| node.ways_via_history_ids }.flatten.uniq
 
       [0,'',way_ids]
     end
+  end
 
   # Get a way including nodes and tags.
   # Returns the way id, a Potlatch-style array of points, a hash of tags, and the version number.
 
   def getway(wayid) #:doc:
+    amf_handle_error_with_timeout("'getway' #{wayid}") do
       if POTLATCH_USE_SQL then
         points = sql_get_nodes_in_way(wayid)
         tags = sql_get_tags_in_way(wayid)
@@ -297,11 +316,7 @@ class AmfController < ApplicationController
         # Ideally we would do ":include => :nodes" here but if we do that
         # then rails only seems to return the first copy of a node when a
         # way includes a node more than once
-        begin
+        way = Way.find(:first, :conditions => { :id => wayid }, :include => { :nodes => :node_tags })
-          way = Way.find(wayid, :include => { :nodes => :node_tags })
-        rescue ActiveRecord::RecordNotFound
-          return [-4, 'way', wayid, [], {}, nil]
-        end
 
         # check case where way has been deleted or doesn't exist
         return [-4, 'way', wayid, [], {}, nil] if way.nil? or !way.visible
@@ -317,6 +332,7 @@ class AmfController < ApplicationController
 
       [0, '', wayid, points, tags, version]
     end
+  end
   
   # Get an old version of a way, and all constituent nodes.
   #
@@ -334,6 +350,7 @@ class AmfController < ApplicationController
   # 5. is this the current, visible version? (boolean)
   
   def getway_old(id, timestamp) #:doc:
+    amf_handle_error_with_timeout("'getway_old' #{id}, #{timestamp}") do
       if timestamp == ''
         # undelete
         old_way = OldWay.find(:first, :conditions => ['visible = ? AND id = ?', true, id], :order => 'version DESC')
@@ -363,6 +380,7 @@ class AmfController < ApplicationController
         return [0, '', id, points, old_way.tags, curway.version, (curway.version==old_way.version and curway.visible)]
       end
     end
+  end
   
   # Find history of a way.
   # Returns 'way', id, and an array of previous versions:
@@ -375,7 +393,6 @@ class AmfController < ApplicationController
   # start date of the way.
 
   def getway_history(wayid) #:doc:
-
     begin
       # Find list of revision dates for way and all constituent nodes
       revdates=[]
@@ -432,6 +449,7 @@ class AmfController < ApplicationController
   # Returns array listing GPXs, each one comprising id, name and description.
   
   def findgpx(searchterm, usertoken)
+    amf_handle_error_with_timeout("'findgpx'") do
       user = getuser(usertoken)
       if !uid then return -1,"You must be logged in to search for GPX traces.",[] end
 
@@ -448,6 +466,7 @@ class AmfController < ApplicationController
       end
       [0,'',gpxs]
     end
+  end
 
   # Get a relation with all tags and members.
   # Returns:
@@ -459,15 +478,13 @@ class AmfController < ApplicationController
   # 5. version.
   
   def getrelation(relid) #:doc:
-    begin
+    amf_handle_error("'getrelation' #{relid}") do
-      rel = Relation.find(relid)
+      rel = Relation.find(:first, :conditions => { :id => relid })
-    rescue ActiveRecord::RecordNotFound
-      return [-4, 'relation', relid, {}, [], nil]
-    end
 
       return [-4, 'relation', relid, {}, [], nil] if rel.nil? or !rel.visible
       [0, '', relid, rel.tags, rel.members, rel.version]
     end
+  end
 
   # Find relations with specified name/id.
   # Returns array of relations, each in same form as getrelation.
@@ -497,6 +514,7 @@ class AmfController < ApplicationController
   # 3. version.
 
   def putrelation(renumberednodes, renumberedways, usertoken, changeset_id, version, relid, tags, members, visible) #:doc:
+    amf_handle_error("'putrelation' #{relid}")  do
       user = getuser(usertoken)
       if !user then return -1,"You are not logged in, so the relation could not be saved." end
 
@@ -552,16 +570,7 @@ class AmfController < ApplicationController
       else
         return [0, '', relid, relid, relation.version]
       end
-  rescue OSM::APIChangesetAlreadyClosedError => ex
+   end
-    return [-1, "The changeset #{ex.changeset.id} was closed at #{ex.changeset.closed_at}.", relid, relid, nil]
-  rescue OSM::APIVersionMismatchError => ex
-    a=ex.to_s.match(/(\d+) of (\w+) (\d+)$/)
-    return [-3, ['relation', a[1]], relid, relid, nil]
-  rescue OSM::APIAlreadyDeletedError => ex
-    return [-1, "The relation has already been deleted.", relid, relid, nil]
-  rescue OSM::APIError => ex
-    # Some error that we don't specifically catch
-    return [-2, "An unusual error happened (in 'putrelation' #{relid}). The server said: #{ex}", relid, relid, nil]
   end
 
   # Save a way to the database, including all nodes. Any nodes in the previous
@@ -587,7 +596,7 @@ class AmfController < ApplicationController
   # 5. hash of node versions (node=>version)
 
   def putway(renumberednodes, usertoken, changeset_id, wayversion, originalway, pointlist, attributes, nodes, deletednodes) #:doc:
-
+    amf_handle_error("'putway' #{originalway}") do
       # -- Initialise
 	
       user = getuser(usertoken)
@@ -673,18 +682,7 @@ class AmfController < ApplicationController
       end # transaction
 
       [0, '', originalway, way.id, renumberednodes, way.version, nodeversions, deletednodes]
-  rescue OSM::APIChangesetAlreadyClosedError => ex
+    end
-    return [-1, "Sorry, your changeset #{ex.changeset.id} was closed (at #{ex.changeset.closed_at}).", originalway, originalway, renumberednodes, wayversion, nodeversions, deletednodes]
-  rescue OSM::APIVersionMismatchError => ex
-    a=ex.to_s.match(/(\d+) of (\w+) (\d+)$/)
-    return [-3, ['way', a[1], a[2].downcase, a[3]], originalway, originalway, renumberednodes, wayversion, nodeversions, deletednodes]
-  rescue OSM::APITooManyWayNodesError => ex
-    return [-1, "You have tried to upload a really long way with #{ex.provided} points: only #{ex.max} are allowed.", originalway, originalway, renumberednodes, wayversion, nodeversions, deletednodes]
-  rescue OSM::APIAlreadyDeletedError => ex
-    return [-1, "The point has already been deleted.", originalway, originalway, renumberednodes, wayversion, nodeversions, deletednodes]
-  rescue OSM::APIError => ex
-    # Some error that we don't specifically catch
-    return [-2, "An unusual error happened (in 'putway' #{originalway}). The server said: #{ex}", originalway, originalway, renumberednodes, wayversion, nodeversions, deletednodes]
   end
 
   # Save POI to the database.
@@ -697,6 +695,7 @@ class AmfController < ApplicationController
   # 4. version.
 
   def putpoi(usertoken, changeset_id, version, id, lon, lat, tags, visible) #:doc:
+    amf_handle_error("'putpoi' #{id}") do
       user = getuser(usertoken)
       if !user then return -1,"You are not logged in, so the point could not be saved." end
 
@@ -740,16 +739,7 @@ class AmfController < ApplicationController
       else
         return [0, '', id, node.id, node.version]
       end 
-  rescue OSM::APIChangesetAlreadyClosedError => ex
+    end
-    return [-1, "The changeset #{ex.changeset.id} was closed at #{ex.changeset.closed_at}",id,id,version]
-  rescue OSM::APIVersionMismatchError => ex
-    a=ex.to_s.match(/(\d+) of (\w+) (\d+)$/)
-    return [-3, ['node', a[1]], id,id,version]
-  rescue OSM::APIAlreadyDeletedError => ex
-    return [-1, "The point has already been deleted",id,id,version]
-  rescue OSM::APIError => ex
-    # Some error that we don't specifically catch
-    return [-2, "An unusual error happened (in 'putpoi' #{id}). The server said: #{ex}",id,id,version]
   end
 
   # Read POI from database
@@ -758,6 +748,7 @@ class AmfController < ApplicationController
   # Returns array of id, long, lat, hash of tags, (current) version.
 
   def getpoi(id,timestamp) #:doc:
+    amf_handle_error("'getpoi' #{id}") do
       n = Node.find(id)
       v = n.version
       unless timestamp == ''
@@ -770,6 +761,7 @@ class AmfController < ApplicationController
         return [-4, 'node', id, nil, nil, {}, nil]
       end
     end
+  end
 
   # Delete way and all constituent nodes.
   # Params:
@@ -782,6 +774,7 @@ class AmfController < ApplicationController
   # Returns 0 (success), unchanged way id, new way version, new node versions.
 
   def deleteway(usertoken, changeset_id, way_id, way_version, deletednodes) #:doc:
+    amf_handle_error("'deleteway' #{way_id}") do
       user = getuser(usertoken)
       unless user then return -1,"You are not logged in, so the way could not be deleted." end
       
@@ -819,16 +812,7 @@ class AmfController < ApplicationController
 
       end # transaction
       [0, '', way_id, old_way.version, nodeversions]
-  rescue OSM::APIChangesetAlreadyClosedError => ex
+    end
-    return [-1, "The changeset #{ex.changeset.id} was closed at #{ex.changeset.closed_at}",way_id,way_version,nodeversions]
-  rescue OSM::APIVersionMismatchError => ex
-    a=ex.to_s.match(/(\d+) of (\w+) (\d+)$/)
-    return [-3, ['way', a[1]],way_id,way_version,nodeversions]
-  rescue OSM::APIAlreadyDeletedError => ex
-    return [-1, "The way has already been deleted.",way_id,way_version,nodeversions]
-  rescue OSM::APIError => ex
-    # Some error that we don't specifically catch
-    return [-2, "An unusual error happened (in 'deleteway' #{way_id}). The server said: #{ex}",way_id,way_version,nodeversions]
   end
 
 

test/functional/amf_controller_test.rb

@@ -552,14 +552,14 @@ class AmfControllerTest < ActionController::TestCase
   # this should be what AMF controller returns when the bbox of a
   # whichways request is invalid or too large.
   def assert_boundary_error(map, msg=nil, error_hint=nil)
-    expected_map = [-2, "Sorry - I can't get the map for that area.#{msg}", [], [], []]
+    expected_map = [-2, "Sorry - I can't get the map for that area.#{msg}"]
     assert_equal expected_map, map, "AMF controller should have returned an error. (#{error_hint})"
   end
 
   # this should be what AMF controller returns when the bbox of a
   # whichways_deleted request is invalid or too large.
   def assert_deleted_boundary_error(map, msg=nil, error_hint=nil)
-    expected_map = [-2, "Sorry - I can't get the map for that area.#{msg}", []]
+    expected_map = [-2, "Sorry - I can't get the map for that area.#{msg}"]
     assert_equal expected_map, map, "AMF controller should have returned an error. (#{error_hint})"
   end
 end