Move the trackpoints call into its own controller (and rename to tracepoints)

app/abilities/ability.rb

@@ -4,7 +4,7 @@ class Ability
   include CanCan::Ability
 
   def initialize(user)
-    can [:trackpoints, :map, :changes, :permissions], :api
+    can [:map, :changes, :permissions], :api
     can [:relation, :relation_history, :way, :way_history, :node, :node_history,
          :changeset, :note, :new_note, :query], :browse
     can :show, :capability
@@ -22,6 +22,7 @@ class Ability
     can [:search_all, :search_nodes, :search_ways, :search_relations], :search
     can [:trackpoints], :swf
     can [:index, :show, :data, :georss, :picture, :icon], Trace
+    can :index, Tracepoint
     can [:terms, :api_users, :login, :logout, :new, :create, :save, :confirm, :confirm_resend, :confirm_email, :lost_password, :reset_password, :show, :api_read, :auth_success, :auth_failure], User
     can [:index, :show, :blocks_on, :blocks_by], UserBlock
     can [:index, :show], Node

app/controllers/api/tracepoints_controller.rb

@@ -0,0 +1,112 @@
+module Api
+  class TracepointsController < ApplicationController
+    skip_before_action :verify_authenticity_token
+    before_action :api_deny_access_handler
+
+    authorize_resource
+
+    before_action :check_api_readable
+    around_action :api_call_handle_error, :api_call_timeout
+
+    # Get an XML response containing a list of tracepoints that have been uploaded
+    # within the specified bounding box, and in the specified page.
+    def index
+      # retrieve the page number
+      page = params["page"].to_s.to_i
+
+      unless page >= 0
+        report_error("Page number must be greater than or equal to 0")
+        return
+      end
+
+      offset = page * TRACEPOINTS_PER_PAGE
+
+      # Figure out the bbox
+      # check boundary is sane and area within defined
+      # see /config/application.yml
+      begin
+        bbox = BoundingBox.from_bbox_params(params)
+        bbox.check_boundaries
+        bbox.check_size
+      rescue StandardError => err
+        report_error(err.message)
+        return
+      end
+
+      # get all the points
+      ordered_points = Tracepoint.bbox(bbox).joins(:trace).where(:gpx_files => { :visibility => %w[trackable identifiable] }).order("gpx_id DESC, trackid ASC, timestamp ASC")
+      unordered_points = Tracepoint.bbox(bbox).joins(:trace).where(:gpx_files => { :visibility => %w[public private] }).order("gps_points.latitude", "gps_points.longitude", "gps_points.timestamp")
+      points = ordered_points.union_all(unordered_points).offset(offset).limit(TRACEPOINTS_PER_PAGE)
+
+      doc = XML::Document.new
+      doc.encoding = XML::Encoding::UTF_8
+      root = XML::Node.new "gpx"
+      root["version"] = "1.0"
+      root["creator"] = "OpenStreetMap.org"
+      root["xmlns"] = "http://www.topografix.com/GPX/1/0"
+
+      doc.root = root
+
+      # initialise these variables outside of the loop so that they
+      # stay in scope and don't get free'd up by the GC during the
+      # loop.
+      gpx_id = -1
+      trackid = -1
+      track = nil
+      trkseg = nil
+      anon_track = nil
+      anon_trkseg = nil
+      gpx_file = nil
+      timestamps = false
+
+      points.each do |point|
+        if gpx_id != point.gpx_id
+          gpx_id = point.gpx_id
+          trackid = -1
+          gpx_file = Trace.find(gpx_id)
+
+          if gpx_file.trackable?
+            track = XML::Node.new "trk"
+            doc.root << track
+            timestamps = true
+
+            if gpx_file.identifiable?
+              track << (XML::Node.new("name") << gpx_file.name)
+              track << (XML::Node.new("desc") << gpx_file.description)
+              track << (XML::Node.new("url") << url_for(:controller => "/traces", :action => "show", :display_name => gpx_file.user.display_name, :id => gpx_file.id))
+            end
+          else
+            # use the anonymous track segment if the user hasn't allowed
+            # their GPX points to be tracked.
+            timestamps = false
+            if anon_track.nil?
+              anon_track = XML::Node.new "trk"
+              doc.root << anon_track
+            end
+            track = anon_track
+          end
+        end
+
+        if trackid != point.trackid
+          if gpx_file.trackable?
+            trkseg = XML::Node.new "trkseg"
+            track << trkseg
+            trackid = point.trackid
+          else
+            if anon_trkseg.nil?
+              anon_trkseg = XML::Node.new "trkseg"
+              anon_track << anon_trkseg
+            end
+            trkseg = anon_trkseg
+          end
+        end
+
+        trkseg << point.to_xml_node(timestamps)
+      end
+
+      response.headers["Content-Disposition"] = "attachment; filename=\"tracks.gpx\""
+
+      render :xml => doc.to_s
+    end
+  end
+end

