diff --git a/README.md b/README.md index 318acfd..b4c2676 100644 --- a/README.md +++ b/README.md @@ -232,6 +232,27 @@ The gem now relies on virtual columns to set a number of derived column values. end ``` +## Upgrading From 3.11 to 3.12 +`#feature_update_warnings` returns `{'file', 'message'}` hashes rather than pre-joined strings, so a caller can lay the +file and the explanation out separately instead of rendering one sentence. Warnings stored before the upgrade come back +with a nil file, so there is no data to migrate. + +```ruby +record.feature_update_warnings +# => [{ 'file' => 'upload.zip/layer.shp', 'message' => 'This shapefile is missing layer.shx. ...' }] + +# A warning stored by 3.11 or earlier, which names its file inside the message +# => [{ 'file' => nil, 'message' => 'upload.zip: This file contains no map data.' }] + +# The 3.11 string, for a caller that wants to keep rendering a sentence +record.feature_update_warnings.map {|warning| [warning['file'], warning['message']].compact.join(': ') } +``` + +The importer messages were also rewritten, `INVALID_ARCHIVE` and `SUPPORTED_FORMATS` included, so a caller matching on +the text of either constant needs updating. They now state what is wrong with a file and nothing further: the gem is +handed a file and cannot know how it arrived, so it does not tell a reader to re-export or upload anything. A host that +knows its own workflow is the right place to add that. + ## Testing Create a postgres database: diff --git a/lib/spatial_features/has_spatial_features/feature_import.rb b/lib/spatial_features/has_spatial_features/feature_import.rb index 85ee814..44c0a8e 100644 --- a/lib/spatial_features/has_spatial_features/feature_import.rb +++ b/lib/spatial_features/has_spatial_features/feature_import.rb @@ -28,6 +28,8 @@ def update_features!(skip_invalid: false, allow_blank: false, force: false, **op options = options.reverse_merge(spatial_features_options) tmpdir = options.fetch(:tmpdir) { Dir.mktmpdir("ruby_spatial_features") } + import_warnings = [] + ActiveRecord::Base.transaction do imports = spatial_feature_imports(options[:import], options[:make_valid], tmpdir) cache_key = Digest::MD5.hexdigest(imports.collect(&:cache_key).join) @@ -45,21 +47,27 @@ def update_features!(skip_invalid: false, allow_blank: false, force: false, **op update_spatial_cache(options.slice(:spatial_cache)) end - # Attribute each warning to the file it came from (e.g. `archive.zip/layer.kml`) - # so a multi-file or multi-source import makes clear which file was affected. + # Name the file each warning came from (e.g. `archive.zip/layer.kml`) so a multi-file + # or multi-source import makes clear which file was affected. The file is kept apart + # from the message so a reader can lay the two out separately. import_warnings = imports.flat_map do |import| - import.warnings.map {|warning| [import.source_identifier.presence, warning].compact.join(': ') } + import.warnings.map {|warning| { 'file' => import.source_identifier.presence, 'message' => warning } } end store_feature_update_warnings(import_warnings) if imports.present? && features.compact_blank.empty? && !allow_blank - raise EmptyImportError, [EMPTY_IMPORT_MESSAGE, *import_warnings].join(' ') + raise EmptyImportError, [EMPTY_IMPORT_MESSAGE, *import_warnings.map {|warning| warning.values.compact.join(': ') }].join(' ') end end end return true rescue StandardError => e + # The transaction that recorded them has rolled back, so without this a failed import + # explains itself only through the exception message — which reaches a reader as one + # unbroken paragraph, and not at all once the job is cleared. + store_feature_update_warnings(import_warnings) if persisted? && import_warnings.present? + raise e if e.is_a?(EmptyImportError) if skip_invalid @@ -67,7 +75,8 @@ def update_features!(skip_invalid: false, allow_blank: false, force: false, **op return nil elsif ENCODING_ERROR.match?(e.message) raise ImportEncodingError, - "One or more features you are trying to import has text encoded in an un-supported format (#{e.message})", + "This file contains text in an unsupported character encoding (#{e.message}). " \ + "Text must be encoded as UTF-8.", e.backtrace else raise ImportError, e.message, e.backtrace 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..5d27720 100644 --- a/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb +++ b/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb @@ -67,14 +67,27 @@ def updating_features_failed? spatial_processing_status(:update_features!) == :failure end - # Non-fatal messages from the most recent successful feature import (e.g. parts of - # the source that were skipped). Stored alongside the status cache so they survive - # job completion, since successful Delayed::Jobs are deleted and can't be read back. + # Non-fatal messages from the most recent feature import (e.g. parts of the source that + # were skipped). Stored alongside the status cache so they survive job completion, since + # successful Delayed::Jobs are deleted and can't be read back. WARNINGS_CACHE_KEY = 'feature_update_warnings'.freeze + # Returns one entry per warning as `{'file' => String|nil, 'message' => String}`. + # + # The pair is kept apart rather than pre-joined so a caller can lay the two out as it + # sees fit, and group by either one. Joining them here would settle that for every caller. + # + # @note Warnings recorded before this became a pair are plain strings that already read + # `"file: message"`. They are returned whole as the message, which renders as written. def feature_update_warnings return [] unless has_attribute?(:spatial_processing_status_cache) - Array(spatial_processing_status_cache[WARNINGS_CACHE_KEY]) + + Array(spatial_processing_status_cache[WARNINGS_CACHE_KEY]).map do |warning| + case warning + when Hash then { 'file' => warning['file'].presence, 'message' => warning['message'].to_s } + else { 'file' => nil, 'message' => warning.to_s } + end + end end def store_feature_update_warnings(warnings) diff --git a/lib/spatial_features/importers/file.rb b/lib/spatial_features/importers/file.rb index a1e8634..fb407fc 100644 --- a/lib/spatial_features/importers/file.rb +++ b/lib/spatial_features/importers/file.rb @@ -3,8 +3,8 @@ module SpatialFeatures module Importers class File < SimpleDelegator - INVALID_ARCHIVE = "This file doesn't contain any map data.".freeze - SUPPORTED_FORMATS = "Please upload a KMZ, KML, zipped ArcGIS shapefile, ESRI JSON, or GeoJSON file.".freeze + INVALID_ARCHIVE = "This file contains no map data.".freeze + SUPPORTED_FORMATS = "Supported formats are KMZ, KML, zipped ArcGIS shapefile, ESRI JSON, and GeoJSON.".freeze FILE_PATTERNS = [/\.kml$/, /\.shp$/, /\.json$/, /\.geojson$/] def self.create_all(data, **options) @@ -21,9 +21,13 @@ def self.create_all(data, **options) def self.invalid_archive_message(path_not_found) found = path_not_found.extensions count = path_not_found.paths.count {|path| !path.end_with?('/') } - contents = " It contains #{count} #{found.to_sentence} #{'file'.pluralize(count)}." if found.any? + problem = if found.any? + "This file contains no map data, only #{count} #{found.to_sentence} #{'file'.pluralize(count)}." + else + INVALID_ARCHIVE + end - [INVALID_ARCHIVE, contents, " ", SUPPORTED_FORMATS].compact.join + [problem, SUPPORTED_FORMATS].join(' ') end # The File importer may be initialized multiple times by `::create_all` if it @@ -61,7 +65,7 @@ def initialize(data, current_file: nil, **options) private def import_error! - raise ImportError, "#{::File.basename(filename)} isn't a file type we can read. " + SUPPORTED_FORMATS + raise ImportError, "This file type is not supported. " + SUPPORTED_FORMATS end def filename diff --git a/lib/spatial_features/importers/kml.rb b/lib/spatial_features/importers/kml.rb index 8bd0e9d..1f0c63c 100644 --- a/lib/spatial_features/importers/kml.rb +++ b/lib/spatial_features/importers/kml.rb @@ -45,7 +45,13 @@ def kml_document @kml_document ||= begin doc = Nokogiri::XML(@data) doc.remove_namespaces! # We don't care about namespaces since the document is going to be filled with placemark geometry and we want it all without needing to deal with namespaces - raise ImportError, "Invalid KML document (root node was '#{doc.root&.name}')" unless doc.root&.name.to_s.casecmp?('kml') + # Named the root element rather than only calling the document invalid: a KML saved as + # a fragment (root ``) looks fine in a text editor, so "invalid" alone left the + # submitter with nothing to act on. + unless doc.root&.name.to_s.casecmp?('kml') + raise ImportError, "This KML file could not be read: its root element is "\ + "'#{doc.root&.name}', not 'kml'." + end discard_network_links(doc) discard_overlays(doc) doc @@ -66,9 +72,9 @@ def discard_overlays(doc) return if overlays.empty? names = overlays.map {|overlay| overlay.at_css('name')&.text.presence }.compact.uniq - described = names.any? ? ": #{names.to_sentence}" : '' + described = names.any? ? " (#{names.to_sentence})" : '' @warnings << "Skipped #{overlays.size} map #{'image'.pluralize(overlays.size)}#{described}. " \ - "A map image is a picture laid over the map, not a marked area, so there is no boundary to import from it." + "A map image is a picture laid over the map rather than a marked area, so it has no boundary to import." overlays.remove end @@ -84,9 +90,9 @@ def discard_network_links(doc) return if network_links.empty? names = network_links.map {|link| link.at_css('name')&.text.presence }.compact.uniq - described = names.any? ? ": #{names.to_sentence}" : '' + described = names.any? ? " (#{names.to_sentence})" : '' @warnings << "Skipped #{network_links.size} network-linked #{'layer'.pluralize(network_links.size)}#{described}. " \ - "Network links point at data stored somewhere else rather than holding it, so there is nothing to import from them." + "A network link points at data held on another server rather than containing it, so there is nothing to import." network_links.remove end diff --git a/lib/spatial_features/importers/shapefile.rb b/lib/spatial_features/importers/shapefile.rb index 28c2bf4..38dbb80 100644 --- a/lib/spatial_features/importers/shapefile.rb +++ b/lib/spatial_features/importers/shapefile.rb @@ -40,8 +40,8 @@ def each_record case e.message when /No such file or directory @ rb_sysopen - (.+)/ raise IncompleteShapefileArchive, - "This shapefile is incomplete — #{::File.basename($1)} is missing. " \ - "A shapefile is a set of files that have to be zipped up together: .shp, .shx, .dbf and .prj." + "This shapefile is missing #{::File.basename($1)}. " \ + "A shapefile is made up of .shp, .shx, .dbf and .prj files." else raise e end @@ -109,7 +109,8 @@ def possible_shp_files @possible_shp_files ||= begin Download.open_each(archive, unzip: /\.shp$/, downcase: true) rescue Unzip::PathNotFound - raise ::SpatialFeatures::Importers::IncompleteShapefileArchive, "This archive has no shapefile (.shp) in it." + raise ::SpatialFeatures::Importers::IncompleteShapefileArchive, + "This archive has no shapefile (.shp) in it. #{::SpatialFeatures::Importers::File::SUPPORTED_FORMATS}" end end diff --git a/lib/spatial_features/importers/unreadable_file.rb b/lib/spatial_features/importers/unreadable_file.rb index c9e7578..935e783 100644 --- a/lib/spatial_features/importers/unreadable_file.rb +++ b/lib/spatial_features/importers/unreadable_file.rb @@ -14,8 +14,8 @@ class UnreadableFile < Base # Fallbacks for failures that didn't come from an importer, whose own messages would # mean nothing to the person who uploaded the file — and in the missing-file case # would put a server filesystem path in front of them. - UNREADABLE = "This file couldn't be opened. It may be damaged, or saved in a format we can't read.".freeze - MISSING = "This file is no longer available on the server. Please upload it again.".freeze + UNREADABLE = "This file could not be opened. It may be damaged or in an unsupported format.".freeze + MISSING = "This file is no longer available on the server.".freeze def initialize(data, error, **options) super(data, **options) diff --git a/lib/spatial_features/validation.rb b/lib/spatial_features/validation.rb index ad01830..0cf31fd 100644 --- a/lib/spatial_features/validation.rb +++ b/lib/spatial_features/validation.rb @@ -28,12 +28,12 @@ def validate_shapefile!(shp_file, default_proj4_projection: nil) case ext when "prj" raise ::SpatialFeatures::Importers::IndeterminateShapefileProjection, - "This shapefile has no projection file — #{File.basename(component_path)} is missing, " \ - "so there is no way to tell where on the earth it belongs. Re-export it with the projection included." + "This shapefile has no projection file (#{File.basename(component_path)}), so its place on the " \ + "earth is unknown." else raise ::SpatialFeatures::Importers::IncompleteShapefileArchive, - "This shapefile is incomplete — #{File.basename(component_path)} is missing. " \ - "A shapefile is a set of files that have to be zipped up together: .shp, .shx, .dbf and .prj." + "This shapefile is missing #{File.basename(component_path)}. " \ + "A shapefile is made up of .shp, .shx, .dbf and .prj files." end end @@ -48,7 +48,8 @@ def validate_shapefile_archive!(path, default_proj4_projection: nil, allow_gener validate_shapefile!(shp_file, default_proj4_projection: default_proj4_projection) end rescue Unzip::PathNotFound - raise ::SpatialFeatures::Importers::IncompleteShapefileArchive, "This archive has no shapefile (.shp) in it." \ + raise ::SpatialFeatures::Importers::IncompleteShapefileArchive, + "This archive has no shapefile (.shp) in it. #{::SpatialFeatures::Importers::File::SUPPORTED_FORMATS}" \ unless allow_generic_zip_files end end diff --git a/lib/spatial_features/version.rb b/lib/spatial_features/version.rb index 0d9b80a..2b1da83 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.12.0" 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..4ed77d6 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 @@ -375,7 +375,7 @@ def test_kml it 'records the skipped NetworkLinks as a warning' do subject.update_features! - expect(subject.feature_update_warnings).to include(a_string_matching(/network-linked/i)) + expect(subject.feature_update_warnings).to include(a_hash_including('message' => a_string_matching(/network-linked/i))) end end @@ -410,8 +410,8 @@ def test_files it 'attributes each warning to the file it came from' do subject.update_features! expect(subject.feature_update_warnings).to include( - a_string_matching(%r{\Akml_file_with_network_link_and_features\.kml: Skipped}), - a_string_matching(%r{\Akml_file_with_network_link\.kml: Skipped}), + { 'file' => 'kml_file_with_network_link_and_features.kml', 'message' => a_string_matching(/\ASkipped/) }, + { 'file' => 'kml_file_with_network_link.kml', 'message' => a_string_matching(/\ASkipped/) }, ) end end @@ -434,8 +434,9 @@ def test_files it 'records why the unreadable file was skipped, against that file' do subject.update_features! - expect(subject.feature_update_warnings) - .to include(a_string_matching(%r{\Aarchive_without_any_known_file\.zip: .*doesn't contain any map data})) + expect(subject.feature_update_warnings).to include( + { 'file' => 'archive_without_any_known_file.zip', 'message' => a_string_matching(/contains no map data/) } + ) end end @@ -457,7 +458,7 @@ def test_files it 'records the parse failure as a warning rather than discarding the whole import' do subject.update_features! - expect(subject.feature_update_warnings).to include(a_string_matching(/shapefile is incomplete/i)) + expect(subject.feature_update_warnings).to include(a_hash_including('message' => a_string_matching(/shapefile is missing/i))) end end @@ -481,8 +482,8 @@ def test_files it 'says the file is unavailable without disclosing where it was looked for' do subject.update_features! - expect(subject.feature_update_warnings).to include(a_string_matching(/no longer available on the server/)) - expect(subject.feature_update_warnings.join).not_to include("/nonexistent/path") + expect(subject.feature_update_warnings).to include(a_hash_including('message' => a_string_matching(/no longer available on the server/))) + expect(subject.feature_update_warnings.to_s).not_to include("/nonexistent/path") end end @@ -499,7 +500,16 @@ def test_files it 'raises an EmptyImportError carrying the reason' do expect { subject.update_features! } - .to raise_error(SpatialFeatures::EmptyImportError, /doesn't contain any map data/) + .to raise_error(SpatialFeatures::EmptyImportError, /contains no map data/) + end + + # The transaction recording them rolls back with the error, so keeping the warnings takes + # a second write. Without it a failed import can only be explained by the exception text, + # which is one unbroken paragraph and disappears with the job. + it 'keeps the warnings against the record' do + expect { subject.update_features! rescue nil } + .to change { subject.reload.feature_update_warnings } + .to include(a_hash_including('file' => 'archive_without_any_known_file.zip')) end end 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..bc2df94 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 @@ -74,6 +74,29 @@ def status!(record, state) end end + describe '#feature_update_warnings' do + let(:klass) { new_dummy_class(:spatial_processing_status_cache => :jsonb) } + + it 'returns the file and the message apart' do + record.store_feature_update_warnings([{ 'file' => 'upload.zip', 'message' => 'Skipped 1 map image.' }]) + + expect(record.feature_update_warnings) + .to eq([{ 'file' => 'upload.zip', 'message' => 'Skipped 1 map image.' }]) + end + + # Records imported before the pair was stored hold a single pre-joined string. They keep + # rendering as written rather than being guessed apart on a colon a filename may contain. + it 'reads a warning stored as a plain string as the message' do + SpatialFeatures::QueuedSpatialProcessing.update_cached_status(record, 'update_features!', 'failure') + cache = record.spatial_processing_status_cache + cache[SpatialFeatures::QueuedSpatialProcessing::WARNINGS_CACHE_KEY] = ['upload.zip: Skipped 1 map image.'] + record.update_column(:spatial_processing_status_cache, cache) + + expect(record.reload.feature_update_warnings) + .to eq([{ 'file' => nil, 'message' => 'upload.zip: Skipped 1 map image.' }]) + end + end + describe '#clear_feature_update_error_status' do let(:klass) { new_dummy_class(:spatial_processing_status_cache => :jsonb) } diff --git a/spec/lib/spatial_features/importers/file_spec.rb b/spec/lib/spatial_features/importers/file_spec.rb index ec713bf..68a9364 100644 --- a/spec/lib/spatial_features/importers/file_spec.rb +++ b/spec/lib/spatial_features/importers/file_spec.rb @@ -75,7 +75,7 @@ it 'names the file types the archive did contain, so the uploader can see what they attached' do expect { subject.new(archive_without_any_known_file) } - .to raise_exception(SpatialFeatures::ImportError, /contains 1 WHATEVER file/) + .to raise_exception(SpatialFeatures::ImportError, /only 1 WHATEVER file/) end end end