diff --git a/lib/spatial_features.rb b/lib/spatial_features.rb index f279b89..fd6393c 100644 --- a/lib/spatial_features.rb +++ b/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' diff --git a/lib/spatial_features/has_spatial_features/feature_import.rb b/lib/spatial_features/has_spatial_features/feature_import.rb index 4900f30..85ee814 100644 --- a/lib/spatial_features/has_spatial_features/feature_import.rb +++ b/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 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 ac26493..c3027be 100644 --- a/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb +++ b/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) diff --git a/lib/spatial_features/importers/file.rb b/lib/spatial_features/importers/file.rb index 1031bb1..a1e8634 100644 --- a/lib/spatial_features/importers/file.rb +++ b/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 diff --git a/lib/spatial_features/importers/kml.rb b/lib/spatial_features/importers/kml.rb index 9257b71..8bd0e9d 100644 --- a/lib/spatial_features/importers/kml.rb +++ b/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 `` 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 diff --git a/lib/spatial_features/importers/shapefile.rb b/lib/spatial_features/importers/shapefile.rb index d3626bb..28c2bf4 100644 --- a/lib/spatial_features/importers/shapefile.rb +++ b/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 diff --git a/lib/spatial_features/importers/unreadable_file.rb b/lib/spatial_features/importers/unreadable_file.rb new file mode 100644 index 0000000..c9e7578 --- /dev/null +++ b/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 diff --git a/lib/spatial_features/unzip.rb b/lib/spatial_features/unzip.rb index b958f67..c694576 100644 --- a/lib/spatial_features/unzip.rb +++ b/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 diff --git a/lib/spatial_features/validation.rb b/lib/spatial_features/validation.rb index fecc4be..ad01830 100644 --- a/lib/spatial_features/validation.rb +++ b/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 diff --git a/lib/spatial_features/version.rb b/lib/spatial_features/version.rb index 9c282ed..51669ab 100644 --- a/lib/spatial_features/version.rb +++ b/lib/spatial_features/version.rb @@ -1,3 +1,3 @@ module SpatialFeatures - VERSION = "3.10.3" + VERSION = "3.11.0" end diff --git a/spec/fixtures/archive_containing_kmz.zip b/spec/fixtures/archive_containing_kmz.zip new file mode 100644 index 0000000..c645af9 Binary files /dev/null and b/spec/fixtures/archive_containing_kmz.zip differ diff --git a/spec/fixtures/kml_file_with_ground_overlay.kml b/spec/fixtures/kml_file_with_ground_overlay.kml new file mode 100644 index 0000000..ef7fdb3 --- /dev/null +++ b/spec/fixtures/kml_file_with_ground_overlay.kml @@ -0,0 +1,21 @@ + + + + kml_file_with_ground_overlay.kml + + Overlay folder + + Provincial Boundary + + https://example.com/wms?service=wms&request=GetMap + + + 55.4 + 52.9 + -117.4 + -122.5 + + + + + diff --git a/spec/fixtures/kml_file_with_ground_overlay_and_features.kml b/spec/fixtures/kml_file_with_ground_overlay_and_features.kml new file mode 100644 index 0000000..c6f2ab2 --- /dev/null +++ b/spec/fixtures/kml_file_with_ground_overlay_and_features.kml @@ -0,0 +1,53 @@ + + + + kml_file_with_ground_overlay_and_features.kml + + Overlay folder + + Provincial Boundary + + https://example.com/wms?service=wms&request=GetMap + + + 55.4 + 52.9 + -117.4 + -122.5 + + + + + Poly folder + 1 + + Poly 1 + This is a description + + 1 + + + + -104.2021395645545,60.47065556909579,0 -98.5406390057758,60.08348491099549,0 -98.40794749318894,63.7611693880032,0 -105.6700904293509,63.9562028375275,0 -104.2021395645545,60.47065556909579,0 + + + + + + + Poly 2 + This is a description also + + 1 + + + + -106.4303166516638,61.16376575099101,0 -100.7627103394292,61.95514185121152,0 -100.2655658472707,65.8275552135498,0 -107.9516815833805,65.21804505333482,0 -106.4303166516638,61.16376575099101,0 + + + + + + + + diff --git a/spec/fixtures/nested_archive_of_shapefiles.zip b/spec/fixtures/nested_archive_of_shapefiles.zip new file mode 100644 index 0000000..f38c241 Binary files /dev/null and b/spec/fixtures/nested_archive_of_shapefiles.zip differ 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 cb658da..867f007 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 @@ -193,7 +193,9 @@ def test_files end.new tmpdir = Dir.mktmpdir - expect(SpatialFeatures::Unzip).to receive(:extract).with(instance_of(File), :tmpdir => tmpdir, :downcase => true).and_call_original + # Each source unpacks into its own directory beneath the import's tmpdir, so that + # two archives holding identically named entries don't overwrite one another. + expect(SpatialFeatures::Unzip).to receive(:extract).with(instance_of(File), :tmpdir => start_with("#{tmpdir}/"), :downcase => true).and_call_original expect(FileUtils).to receive(:remove_entry).with(tmpdir) subject.update_features!(:tmpdir => tmpdir) end @@ -414,6 +416,112 @@ def test_files end end + context 'when one of several source files cannot be read' 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("shapefile.zip"), fixture_file_path("archive_without_any_known_file.zip")] + end + end.create + end + + it 'still imports the files it could read' do + subject.update_features! + expect(subject.features).to be_present + end + + 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})) + end + end + + context 'when a source file fails while its features are being read' 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("shapefile.zip"), fixture_file_path("shapefile_without_shape_index.zip")] + end + end.create + end + + it 'still imports the files it could read' do + subject.update_features! + expect(subject.features).to be_present + end + + 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)) + end + end + + # The uploader is shown these reasons, so a file missing from disk must not put a + # server filesystem path in front of them. + context 'when a source file is missing from disk' 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("shapefile.zip"), "/nonexistent/path/to/missing_upload.zip"] + end + end.create + end + + it 'still imports the files that are present' do + subject.update_features! + expect(subject.features).to be_present + end + + 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") + end + end + + context 'when every source file is unreadable' 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 + + it 'raises an EmptyImportError carrying the reason' do + expect { subject.update_features! } + .to raise_error(SpatialFeatures::EmptyImportError, /doesn't contain any map data/) + end + end + + # Two KMZs both hold a `doc.kml`. Sharing one directory meant the second extraction + # collided with the first and took the whole import down with it. + context 'when several sources contain identically named entries' 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("test.kmz"), fixture_file_path("test.kmz")] + end + end.create + end + + it 'unpacks each source without collision' do + expect { subject.update_features! }.not_to raise_error + expect(subject.features).to be_present + end + end + describe 'spatial caching' do let(:other_class) { new_dummy_class } subject do diff --git a/spec/lib/spatial_features/importers/file_spec.rb b/spec/lib/spatial_features/importers/file_spec.rb index c7ca292..ec713bf 100644 --- a/spec/lib/spatial_features/importers/file_spec.rb +++ b/spec/lib/spatial_features/importers/file_spec.rb @@ -72,6 +72,11 @@ it 'raises an exception' do expect { subject.new(archive_without_any_known_file) }.to raise_exception(SpatialFeatures::ImportError) end + + 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/) + end end end @@ -126,6 +131,21 @@ end end + # Proponents routinely forward the archive they were emailed — a ZIP holding a ZIP per + # layer, or a ZIP holding a KMZ — rather than the layer files themselves. + context 'when given a zip archive containing other archives' do + it 'unwraps the nested archives and imports each shapefile inside them' do + importers = subject.create_all(nested_archive_of_shapefiles) + expect(importers.flat_map(&:features)).to be_present + expect(importers.count).to eq(2) + end + + it 'unwraps a KMZ nested inside a zip archive' do + importers = subject.create_all(archive_containing_kmz) + expect(importers.flat_map(&:features)).to be_present + end + end + context 'when given a zip archive with multiple kml files' do let(:file) { archive_with_multiple_kmls } diff --git a/spec/lib/spatial_features/importers/kml_file_spec.rb b/spec/lib/spatial_features/importers/kml_file_spec.rb index b2cf4a9..e329ac5 100644 --- a/spec/lib/spatial_features/importers/kml_file_spec.rb +++ b/spec/lib/spatial_features/importers/kml_file_spec.rb @@ -82,6 +82,25 @@ end end + # An overlay drapes a picture over the map instead of marking an area, so it never + # contributes geometry — whether or not the file also holds real placemarks. + shared_examples_for 'kml importer that skips map images' do |data, expected_feature_count| + subject { SpatialFeatures::Importers::KMLFile.new(data) } + + describe '#features' do + it "imports the #{expected_feature_count} embedded features and nothing from the overlay" do + expect(subject.features.count).to eq(expected_feature_count) + end + end + + describe '#warnings' do + it 'records a warning naming the skipped map image' do + subject.features + expect(subject.warnings).to include(a_string_matching(/map image.*Provincial Boundary/i)) + end + end + end + shared_examples_for 'kml importer with only network links' do |data| subject { SpatialFeatures::Importers::KMLFile.new(data) } @@ -138,4 +157,12 @@ context 'when given KML with both NetworkLinks and embedded features' do it_behaves_like 'kml importer that skips network links', kml_file_with_network_link_and_features end + + context 'when given KML with only a GroundOverlay' do + it_behaves_like 'kml importer that skips map images', kml_file_with_ground_overlay, 0 + end + + context 'when given KML with both a GroundOverlay and embedded features' do + it_behaves_like 'kml importer that skips map images', kml_file_with_ground_overlay_and_features, 2 + end end diff --git a/spec/lib/spatial_features/importers/shapefile_spec.rb b/spec/lib/spatial_features/importers/shapefile_spec.rb index 47ee023..dcbc843 100644 --- a/spec/lib/spatial_features/importers/shapefile_spec.rb +++ b/spec/lib/spatial_features/importers/shapefile_spec.rb @@ -41,7 +41,7 @@ let(:subject) { SpatialFeatures::Importers::Shapefile.new(shapefile_without_shape_format) } it 'raises an exception' do - expect { subject.features }.to raise_exception(SpatialFeatures::Importers::IncompleteShapefileArchive, /missing a SHP file/) + expect { subject.features }.to raise_exception(SpatialFeatures::Importers::IncompleteShapefileArchive, /no shapefile \(\.shp\) in it/) end end diff --git a/spec/lib/spatial_features/validation_spec.rb b/spec/lib/spatial_features/validation_spec.rb index b642945..99bca4c 100644 --- a/spec/lib/spatial_features/validation_spec.rb +++ b/spec/lib/spatial_features/validation_spec.rb @@ -10,7 +10,7 @@ it 'performs validation without allow_generic_zip_files option' do expect { SpatialFeatures::Validation.validate_shapefile_archive!(archive_path, allow_generic_zip_files: false) }.to \ - raise_exception(SpatialFeatures::Importers::IncompleteShapefileArchive, /missing a SHP file/i) + raise_exception(SpatialFeatures::Importers::IncompleteShapefileArchive, /no shapefile \(\.shp\) in it/i) end end end diff --git a/spec/support/fixtures.rb b/spec/support/fixtures.rb index f686a20..4bc583a 100644 --- a/spec/support/fixtures.rb +++ b/spec/support/fixtures.rb @@ -93,3 +93,19 @@ def archive_with_multiple_shps def archive_with_multiple_kmls open_fixture_file("archive_with_multiple_kmls.zip") end + +def nested_archive_of_shapefiles + open_fixture_file("nested_archive_of_shapefiles.zip") +end + +def archive_containing_kmz + open_fixture_file("archive_containing_kmz.zip") +end + +def kml_file_with_ground_overlay + open_fixture_file("kml_file_with_ground_overlay.kml") +end + +def kml_file_with_ground_overlay_and_features + open_fixture_file("kml_file_with_ground_overlay_and_features.kml") +end