From 317b8f9d45b25c4060bda337edfb73594834d275 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Sat, 23 Feb 2019 15:53:49 +0100 Subject: [PATCH] Move the trackpoints call into its own controller (and rename to tracepoints) --- app/abilities/ability.rb | 3 +- app/controllers/api/tracepoints_controller.rb | 112 ++++++++++++++ app/controllers/api_controller.rb | 101 ------------ config/routes.rb | 2 +- test/controllers/api/tracepoints_test.rb | 146 ++++++++++++++++++ test/controllers/api_controller_test.rb | 124 ++------------- 6 files changed, 276 insertions(+), 212 deletions(-) create mode 100644 app/controllers/api/tracepoints_controller.rb create mode 100644 test/controllers/api/tracepoints_test.rb diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index f28acd5c3..3568817a3 100644 --- a/app/abilities/ability.rb +++ b/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 diff --git a/app/controllers/api/tracepoints_controller.rb b/app/controllers/api/tracepoints_controller.rb new file mode 100644 index 000000000..56cd36138 --- /dev/null +++ b/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 diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 2e1a07c3c..8f9cb6adc 100644 --- a/app/controllers/api_controller.rb +++ b/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 diff --git a/config/routes.rb b/config/routes.rb index 32e6f7367..d90d928a0 100644 --- a/config/routes.rb +++ b/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" diff --git a/test/controllers/api/tracepoints_test.rb b/test/controllers/api/tracepoints_test.rb new file mode 100644 index 000000000..b9a1c126f --- /dev/null +++ b/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 diff --git a/test/controllers/api_controller_test.rb b/test/controllers/api_controller_test.rb index cb759ec14..9fc0c1567 100644 --- a/test/controllers/api_controller_test.rb +++ b/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 tq - 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 - 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 + get :map + 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_bbox_too_big @badbigbbox.each do |bbox| - %w[trackpoints map].each do |tq| - get tq, :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 + get :map, :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| - %w[trackpoints map].each do |tq| - get tq, :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 + get :map, :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| - %w[trackpoints map].each do |tq| - get tq, :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 + get :map, :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| - %w[trackpoints map].each do |tq| - get tq, :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 + get :map, :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 -- 2.39.5