app/controllers/api_controller.rb

@@ -8,107 +8,6 @@ class ApiController < ApplicationController
   before_action :setup_user_auth, :only => [:permissions]
   around_action :api_call_handle_error, :api_call_timeout
 
-  # Get an XML response containing a list of tracepoints that have been uploaded
-  # within the specified bounding box, and in the specified page.
-  def trackpoints
-    # retrieve the page number
-    page = params["page"].to_s.to_i
-
-    unless page >= 0
-      report_error("Page number must be greater than or equal to 0")
-      return
-    end
-
-    offset = page * TRACEPOINTS_PER_PAGE
-
-    # Figure out the bbox
-    # check boundary is sane and area within defined
-    # see /config/application.yml
-    begin
-      bbox = BoundingBox.from_bbox_params(params)
-      bbox.check_boundaries
-      bbox.check_size
-    rescue StandardError => err
-      report_error(err.message)
-      return
-    end
-
-    # get all the points
-    ordered_points = Tracepoint.bbox(bbox).joins(:trace).where(:gpx_files => { :visibility => %w[trackable identifiable] }).order("gpx_id DESC, trackid ASC, timestamp ASC")
-    unordered_points = Tracepoint.bbox(bbox).joins(:trace).where(:gpx_files => { :visibility => %w[public private] }).order("gps_points.latitude", "gps_points.longitude", "gps_points.timestamp")
-    points = ordered_points.union_all(unordered_points).offset(offset).limit(TRACEPOINTS_PER_PAGE)
-
-    doc = XML::Document.new
-    doc.encoding = XML::Encoding::UTF_8
-    root = XML::Node.new "gpx"
-    root["version"] = "1.0"
-    root["creator"] = "OpenStreetMap.org"
-    root["xmlns"] = "http://www.topografix.com/GPX/1/0"
-
-    doc.root = root
-
-    # initialise these variables outside of the loop so that they
-    # stay in scope and don't get free'd up by the GC during the
-    # loop.
-    gpx_id = -1
-    trackid = -1
-    track = nil
-    trkseg = nil
-    anon_track = nil
-    anon_trkseg = nil
-    gpx_file = nil
-    timestamps = false
-
-    points.each do |point|
-      if gpx_id != point.gpx_id
-        gpx_id = point.gpx_id
-        trackid = -1
-        gpx_file = Trace.find(gpx_id)
-
-        if gpx_file.trackable?
-          track = XML::Node.new "trk"
-          doc.root << track
-          timestamps = true
-
-          if gpx_file.identifiable?
-            track << (XML::Node.new("name") << gpx_file.name)
-            track << (XML::Node.new("desc") << gpx_file.description)
-            track << (XML::Node.new("url") << url_for(:controller => "traces", :action => "show", :display_name => gpx_file.user.display_name, :id => gpx_file.id))
-          end
-        else
-          # use the anonymous track segment if the user hasn't allowed
-          # their GPX points to be tracked.
-          timestamps = false
-          if anon_track.nil?
-            anon_track = XML::Node.new "trk"
-            doc.root << anon_track
-          end
-          track = anon_track
-        end
-      end
-
-      if trackid != point.trackid
-        if gpx_file.trackable?
-          trkseg = XML::Node.new "trkseg"
-          track << trkseg
-          trackid = point.trackid
-        else
-          if anon_trkseg.nil?
-            anon_trkseg = XML::Node.new "trkseg"
-            anon_track << anon_trkseg
-          end
-          trkseg = anon_trkseg
-        end
-      end
-
-      trkseg << point.to_xml_node(timestamps)
-    end
-
-    response.headers["Content-Disposition"] = "attachment; filename=\"tracks.gpx\""
-
-    render :xml => doc.to_s
-  end
-
   # This is probably the most common call of all. It is used for getting the
   # OSM data for a specified bounding box, usually for editing. First the
   # bounding box (bbox) is checked to make sure that it is sane. All nodes

