From: Tom Hughes Date: Mon, 28 Jan 2019 19:04:02 +0000 (+0000) Subject: Merge remote-tracking branch 'upstream/pull/2120' X-Git-Tag: live~2752 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/3e7bc943fe03dcd74666b6f26b3a831e06cac1d8?hp=2f28495d6b041c99536fe494c447c4e93fa79d16 Merge remote-tracking branch 'upstream/pull/2120' --- diff --git a/Gemfile b/Gemfile index 3cf075045..fa1bed468 100644 --- a/Gemfile +++ b/Gemfile @@ -114,6 +114,9 @@ gem "canonical-rails" # Used to generate logstash friendly log files gem "logstasher" +# Used to generate images for traces +gem "gd2-ffij" + # Gems useful for development group :development do gem "annotate" @@ -125,6 +128,7 @@ end # Gems needed for running tests group :test do + gem "fakefs", :require => "fakefs/safe" gem "minitest", "~> 5.1", :platforms => [:ruby_19, :ruby_20] gem "rails-controller-testing" gem "rubocop" diff --git a/Gemfile.lock b/Gemfile.lock index 3b70d157a..36e6ae9dd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -117,10 +117,13 @@ GEM factory_bot_rails (4.11.1) factory_bot (~> 4.11.1) railties (>= 3.0.0) + fakefs (0.18.1) faraday (0.15.4) multipart-post (>= 1.2, < 3) ffi (1.9.25) fspath (3.1.0) + gd2-ffij (0.3.0) + ffi (>= 1.0.0) geoip (1.6.4) globalid (0.4.1) activesupport (>= 4.2.0) @@ -401,7 +404,9 @@ DEPENDENCIES delayed_job_active_record dynamic_form factory_bot_rails + fakefs faraday + gd2-ffij geoip htmlentities http_accept_language (~> 2.0.0) diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb index 253bc4160..2e4997877 100644 --- a/app/controllers/traces_controller.rb +++ b/app/controllers/traces_controller.rb @@ -127,6 +127,7 @@ class TracesController < ApplicationController flash[:notice] = t ".trace_uploaded" flash[:warning] = t ".traces_waiting", :count => current_user.traces.where(:inserted => false).count if current_user.traces.where(:inserted => false).count > 4 + TraceImporterJob.perform_later(@trace) if TRACE_USE_JOB_QUEUE redirect_to :action => :index, :display_name => current_user.display_name else flash[:error] = t("traces.create.upload_failed") if @trace.valid? @@ -210,6 +211,7 @@ class TracesController < ApplicationController trace.visible = false trace.save flash[:notice] = t ".scheduled_for_deletion" + TraceDestroyerJob.perform_later(trace) if TRACE_USE_JOB_QUEUE redirect_to :action => :index, :display_name => trace.user.display_name end rescue ActiveRecord::RecordNotFound diff --git a/app/jobs/application_job.rb b/app/jobs/application_job.rb new file mode 100644 index 000000000..d394c3d10 --- /dev/null +++ b/app/jobs/application_job.rb @@ -0,0 +1,7 @@ +class ApplicationJob < ActiveJob::Base + # Automatically retry jobs that encountered a deadlock + # retry_on ActiveRecord::Deadlocked + + # Most jobs are safe to ignore if the underlying records are no longer available + # discard_on ActiveJob::DeserializationError +end diff --git a/app/jobs/trace_destroyer_job.rb b/app/jobs/trace_destroyer_job.rb new file mode 100644 index 000000000..ff5da5b0a --- /dev/null +++ b/app/jobs/trace_destroyer_job.rb @@ -0,0 +1,10 @@ +class TraceDestroyerJob < ApplicationJob + queue_as :default + + def perform(trace) + trace.destroy + rescue StandardError => ex + logger.info ex.to_s + ex.backtrace.each { |l| logger.info l } + end +end diff --git a/app/jobs/trace_importer_job.rb b/app/jobs/trace_importer_job.rb new file mode 100644 index 000000000..d41de5836 --- /dev/null +++ b/app/jobs/trace_importer_job.rb @@ -0,0 +1,19 @@ +class TraceImporterJob < ApplicationJob + queue_as :default + + def perform(trace) + gpx = trace.import + + if gpx.actual_points.positive? + Notifier.gpx_success(trace, gpx.actual_points).deliver_later + else + Notifier.gpx_failure(trace, "0 points parsed ok. Do they all have lat,lng,alt,timestamp?").deliver_later + trace.destroy + end + rescue StandardError => ex + logger.info ex.to_s + ex.backtrace.each { |l| logger.info l } + Notifier.gpx_failure(trace, ex.to_s + "\n" + ex.backtrace.join("\n")).deliver_later + trace.destroy + end +end diff --git a/config/example.application.yml b/config/example.application.yml index b6884825b..b0b468d9e 100644 --- a/config/example.application.yml +++ b/config/example.application.yml @@ -60,6 +60,10 @@ defaults: &defaults spam_threshold: 50 # Default legale (jurisdiction location) for contributor terms default_legale: GB + # Use the built-in jobs queue for importing traces + # Leave as false if you are using the external high-speed gpx importer + # https://github.com/openstreetmap/gpx-import + trace_use_job_queue: false # Location of GPX traces and images gpx_trace_dir: "/home/osm/traces" gpx_image_dir: "/home/osm/images" diff --git a/lib/daemons/gpx_import.rb b/lib/daemons/gpx_import.rb deleted file mode 100755 index a0344b58c..000000000 --- a/lib/daemons/gpx_import.rb +++ /dev/null @@ -1,59 +0,0 @@ -#!/usr/bin/env ruby - -# You might want to change this -# ENV["RAILS_ENV"] ||= "development" - -require File.dirname(__FILE__) + "/../../config/environment" - -terminated = false - -logger = ActiveRecord::Base.logger - -loop do - ActiveRecord::Base.logger.info("GPX Import daemon wake @ #{Time.now}.") - - Trace.find(:all, :conditions => { :inserted => false, :visible => true }, :order => "id").each do |trace| - Signal.trap("TERM") do - terminated = true - end - - begin - gpx = trace.import - - if gpx.actual_points.positive? - Notifier.gpx_success(trace, gpx.actual_points).deliver - else - Notifier.gpx_failure(trace, "0 points parsed ok. Do they all have lat,lng,alt,timestamp?").deliver - trace.destroy - end - rescue StandardError => ex - logger.info ex.to_s - ex.backtrace.each { |l| logger.info l } - Notifier.gpx_failure(trace, ex.to_s + "\n" + ex.backtrace.join("\n")).deliver - trace.destroy - end - - Signal.trap("TERM", "DEFAULT") - - exit if terminated - end - - Trace.find(:all, :conditions => { :visible => false }, :order => "id").each do |trace| - Signal.trap("TERM") do - terminated = true - end - - begin - trace.destroy - rescue StandardError => ex - logger.info ex.to_s - ex.backtrace.each { |l| logger.info l } - end - - Signal.trap("TERM", "DEFAULT") - - exit if terminated - end - - sleep 5.minutes.value -end diff --git a/lib/daemons/gpx_import_ctl b/lib/daemons/gpx_import_ctl deleted file mode 100755 index 495ce1fb1..000000000 --- a/lib/daemons/gpx_import_ctl +++ /dev/null @@ -1,23 +0,0 @@ -#!/usr/bin/env ruby -require "rubygems" -require "daemons" -require "yaml" -require "erb" - -class Hash - def with_symbols! - each_key { |key| self[key.to_s.to_sym] = self[key] } - self - end -end - -options = YAML.safe_load( - ERB.new( - IO.read( - File.dirname(__FILE__) + "/../../config/daemons.yml" - ) - ).result -).with_symbols! -options[:dir_mode] = options[:dir_mode].to_sym - -Daemons.run File.dirname(__FILE__) + "/gpx_import.rb", options diff --git a/lib/gpx.rb b/lib/gpx.rb index 8510df916..5664e0099 100644 --- a/lib/gpx.rb +++ b/lib/gpx.rb @@ -45,71 +45,47 @@ module GPX end end - def picture(min_lat, min_lon, max_lat, max_lon, num_points) - frames = 10 + def picture(min_lat, min_lon, max_lat, max_lon, _num_points) + # frames = 10 width = 250 height = 250 proj = OSM::Mercator.new(min_lat, min_lon, max_lat, max_lon, width, height) - linegc = Magick::Draw.new - linegc.stroke_linejoin("miter") - linegc.stroke_width(1) - linegc.stroke("#BBBBBB") - linegc.fill("#BBBBBB") - - highlightgc = Magick::Draw.new - highlightgc.stroke_linejoin("miter") - highlightgc.stroke_width(3) - highlightgc.stroke("#000000") - highlightgc.fill("#000000") - - images = Array(frames) do - Magick::Image.new(width, height) do |image| - image.background_color = "white" - image.format = "GIF" - end - end - - oldpx = 0.0 - oldpy = 0.0 + # TODO: create animated gif + # https://github.com/openstreetmap/openstreetmap-website/issues/281 + image = GD2::Image::IndexedColor.new(width, height) - m = 0 - mm = 0 - points do |p| - px = proj.x(p.longitude) - py = proj.y(p.latitude) + black = image.palette.allocate(GD2::Color[0, 0, 0]) + white = image.palette.allocate(GD2::Color[255, 255, 255]) - if m.positive? - frames.times do |n| - gc = if n == mm - highlightgc.dup - else - linegc.dup - end + image.draw do |pen| + pen.color = white + pen.rectangle(0, 0, width, height, true) + end - gc.line(px, py, oldpx, oldpy) + image.draw do |pen| + pen.color = black + pen.anti_aliasing = true + pen.dont_blend = false - gc.draw(images[n]) - end - end + oldpx = 0.0 + oldpy = 0.0 - m += 1 - mm += 1 if m > num_points.to_f / frames.to_f * (mm + 1) + first = true - oldpy = py - oldpx = px - end + points do |p| + px = proj.x(p.longitude) + py = proj.y(p.latitude) - il = Magick::ImageList.new + pen.line(px, py, oldpx, oldpy) unless first - images.each do |f| - il << f + first = false + oldpy = py + oldpx = px + end end - il.delay = 50 - il.format = "GIF" - - il.to_blob + image.gif end def icon(min_lat, min_lon, max_lat, max_lon) @@ -117,34 +93,39 @@ module GPX height = 50 proj = OSM::Mercator.new(min_lat, min_lon, max_lat, max_lon, width, height) - gc = Magick::Draw.new - gc.stroke_linejoin("miter") - gc.stroke_width(1) - gc.stroke("#000000") - gc.fill("#000000") + image = GD2::Image::IndexedColor.new(width, height) - image = Magick::Image.new(width, height) do |i| - i.background_color = "white" - i.format = "GIF" + black = image.palette.allocate(GD2::Color[0, 0, 0]) + white = image.palette.allocate(GD2::Color[255, 255, 255]) + + image.draw do |pen| + pen.color = white + pen.rectangle(0, 0, width, height, true) end - oldpx = 0.0 - oldpy = 0.0 + image.draw do |pen| + pen.color = black + pen.anti_aliasing = true + pen.dont_blend = false + + oldpx = 0.0 + oldpy = 0.0 - first = true + first = true - points do |p| - px = proj.x(p.longitude) - py = proj.y(p.latitude) + points do |p| + px = proj.x(p.longitude) + py = proj.y(p.latitude) - gc.dup.line(px, py, oldpx, oldpy).draw(image) unless first + pen.line(px, py, oldpx, oldpy) unless first - first = false - oldpy = py - oldpx = px + first = false + oldpy = py + oldpx = px + end end - image.to_blob + image.gif end end diff --git a/test/jobs/trace_destroyer_job_test.rb b/test/jobs/trace_destroyer_job_test.rb new file mode 100644 index 000000000..ed86a1616 --- /dev/null +++ b/test/jobs/trace_destroyer_job_test.rb @@ -0,0 +1,18 @@ +require "test_helper" +require "minitest/mock" + +class TraceDestroyerJobTest < ActiveJob::TestCase + def test_destroy_called + trace = Minitest::Mock.new + + # Tiny little bit of mocking to make activejob happy + trace.expect :is_a?, false, [TraceDestroyerJob] + + # Check that trace.destroy is called + trace.expect :destroy, true + + TraceDestroyerJob.perform_now(trace) + + assert_mock trace + end +end diff --git a/test/jobs/trace_importer_job_test.rb b/test/jobs/trace_importer_job_test.rb new file mode 100644 index 000000000..73c8cebbe --- /dev/null +++ b/test/jobs/trace_importer_job_test.rb @@ -0,0 +1,64 @@ +require "test_helper" +require "minitest/mock" + +class TraceImporterJobTest < ActiveJob::TestCase + def test_success_notification + # Check that the user gets a success notification when the trace has valid points + trace = create(:trace) + + gpx = Minitest::Mock.new + def gpx.actual_points + 5 + end + + trace.stub(:import, gpx) do + perform_enqueued_jobs do + TraceImporterJob.perform_now(trace) + end + end + + assert_performed_jobs 1 + + email = ActionMailer::Base.deliveries.last + assert_equal trace.user.email, email.to[0] + assert_match(/success/, email.subject) + end + + def test_failure_notification + # Check that the user gets a failure notification when the trace has no valid points + trace = create(:trace) + + gpx = Minitest::Mock.new + def gpx.actual_points + 0 + end + + trace.stub(:import, gpx) do + perform_enqueued_jobs do + TraceImporterJob.perform_now(trace) + end + end + + assert_performed_jobs 1 + + email = ActionMailer::Base.deliveries.last + assert_equal trace.user.email, email.to[0] + assert_match(/failure/, email.subject) + end + + def test_error_notification + # Check that the user gets a failure notification when something goes badly wrong + trace = create(:trace) + trace.stub(:import, -> { raise }) do + perform_enqueued_jobs do + TraceImporterJob.perform_now(trace) + end + end + + assert_performed_jobs 1 + + email = ActionMailer::Base.deliveries.last + assert_equal trace.user.email, email.to[0] + assert_match(/failure/, email.subject) + end +end diff --git a/test/models/trace_test.rb b/test/models/trace_test.rb index 4a6ac109d..8bae041fc 100644 --- a/test/models/trace_test.rb +++ b/test/models/trace_test.rb @@ -192,6 +192,66 @@ class TraceTest < ActiveSupport::TestCase trace.destroy end + # When testing the trace.import method, care needs to be taken regarding the icon + # fixture files, since the fixtures could be overwritten by newly generated files. + # We use FakeFS to temporarily protect the real fixture files from being overwritten. + + def test_import_removes_previous_tracepoints + FakeFS do + FakeFS::FileSystem.clone(Rails.root.join("test", "gpx")) + trace = create(:trace, :fixture => "a") + # Tracepoints don't have a primary key, so we use a specific latitude to + # check for successful deletion + create(:tracepoint, :latitude => 54321, :trace => trace) + assert_equal 1, Tracepoint.where(:latitude => 54321).count + + trace.import + + assert_equal 0, Tracepoint.where(:latitude => 54321).count + end + end + + def test_import_creates_tracepoints + FakeFS do + FakeFS::FileSystem.clone(Rails.root.join("test", "gpx")) + trace = create(:trace, :fixture => "a") + assert_equal 0, Tracepoint.where(:gpx_id => trace.id).count + + trace.import + + trace.reload + assert_equal 1, Tracepoint.where(:gpx_id => trace.id).count + end + end + + def test_import_creates_icon + FakeFS do + FakeFS::FileSystem.clone(Rails.root.join("test", "gpx")) + trace = create(:trace, :fixture => "a") + icon_path = File.join(GPX_IMAGE_DIR, "#{trace.id}_icon.gif") + FileUtils.rm(icon_path) + assert_equal false, File.exist?(icon_path) + + trace.import + + assert_equal true, File.exist?(icon_path) + end + end + + def test_import_creates_large_picture + FakeFS do + FakeFS::FileSystem.clone(Rails.root.join("test", "gpx")) + trace = create(:trace, :fixture => "a") + large_picture_path = File.join(GPX_IMAGE_DIR, "#{trace.id}.gif") + FileUtils.rm(large_picture_path) + assert_equal false, File.exist?(large_picture_path) + + trace.import + + assert_equal true, File.exist?(large_picture_path) + end + end + private def check_query(query, traces)