fix: Skip unsupported KML NetworkLinks instead of failing the whole import

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

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

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

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

lib/spatial_features/version.rb

@@ -1,3 +1,3 @@
 module SpatialFeatures
-  VERSION = "3.10.1"
+  VERSION = "3.10.2"
 end

spec/fixtures/kml_file_with_network_link_and_features.kml

@@ -0,0 +1,48 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<kml xmlns="http://www.opengis.net/kml/2.2" xmlns:gx="http://www.google.com/kml/ext/2.2" xmlns:kml="http://www.opengis.net/kml/2.2" xmlns:atom="http://www.w3.org/2005/Atom">
+<Document>
+	<name>kml_file_with_network_link_and_features.kml</name>
+	<Folder>
+		<name>Remote folder</name>
+		<NetworkLink>
+			<name>Remote Layer</name>
+			<Link>
+				<href>https://example.com/remote.kmz</href>
+				<viewRefreshMode>onRequest</viewRefreshMode>
+			</Link>
+		</NetworkLink>
+	</Folder>
+	<Folder>
+		<name>Poly folder</name>
+		<open>1</open>
+		<Placemark>
+			<name>Poly 1</name>
+			<description>This is a description</description>
+			<Polygon>
+				<tessellate>1</tessellate>
+				<outerBoundaryIs>
+					<LinearRing>
+						<coordinates>
+							-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
+						</coordinates>
+					</LinearRing>
+				</outerBoundaryIs>
+			</Polygon>
+		</Placemark>
+		<Placemark>
+			<name>Poly 2</name>
+			<description>This is a description also</description>
+			<Polygon>
+				<tessellate>1</tessellate>
+				<outerBoundaryIs>
+					<LinearRing>
+						<coordinates>
+							-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
+						</coordinates>
+					</LinearRing>
+				</outerBoundaryIs>
+			</Polygon>
+		</Placemark>
+	</Folder>
+</Document>
+</kml>

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

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

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