From f4272be185e5fe578d8e0510b4324e5c1e956170 Mon Sep 17 00:00:00 2001 From: Nicholas Jakobsen Date: Wed, 3 Jun 2026 23:59:37 -0700 Subject: [PATCH] fix: Skip unsupported KML NetworkLinks instead of failing the whole import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A KMZ exported from Google Earth that mixes embedded placemark geometry with `` elements (references to remotely hosted layers) failed to import entirely, surfacing only a generic map data error. `SpatialFeatures::Importers::KML#kml_document` raised `ImportError` the moment it encountered any `NetworkLink`, so the BC Hydro Facilities IVMP KMZ reported by Michelle from Ktunaxa — 133 importable points and polygons alongside two NetworkLinks — produced zero features. Instead of raising `ImportError` when a `NetworkLink` is present, the importer now removes the NetworkLink nodes and records a non-fatal warning naming the skipped layers before importing the embedded geometry. Warnings flow through a new `warnings` accumulator on `Importers::Base`, are aggregated in `update_features!`, and are persisted to `spatial_processing_status_cache` (readable via `feature_update_warnings`) so they survive job completion, since successful `Delayed::Job`s are deleted and cannot be read back. A file containing only NetworkLinks still fails, but now via an `EmptyImportError` whose message names the skipped layers rather than a bare "no features found". Each warning is prefixed with its importer's `source_identifier` (e.g. `archive.zip/layer.kml`), so a multi-file or multi-source import makes clear which file the warning belongs to. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../has_spatial_features/feature_import.rb | 9 ++- .../queued_spatial_processing.rb | 24 ++++++++ lib/spatial_features/importers/base.rb | 7 +++ lib/spatial_features/importers/kml.rb | 19 +++++- lib/spatial_features/version.rb | 2 +- ...ml_file_with_network_link_and_features.kml | 48 +++++++++++++++ .../feature_import_spec.rb | 59 +++++++++++++++++++ .../importers/kml_file_spec.rb | 42 ++++++++++--- spec/support/fixtures.rb | 4 ++ 9 files changed, 204 insertions(+), 10 deletions(-) create mode 100644 spec/fixtures/kml_file_with_network_link_and_features.kml diff --git a/lib/spatial_features/has_spatial_features/feature_import.rb b/lib/spatial_features/has_spatial_features/feature_import.rb index 680e9c7..ed72cb4 100644 --- a/lib/spatial_features/has_spatial_features/feature_import.rb +++ b/lib/spatial_features/has_spatial_features/feature_import.rb @@ -41,8 +41,15 @@ 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. + import_warnings = imports.flat_map do |import| + import.warnings.map {|warning| [import.source_identifier.presence, warning].compact.join(': ') } + end + store_feature_update_warnings(import_warnings) + if imports.present? && features.compact_blank.empty? && !allow_blank - raise EmptyImportError, "No spatial features were found when updating" + raise EmptyImportError, ["No spatial features were found when updating.", *import_warnings].join(' ') end end 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 bd7275e..ac26493 100644 --- a/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb +++ b/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb @@ -45,6 +45,30 @@ 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. + WARNINGS_CACHE_KEY = 'feature_update_warnings'.freeze + + def feature_update_warnings + return [] unless has_attribute?(:spatial_processing_status_cache) + Array(spatial_processing_status_cache[WARNINGS_CACHE_KEY]) + end + + def store_feature_update_warnings(warnings) + return unless has_attribute?(:spatial_processing_status_cache) + + cache = spatial_processing_status_cache + warnings = Array(warnings).reject(&:blank?) + if warnings.present? + cache[WARNINGS_CACHE_KEY] = warnings + else + cache.delete(WARNINGS_CACHE_KEY) + end + self.spatial_processing_status_cache = cache + update_column(:spatial_processing_status_cache, cache) if persisted? && will_save_change_to_spatial_processing_status_cache? + end + def spatial_processing_status(method_name, use_cache: true) if has_attribute?(:spatial_processing_status_cache) update_spatial_processing_status(method_name) unless use_cache diff --git a/lib/spatial_features/importers/base.rb b/lib/spatial_features/importers/base.rb index 43a8b4c..d7b823e 100644 --- a/lib/spatial_features/importers/base.rb +++ b/lib/spatial_features/importers/base.rb @@ -4,12 +4,19 @@ module SpatialFeatures module Importers class Base attr_reader :errors + + # Non-fatal messages about parts of the source that could not be imported but + # did not prevent the remaining features from importing (e.g. unsupported + # elements). Unlike `errors`, warnings never abort the import. + attr_reader :warnings + attr_accessor :source_identifier # An identifier for the source of the features. Used to differentiate groups of features on the spatial model. def initialize(data, make_valid: false, tmpdir: nil, source_identifier: nil, feature_name: ->(record) { record.name }) @make_valid = make_valid @data = data @errors = [] + @warnings = [] @tmpdir = tmpdir @source_identifier = source_identifier @feature_name = feature_name diff --git a/lib/spatial_features/importers/kml.rb b/lib/spatial_features/importers/kml.rb index 7b33f1c..9257b71 100644 --- a/lib/spatial_features/importers/kml.rb +++ b/lib/spatial_features/importers/kml.rb @@ -46,11 +46,28 @@ def kml_document 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') - raise ImportError, "NetworkLink elements are not supported" unless doc.search('NetworkLink').empty? + discard_network_links(doc) doc end 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 + # geometry still imports and the user is told which layers were skipped. If the + # file contained nothing but NetworkLinks the import ends up empty and the + # EmptyImportError surfaces the warning as the reason. + def discard_network_links(doc) + network_links = doc.search('NetworkLink') + return if network_links.empty? + + 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}." + + network_links.remove + end + def blank_feature?(feature) feature.css('coordinates').text.blank? end diff --git a/lib/spatial_features/version.rb b/lib/spatial_features/version.rb index a79d9d3..fd4095f 100644 --- a/lib/spatial_features/version.rb +++ b/lib/spatial_features/version.rb @@ -1,3 +1,3 @@ module SpatialFeatures - VERSION = "3.10.1" + VERSION = "3.10.2" end diff --git a/spec/fixtures/kml_file_with_network_link_and_features.kml b/spec/fixtures/kml_file_with_network_link_and_features.kml new file mode 100644 index 0000000..eacd58e --- /dev/null +++ b/spec/fixtures/kml_file_with_network_link_and_features.kml @@ -0,0 +1,48 @@ + + + + kml_file_with_network_link_and_features.kml + + Remote folder + + Remote Layer + + https://example.com/remote.kmz + onRequest + + + + + 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/lib/spatial_features/has_spatial_features/feature_import_spec.rb b/spec/lib/spatial_features/has_spatial_features/feature_import_spec.rb index 6c79824..ab3c37b 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 @@ -330,6 +330,65 @@ def test_kml end end + context 'when the source contains NetworkLinks alongside embedded features' do + subject do + new_dummy_class(:spatial_processing_status_cache => :jsonb) do + has_spatial_features :import => { :test_kml => :KMLFile } + + def test_kml + fixture_file_path("kml_file_with_network_link_and_features.kml") + end + end.create + end + + it 'imports the embedded features' do + subject.update_features! + expect(subject.features.count).to eq(2) + end + + 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)) + end + end + + context 'when the source contains only NetworkLinks' do + subject do + new_dummy_class(:spatial_processing_status_cache => :jsonb) do + has_spatial_features :import => { :test_kml => :KMLFile } + + def test_kml + fixture_file_path("kml_file_with_network_link.kml") + end + end.create + end + + it 'raises an EmptyImportError that explains the skipped NetworkLinks' do + expect { subject.update_features! }.to raise_error(SpatialFeatures::EmptyImportError, /network-linked/i) + end + end + + context 'when multiple source files each contain NetworkLinks' 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("kml_file_with_network_link_and_features.kml"), + fixture_file_path("kml_file_with_network_link.kml")] + end + end.create + end + + 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}), + ) + end + end + describe 'spatial caching' do let(:other_class) { new_dummy_class } subject do diff --git a/spec/lib/spatial_features/importers/kml_file_spec.rb b/spec/lib/spatial_features/importers/kml_file_spec.rb index ce647e8..b2cf4a9 100644 --- a/spec/lib/spatial_features/importers/kml_file_spec.rb +++ b/spec/lib/spatial_features/importers/kml_file_spec.rb @@ -55,24 +55,48 @@ end end - shared_examples_for 'kml importer with unimportable file' do |data| + shared_examples_for 'kml importer without any features' do |data| subject { SpatialFeatures::Importers::KMLFile.new(data) } describe '#features' do - it 'raises an exception' do - expect { subject.features }.to raise_exception(SpatialFeatures::ImportError) + it 'has no valid records' do + expect(subject.features.count).to eq(0) end end end - shared_examples_for 'kml importer without any features' do |data| + shared_examples_for 'kml importer that skips network links' do |data| subject { SpatialFeatures::Importers::KMLFile.new(data) } describe '#features' do - it 'has no valid records' do + it 'imports the embedded features' do + expect(subject.features.count).to eq(2) + end + end + + describe '#warnings' do + it 'records a warning naming the skipped network-linked layers' do + subject.features + expect(subject.warnings).to include(a_string_matching(/network-linked/i)) + end + end + end + + shared_examples_for 'kml importer with only network links' do |data| + subject { SpatialFeatures::Importers::KMLFile.new(data) } + + describe '#features' do + it 'imports nothing' do expect(subject.features.count).to eq(0) end end + + describe '#warnings' do + it 'records a warning naming the skipped network-linked layers' do + subject.features + expect(subject.warnings).to include(a_string_matching(/network-linked/i)) + end + end end context 'when given a path to a KML file' do @@ -107,7 +131,11 @@ it_behaves_like 'kml importer with an invalid placemark', kml_file_with_invalid_placemark end - context 'when given KML with a NetworkLink' do - it_behaves_like 'kml importer with unimportable file', kml_file_with_network_link + context 'when given KML with only NetworkLinks' do + it_behaves_like 'kml importer with only network links', kml_file_with_network_link + end + + 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 end diff --git a/spec/support/fixtures.rb b/spec/support/fixtures.rb index 175bebd..f686a20 100644 --- a/spec/support/fixtures.rb +++ b/spec/support/fixtures.rb @@ -26,6 +26,10 @@ def kml_file_with_network_link open_fixture_file("kml_file_with_network_link.kml") end +def kml_file_with_network_link_and_features + open_fixture_file("kml_file_with_network_link_and_features.kml") +end + def kml_file_with_altitude open_fixture_file("kml_file_with_altitude.kml") end