Fix bug when clipping geometry collections in vector tiles (#93562)

This commit ensures that the tile envelope is cloned for each geometry when needed.
elastic · Feb 7, 2023 · 94a995f · 94a995f
1 parent 469c20a
commit 94a995f
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 8 deletions.
diff --git a/docs/changelog/93562.yaml b/docs/changelog/93562.yaml
@@ -0,0 +1,5 @@
+pr: 93562
+summary: Fix bug when clipping Geometry collections in vector tiles
+area: Geo
+type: bug
+issues: []
diff --git a/.../vector-tile/src/main/java/org/elasticsearch/xpack/vectortile/feature/FeatureFactory.java b/.../vector-tile/src/main/java/org/elasticsearch/xpack/vectortile/feature/FeatureFactory.java
@@ -116,10 +116,7 @@ public List<byte[]> getFeatures(Geometry geometry) {
         // Get geometry in spherical mercator
         final org.locationtech.jts.geom.Geometry jtsGeometry = geometry.visit(builder);
         // clip the geometry to the tile
-        final List<org.locationtech.jts.geom.Geometry> flatGeometries = clipGeometries(
-            clipTile.copy(),
-            JtsAdapter.flatFeatureList(jtsGeometry)
-        );
+        final List<org.locationtech.jts.geom.Geometry> flatGeometries = clipGeometries(clipTile, JtsAdapter.flatFeatureList(jtsGeometry));
         // simplify geometry using the pixel precision
         simplifyGeometry(flatGeometries, pixelPrecision);
         // convert coordinates to MVT geometry
@@ -144,13 +141,13 @@ private static List<org.locationtech.jts.geom.Geometry> clipGeometries(
                     intersected.add(geometry);
                 } else if (matrix.isWithin()) {
                     // the clipped geometry is the envelope
-                    intersected.add(envelope);
+                    intersected.add(envelope.copy());
                 } else if (matrix.isIntersects()) {
-                    // clip it
-                    intersected.add(envelope.intersection(geometry));
+                    // clip it (clone envelope as coordinates are copied by reference)
+                    intersected.add(envelope.copy().intersection(geometry));
                 } else {
                     // disjoint
-                    assert envelope.intersection(geometry).isEmpty();
+                    assert envelope.copy().intersection(geometry).isEmpty();
                 }
             } catch (TopologyException e) {
                 // ignore

diff --git a/...or-tile/src/test/java/org/elasticsearch/xpack/vectortile/feature/FeatureFactoryTests.java b/...or-tile/src/test/java/org/elasticsearch/xpack/vectortile/feature/FeatureFactoryTests.java
@@ -222,14 +222,34 @@ public void testManyIntersectingGeometries() {
         assertThat(builder.getFeatures(GeoTileUtils.toBoundingBox(2 * x + 1, 2 * y, z + 1)), iterableWithSize(1));
         assertThat(builder.getFeatures(GeoTileUtils.toBoundingBox(2 * x, 2 * y + 1, z + 1)), iterableWithSize(1));
         assertThat(builder.getFeatures(GeoTileUtils.toBoundingBox(2 * x + 1, 2 * y + 1, z + 1)), iterableWithSize(1));
+        final GeometryCollection<Geometry> withinCollection = new GeometryCollection<>(
+            List.of(
+                GeoTileUtils.toBoundingBox(2 * x, 2 * y, z + 1),
+                GeoTileUtils.toBoundingBox(2 * x + 1, 2 * y, z + 1),
+                GeoTileUtils.toBoundingBox(2 * x, 2 * y + 1, z + 1),
+                GeoTileUtils.toBoundingBox(2 * x + 1, 2 * y + 1, z + 1)
+            )
+        );
+        assertThat(builder.getFeatures(withinCollection), iterableWithSize(4));
         // intersecting geometries
         assertThat(builder.getFeatures(expandByHalf(GeoTileUtils.toBoundingBox(2 * x, 2 * y, z + 1))), iterableWithSize(1));
         assertThat(builder.getFeatures(expandByHalf(GeoTileUtils.toBoundingBox(2 * x + 1, 2 * y, z + 1))), iterableWithSize(1));
         assertThat(builder.getFeatures(expandByHalf(GeoTileUtils.toBoundingBox(2 * x, 2 * y + 1, z + 1))), iterableWithSize(1));
         assertThat(builder.getFeatures(expandByHalf(GeoTileUtils.toBoundingBox(2 * x + 1, 2 * y + 1, z + 1))), iterableWithSize(1));
+        final GeometryCollection<Geometry> intersectCollection = new GeometryCollection<>(
+            List.of(
+                expandByHalf(GeoTileUtils.toBoundingBox(2 * x, 2 * y, z + 1)),
+                expandByHalf(GeoTileUtils.toBoundingBox(2 * x + 1, 2 * y, z + 1))
+            )
+        );
+        assertThat(builder.getFeatures(intersectCollection), iterableWithSize(2));
         // contain geometries
         assertThat(builder.getFeatures(GeoTileUtils.toBoundingBox(x / 4, y / 4, z - 2)), iterableWithSize(1));
         assertThat(builder.getFeatures(GeoTileUtils.toBoundingBox(x / 4, y / 4, z - 2)), iterableWithSize(1));
+        final GeometryCollection<Geometry> containCollection = new GeometryCollection<>(
+            List.of(GeoTileUtils.toBoundingBox(x / 4, y / 4, z - 2), GeoTileUtils.toBoundingBox(x / 4, y / 4, z - 2))
+        );
+        assertThat(builder.getFeatures(containCollection), iterableWithSize(2));
     }
 
     private Rectangle expandByHalf(Rectangle rectangle) {