From 5eb9091884ee02d45a344033a9eb61ebc24f40d0 Mon Sep 17 00:00:00 2001 From: Nicholas Jakobsen Date: Mon, 17 Aug 2026 23:23:38 -0700 Subject: [PATCH 1/3] perf: Resolve a KML geometry's Placemark by walking its parents `each_record` read a geometry's name and metadata from `feature.ancestors('Placemark').first`. `Nokogiri::XML::Node#ancestors` answers a selector by walking to the document root and searching the whole document from there, then scanning the results for each ancestor, and it caches nothing. Called once per geometry, that costs geometries x document size. A KML where each Placemark holds a single geometry never shows it; one exported from design software, where a Placemark is a `` of hundreds of faces, turns a few thousand Placemarks into six figures of geometries and spends minutes there. Instead of asking Nokogiri for the ancestors matching a selector, `enclosing_placemark` walks the parent chain and stops at the first Placemark. The answer is the same, since `ancestors` returns matches nearest-first and only the first was ever read. On documents built by repeating the `test.kml` fixture's Placemark, 20,000 geometries go from 14.44s to 0.0155s. The walk stops at whatever no longer responds to `parent`, which is how a geometry with no Placemark ancestor returns nil. `Nokogiri::XML::Document` is that node, and it responds to `name` but not `parent`. Adds `kml_file_with_multi_geometry_placemarks.kml`, built by `fixtures:build` from the same squares as the other KML fixtures, covering that every part of a MultiGeometry takes the name and metadata of the Placemark holding it. Its fourth example asserts the document is not searched again once parsed, which fails against the previous implementation. Closes https://github.com/culturecode/spatial_features/issues/66 Co-Authored-By: Claude Opus 5 (1M context) --- lib/spatial_features/importers/kml.rb | 18 ++++++++++- spec/fixtures/README.md | 7 +++-- ...ml_file_with_multi_geometry_placemarks.kml | 29 +++++++++++++++++ .../spatial_features/importers/kml_spec.rb | 31 +++++++++++++++++++ spec/support/fixtures.rb | 4 +++ tasks/fixtures.rake | 17 ++++++++++ 6 files changed, 102 insertions(+), 4 deletions(-) create mode 100644 spec/fixtures/kml_file_with_multi_geometry_placemarks.kml diff --git a/lib/spatial_features/importers/kml.rb b/lib/spatial_features/importers/kml.rb index 8bd0e9d..96c447c 100644 --- a/lib/spatial_features/importers/kml.rb +++ b/lib/spatial_features/importers/kml.rb @@ -22,7 +22,7 @@ def initialize(data, base_dir: nil, **options) def each_record(&block) {'Polygon' => 'POLYGON', 'LineString' => 'LINE', 'Point' => 'POINT'}.each do |kml_type, sql_type| kml_document.css(kml_type).each do |feature| - if (placemark = feature.ancestors('Placemark').first) + if (placemark = enclosing_placemark(feature)) metadata = extract_metadata(placemark) name = placemark.css('name').text else @@ -41,6 +41,22 @@ def each_record(&block) end end + # Returns the Placemark a geometry node sits inside. + # + # @param feature [Nokogiri::XML::Node] a Polygon, LineString or Point node. + # @return [Nokogiri::XML::Element, nil] the nearest enclosing Placemark. Nil when the + # geometry sits outside one. + # @note Walks the parent chain rather than calling `Nokogiri::XML::Node#ancestors` with + # a selector. That method answers the selector by searching the whole document on + # every call, which is minutes of work on a file holding six figures of geometries. + def enclosing_placemark(feature) + node = feature.parent + while node + return node if node.name == 'Placemark' + node = node.respond_to?(:parent) ? node.parent : nil + end + end + def kml_document @kml_document ||= begin doc = Nokogiri::XML(@data) diff --git a/spec/fixtures/README.md b/spec/fixtures/README.md index a6d153d..9d7497b 100644 --- a/spec/fixtures/README.md +++ b/spec/fixtures/README.md @@ -17,9 +17,10 @@ The geometry is deliberately dull: squares on a regular grid, in NAD83 / UTM zon shapefiles and in round degrees for the KML. What matters in each fixture is its structure rather than its coordinates — a missing `.shx`, a `.shx` whose basename does not match the `.shp`, a `__MACOSX` decoy carrying a real extension, a single-vertex LineString, a null -geometry, a name longer than the name column. The specs assert on feature counts and on those -structures, so regenerating with different coordinates is safe, while changing a count, a -component filename, or a placemark name is not. +geometry, a name longer than the name column, a Placemark holding many polygons in one +`MultiGeometry`. The specs assert on feature counts and on those structures, so regenerating +with different coordinates is safe, while changing a count, a component filename, or a +placemark name is not. Regenerating rewrites the archives in place, and their entry timestamps change every run, so expect git to report them modified even when nothing about their contents has. Run the suite diff --git a/spec/fixtures/kml_file_with_multi_geometry_placemarks.kml b/spec/fixtures/kml_file_with_multi_geometry_placemarks.kml new file mode 100644 index 0000000..a5591ab --- /dev/null +++ b/spec/fixtures/kml_file_with_multi_geometry_placemarks.kml @@ -0,0 +1,29 @@ + + + + kml_file_with_multi_geometry_placemarks.kml + + Multi geometry folder + + Poly 1 + This is a description + + 10,20 10.5,20 10.5,20.5 10,20.5 10,20 + 11,20 11.5,20 11.5,20.5 11,20.5 11,20 + 12,20 12.5,20 12.5,20.5 12,20.5 12,20 + 13,20 13.5,20 13.5,20.5 13,20.5 13,20 + + + + Poly 2 + This is a description also + + 30,40 30.5,40 30.5,40.5 30,40.5 30,40 + 31,40 31.5,40 31.5,40.5 31,40.5 31,40 + 32,40 32.5,40 32.5,40.5 32,40.5 32,40 + 33,40 33.5,40 33.5,40.5 33,40.5 33,40 + + + + + diff --git a/spec/lib/spatial_features/importers/kml_spec.rb b/spec/lib/spatial_features/importers/kml_spec.rb index 3b2aee8..67b76f9 100644 --- a/spec/lib/spatial_features/importers/kml_spec.rb +++ b/spec/lib/spatial_features/importers/kml_spec.rb @@ -103,6 +103,37 @@ end end + context 'when a Placemark holds a MultiGeometry' do + let(:data) { kml_file_with_multi_geometry_placemarks.read } + + describe '#features' do + it 'returns one feature per part' do + expect(subject.features.count).to eq(8) + end + + it 'names each part after the Placemark holding it' do + expect(subject.features.map(&:name)) + .to contain_exactly(*['Poly 1'] * 4, *['Poly 2'] * 4) + end + + it 'gives each part the metadata of the Placemark holding it' do + expect(subject.features) + .to all(have_attributes :metadata => include('description' => be_present)) + end + + # `Nokogiri::XML::Node#ancestors` answers a selector by searching the whole document, + # so resolving a part's Placemark that way costs the document's size per part. The + # document is parsed first, since parsing searches it for NetworkLinks and overlays. + it 'resolves each part without searching the document again' do + subject.send(:kml_document) + + expect_any_instance_of(Nokogiri::XML::Document).not_to receive(:search) + + subject.features + end + end + end + context 'when the input is xml but not kml' do let(:data) { "hi" } diff --git a/spec/support/fixtures.rb b/spec/support/fixtures.rb index a127ca6..d5e35dd 100644 --- a/spec/support/fixtures.rb +++ b/spec/support/fixtures.rb @@ -38,6 +38,10 @@ def kml_file_with_invalid_altitude open_fixture_file("kml_file_with_invalid_altitude.kml") end +def kml_file_with_multi_geometry_placemarks + open_fixture_file("kml_file_with_multi_geometry_placemarks.kml") +end + def kml_file_without_features open_fixture_file("kml_file_without_features.kml") end diff --git a/tasks/fixtures.rake b/tasks/fixtures.rake index 73d6857..225bbe1 100644 --- a/tasks/fixtures.rake +++ b/tasks/fixtures.rake @@ -171,6 +171,12 @@ module FixtureGenerator write(::File.join(dir, 'kml_file_with_ground_overlay_and_features.kml'), document('kml_file_with_ground_overlay_and_features.kml', overlay_folder + poly_folder)) + # Two Placemarks, each holding several polygons in one MultiGeometry, the shape design + # software exports when it models an object out of many faces. Each polygon imports as + # its own feature, taking the name and metadata of the Placemark holding it. + write(::File.join(dir, 'kml_file_with_multi_geometry_placemarks.kml'), + document('kml_file_with_multi_geometry_placemarks.kml', multi_geometry_folder)) + # Geometry with no Placemark ancestor, so it imports with no name and no metadata. write_kmz(::File.join(dir, 'kmz_file_features_without_placemarks.kmz'), document('Geometry Without Placemarks', @@ -278,6 +284,17 @@ module FixtureGenerator placemark('Poly 2', 'This is a description also', polygon(square(30, 40)))) end + # The two placemarks of `poly_folder`, each holding `parts` polygons rather than one. + def multi_geometry_folder(parts: 4) + placemarks = [['Poly 1', 'This is a description', 10, 20], + ['Poly 2', 'This is a description also', 30, 40]] + + folder('Multi geometry folder', placemarks.map { |name, description, lon, lat| + squares = Array.new(parts) {|i| polygon(square(lon + i, lat, :size => 0.5)) } + placemark(name, description, " \n" + squares.join + " \n") + }.join) + end + def overlay_folder folder('Overlay folder', " \n Basemap Overlay\n" \ From 2e9cd351ccb998cb45d4940f3266d0690fcb569a Mon Sep 17 00:00:00 2001 From: Nicholas Jakobsen Date: Tue, 18 Aug 2026 00:08:32 -0700 Subject: [PATCH 2/3] perf: Read a Placemark's geometry from the Placemark down Reading a geometry's Placemark, however cheaply, is work repeated for every geometry, and it makes the Placemark's metadata repeated work too: `extract_metadata` parses the CDATA table in a `` into a hash once per geometry, so a Placemark holding several hundred parts parsed the same description several hundred times. Instead of walking up from each geometry to its Placemark, `each_record` now starts at each Placemark and reads the geometry below it. The Placemark's metadata, name and image paths are read once and shared by its parts, and no geometry ever asks what it belongs to. Measured on a 33.8 MB CAD-derived KML holding 76,190 geometries in 1,233 Placemarks, with the per-geometry database round trip stubbed out so the figures cover the traversal alone: ancestors + metadata per geometry 902.02s (before this branch) parent walk + metadata per geometry 10.76s (previous commit) Placemark-first 2.86s Eliminating the upward search is worth 891s of that and eliminating the repeated metadata 7.9s, so the second is small in absolute terms while still being most of what was left. Geometry outside any Placemark still imports with no name and no metadata, which iterating Placemarks alone would silently drop. A second pass matches it with one XPath over the document rather than by asking each element for its ancestors. `kmz_file_features_without_placemarks.kmz` covers it, and fails when that pass is removed. Each part takes its own copy of the Placemark's metadata, since a hash shared between features would be one object behind several records. Image paths are read once per Placemark rather than once per part, because `images_from_metadata` removes the key it reads, so calling it per part would leave every part after the first without images. Iteration order changes. It was every Polygon in the document, then every LineString, then every Point; it is now each Placemark's geometry together in document order, then any geometry outside a Placemark. Nothing asserts on feature sequence, `features_hash` is an MD5 of the source bytes rather than of what the importer emits, and `mvt_sql` orders by id at query time. Feature ids do land in a different order. Closes https://github.com/culturecode/spatial_features/issues/66 Co-Authored-By: Claude Opus 5 (1M context) --- lib/spatial_features/importers/kml.rb | 66 ++++++++++--------- .../kml_file_with_multi_geometry_photos.kml | 21 ++++++ .../spatial_features/importers/kml_spec.rb | 36 ++++++++-- spec/support/fixtures.rb | 4 ++ tasks/fixtures.rake | 21 +++++- 5 files changed, 112 insertions(+), 36 deletions(-) create mode 100644 spec/fixtures/kml_file_with_multi_geometry_photos.kml diff --git a/lib/spatial_features/importers/kml.rb b/lib/spatial_features/importers/kml.rb index 96c447c..f9df91f 100644 --- a/lib/spatial_features/importers/kml.rb +++ b/lib/spatial_features/importers/kml.rb @@ -6,6 +6,16 @@ class KML < Base # keys that may contain tags IMAGE_METADATA_KEYS = %w[pdfmaps_photos].freeze + # The elements that become features. A Placemark holds them directly or inside a + # MultiGeometry, and each one becomes a feature of its own. + GEOMETRY_TYPES = %w[Polygon LineString Point].freeze + GEOMETRY_SELECTOR = GEOMETRY_TYPES.join(', ').freeze + + # Geometry that sits outside any Placemark, which imports with no name and no + # metadata. One pass over the document matches all of it. + UNPLACED_GEOMETRY_XPATH = + GEOMETRY_TYPES.map {|type| "//#{type}[not(ancestor::Placemark)]" }.join(' | ').freeze + # matches a coordinate pair with an optional altitude, including invalid altitudes like NaN # -118.1,50.9,NaN # -118.1,50.9,0 @@ -20,41 +30,37 @@ def initialize(data, base_dir: nil, **options) private def each_record(&block) - {'Polygon' => 'POLYGON', 'LineString' => 'LINE', 'Point' => 'POINT'}.each do |kml_type, sql_type| - kml_document.css(kml_type).each do |feature| - if (placemark = enclosing_placemark(feature)) - metadata = extract_metadata(placemark) - name = placemark.css('name').text - else - metadata = {} - end - - next if blank_feature?(feature) - - geog = geom_from_kml(feature) - next if geog.blank? - - importable_image_paths = images_from_metadata(metadata) - - yield OpenStruct.new(geog: geog, name: name, metadata: metadata, importable_image_paths: importable_image_paths) + kml_document.css('Placemark').each do |placemark| + metadata = extract_metadata(placemark) + importable_image_paths = images_from_metadata(metadata) + name = placemark.css('name').text + + placemark.css(GEOMETRY_SELECTOR).each do |geometry| + # A hash of its own per feature, since each is stored on a separate record. + yield_feature(geometry, name, metadata.dup, importable_image_paths, &block) end end + + kml_document.xpath(UNPLACED_GEOMETRY_XPATH).each do |geometry| + yield_feature(geometry, nil, {}, [], &block) + end end - # Returns the Placemark a geometry node sits inside. + # Yields the feature built from a geometry element. # - # @param feature [Nokogiri::XML::Node] a Polygon, LineString or Point node. - # @return [Nokogiri::XML::Element, nil] the nearest enclosing Placemark. Nil when the - # geometry sits outside one. - # @note Walks the parent chain rather than calling `Nokogiri::XML::Node#ancestors` with - # a selector. That method answers the selector by searching the whole document on - # every call, which is minutes of work on a file holding six figures of geometries. - def enclosing_placemark(feature) - node = feature.parent - while node - return node if node.name == 'Placemark' - node = node.respond_to?(:parent) ? node.parent : nil - end + # @param geometry [Nokogiri::XML::Element] a Polygon, LineString or Point node. + # @param metadata [Hash] stored on the feature as it stands, so it must already have + # had its image keys removed. + # @yield [OpenStruct] nothing is yielded when the element holds no coordinates, or + # when PostGIS cannot read it. + def yield_feature(geometry, name, metadata, importable_image_paths, &block) + return if blank_feature?(geometry) + + geog = geom_from_kml(geometry) + return if geog.blank? + + block.call OpenStruct.new(geog: geog, name: name, metadata: metadata, + importable_image_paths: importable_image_paths) end def kml_document diff --git a/spec/fixtures/kml_file_with_multi_geometry_photos.kml b/spec/fixtures/kml_file_with_multi_geometry_photos.kml new file mode 100644 index 0000000..6029bdf --- /dev/null +++ b/spec/fixtures/kml_file_with_multi_geometry_photos.kml @@ -0,0 +1,21 @@ + + + + kml_file_with_multi_geometry_photos.kml + + Multi geometry photo folder + + Photo Poly + This is a description + + <img src="images/two_a.jpg"><img src="images/two_b.jpg"> + + + 50,60 50.5,60 50.5,60.5 50,60.5 50,60 + 51,60 51.5,60 51.5,60.5 51,60.5 51,60 + 52,60 52.5,60 52.5,60.5 52,60.5 52,60 + + + + + diff --git a/spec/lib/spatial_features/importers/kml_spec.rb b/spec/lib/spatial_features/importers/kml_spec.rb index 67b76f9..63f119d 100644 --- a/spec/lib/spatial_features/importers/kml_spec.rb +++ b/spec/lib/spatial_features/importers/kml_spec.rb @@ -121,16 +121,44 @@ .to all(have_attributes :metadata => include('description' => be_present)) end - # `Nokogiri::XML::Node#ancestors` answers a selector by searching the whole document, - # so resolving a part's Placemark that way costs the document's size per part. The - # document is parsed first, since parsing searches it for NetworkLinks and overlays. - it 'resolves each part without searching the document again' do + # Reading a part's Placemark by searching the document, or by asking the part for its + # ancestors, costs the document's size per part. The document is parsed first, since + # parsing searches it for NetworkLinks and overlays. + it 'reads the parts without searching the document again' do subject.send(:kml_document) expect_any_instance_of(Nokogiri::XML::Document).not_to receive(:search) subject.features end + + it 'reads a Placemark\'s metadata once however many parts it holds' do + expect(subject).to receive(:extract_metadata).twice.and_call_original + + subject.features + end + end + end + + context 'when a Placemark holding a MultiGeometry names photos' do + subject { SpatialFeatures::Importers::KML.new(data, :base_dir => Pathname.new('/base')) } + let(:data) { kml_file_with_multi_geometry_photos.read } + + describe '#features' do + it 'returns one feature per part' do + expect(subject.features.count).to eq(3) + end + + it 'gives every part the photos the Placemark names' do + expect(subject.features).to all(have_attributes :importable_image_paths => + [Pathname.new('/base/images/two_a.jpg'), Pathname.new('/base/images/two_b.jpg')]) + end + + it 'leaves the image keys out of every part\'s metadata' do + keys = subject.features.flat_map {|feature| feature.metadata.keys } + + expect(keys & SpatialFeatures::Importers::KML::IMAGE_METADATA_KEYS).to eq([]) + end end end diff --git a/spec/support/fixtures.rb b/spec/support/fixtures.rb index d5e35dd..976202f 100644 --- a/spec/support/fixtures.rb +++ b/spec/support/fixtures.rb @@ -38,6 +38,10 @@ def kml_file_with_invalid_altitude open_fixture_file("kml_file_with_invalid_altitude.kml") end +def kml_file_with_multi_geometry_photos + open_fixture_file("kml_file_with_multi_geometry_photos.kml") +end + def kml_file_with_multi_geometry_placemarks open_fixture_file("kml_file_with_multi_geometry_placemarks.kml") end diff --git a/tasks/fixtures.rake b/tasks/fixtures.rake index 225bbe1..4593ae1 100644 --- a/tasks/fixtures.rake +++ b/tasks/fixtures.rake @@ -177,6 +177,11 @@ module FixtureGenerator write(::File.join(dir, 'kml_file_with_multi_geometry_placemarks.kml'), document('kml_file_with_multi_geometry_placemarks.kml', multi_geometry_folder)) + # The same shape carrying photos, so that the images a Placemark names reach every one + # of its parts rather than only the first. + write(::File.join(dir, 'kml_file_with_multi_geometry_photos.kml'), + document('kml_file_with_multi_geometry_photos.kml', multi_geometry_photo_folder)) + # Geometry with no Placemark ancestor, so it imports with no name and no metadata. write_kmz(::File.join(dir, 'kmz_file_features_without_placemarks.kmz'), document('Geometry Without Placemarks', @@ -290,11 +295,23 @@ module FixtureGenerator ['Poly 2', 'This is a description also', 30, 40]] folder('Multi geometry folder', placemarks.map { |name, description, lon, lat| - squares = Array.new(parts) {|i| polygon(square(lon + i, lat, :size => 0.5)) } - placemark(name, description, " \n" + squares.join + " \n") + placemark(name, description, multi_geometry(parts) {|i| polygon(square(lon + i, lat, :size => 0.5)) }) }.join) end + # A single placemark whose ExtendedData names two photos, holding `parts` polygons, so + # every part of it carries the same images. + def multi_geometry_photo_folder(parts: 3) + folder('Multi geometry photo folder', + placemark('Photo Poly', 'This is a description', + photos('two_a.jpg', 'two_b.jpg') + + multi_geometry(parts) {|i| polygon(square(50 + i, 60, :size => 0.5)) })) + end + + def multi_geometry(parts, &block) + " \n" + Array.new(parts, &block).join + " \n" + end + def overlay_folder folder('Overlay folder', " \n Basemap Overlay\n" \ From 3047dd92f1868e982d031cd585afde69c8dd93a0 Mon Sep 17 00:00:00 2001 From: Nicholas Jakobsen Date: Tue, 18 Aug 2026 11:47:14 -0700 Subject: [PATCH 3/3] fix: Read a nested Placemark's geometry once rather than twice Reading a Placemark's geometry with `placemark.css(...)` matches everything below it, including the geometry of a Placemark nested inside it. Both Placemarks are iterated, so the inner geometry was read twice and imported as two features. KML 2.2 does not allow the nesting, but a document that does it produced duplicates rather than being read the way the previous implementation read it, which took each geometry's nearest enclosing Placemark and so claimed it once. Instead of every Placemark claiming all the geometry below it, `geometries_in` scopes the match to elements whose nearest enclosing Placemark is that one. Scoping is expressed by depth, since XPath 1.0 has no node-identity operator and cannot ask whether an ancestor is a particular node. The scoped path counts a candidate's ancestors, so it costs an upward walk per element. Measured on a document holding 76,190 geometries in 1,233 Placemarks, it takes 0.330s against 0.045s for the plain selector, and this branch exists to take that class of work out of the loop. It is therefore used only where it is needed: `//Placemark//Placemark` answers once per document whether anything nests, which costs 0.021s on a 32 MB document, and a document that does not nest keeps the plain selector. Adds `kml_file_with_nested_placemarks.kml`, built by `fixtures:build` from the same squares as the other KML fixtures. Reading it yields two features rather than three, and yields three when the scoping is removed. Co-Authored-By: Claude Opus 5 (1M context) --- lib/spatial_features/importers/kml.rb | 31 ++++++++++++++++++- spec/fixtures/README.md | 3 +- .../kml_file_with_nested_placemarks.kml | 19 ++++++++++++ .../spatial_features/importers/kml_spec.rb | 14 +++++++++ spec/support/fixtures.rb | 4 +++ tasks/fixtures.rake | 14 +++++++++ 6 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 spec/fixtures/kml_file_with_nested_placemarks.kml diff --git a/lib/spatial_features/importers/kml.rb b/lib/spatial_features/importers/kml.rb index f9df91f..5b29ea6 100644 --- a/lib/spatial_features/importers/kml.rb +++ b/lib/spatial_features/importers/kml.rb @@ -11,6 +11,10 @@ class KML < Base GEOMETRY_TYPES = %w[Polygon LineString Point].freeze GEOMETRY_SELECTOR = GEOMETRY_TYPES.join(', ').freeze + # Matches what `GEOMETRY_SELECTOR` matches, for the path that scopes elements to one + # Placemark by counting how many Placemarks they sit inside. + GEOMETRY_SELF_TEST = GEOMETRY_TYPES.map {|type| "self::#{type}" }.join(' or ').freeze + # Geometry that sits outside any Placemark, which imports with no name and no # metadata. One pass over the document matches all of it. UNPLACED_GEOMETRY_XPATH = @@ -35,7 +39,7 @@ def each_record(&block) importable_image_paths = images_from_metadata(metadata) name = placemark.css('name').text - placemark.css(GEOMETRY_SELECTOR).each do |geometry| + geometries_in(placemark).each do |geometry| # A hash of its own per feature, since each is stored on a separate record. yield_feature(geometry, name, metadata.dup, importable_image_paths, &block) end @@ -46,6 +50,31 @@ def each_record(&block) end end + # Returns the geometry elements belonging to a Placemark. + # + # A Placemark inside another Placemark owns its own geometry, so the outer one has to + # leave it alone or the same element is read twice. Scoping by depth costs an upward + # walk per element, which is 7x the plain selector on a document holding six figures + # of geometry, so it is used only for a document that nests. + # + # @param placemark [Nokogiri::XML::Element] + # @return [Nokogiri::XML::NodeSet] the Polygon, LineString and Point elements whose + # nearest enclosing Placemark is this one. + def geometries_in(placemark) + return placemark.css(GEOMETRY_SELECTOR) unless nested_placemarks? + + depth = placemark.xpath('count(ancestor::Placemark)').to_i + 1 + placemark.xpath(".//*[#{GEOMETRY_SELF_TEST}][count(ancestor::Placemark) = #{depth}]") + end + + # Returns true when any Placemark in the document holds another, which KML 2.2 does + # not allow. Read once per document. + def nested_placemarks? + return @nested_placemarks if defined?(@nested_placemarks) + + @nested_placemarks = kml_document.at_xpath('//Placemark//Placemark').present? + end + # Yields the feature built from a geometry element. # # @param geometry [Nokogiri::XML::Element] a Polygon, LineString or Point node. diff --git a/spec/fixtures/README.md b/spec/fixtures/README.md index 9d7497b..33624f9 100644 --- a/spec/fixtures/README.md +++ b/spec/fixtures/README.md @@ -18,7 +18,8 @@ shapefiles and in round degrees for the KML. What matters in each fixture is its rather than its coordinates — a missing `.shx`, a `.shx` whose basename does not match the `.shp`, a `__MACOSX` decoy carrying a real extension, a single-vertex LineString, a null geometry, a name longer than the name column, a Placemark holding many polygons in one -`MultiGeometry`. The specs assert on feature counts and on those structures, so regenerating +`MultiGeometry`, a Placemark holding another Placemark. The specs assert on feature counts +and on those structures, so regenerating with different coordinates is safe, while changing a count, a component filename, or a placemark name is not. diff --git a/spec/fixtures/kml_file_with_nested_placemarks.kml b/spec/fixtures/kml_file_with_nested_placemarks.kml new file mode 100644 index 0000000..811503b --- /dev/null +++ b/spec/fixtures/kml_file_with_nested_placemarks.kml @@ -0,0 +1,19 @@ + + + + kml_file_with_nested_placemarks.kml + + Nested placemark folder + + Outer Poly + This is a description + 10,20 11,20 11,21 10,21 10,20 + + Inner Poly + The inner description + 30,40 31,40 31,41 30,41 30,40 + + + + + diff --git a/spec/lib/spatial_features/importers/kml_spec.rb b/spec/lib/spatial_features/importers/kml_spec.rb index 63f119d..ea7dd7e 100644 --- a/spec/lib/spatial_features/importers/kml_spec.rb +++ b/spec/lib/spatial_features/importers/kml_spec.rb @@ -162,6 +162,20 @@ end end + context 'when a Placemark holds another Placemark' do + let(:data) { kml_file_with_nested_placemarks.read } + + describe '#features' do + it 'returns one feature per geometry rather than reading the inner one twice' do + expect(subject.features.count).to eq(2) + end + + it 'returns each geometry once' do + expect(subject.features.map(&:geog).uniq.size).to eq(2) + end + end + end + context 'when the input is xml but not kml' do let(:data) { "hi" } diff --git a/spec/support/fixtures.rb b/spec/support/fixtures.rb index 976202f..054d548 100644 --- a/spec/support/fixtures.rb +++ b/spec/support/fixtures.rb @@ -22,6 +22,10 @@ def kml_file_with_invalid_placemark open_fixture_file("kml_file_with_invalid_placemark.kml") end +def kml_file_with_nested_placemarks + open_fixture_file("kml_file_with_nested_placemarks.kml") +end + def kml_file_with_network_link open_fixture_file("kml_file_with_network_link.kml") end diff --git a/tasks/fixtures.rake b/tasks/fixtures.rake index 4593ae1..337e0ee 100644 --- a/tasks/fixtures.rake +++ b/tasks/fixtures.rake @@ -182,6 +182,12 @@ module FixtureGenerator write(::File.join(dir, 'kml_file_with_multi_geometry_photos.kml'), document('kml_file_with_multi_geometry_photos.kml', multi_geometry_photo_folder)) + # A Placemark holding another, which KML 2.2 does not allow. The inner geometry belongs + # to the inner Placemark, so each polygon imports once rather than the outer Placemark + # also claiming the inner one. + write(::File.join(dir, 'kml_file_with_nested_placemarks.kml'), + document('kml_file_with_nested_placemarks.kml', nested_placemark_folder)) + # Geometry with no Placemark ancestor, so it imports with no name and no metadata. write_kmz(::File.join(dir, 'kmz_file_features_without_placemarks.kmz'), document('Geometry Without Placemarks', @@ -312,6 +318,14 @@ module FixtureGenerator " \n" + Array.new(parts, &block).join + " \n" end + # One Placemark holding a polygon and, beside it, a second Placemark of its own. + def nested_placemark_folder + inner = placemark('Inner Poly', 'The inner description', polygon(square(30, 40))) + + folder('Nested placemark folder', + placemark('Outer Poly', 'This is a description', polygon(square(10, 20)) + inner)) + end + def overlay_folder folder('Overlay folder', " \n Basemap Overlay\n" \