From 249dafacb150d616869a8f4935320eef06e41fdf Mon Sep 17 00:00:00 2001
From: Michael Barry <msbarry@users.noreply.github.com>
Date: Thu, 14 Nov 2024 06:54:27 -0500
Subject: [PATCH] fix match ordering (#1101)

---
 .../expression/MultiExpression.java           | 21 +++++++++--
 .../expression/MultiExpressionTest.java       | 37 +++++++------------
 .../custommap/ConfigExpressionParser.java     |  2 +-
 .../expression/ConfigExpression.java          |  2 +-
 .../custommap/ConfiguredFeatureTest.java      | 34 +++++++++++++++++
 pom.xml                                       |  2 +-
 6 files changed, 69 insertions(+), 29 deletions(-)

diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java
index db08f14607..d70d14c833 100644
--- a/planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java
+++ b/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);
     };
   }
 
diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java
index e7f5b678e2..83fdf36a0e 100644
--- a/planetiler-core/src/test/java/com/onthegomap/planetiler/expression/MultiExpressionTest.java
+++ b/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) {
diff --git a/planetiler-custommap/src/main/java/com/onthegomap/planetiler/custommap/ConfigExpressionParser.java b/planetiler-custommap/src/main/java/com/onthegomap/planetiler/custommap/ConfigExpressionParser.java
index 1137bfffe9..8db83addf1 100644
--- a/planetiler-custommap/src/main/java/com/onthegomap/planetiler/custommap/ConfigExpressionParser.java
+++ 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) {
diff --git a/planetiler-custommap/src/main/java/com/onthegomap/planetiler/custommap/expression/ConfigExpression.java b/planetiler-custommap/src/main/java/com/onthegomap/planetiler/custommap/expression/ConfigExpression.java
index 4e4853bbd2..65cb28700c 100644
--- a/planetiler-custommap/src/main/java/com/onthegomap/planetiler/custommap/expression/ConfigExpression.java
+++ b/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()
           );
         }
diff --git a/planetiler-custommap/src/test/java/com/onthegomap/planetiler/custommap/ConfiguredFeatureTest.java b/planetiler-custommap/src/test/java/com/onthegomap/planetiler/custommap/ConfiguredFeatureTest.java
index 4a9d2fd82e..3ad107562c 100644
--- a/planetiler-custommap/src/test/java/com/onthegomap/planetiler/custommap/ConfiguredFeatureTest.java
+++ b/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);
+  }
 }
diff --git a/pom.xml b/pom.xml
index 6cca7720b6..f07f5c1ba7 100644
--- a/pom.xml
+++ b/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>