config/routes.rb

@@ -57,7 +57,7 @@ OpenStreetMap::Application.routes.draw do
 
     get "map" => "api#map"
 
-    get "trackpoints" => "api#trackpoints"
+    get "trackpoints" => "api/tracepoints#index"
 
     get "changes" => "api#changes"
 

test/controllers/api/tracepoints_test.rb

@@ -0,0 +1,146 @@
+require "test_helper"
+
+module Api
+  class TracepointsControllerTest < ActionController::TestCase
+    def setup
+      super
+      @badbigbbox = %w[-0.1,-0.1,1.1,1.1 10,10,11,11]
+      @badmalformedbbox = %w[-0.1 hello
+                             10N2W10.1N2.1W]
+      @badlatmixedbbox = %w[0,0.1,0.1,0 -0.1,80,0.1,70 0.24,54.34,0.25,54.33]
+      @badlonmixedbbox = %w[80,-0.1,70,0.1 54.34,0.24,54.33,0.25]
+      # @badlatlonoutboundsbbox = %w{ 191,-0.1,193,0.1  -190.1,89.9,-190,90 }
+      @goodbbox = %w[-0.1,-0.1,0.1,0.1 51.1,-0.1,51.2,0
+                     -0.1,%20-0.1,%200.1,%200.1 -0.1edcd,-0.1d,0.1,0.1 -0.1E,-0.1E,0.1S,0.1N S0.1,W0.1,N0.1,E0.1]
+      # That last item in the goodbbox really shouldn't be there, as the API should
+      # reall reject it, however this is to test to see if the api changes.
+    end
+
+    ##
+    # test all routes which lead to this controller
+    def test_routes
+      assert_routing(
+        { :path => "/api/0.6/trackpoints", :method => :get },
+        { :controller => "api/tracepoints", :action => "index" }
+      )
+    end
+
+    def test_tracepoints
+      point = create(:trace, :visibility => "public", :latitude => 1, :longitude => 1) do |trace|
+        create(:tracepoint, :trace => trace, :latitude => 1 * GeoRecord::SCALE, :longitude => 1 * GeoRecord::SCALE)
+      end
+      minlon = point.longitude - 0.001
+      minlat = point.latitude - 0.001
+      maxlon = point.longitude + 0.001
+      maxlat = point.latitude + 0.001
+      bbox = "#{minlon},#{minlat},#{maxlon},#{maxlat}"
+      get :index, :params => { :bbox => bbox }
+      assert_response :success
+      assert_select "gpx[version='1.0'][creator='OpenStreetMap.org']", :count => 1 do
+        assert_select "trk" do
+          assert_select "trkseg"
+        end
+      end
+    end
+
+    def test_tracepoints_trackable
+      point = create(:trace, :visibility => "trackable", :latitude => 51.51, :longitude => -0.14) do |trace|
+        create(:tracepoint, :trace => trace, :trackid => 1, :latitude => (51.510 * GeoRecord::SCALE).to_i, :longitude => (-0.140 * GeoRecord::SCALE).to_i)
+        create(:tracepoint, :trace => trace, :trackid => 2, :latitude => (51.511 * GeoRecord::SCALE).to_i, :longitude => (-0.141 * GeoRecord::SCALE).to_i)
+      end
+      minlon = point.longitude - 0.002
+      minlat = point.latitude - 0.002
+      maxlon = point.longitude + 0.002
+      maxlat = point.latitude + 0.002
+      bbox = "#{minlon},#{minlat},#{maxlon},#{maxlat}"
+      get :index, :params => { :bbox => bbox }
+      assert_response :success
+      assert_select "gpx[version='1.0'][creator='OpenStreetMap.org']", :count => 1 do
+        assert_select "trk", :count => 1 do
+          assert_select "trk > trkseg", :count => 2 do |trksegs|
+            trksegs.each do |trkseg|
+              assert_select trkseg, "trkpt", :count => 1 do |trkpt|
+                assert_select trkpt[0], "time", :count => 1
+              end
+            end
+          end
+        end
+      end
+    end
+
+    def test_tracepoints_identifiable
+      point = create(:trace, :visibility => "identifiable", :latitude => 51.512, :longitude => 0.142) do |trace|
+        create(:tracepoint, :trace => trace, :latitude => (51.512 * GeoRecord::SCALE).to_i, :longitude => (0.142 * GeoRecord::SCALE).to_i)
+      end
+      minlon = point.longitude - 0.002
+      minlat = point.latitude - 0.002
+      maxlon = point.longitude + 0.002
+      maxlat = point.latitude + 0.002
+      bbox = "#{minlon},#{minlat},#{maxlon},#{maxlat}"
+      get :index, :params => { :bbox => bbox }
+      assert_response :success
+      assert_select "gpx[version='1.0'][creator='OpenStreetMap.org']", :count => 1 do
+        assert_select "trk", :count => 1 do
+          assert_select "trk>name", :count => 1
+          assert_select "trk>desc", :count => 1
+          assert_select "trk>url", :count => 1
+          assert_select "trkseg", :count => 1 do
+            assert_select "trkpt", :count => 1 do
+              assert_select "time", :count => 1
+            end
+          end
+        end
+      end
+    end
+
+    def test_index_without_bbox
+      get :index
+      assert_response :bad_request
+      assert_equal "The parameter bbox is required, and must be of the form min_lon,min_lat,max_lon,max_lat", @response.body, "A bbox param was expected"
+    end
+
+    def test_traces_page_less_than_0
+      -10.upto(-1) do |i|
+        get :index, :params => { :page => i, :bbox => "-0.1,-0.1,0.1,0.1" }
+        assert_response :bad_request
+        assert_equal "Page number must be greater than or equal to 0", @response.body, "The page number was #{i}"
+      end
+      0.upto(10) do |i|
+        get :index, :params => { :page => i, :bbox => "-0.1,-0.1,0.1,0.1" }
+        assert_response :success, "The page number was #{i} and should have been accepted"
+      end
+    end
+
+    def test_bbox_too_big
+      @badbigbbox.each do |bbox|
+        get :index, :params => { :bbox => bbox }
+        assert_response :bad_request, "The bbox:#{bbox} was expected to be too big"
+        assert_equal "The maximum bbox size is #{MAX_REQUEST_AREA}, and your request was too large. Either request a smaller area, or use planet.osm", @response.body, "bbox: #{bbox}"
+      end
+    end
+
+    def test_bbox_malformed
+      @badmalformedbbox.each do |bbox|
+        get :index, :params => { :bbox => bbox }
+        assert_response :bad_request, "The bbox:#{bbox} was expected to be malformed"
+        assert_equal "The parameter bbox is required, and must be of the form min_lon,min_lat,max_lon,max_lat", @response.body, "bbox: #{bbox}"
+      end
+    end
+
+    def test_bbox_lon_mixedup
+      @badlonmixedbbox.each do |bbox|
+        get :index, :params => { :bbox => bbox }
+        assert_response :bad_request, "The bbox:#{bbox} was expected to have the longitude mixed up"
+        assert_equal "The minimum longitude must be less than the maximum longitude, but it wasn't", @response.body, "bbox: #{bbox}"
+      end
+    end
+
+    def test_bbox_lat_mixedup
+      @badlatmixedbbox.each do |bbox|
+        get :index, :params => { :bbox => bbox }
+        assert_response :bad_request, "The bbox:#{bbox} was expected to have the latitude mixed up"
+        assert_equal "The minimum latitude must be less than the maximum latitude, but it wasn't", @response.body, "bbox: #{bbox}"
+      end
+    end
+  end
+end

