From 7c10c451fe6a98326b5b7e898e1deb3d057edeb8 Mon Sep 17 00:00:00 2001 From: Nicholas Jakobsen Date: Sat, 8 Aug 2026 04:36:33 -0700 Subject: [PATCH] feat: Keep an import warning's file apart from its message, and settle the wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A warning was stored as one pre-joined string, `"upload.zip: This file contains no map data."`, and the host app had nothing to lay out but a sentence. Several files in one upload routinely fail the same way, so a reader got the same explanation repeated once per file, joined into a paragraph by `to_sentence`. Storing the pair instead lets a caller show the explanation once and list the files it covers. Warnings are now also kept when an import fails. The transaction that recorded them rolls back with the `EmptyImportError`, so a failed import could previously only explain itself through the exception message — one unbroken paragraph, gone as soon as the job was cleared. The messages themselves followed no single shape: two led with an em dash before naming the missing part, one spliced three clauses together before its first full stop, and one was written for a developer. They now follow one rule. Each says what is wrong with the file, and where it helps, what a valid file holds instead. None tells the reader what to do about it: the gem is handed a file and cannot know whether it was uploaded, fetched from a URL or configured by an administrator, so it cannot know what action is open to whoever reads the message. A host knows its own workflow and is the place to add one. Specifics that an em dash used to introduce sit in the first sentence or in parentheses, which is also how a skipped layer's name now reads. The file is the subject of every message, never the reader and never the library. A gem has no voice to speak in and cannot know who is reading, so `"This isn't a file type we can read."` becomes `"This file type is not supported."` and `"Please upload a KMZ..."` becomes `"Supported formats are KMZ..."`. The encoding error, which disagreed with itself grammatically (`One or more features ... has`) and spelled unsupported with a hyphen, now names the encoding a file needs instead. `Invalid KML document (root node was 'Folder')` was developer-facing — it now names the root element in a sentence a submitter can read. `INVALID_ARCHIVE` changes from "This file doesn't contain any map data." to "This file contains no map data.", and `SUPPORTED_FORMATS` from "Please upload a KMZ, ..." to "Upload a KMZ, ...", so a caller matching on either text needs updating. The README gains an `Upgrading From 3.11 to 3.12` section, the way every earlier breaking change in this gem has been documented. It covers the return type, the pre-joined strings a caller will still meet in records stored before the upgrade, and the one-liner that rebuilds the 3.11 sentence for a caller that wants to keep rendering one. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 21 ++++++++++++++ .../has_spatial_features/feature_import.rb | 19 +++++++++---- .../queued_spatial_processing.rb | 21 +++++++++++--- lib/spatial_features/importers/file.rb | 14 ++++++---- lib/spatial_features/importers/kml.rb | 16 +++++++---- lib/spatial_features/importers/shapefile.rb | 7 +++-- .../importers/unreadable_file.rb | 4 +-- lib/spatial_features/validation.rb | 11 ++++---- lib/spatial_features/version.rb | 2 +- .../feature_import_spec.rb | 28 +++++++++++++------ .../queued_spatial_processing_spec.rb | 23 +++++++++++++++ .../spatial_features/importers/file_spec.rb | 2 +- 12 files changed, 128 insertions(+), 40 deletions(-) diff --git a/README.md b/README.md index 318acfd3..b4c26761 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 85ee8142..44c0a8e0 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 4fc2a307..5d27720b 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 a1e86347..fb407fc8 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 8bd0e9df..1f0c63cc 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 28c2bf41..38dbb806 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 c9e75785..935e783d 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 ad01830f..0cf31fd9 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 0d9b80a1..2b1da83b 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 867f0071..4ed77d63 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 1e1398c8..bc2df94e 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 ec713bf4..68a93646 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