diff --git a/lib/spatial_features/has_spatial_features/feature_import.rb b/lib/spatial_features/has_spatial_features/feature_import.rb index 85ee814..2efa762 100644 --- a/lib/spatial_features/has_spatial_features/feature_import.rb +++ b/lib/spatial_features/has_spatial_features/feature_import.rb @@ -58,6 +58,19 @@ def update_features!(skip_invalid: false, allow_blank: false, force: false, **op end end + # This import produced geometry, so a failure recorded before it no longer describes + # the record. `SpatialProcessingJob`'s hooks are the only thing that records one, and + # an import run outside a job never reaches them. + # + # Conditioned on features rather than on the import returning, because `allow_blank` + # accepts an import that produced none — clearing there would retire the failure on a + # record that still has no geometry. Written directly rather than through + # `#clear_feature_update_error_status`, whose `with_lock` refuses a record holding + # unsaved changes and would turn this successful import into an `ImportError`. + if persisted? && features? && updating_features_failed? + QueuedSpatialProcessing.update_cached_status(self, 'update_features!', nil) + end + return true rescue StandardError => e raise e if e.is_a?(EmptyImportError) diff --git a/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb b/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb index 4fc2a30..0f3f72c 100644 --- a/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb +++ b/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb @@ -4,20 +4,26 @@ module QueuedSpatialProcessing mattr_accessor :priority_offset, default: 0 # Offsets the queued priority of spatial tasks. Lower numbers run with higher priority class_methods do - # Records whose most recent feature import failed. `->>` yields NULL for a missing - # key, so a record that has never been imported is not included. + # Returns the records whose most recent queued feature update failed. + # + # @return [ActiveRecord::Relation] empty when the model has no + # `spatial_processing_status_cache` column. `->>` yields NULL for a missing key, so + # a record that has never been imported is excluded. def with_failed_feature_updates + return none unless column_names.include?('spatial_processing_status_cache') + where("spatial_processing_status_cache->>'update_features!' = 'failure'") end - # Re-import everything whose last attempt failed. Nothing else retries these, so a - # record broken by a defect stays broken after the defect is fixed unless something - # sweeps it up. + # Queues a feature update for every record whose most recent queued update failed. + # + # Queuing returns immediately, so a caller does not wait on the imports. Each import + # then runs through `SpatialProcessingJob`, whose hooks record the outcome, including + # for a record whose sources are unchanged and whose import is a no-op. # - # Queued rather than run inline, so a caller (a deploy migration, a console) doesn't - # block on reimporting shapefiles, and so `SpatialProcessingJob`'s callbacks maintain - # `spatial_processing_status_cache` — calling `#update_features!` directly bypasses - # them and leaves a record flagged as failed even when the retry succeeded. + # @param options [Hash] passed to `#delay_update_features!`, so `:priority` sets the + # queue priority and the rest reach `#update_features!`. + # @return [void] def retry_failed_feature_updates!(**options) with_failed_feature_updates.find_each do |record| record.delay_update_features!(**options) @@ -63,6 +69,18 @@ def clear_feature_update_error_status end end + # Returns true when the most recent queued feature update failed. + # + # Reports the state of the queued update rather than the outcome of the last import. + # `SpatialProcessingJob`'s hooks are what record a failure, so an import run outside a job + # never records one. Such an import does clear a failure recorded before it, if it + # produced geometry. + # + # @return [Boolean] false when the model has no `spatial_processing_status_cache` column, + # when no update has been queued, and when the last queued update succeeded or is + # still running. + # @note A record can hold usable features and still answer true, so a caller asking + # whether the geometry is usable wants `#features?` as well. def updating_features_failed? spatial_processing_status(:update_features!) == :failure end diff --git a/lib/spatial_features/version.rb b/lib/spatial_features/version.rb index 0d9b80a..fbfc61f 100644 --- a/lib/spatial_features/version.rb +++ b/lib/spatial_features/version.rb @@ -1,3 +1,3 @@ module SpatialFeatures - VERSION = "3.11.1" + VERSION = "3.11.2" end diff --git a/spec/lib/spatial_features/has_spatial_features/feature_import_spec.rb b/spec/lib/spatial_features/has_spatial_features/feature_import_spec.rb index 867f007..abac410 100644 --- a/spec/lib/spatial_features/has_spatial_features/feature_import_spec.rb +++ b/spec/lib/spatial_features/has_spatial_features/feature_import_spec.rb @@ -486,6 +486,71 @@ def test_files end end + # Inline imports don't run the job hooks that maintain the status, so without this a + # record repaired by an inline import (a nightly rake task, a console) reads as broken + # forever — and is picked up again by every later `::retry_failed_feature_updates!`. + context 'when a record with a recorded failure imports successfully' do + subject do + # `title` is a column the import never writes, so it stays dirty across the import. + new_dummy_class(:spatial_processing_status_cache => :jsonb, :title => :string) do + has_spatial_features :import => { :test_files => :File } + + def test_files + [fixture_file_path("shapefile.zip")] + end + end.create + end + + before do + SpatialFeatures::QueuedSpatialProcessing.update_cached_status(subject, 'update_features!', 'failure') + end + + it 'stops reporting the stale failure' do + expect { subject.update_features!(:force => true) } + .to change { subject.updating_features_failed? } + .from(true).to(false) + end + + it 'is no longer selected for retry' do + subject.update_features!(:force => true) + expect(subject.class.with_failed_feature_updates).not_to include(subject) + end + + # `#clear_feature_update_error_status` locks the row, and `lock!` refuses a record + # holding unsaved changes. Reaching it from here would report a successful import as a + # failure, and would do it only for records that had already failed. + it 'succeeds when the record holds unsaved changes' do + subject.title = 'unsaved' + + expect { subject.update_features!(:force => true) }.not_to raise_error + end + end + + # `allow_blank` accepts an import that produced nothing, so the import completes. The + # record still has no geometry, and the nightly geometry task passes this option over + # every record, so clearing here would retire failures wholesale. + context 'when an import completes without producing features' do + subject do + new_dummy_class(:spatial_processing_status_cache => :jsonb) do + has_spatial_features :import => { :test_files => :File } + + def test_files + [fixture_file_path("archive_without_any_known_file.zip")] + end + end.create + end + + before do + SpatialFeatures::QueuedSpatialProcessing.update_cached_status(subject, 'update_features!', 'failure') + end + + it 'keeps reporting the failure' do + expect { subject.update_features!(:force => true, :allow_blank => true) } + .not_to change { subject.reload.updating_features_failed? } + .from(true) + end + end + context 'when every source file is unreadable' do subject do new_dummy_class(:spatial_processing_status_cache => :jsonb) do diff --git a/spec/lib/spatial_features/has_spatial_features/queued_spatial_processing_spec.rb b/spec/lib/spatial_features/has_spatial_features/queued_spatial_processing_spec.rb index 1e1398c..0b5428a 100644 --- a/spec/lib/spatial_features/has_spatial_features/queued_spatial_processing_spec.rb +++ b/spec/lib/spatial_features/has_spatial_features/queued_spatial_processing_spec.rb @@ -65,6 +65,16 @@ def status!(record, state) expect { klass.retry_failed_feature_updates! }.not_to change { Delayed::Job.count } end + # Every other method in this file guards on the column. Without it the scope raises + # `PG::UndefinedColumn` on a spatial model that does not carry one. + it 'finds nothing on a model with no status column' do + plain = new_dummy_class + plain.create + + expect { plain.with_failed_feature_updates.to_a }.not_to raise_error + expect(plain.with_failed_feature_updates).to be_empty + end + it 'passes options through to the queued job' do klass.create.tap {|record| status!(record, 'failure') }