Handle immutable output from post process layer/tile feature methods (#920)

planetiler-core/src/main/java/com/onthegomap/planetiler/ForwardingProfile.java

@@ -1,5 +1,7 @@
 package com.onthegomap.planetiler;
 
+import static com.onthegomap.planetiler.util.MutableCollections.makeMutable;
+
 import com.onthegomap.planetiler.config.PlanetilerConfig;
 import com.onthegomap.planetiler.expression.Expression;
 import com.onthegomap.planetiler.expression.MultiExpression;
@@ -8,6 +10,7 @@ import com.onthegomap.planetiler.geo.TileCoord;
 import com.onthegomap.planetiler.reader.SourceFeature;
 import com.onthegomap.planetiler.reader.osm.OsmElement;
 import com.onthegomap.planetiler.reader.osm.OsmRelationInfo;
+import com.onthegomap.planetiler.util.MutableCollections;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -216,12 +219,12 @@ public abstract class ForwardingProfile implements Profile {
     throws GeometryException {
     // delegate feature post-processing to each layer, if it implements FeaturePostProcessor
     List<LayerPostProcesser> postProcessers = layerPostProcessors.get(layer);
-    List<VectorTile.Feature> result = items;
+    List<VectorTile.Feature> result = makeMutable(items);
     if (postProcessers != null) {
       for (var handler : postProcessers) {
         var thisResult = handler.postProcess(zoom, result);
-        if (thisResult != null) {
-          result = thisResult;
+        if (thisResult != null && result != thisResult) {
+          result = makeMutable(thisResult);
         }
       }
     }
@@ -231,12 +234,12 @@ public abstract class ForwardingProfile implements Profile {
   @Override
   public Map<String, List<VectorTile.Feature>> postProcessTileFeatures(TileCoord tileCoord,
     Map<String, List<VectorTile.Feature>> layers) throws GeometryException {
-    var result = layers;
+    var result = MutableCollections.makeMutableMultimap(layers);
     for (TilePostProcessor postProcessor : tilePostProcessors) {
       // TODO catch failures to isolate from other tile postprocessors?
       var thisResult = postProcessor.postProcessTile(tileCoord, result);
-      if (thisResult != null) {
-        result = thisResult;
+      if (thisResult != null && result != thisResult) {
+        result = MutableCollections.makeMutableMultimap(thisResult);
       }
     }
     return result;

planetiler-core/src/main/java/com/onthegomap/planetiler/collection/FeatureGroup.java

@@ -1,5 +1,7 @@
 package com.onthegomap.planetiler.collection;
 
+import static com.onthegomap.planetiler.util.MutableCollections.makeMutable;
+
 import com.carrotsearch.hppc.LongLongHashMap;
 import com.onthegomap.planetiler.Profile;
 import com.onthegomap.planetiler.VectorTile;
@@ -492,8 +494,8 @@ public final class FeatureGroup implements Iterable<FeatureGroup.TileFeatures>,
         return;
       }
       try {
-        List<VectorTile.Feature> postProcessed = profile
-          .postProcessLayerFeatures(layer, tileCoord.z(), features);
+        List<VectorTile.Feature> postProcessed = makeMutable(profile
+          .postProcessLayerFeatures(layer, tileCoord.z(), makeMutable(features)));
         features = postProcessed == null ? features : postProcessed;
         // lines are stored using a higher precision so that rounding does not
         // introduce artificial intersections between endpoints to confuse line merging,

planetiler-core/src/main/java/com/onthegomap/planetiler/util/MutableCollections.java

@@ -0,0 +1,59 @@
+package com.onthegomap.planetiler.util;
+
+import java.util.AbstractSequentialList;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.NavigableMap;
+import java.util.SequencedMap;
+import java.util.TreeMap;
+
+/** Utilities for converting immutable collections to mutable ones. */
+public class MutableCollections {
+  private MutableCollections() {}
+
+  /** Return a mutable copy of {@code list} or the original list if it is already mutable. */
+  public static <T> List<T> makeMutable(List<T> list) {
+    return switch (list) {
+      case ArrayList<T> l -> l;
+      case LinkedList<T> l -> l;
+      case AbstractSequentialList<T> l -> new LinkedList<>(l);
+      case null -> list;
+      default -> new ArrayList<>(list);
+    };
+  }
+
+  /**
+   * Return a mutable copy of {@code map} with mutable list values or the original collections if they are already
+   * mutable.
+   */
+  public static <K, V> Map<K, List<V>> makeMutableMultimap(Map<K, List<V>> map) {
+    var mutableMap = makeMutableMap(map);
+    if (mutableMap != null) {
+      for (var entry : map.entrySet()) {
+        var key = entry.getKey();
+        var value = entry.getValue();
+        var mutableList = makeMutable(value);
+        if (mutableList != value) {
+          mutableMap.put(key, mutableList);
+        }
+      }
+    }
+    return mutableMap;
+  }
+
+  /** Return a mutable copy of {@code map} or the original list if it is already mutable. */
+  public static <K, V> Map<K, V> makeMutableMap(Map<K, V> map) {
+    return switch (map) {
+      case HashMap<K, V> m -> m;
+      case TreeMap<K, V> m -> m;
+      case NavigableMap<K, V> m -> new TreeMap<>(m);
+      case SequencedMap<K, V> m -> new LinkedHashMap<>(m);
+      case null -> map;
+      default -> new HashMap<>(map);
+    };
+  }
+}

planetiler-core/src/test/java/com/onthegomap/planetiler/ForwardingProfileTests.java

@@ -179,6 +179,21 @@ class ForwardingProfileTests {
     });
     assertEquals(List.of(feature), profile.postProcessLayerFeatures("a", 0, List.of(feature)));
 
+    // allow mutations on initial input
+    profile.registerHandler(new ForwardingProfile.LayerPostProcesser() {
+      @Override
+      public List<VectorTile.Feature> postProcess(int zoom, List<VectorTile.Feature> items) {
+        items.set(0, items.getFirst());
+        return null;
+      }
+
+      @Override
+      public String name() {
+        return "a";
+      }
+    });
+    assertEquals(List.of(feature), profile.postProcessLayerFeatures("a", 0, List.of(feature)));
+
     // empty list removes
     profile.registerHandler(new ForwardingProfile.LayerPostProcesser() {
       @Override
@@ -195,6 +210,23 @@ class ForwardingProfileTests {
     // doesn't touch elements in another layer
     assertEquals(List.of(feature), profile.postProcessLayerFeatures("b", 0, List.of(feature)));
 
+    // allow mutations on subsequent input
+    profile.registerHandler(new ForwardingProfile.LayerPostProcesser() {
+      @Override
+      public List<VectorTile.Feature> postProcess(int zoom, List<VectorTile.Feature> items) {
+        items.add(null);
+        items.removeLast();
+        return items;
+      }
+
+      @Override
+      public String name() {
+        return "a";
+      }
+    });
+    assertEquals(List.of(), profile.postProcessLayerFeatures("a", 0, List.of(feature)));
+    assertEquals(List.of(), profile.postProcessLayerFeatures("a", 0, new ArrayList<>(List.of(feature))));
+
     // 2 handlers for same layer run one after another
     var skip1 = new ForwardingProfile.LayerPostProcesser() {
       @Override
@@ -243,10 +275,36 @@ class ForwardingProfileTests {
     assertEquals(Map.of("a", List.of(feature)),
       profile.postProcessTileFeatures(TileCoord.ofXYZ(0, 0, 0), Map.of("a", List.of(feature))));
 
+    // allow mutation on initial input
+    profile.registerHandler((ForwardingProfile.TilePostProcessor) (tileCoord, layers) -> {
+      if (layers.containsKey("a")) {
+        var list = layers.get("a");
+        var item = list.getFirst();
+        list.set(0, item);
+        layers.put("a", list);
+      }
+      return layers;
+    });
+    assertEquals(Map.of("a", List.of(feature)),
+      profile.postProcessTileFeatures(TileCoord.ofXYZ(0, 0, 0), Map.of("a", List.of(feature))));
+    assertEquals(Map.of("a", List.of(feature)),
+      profile.postProcessTileFeatures(TileCoord.ofXYZ(0, 0, 0),
+        new HashMap<>(Map.of("a", new ArrayList<>(List.of(feature))))));
+
     // empty map removes
     profile.registerHandler((ForwardingProfile.TilePostProcessor) (tileCoord, layers) -> Map.of());
     assertEquals(Map.of(),
       profile.postProcessTileFeatures(TileCoord.ofXYZ(0, 0, 0), Map.of("a", List.of(feature))));
+
+    // allow mutation on subsequent inputs
+    profile.registerHandler((ForwardingProfile.TilePostProcessor) (tileCoord, layers) -> {
+      layers.put("a", List.of());
+      layers.remove("a");
+      return layers;
+    });
+    assertEquals(Map.of(),
+      profile.postProcessTileFeatures(TileCoord.ofXYZ(0, 0, 0), Map.of("a", List.of(feature))));
+
     // also touches elements in another layer
     assertEquals(Map.of(),
       profile.postProcessTileFeatures(TileCoord.ofXYZ(0, 0, 0), Map.of("b", List.of(feature))));
@@ -285,6 +343,7 @@ class ForwardingProfileTests {
         Map.of("c", List.of(feature, feature, feature, feature))));
   }
 
+
   @Test
   void testCaresAboutSource() {
     profile.registerSourceHandler("a", (x, y) -> {

planetiler-core/src/test/java/com/onthegomap/planetiler/util/MutableCollectionsTest.java

@@ -0,0 +1,105 @@
+package com.onthegomap.planetiler.util;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.TreeMap;
+import org.junit.jupiter.api.Test;
+
+class MutableCollectionsTest {
+  @Test
+  void testListOf() {
+    var mutable = MutableCollections.makeMutable(List.of(1, 2, 3));
+    mutable.add(4);
+    assertEquals(List.of(1, 2, 3, 4), mutable);
+  }
+
+  @Test
+  void testListOf0() {
+    var mutable = MutableCollections.makeMutable(List.<Integer>of());
+    mutable.add(1);
+    mutable.add(2);
+    mutable.add(3);
+    mutable.add(4);
+    assertEquals(List.of(1, 2, 3, 4), mutable);
+  }
+
+  @Test
+  void testArrayList() {
+    var mutable = MutableCollections.makeMutable(new ArrayList<>(List.of(1, 2, 3)));
+    mutable.add(4);
+    assertEquals(List.of(1, 2, 3, 4), mutable);
+  }
+
+  @Test
+  void testLinkedList() {
+    var mutable = MutableCollections.makeMutable(new LinkedList<>(List.of(1, 2, 3)));
+    mutable.add(4);
+    assertEquals(List.of(1, 2, 3, 4), mutable);
+  }
+
+  @Test
+  void testUnmodifiableCollection() {
+    var mutable = MutableCollections.makeMutable(Collections.unmodifiableList(new ArrayList<>(List.of(1, 2, 3))));
+    mutable.add(4);
+    assertEquals(List.of(1, 2, 3, 4), mutable);
+  }
+
+  @Test
+  void testGuavaList() {
+    var mutable = MutableCollections.makeMutable(ImmutableList.builder().add(1, 2, 3).build());
+    mutable.add(4);
+    assertEquals(List.of(1, 2, 3, 4), mutable);
+  }
+
+  @Test
+  void testMapOs() {
+    var mutable = MutableCollections.makeMutableMap(Map.of(1, 2, 3, 4));
+    mutable.put(5, 6);
+    assertEquals(Map.of(1, 2, 3, 4, 5, 6), mutable);
+  }
+
+  @Test
+  void testHashMap() {
+    var mutable = MutableCollections.makeMutableMap(new HashMap<>(Map.of(1, 2, 3, 4)));
+    mutable.put(5, 6);
+    assertEquals(Map.of(1, 2, 3, 4, 5, 6), mutable);
+  }
+
+  @Test
+  void testTreeMap() {
+    var mutable = MutableCollections.makeMutableMap(new TreeMap<>(Map.of(1, 2, 3, 4)));
+    mutable.put(5, 6);
+    assertEquals(Map.of(1, 2, 3, 4, 5, 6), mutable);
+  }
+
+  @Test
+  void testUnmodifiableMap() {
+    var mutable = MutableCollections.makeMutableMap(Collections.unmodifiableMap(new TreeMap<>(Map.of(1, 2, 3, 4))));
+    mutable.put(5, 6);
+    assertEquals(Map.of(1, 2, 3, 4, 5, 6), mutable);
+  }
+
+  @Test
+  void testGuavaMap() {
+    var mutable = MutableCollections.makeMutableMap(ImmutableMap.builder().put(1, 2).put(3, 4).build());
+    mutable.put(5, 6);
+    assertEquals(Map.of(1, 2, 3, 4, 5, 6), mutable);
+  }
+
+  @Test
+  void testMultimap() {
+    var mutable = MutableCollections.makeMutableMultimap(Map.of(1, List.of(2, 3), 4, List.of(5, 6)));
+    var map = mutable.get(1);
+    map.add(3);
+    mutable.put(7, map);
+    assertEquals(Map.of(1, List.of(2, 3, 3), 4, List.of(5, 6), 7, List.of(2, 3, 3)), mutable);
+  }
+}