test/controllers/api_controller_test.rb

@@ -26,10 +26,6 @@ class ApiControllerTest < ActionController::TestCase
       { :path => "/api/0.6/map", :method => :get },
       { :controller => "api", :action => "map" }
     )
-    assert_routing(
-      { :path => "/api/0.6/trackpoints", :method => :get },
-      { :controller => "api", :action => "trackpoints" }
-    )
     assert_routing(
       { :path => "/api/0.6/changes", :method => :get },
       { :controller => "api", :action => "changes" }
@@ -139,131 +135,41 @@ class ApiControllerTest < ActionController::TestCase
     end
   end
 
-  def test_tracepoints
-    point = create(:trace, :visibility => "public", :latitude => 1, :longitude => 1) do |trace|
-      create(:tracepoint, :trace => trace, :latitude => 1 * GeoRecord::SCALE, :longitude => 1 * GeoRecord::SCALE)
-    end
-    minlon = point.longitude - 0.001
-    minlat = point.latitude - 0.001
-    maxlon = point.longitude + 0.001
-    maxlat = point.latitude + 0.001
-    bbox = "#{minlon},#{minlat},#{maxlon},#{maxlat}"
-    get :trackpoints, :params => { :bbox => bbox }
-    assert_response :success
-    assert_select "gpx[version='1.0'][creator='OpenStreetMap.org']", :count => 1 do
-      assert_select "trk" do
-        assert_select "trkseg"
-      end
-    end
-  end
-
-  def test_tracepoints_trackable
-    point = create(:trace, :visibility => "trackable", :latitude => 51.51, :longitude => -0.14) do |trace|
-      create(:tracepoint, :trace => trace, :trackid => 1, :latitude => (51.510 * GeoRecord::SCALE).to_i, :longitude => (-0.140 * GeoRecord::SCALE).to_i)
-      create(:tracepoint, :trace => trace, :trackid => 2, :latitude => (51.511 * GeoRecord::SCALE).to_i, :longitude => (-0.141 * GeoRecord::SCALE).to_i)
-    end
-    minlon = point.longitude - 0.002
-    minlat = point.latitude - 0.002
-    maxlon = point.longitude + 0.002
-    maxlat = point.latitude + 0.002
-    bbox = "#{minlon},#{minlat},#{maxlon},#{maxlat}"
-    get :trackpoints, :params => { :bbox => bbox }
-    assert_response :success
-    assert_select "gpx[version='1.0'][creator='OpenStreetMap.org']", :count => 1 do
-      assert_select "trk", :count => 1 do
-        assert_select "trk > trkseg", :count => 2 do |trksegs|
-          trksegs.each do |trkseg|
-            assert_select trkseg, "trkpt", :count => 1 do |trkpt|
-              assert_select trkpt[0], "time", :count => 1
-            end
-          end
-        end
-      end
-    end
-  end
-
-  def test_tracepoints_identifiable
-    point = create(:trace, :visibility => "identifiable", :latitude => 51.512, :longitude => 0.142) do |trace|
-      create(:tracepoint, :trace => trace, :latitude => (51.512 * GeoRecord::SCALE).to_i, :longitude => (0.142 * GeoRecord::SCALE).to_i)
-    end
-    minlon = point.longitude - 0.002
-    minlat = point.latitude - 0.002
-    maxlon = point.longitude + 0.002
-    maxlat = point.latitude + 0.002
-    bbox = "#{minlon},#{minlat},#{maxlon},#{maxlat}"
-    get :trackpoints, :params => { :bbox => bbox }
-    assert_response :success
-    assert_select "gpx[version='1.0'][creator='OpenStreetMap.org']", :count => 1 do
-      assert_select "trk", :count => 1 do
-        assert_select "trk>name", :count => 1
-        assert_select "trk>desc", :count => 1
-        assert_select "trk>url", :count => 1
-        assert_select "trkseg", :count => 1 do
-          assert_select "trkpt", :count => 1 do
-            assert_select "time", :count => 1
-          end
-        end
-      end
-    end
-  end
-
   def test_map_without_bbox
-    %w[trackpoints map].each do |tq|
+    get :map
-      get tq
+    assert_response :bad_request
-      assert_response :bad_request
+    assert_equal "The parameter bbox is required, and must be of the form min_lon,min_lat,max_lon,max_lat", @response.body, "A bbox param was expected"
-      assert_equal "The parameter bbox is required, and must be of the form min_lon,min_lat,max_lon,max_lat", @response.body, "A bbox param was expected"
-    end
-  end
-
-  def test_traces_page_less_than_0
-    -10.upto(-1) do |i|
-      get :trackpoints, :params => { :page => i, :bbox => "-0.1,-0.1,0.1,0.1" }
-      assert_response :bad_request
-      assert_equal "Page number must be greater than or equal to 0", @response.body, "The page number was #{i}"
-    end
-    0.upto(10) do |i|
-      get :trackpoints, :params => { :page => i, :bbox => "-0.1,-0.1,0.1,0.1" }
-      assert_response :success, "The page number was #{i} and should have been accepted"
-    end
   end
 
   def test_bbox_too_big
     @badbigbbox.each do |bbox|
-      %w[trackpoints map].each do |tq|
+      get :map, :params => { :bbox => bbox }
-        get tq, :params => { :bbox => bbox }
+      assert_response :bad_request, "The bbox:#{bbox} was expected to be too big"
-        assert_response :bad_request, "The bbox:#{bbox} was expected to be too big"
+      assert_equal "The maximum bbox size is #{MAX_REQUEST_AREA}, and your request was too large. Either request a smaller area, or use planet.osm", @response.body, "bbox: #{bbox}"
-        assert_equal "The maximum bbox size is #{MAX_REQUEST_AREA}, and your request was too large. Either request a smaller area, or use planet.osm", @response.body, "bbox: #{bbox}"
-      end
     end
   end
 
   def test_bbox_malformed
     @badmalformedbbox.each do |bbox|
-      %w[trackpoints map].each do |tq|
+      get :map, :params => { :bbox => bbox }
-        get tq, :params => { :bbox => bbox }
+      assert_response :bad_request, "The bbox:#{bbox} was expected to be malformed"
-        assert_response :bad_request, "The bbox:#{bbox} was expected to be malformed"
+      assert_equal "The parameter bbox is required, and must be of the form min_lon,min_lat,max_lon,max_lat", @response.body, "bbox: #{bbox}"
-        assert_equal "The parameter bbox is required, and must be of the form min_lon,min_lat,max_lon,max_lat", @response.body, "bbox: #{bbox}"
-      end
     end
   end
 
   def test_bbox_lon_mixedup
     @badlonmixedbbox.each do |bbox|
-      %w[trackpoints map].each do |tq|
+      get :map, :params => { :bbox => bbox }
-        get tq, :params => { :bbox => bbox }
+      assert_response :bad_request, "The bbox:#{bbox} was expected to have the longitude mixed up"
-        assert_response :bad_request, "The bbox:#{bbox} was expected to have the longitude mixed up"
+      assert_equal "The minimum longitude must be less than the maximum longitude, but it wasn't", @response.body, "bbox: #{bbox}"
-        assert_equal "The minimum longitude must be less than the maximum longitude, but it wasn't", @response.body, "bbox: #{bbox}"
-      end
     end
   end
 
   def test_bbox_lat_mixedup
     @badlatmixedbbox.each do |bbox|
-      %w[trackpoints map].each do |tq|
+      get :map, :params => { :bbox => bbox }
-        get tq, :params => { :bbox => bbox }
+      assert_response :bad_request, "The bbox:#{bbox} was expected to have the latitude mixed up"
-        assert_response :bad_request, "The bbox:#{bbox} was expected to have the latitude mixed up"
+      assert_equal "The minimum latitude must be less than the maximum latitude, but it wasn't", @response.body, "bbox: #{bbox}"
-        assert_equal "The minimum latitude must be less than the maximum latitude, but it wasn't", @response.body, "bbox: #{bbox}"
-      end
     end
   end