Import the files we can read instead of discarding the whole upload

lib/spatial_features.rb

@@ -28,6 +28,7 @@
 require 'spatial_features/importers/kml_file_arcgis'
 require 'spatial_features/importers/geomark'
 require 'spatial_features/importers/shapefile'
+require 'spatial_features/importers/unreadable_file'
 
 require 'spatial_features/engine'
 

lib/spatial_features/has_spatial_features/feature_import.rb

@@ -6,6 +6,10 @@ module FeatureImport
     extend ActiveSupport::Concern
     include QueuedSpatialProcessing
 
+    # Read by submitters as often as by staff, so it says what happened rather than which
+    # method failed. Any per-file reasons are appended after it.
+    EMPTY_IMPORT_MESSAGE = "No mapped areas could be imported.".freeze
+
     included do
       extend ActiveModel::Callbacks
       define_model_callbacks :update_features
@@ -49,7 +53,7 @@ def update_features!(skip_invalid: false, allow_blank: false, force: false, **op
           store_feature_update_warnings(import_warnings)
 
           if imports.present? && features.compact_blank.empty? && !allow_blank
-            raise EmptyImportError, ["No spatial features were found when updating.", *import_warnings].join(' ')
+            raise EmptyImportError, [EMPTY_IMPORT_MESSAGE, *import_warnings].join(' ')
           end
         end
       end
@@ -105,14 +109,30 @@ def spatial_feature_imports(import_options, make_valid, tmpdir)
           options = {}
         end
 
-        Array.wrap(send(data_method)).flat_map do |data|
+        Array.wrap(send(data_method)).each_with_index.flat_map do |data, index|
           next unless data.present?
 
-          spatial_importer_from_name(importer_name).create_all(data, **options, make_valid: make_valid, tmpdir: tmpdir)
+          source_tmpdir = spatial_import_tmpdir(tmpdir, data_method, index)
+
+          begin
+            spatial_importer_from_name(importer_name).create_all(data, **options, make_valid: make_valid, tmpdir: source_tmpdir)
+          rescue ImportError, Zip::Error, Errno::ENOENT => e
+            # One unreadable file must not discard the geometry of the files uploaded
+            # beside it. Stand in for it so the rest of the import proceeds and the
+            # reason reaches the user as a warning against that file.
+            Importers::UnreadableFile.new(data, e, **options, make_valid: make_valid, tmpdir: source_tmpdir)
+          end
         end
       end.compact
     end
 
+    # Every source file unpacks into its own directory. Two KMZs on one record both hold a
+    # `doc.kml`, so sharing one tmpdir means the second extraction collides with the first
+    # and the whole import dies on an archive that is perfectly fine.
+    def spatial_import_tmpdir(tmpdir, data_method, index)
+      ::File.join(tmpdir, "#{data_method}-#{index}").tap {|dir| FileUtils.mkdir_p(dir) }
+    end
+
     def spatial_importer_from_name(importer_name)
       "SpatialFeatures::Importers::#{importer_name}".constantize
     end
@@ -133,7 +153,7 @@ def import_features(imports, skip_invalid)
       features.delete_all
       valid, invalid = Feature.defer_aggregate_refresh do
         Feature.without_caching_derivatives do
-          imports.flat_map(&:features).partition do |feature|
+          imports.flat_map {|import| features_from(import) }.partition do |feature|
             feature.spatial_model = self
             if feature.save
               handle_images(feature)
@@ -167,6 +187,16 @@ def import_features(imports, skip_invalid)
       valid
     end
 
+    # Parse failures surface lazily, when an importer's features are first read (e.g. a
+    # shapefile archive missing its `.shx`), so they need the same containment as a file
+    # that couldn't be opened at all: record the reason against that source and keep going.
+    def features_from(import)
+      import.features
+    rescue ImportError => e
+      import.warnings << e.message
+      []
+    end
+
     def features_cache_key_matches?(cache_key)
       has_spatial_features_hash? && cache_key == features_hash
     end

lib/spatial_features/has_spatial_features/queued_spatial_processing.rb

@@ -102,8 +102,10 @@ def queued_feature_update_jobs
       spatial_processing_jobs('update_features!').where(failed_at: nil, locked_at: nil)
     end
 
+    # Most recent first: a record that has failed more than once must report why the
+    # current attempt failed, not whichever row the database happened to return.
     def failed_feature_update_jobs
-      spatial_processing_jobs('update_features!').where.not(failed_at: nil)
+      spatial_processing_jobs('update_features!').where.not(failed_at: nil).order(failed_at: :desc)
     end
 
     def spatial_processing_jobs(method_name = nil)

lib/spatial_features/importers/file.rb

@@ -3,16 +3,27 @@
 module SpatialFeatures
   module Importers
     class File < SimpleDelegator
-      INVALID_ARCHIVE = "Archive did not contain a .kml, .shp, .json, or .geojson file.".freeze
-      SUPPORTED_FORMATS = "Supported formats are KMZ, KML, zipped ArcGIS shapefiles, ESRI JSON, and GeoJSON.".freeze
+      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
 
       FILE_PATTERNS = [/\.kml$/, /\.shp$/, /\.json$/, /\.geojson$/]
       def self.create_all(data, **options)
         Download.open_each(data, unzip: FILE_PATTERNS, downcase: true, tmpdir: options[:tmpdir]).map do |file|
           new(data, **options, current_file: file)
         end
-      rescue Unzip::PathNotFound
-        raise ImportError, INVALID_ARCHIVE + " " + SUPPORTED_FORMATS
+      rescue Unzip::PathNotFound => e
+        raise ImportError, invalid_archive_message(e)
+      end
+
+      # Name what the archive held instead of only what we needed — a submitter who is
+      # told their upload contains PDFs knows immediately that they attached the wrong
+      # file, where "did not contain a .shp" leaves them guessing.
+      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?
+
+        [INVALID_ARCHIVE, contents, " ", SUPPORTED_FORMATS].compact.join
       end
 
       # The File importer may be initialized multiple times by `::create_all` if it
@@ -24,8 +35,8 @@ def self.create_all(data, **options)
       def initialize(data, current_file: nil, **options)
         begin
           @current_file = current_file || Download.open_each(data, unzip: FILE_PATTERNS, downcase: true, tmpdir: options[:tmpdir]).first
-        rescue Unzip::PathNotFound
-          raise ImportError, INVALID_ARCHIVE
+        rescue Unzip::PathNotFound => e
+          raise ImportError, self.class.invalid_archive_message(e)
         end
 
         case ::File.extname(data).downcase
@@ -43,14 +54,14 @@ def initialize(data, current_file: nil, **options)
         when '.json', '.geojson'
           __setobj__(ESRIGeoJSON.new(@current_file.path, **options))
         else
-          import_error
+          import_error!
         end
       end
 
       private
 
       def import_error!
-        raise ImportError, "Could not import #{filename}. " + SUPPORTED_FORMATS
+        raise ImportError, "#{::File.basename(filename)} isn't a file type we can read. " + SUPPORTED_FORMATS
       end
 
       def filename

lib/spatial_features/importers/kml.rb

@@ -47,10 +47,32 @@ def kml_document
           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')
           discard_network_links(doc)
+          discard_overlays(doc)
           doc
         end
       end
 
+      # Overlays drape a georeferenced picture over the map — often a WMS raster exported
+      # from a government catalogue — instead of describing an area, so there is no
+      # geometry in them to import. A PhotoOverlay also carries a `<Point>` marking where
+      # the photo was taken, which is not a footprint either, so the nodes are removed
+      # rather than left for `each_record` to pick up. Treated like NetworkLinks: any real
+      # geometry in the file still imports, and a file that is nothing but overlays fails
+      # with a reason that tells the user what they actually uploaded.
+      OVERLAY_ELEMENTS = %w[GroundOverlay PhotoOverlay ScreenOverlay].freeze
+
+      def discard_overlays(doc)
+        overlays = doc.search(*OVERLAY_ELEMENTS)
+        return if overlays.empty?
+
+        names = overlays.map {|overlay| overlay.at_css('name')&.text.presence }.compact.uniq
+        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."
+
+        overlays.remove
+      end
+
       # NetworkLinks reference geometry hosted elsewhere (e.g. a remote KMZ) rather
       # than embedding it, so there is nothing for us to import from them. Rather than
       # failing the whole file, we drop them and record a warning so any embedded
@@ -63,7 +85,8 @@ def discard_network_links(doc)
 
         names = network_links.map {|link| link.at_css('name')&.text.presence }.compact.uniq
         described = names.any? ? ": #{names.to_sentence}" : ''
-        @warnings << "Skipped #{network_links.size} network-linked #{'layer'.pluralize(network_links.size)} that reference remote data and cannot be imported#{described}."
+        @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."
 
         network_links.remove
       end

lib/spatial_features/importers/shapefile.rb

@@ -21,8 +21,11 @@ def self.create_all(data, **options)
         Download.open_each(data, unzip: [/\.shp$/], downcase: true).map do |file|
           new(file, **options)
         end
-      rescue Unzip::PathNotFound
-        raise ImportError, INVALID_ARCHIVE
+      rescue Unzip::PathNotFound => e
+        # `INVALID_ARCHIVE` lives on `Importers::File` and isn't in this class's constant
+        # lookup path, so the bare reference here raised NameError instead of the intended
+        # ImportError whenever a model imported with `'Shapefile'` directly.
+        raise ImportError, File.invalid_archive_message(e)
       end
 
       private
@@ -36,7 +39,9 @@ def each_record
       rescue Errno::ENOENT => e
         case e.message
         when /No such file or directory @ rb_sysopen - (.+)/
-          raise IncompleteShapefileArchive, "Shapefile archive is missing a required file: #{::File.basename($1)}"
+          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."
         else
           raise e
         end
@@ -104,7 +109,7 @@ def possible_shp_files
         @possible_shp_files ||= begin
           Download.open_each(archive, unzip: /\.shp$/, downcase: true)
         rescue Unzip::PathNotFound
-          raise ::SpatialFeatures::Importers::IncompleteShapefileArchive, "Shapefile archive is missing a SHP file"
+          raise ::SpatialFeatures::Importers::IncompleteShapefileArchive, "This archive has no shapefile (.shp) in it."
         end
       end
 

lib/spatial_features/importers/unreadable_file.rb

@@ -0,0 +1,47 @@
+require 'digest/md5'
+
+module SpatialFeatures
+  module Importers
+    # Stands in for a source file we couldn't read at all — an archive with no map data in
+    # it, a shapefile missing a component, an unsupported format. It behaves like an
+    # importer that found nothing and carries the reason as a warning, so the files
+    # uploaded beside it still import and the user is told which file was skipped and why.
+    #
+    # When every source is unreadable the import ends up empty and `update_features!`
+    # raises `EmptyImportError` with these warnings as the reason, the same way a file
+    # containing only NetworkLinks does.
+    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
+
+      def initialize(data, error, **options)
+        super(data, **options)
+        self.source_identifier ||= ::File.basename(data.to_s)
+        @warnings << reason_for(error)
+      end
+
+      # Include the reason so that fixing an importer (or the user re-uploading) produces a
+      # different key and the record re-imports rather than matching its cached hash.
+      def cache_key
+        @cache_key ||= Digest::MD5.hexdigest([@data, *@warnings].join)
+      end
+
+      private
+
+      def reason_for(error)
+        case error
+        when ImportError then error.message
+        when Errno::ENOENT then MISSING
+        else UNREADABLE
+        end
+      end
+
+      def each_record
+        # Nothing could be read, so there is nothing to yield.
+      end
+    end
+  end
+end

lib/spatial_features/unzip.rb

@@ -5,17 +5,52 @@ module Unzip
     # paths containing '__macosx' or beginning with a '.'
     IGNORED_ENTRY_PATHS = /(\A|\/)(__macosx|\.)/i.freeze
 
-    def self.paths(file_path, find: nil, **extract_options)
+    # Archives that may themselves hold the file we're looking for. A KMZ is a ZIP with a
+    # KML inside it, and proponents routinely forward a ZIP of per-layer ZIPs rather than
+    # the layers themselves.
+    NESTED_ARCHIVE_PATTERN = /\.(zip|kmz)$/i.freeze
+
+    # How many times we'll unwrap a nested archive before giving up. Two levels covers
+    # every case we've seen in practice (a ZIP of shapefile ZIPs, a ZIP holding a KMZ)
+    # while bounding the work a deeply nested archive can cost us.
+    MAX_NESTING_DEPTH = 2
+
+    def self.paths(file_path, find: nil, depth: 0, **extract_options)
       paths = extract(file_path, **extract_options)
 
       if find = Array.wrap(find).presence
-        paths = paths.select {|path| find.any? {|pattern| path.index(pattern) } }
-        raise(PathNotFound, "Archive did not contain a file matching #{find}") if paths.empty?
+        matches = matching_paths(paths, find)
+        # Only unwrap nested archives once the archive has yielded nothing usable, so
+        # archives that already import keep their current behaviour exactly.
+        matches = paths_in_nested_archives(paths, find: find, depth: depth, **extract_options) if matches.empty?
+        raise(PathNotFound.new("Archive did not contain a file matching #{find}", paths)) if matches.empty?
+        paths = matches
       end
 
       return Array(paths)
     end
 
+    def self.matching_paths(paths, find)
+      paths.select {|path| find.any? {|pattern| path.index(pattern) } }
+    end
+
+    # Unwrap any archives the archive itself contained and search those instead. Each one
+    # extracts into its own directory so identically named entries (`doc.kml` in several
+    # KMZs) don't overwrite one another, and so a shapefile's sibling components stay
+    # beside it for `Validation.validate_shapefile!`.
+    def self.paths_in_nested_archives(paths, find:, depth:, tmpdir: nil, **extract_options)
+      return [] unless depth < MAX_NESTING_DEPTH
+
+      paths.grep(NESTED_ARCHIVE_PATTERN).flat_map do |archive|
+        begin
+          paths(archive, find: find, depth: depth + 1,
+                         tmpdir: Dir.mktmpdir(nil, File.dirname(archive)), **extract_options)
+        rescue PathNotFound, Zip::Error
+          [] # This nested archive held nothing we can use; the others may still.
+        end
+      end
+    end
+
     def self.extract(file_path, tmpdir: nil, downcase: false)
       tmpdir ||= Dir.mktmpdir
       [].tap do |paths|
@@ -55,6 +90,24 @@ def self.is_zip?(file)
 
     # EXCEPTIONS
 
-    class PathNotFound < StandardError; end
+    class PathNotFound < StandardError
+      # The paths the archive did contain, so callers can tell the user what we found
+      # instead of only what we needed.
+      attr_reader :paths
+
+      def initialize(message = nil, paths = [])
+        super(message)
+        @paths = Array(paths)
+      end
+
+      # Distinct file types in the archive, upcased for display (e.g. `["PDF", "PNG"]`).
+      def extensions
+        paths.reject {|path| path.end_with?('/') }
+             .map {|path| File.extname(path).delete('.').upcase }
+             .reject(&:empty?)
+             .uniq
+             .sort
+      end
+    end
   end
 end

lib/spatial_features/validation.rb

@@ -22,11 +22,18 @@ def validate_shapefile!(shp_file, default_proj4_projection: nil)
           component_path = "#{path}.#{ext}"
           next if ::File.file?(component_path) && ::File.readable?(component_path)
 
+          # A shapefile is a set of files that must travel together, and an export that
+          # drops one is the single most common thing wrong with an upload. Name the
+          # missing part and the way out, rather than only the file we couldn't find.
           case ext
             when "prj"
-              raise ::SpatialFeatures::Importers::IndeterminateShapefileProjection, "Shapefile archive is missing a projection file: #{File.basename(component_path)}"
+              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."
             else
-              raise ::SpatialFeatures::Importers::IncompleteShapefileArchive, "Shapefile archive is missing a required file: #{File.basename(component_path)}"
+              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."
             end
         end
 
@@ -41,7 +48,7 @@ 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, "Shapefile archive is missing a SHP file" \
+        raise ::SpatialFeatures::Importers::IncompleteShapefileArchive, "This archive has no shapefile (.shp) in it." \
           unless allow_generic_zip_files
       end
     end