fix match ordering (#1101)

planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java

@@ -40,12 +40,27 @@ public record MultiExpression<T>(List<Entry<T>> expressions) implements Simplifi
   private static final Logger LOGGER = LoggerFactory.getLogger(MultiExpression.class);
   private static final Comparator<WithId> BY_ID = Comparator.comparingInt(WithId::id);
 
+  /**
+   * Returns a new multi-expression from {@code expressions} where multiple expressions for the same key get OR'd
+   * together.
+   * <p>
+   * If the order in which expresions match matter, use {@link #ofOrdered(List)} instead.
+   */
   public static <T> MultiExpression<T> of(List<Entry<T>> expressions) {
     LinkedHashMap<T, Expression> map = new LinkedHashMap<>();
     for (var expression : expressions) {
       map.merge(expression.result, expression.expression, Expression::or);
     }
-    return new MultiExpression<>(map.entrySet().stream().map(e -> entry(e.getKey(), e.getValue())).collect(Collectors.toList()));
+    return new MultiExpression<>(
+      map.entrySet().stream().map(e -> entry(e.getKey(), e.getValue())).collect(Collectors.toList()));
+  }
+
+  /**
+   * Returns a new multi-expression from {@code expressions} where multiple expressions for the same key stay separate,
+   * in cases where the order in which expressions matches is important.
+   */
+  public static <T> MultiExpression<T> ofOrdered(List<Entry<T>> expressions) {
+    return new MultiExpression<>(new ArrayList<>(expressions));
   }
 
   public static <T> Entry<T> entry(T result, Expression expression) {
@@ -63,8 +78,8 @@ private static boolean mustAlwaysEvaluate(Expression expression) {
       case Expression.Not(var child) -> !mustAlwaysEvaluate(child);
       case Expression.MatchAny any when any.mustAlwaysEvaluate() -> true;
       case null, default -> !(expression instanceof Expression.MatchAny) &&
-          !(expression instanceof Expression.MatchField) &&
-          !FALSE.equals(expression);
+        !(expression instanceof Expression.MatchField) &&
+        !FALSE.equals(expression);
     };
   }
 

planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java

@@ -729,33 +729,24 @@ private static SourceFeature point(String source, String layer, Map<String, Obje
     return SimpleFeature.create(newPoint(0, 0), tags, source, layer, 1);
   }
 
-
   @Test
-  void testNestedMatching() {
-    var index = MultiExpression.of(List.of(
-      entry("a", matchField("a.b")),
-      entry("b", matchAny("a.b", "c", "d")),
-      entry("c", matchAny("a", "e")),
-      entry("d", matchAny("a[].b", "c"))
+  void testMatchOrder() {
+    var index = MultiExpression.ofOrdered(List.of(
+      entry("green", matchAny("natural", "tree")),
+      entry("black", matchAny("historic", "memorial")),
+      entry("green", matchAny("tourism", "viewpoint"))
     )).index();
 
-    assertSameElements(List.of(), index.getMatches(WithTags.from(Map.of("k", "v"))));
-    assertSameElements(List.of("c"), index.getMatches(WithTags.from(Map.of("a", "e"))));
-    assertSameElements(List.of("c"), index.getMatches(WithTags.from(Map.of("a", List.of("e")))));
-    assertSameElements(List.of("c"), index.getMatches(WithTags.from(Map.of("a", List.of("e", "f")))));
-    assertSameElements(List.of(), index.getMatches(WithTags.from(Map.of("a", List.of("g", "f")))));
-
-
-    assertSameElements(List.of("a"), index.getMatches(WithTags.from(Map.of("a.b", "e"))));
-    assertSameElements(List.of("a", "b"), index.getMatches(WithTags.from(Map.of("a.b", "c"))));
-    assertSameElements(List.of(), index.getMatches(WithTags.from(Map.of("a", Map.of("b", List.of())))));
-    assertSameElements(List.of("a", "b", "d"), index.getMatches(WithTags.from(Map.of("a", Map.of("b", List.of("c"))))));
-    assertSameElements(List.of("a", "b"), index.getMatches(WithTags.from(Map.of("a", Map.of("b", List.of("d"))))));
-    assertSameElements(List.of("a"), index.getMatches(WithTags.from(Map.of("a", Map.of("b", List.of("e"))))));
-    assertSameElements(List.of("a"), index.getMatches(WithTags.from(Map.of("a", Map.of("b", Map.of("c", "e"))))));
+    assertEquals(List.of("black", "green"), index.getMatches(WithTags.from(Map.of(
+      "historic", "memorial",
+      "tourism", "viewpoint"
+    ))));
 
-    assertSameElements(List.of("a", "b", "c", "d"),
-      index.getMatches(WithTags.from(Map.of("a", List.of("e", Map.of("b", List.of("c")))))));
+    // multiple matches allowed now when there are duplicate keys
+    assertEquals(List.of("green", "green"), index.getMatches(WithTags.from(Map.of(
+      "natural", "tree",
+      "tourism", "viewpoint"
+    ))));
   }
 
   private static <T> void assertSameElements(List<T> a, List<T> b) {

planetiler-custommap/src/main/java/com/onthegomap/planetiler/custommap/ConfigExpressionParser.java

@@ -146,7 +146,7 @@ private <O> ConfigExpression.Match<I, O> parseMatch(Object match, boolean allowE
     } else {
       throw new ParseException("Invalid match block. Expected a list or map, but got: " + match);
     }
-    return ConfigExpression.match(signature(output), MultiExpression.of(List.copyOf(conditions)), fallback);
+    return ConfigExpression.match(signature(output), MultiExpression.ofOrdered(List.copyOf(conditions)), fallback);
   }
 
   private <O> Signature<I, O> signature(Class<O> outputClass) {

planetiler-custommap/src/main/java/com/onthegomap/planetiler/custommap/expression/ConfigExpression.java

@@ -152,7 +152,7 @@ public ConfigExpression<I, O> simplifyOnce() {
         if (Expression.TRUE.equals(expression.expression())) {
           return new Match<>(
             signature,
-            MultiExpression.of(expressions.stream().limit(i).toList()),
+            MultiExpression.ofOrdered(expressions.stream().limit(i).toList()),
             expression.result()
           );
         }

planetiler-custommap/src/test/java/com/onthegomap/planetiler/custommap/ConfiguredFeatureTest.java

@@ -1602,4 +1602,38 @@ void testGeometryAttributesArea(String expression, double expected) {
     testFeature(config, sfMatch,
       any -> assertEquals(expected, (Double) any.getAttrsAtZoom(14).get("attr"), expected / 1e3), 1);
   }
+
+
+  @ParameterizedTest
+  @ValueSource(booleans = {false, true})
+  void testMatchOrdering(boolean withFallback) {
+    var config = """
+      sources:
+        osm:
+          type: osm
+          url: geofabrik:rhode-island
+          local_path: data/rhode-island.osm.pbf
+      layers:
+      - id: testLayer
+        features:
+        - source: osm
+          attributes:
+            - key: attr
+              value:
+              - if: {natural: tree}
+                value: green
+              - if: {historic: memorial}
+                value: black
+              - if: {tourism: viewpoint}
+                value: green
+              - if: ${%s}
+                value: fallback
+
+      """.formatted(withFallback ? "true" : "false");
+    testFeature(config, SimpleFeature.createFakeOsmFeature(newPoint(0, 0), Map.of(
+      "historic", "memorial",
+      "tourism", "viewpoint"
+    ), "osm", null, 1, emptyList(), OSM_INFO), feature -> assertEquals("black", feature.getAttrsAtZoom(14).get("attr")),
+      1);
+  }
 }

pom.xml

@@ -186,7 +186,7 @@
             <importOrder/>
             <removeUnusedImports/>
             <eclipse>
-              <version>4.29</version>
+              <version>4.33</version>
               <!--suppress UnresolvedMavenProperty -->
               <file>${maven.multiModuleProjectDirectory}/eclipse-formatter.xml</file>
             </eclipse>