Merge pull request #5366 from ibi-group/remove-duplicate-nearby-stops

De-duplicate stops returned by `stopsByRadius`

src/ext/java/org/opentripplanner/ext/geocoder/StopClusterMapper.java

@@ -2,13 +2,10 @@
 
 import java.util.Collection;
 import java.util.Optional;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Function;
-import java.util.function.Predicate;
 import java.util.stream.Stream;
 import org.opentripplanner.framework.geometry.WgsCoordinate;
 import org.opentripplanner.framework.i18n.I18NString;
+import org.opentripplanner.framework.lang.PredicateUtils;
 import org.opentripplanner.transit.model.site.StopLocation;
 import org.opentripplanner.transit.model.site.StopLocationsGroup;
 import org.opentripplanner.transit.service.TransitService;
@@ -44,7 +41,7 @@ Stream<StopCluster> generateStopClusters(
       .filter(sl -> sl.getName() != null)
       // if they are very close to each other and have the same name, only one is chosen (at random)
       .filter(
-        distinctByKey(sl ->
+        PredicateUtils.distinctByKey(sl ->
           new DeduplicationKey(sl.getName(), sl.getCoordinate().roundToApproximate10m())
         )
       )
@@ -84,10 +81,5 @@ private static StopCluster.Coordinate toCoordinate(WgsCoordinate c) {
     return new StopCluster.Coordinate(c.latitude(), c.longitude());
   }
 
-  private static <T> Predicate<T> distinctByKey(Function<? super T, ?> keyExtractor) {
-    Set<Object> seen = ConcurrentHashMap.newKeySet();
-    return t -> seen.add(keyExtractor.apply(t));
-  }
-
   private record DeduplicationKey(I18NString name, WgsCoordinate coordinate) {}
 }

src/ext/java/org/opentripplanner/ext/gtfsgraphqlapi/datafetchers/QueryTypeImpl.java

@@ -31,6 +31,7 @@
 import org.opentripplanner.ext.gtfsgraphqlapi.GraphQLUtils;
 import org.opentripplanner.ext.gtfsgraphqlapi.generated.GraphQLDataFetchers;
 import org.opentripplanner.ext.gtfsgraphqlapi.generated.GraphQLTypes;
+import org.opentripplanner.ext.gtfsgraphqlapi.generated.GraphQLTypes.GraphQLQueryTypeStopsByRadiusArgs;
 import org.opentripplanner.ext.gtfsgraphqlapi.mapping.RouteRequestMapper;
 import org.opentripplanner.framework.time.ServiceDateUtils;
 import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore;
@@ -721,7 +722,7 @@ public DataFetcher<Iterable<Object>> stopsByBbox() {
   public DataFetcher<Connection<NearbyStop>> stopsByRadius() {
     return environment -> {
       // TODO implement rest of the args
-      GraphQLTypes.GraphQLQueryTypeStopsByRadiusArgs args = new GraphQLTypes.GraphQLQueryTypeStopsByRadiusArgs(
+      GraphQLQueryTypeStopsByRadiusArgs args = new GraphQLQueryTypeStopsByRadiusArgs(
         environment.getArguments()
       );
 

src/main/java/org/opentripplanner/framework/lang/PredicateUtils.java

@@ -0,0 +1,26 @@
+package org.opentripplanner.framework.lang;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Function;
+import java.util.function.Predicate;
+
+/**
+ * Utility for building predicates to be used for filtering streams.
+ */
+public class PredicateUtils {
+
+  /**
+   * Build a predicate that uses the {@code keyExtractor} to remove any key that has already
+   * been seen by this (stateful) predicate.
+   * <p>
+   * This is useful for removing duplicates from a stream where the key to be compared is not the
+   * entity itself but a field of it.
+   * <p>
+   * Note: Duplicate check is based on equality not identity.
+   */
+  public static <T> Predicate<T> distinctByKey(Function<? super T, ?> keyExtractor) {
+    Map<Object, Boolean> seen = new ConcurrentHashMap<>();
+    return t -> seen.putIfAbsent(keyExtractor.apply(t), Boolean.TRUE) == null;
+  }
+}

src/main/java/org/opentripplanner/routing/graphfinder/StopFinderTraverseVisitor.java

@@ -4,21 +4,21 @@
 import java.util.List;
 import org.opentripplanner.astar.spi.SkipEdgeStrategy;
 import org.opentripplanner.astar.spi.TraverseVisitor;
+import org.opentripplanner.framework.lang.PredicateUtils;
 import org.opentripplanner.street.model.edge.Edge;
 import org.opentripplanner.street.model.vertex.TransitStopVertex;
 import org.opentripplanner.street.model.vertex.Vertex;
 import org.opentripplanner.street.search.state.State;
 
-// TODO Seems like this should be merged with the PlaceFinderTraverseVisitor
-
 /**
  * A TraverseVisitor used in finding stops while walking the street graph.
  */
 public class StopFinderTraverseVisitor implements TraverseVisitor<State, Edge> {
 
   private final double radiusMeters;
+
   /** A list of closest stops found while walking the graph */
-  public final List<NearbyStop> stopsFound = new ArrayList<>();
+  private final List<NearbyStop> stopsFound = new ArrayList<>();
 
   public StopFinderTraverseVisitor(double radiusMeters) {
     this.radiusMeters = radiusMeters;
@@ -27,18 +27,24 @@ public StopFinderTraverseVisitor(double radiusMeters) {
   @Override
   public void visitEdge(Edge edge) {}
 
-  // Accumulate stops into ret as the search runs.
   @Override
   public void visitVertex(State state) {
     Vertex vertex = state.getVertex();
-    if (vertex instanceof TransitStopVertex) {
-      stopsFound.add(NearbyStop.nearbyStopForState(state, ((TransitStopVertex) vertex).getStop()));
+    if (vertex instanceof TransitStopVertex tsv) {
+      stopsFound.add(NearbyStop.nearbyStopForState(state, tsv.getStop()));
     }
   }
 
   @Override
   public void visitEnqueue() {}
 
+  /**
+   * @return A de-duplicated list of nearby stops found by this visitor.
+   */
+  public List<NearbyStop> stopsFound() {
+    return stopsFound.stream().filter(PredicateUtils.distinctByKey(ns -> ns.stop)).toList();
+  }
+
   /**
    * @return A SkipEdgeStrategy that will stop exploring edges after the distance radius has been
    * reached.

src/main/java/org/opentripplanner/routing/graphfinder/StreetGraphFinder.java

@@ -41,7 +41,7 @@ public List<NearbyStop> findClosestStops(Coordinate coordinate, double radiusMet
       visitor,
       visitor.getSkipEdgeStrategy()
     );
-    return visitor.stopsFound;
+    return visitor.stopsFound();
   }
 
   @Override

src/test/java/org/opentripplanner/framework/lang/PredicateUtilsTest.java

@@ -0,0 +1,27 @@
+package org.opentripplanner.framework.lang;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.List;
+import java.util.stream.Stream;
+import org.junit.jupiter.api.Test;
+
+class PredicateUtilsTest {
+
+  private static String makeHello() {
+    return new String("HELLO");
+  }
+
+  @Test
+  void distinctByKey() {
+    var first = new Wrapper(10, makeHello());
+    var last = new Wrapper(20, "HI");
+    var stream = Stream.of(first, new Wrapper(20, makeHello()), last);
+
+    var deduplicated = stream.filter(PredicateUtils.distinctByKey(w -> w.string)).toList();
+
+    assertEquals(List.of(first, last), deduplicated);
+  }
+
+  private record Wrapper(int i, String string) {}
+}

src/test/java/org/opentripplanner/routing/graphfinder/StopFinderTraverseVisitorTest.java

@@ -0,0 +1,44 @@
+package org.opentripplanner.routing.graphfinder;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.List;
+import org.junit.jupiter.api.Test;
+import org.opentripplanner.street.model.vertex.TransitStopVertex;
+import org.opentripplanner.street.search.state.TestStateBuilder;
+import org.opentripplanner.transit.model._data.TransitModelForTest;
+import org.opentripplanner.transit.model.site.RegularStop;
+
+class StopFinderTraverseVisitorTest {
+
+  static final RegularStop STOP = TransitModelForTest.stopForTest("a-stop", 1, 1);
+
+  @Test
+  void deduplicateStops() {
+    var visitor = new StopFinderTraverseVisitor(1000);
+
+    assertEquals(List.of(), visitor.stopsFound());
+    var state1 = TestStateBuilder.ofWalking().streetEdge().stop(STOP).build();
+
+    visitor.visitVertex(state1);
+
+    var transitStopVertex = (TransitStopVertex) state1.getVertex();
+    final NearbyStop nearbyStop = NearbyStop.nearbyStopForState(
+      state1,
+      transitStopVertex.getStop()
+    );
+
+    assertEquals(List.of(nearbyStop), visitor.stopsFound());
+
+    visitor.visitVertex(state1);
+
+    assertEquals(List.of(nearbyStop), visitor.stopsFound());
+
+    // taking a different path to the same stop
+    var state2 = TestStateBuilder.ofWalking().streetEdge().streetEdge().stop(STOP).build();
+
+    visitor.visitVertex(state2);
+
+    assertEquals(List.of(nearbyStop), visitor.stopsFound());
+  }
+}

src/test/java/org/opentripplanner/street/search/state/TestStateBuilder.java

@@ -34,6 +34,7 @@
 import org.opentripplanner.street.search.request.StreetSearchRequest;
 import org.opentripplanner.transit.model._data.TransitModelForTest;
 import org.opentripplanner.transit.model.basic.Accessibility;
+import org.opentripplanner.transit.model.site.RegularStop;
 
 /**
  * Builds up a state chain for use in tests.
@@ -184,23 +185,16 @@ public TestStateBuilder elevator() {
     return this;
   }
 
+  public TestStateBuilder stop(RegularStop stop) {
+    return arriveAtStop(stop);
+  }
+
   /**
    * Add a state that arrives at a transit stop.
    */
   public TestStateBuilder stop() {
     count++;
-    var from = (StreetVertex) currentState.vertex;
-    var to = new TransitStopVertexBuilder()
-      .withStop(TransitModelForTest.stopForTest("stop", count, count))
-      .build();
-
-    var edge = StreetTransitStopLink.createStreetTransitStopLink(from, to);
-    var states = edge.traverse(currentState);
-    if (states.length != 1) {
-      throw new IllegalStateException("Only single state transitions are supported.");
-    }
-    currentState = states[0];
-    return this;
+    return arriveAtStop(TransitModelForTest.stopForTest("stop", count, count));
   }
 
   public TestStateBuilder enterStation(String id) {
@@ -247,6 +241,20 @@ public TestStateBuilder pathway(String s) {
     return this;
   }
 
+  @Nonnull
+  private TestStateBuilder arriveAtStop(RegularStop stop) {
+    var from = (StreetVertex) currentState.vertex;
+    var to = new TransitStopVertexBuilder().withStop(stop).build();
+
+    var edge = StreetTransitStopLink.createStreetTransitStopLink(from, to);
+    var states = edge.traverse(currentState);
+    if (states.length != 1) {
+      throw new IllegalStateException("Only single state transitions are supported.");
+    }
+    currentState = states[0];
+    return this;
+  }
+
   @Nonnull
   private static ElevatorOffboardVertex elevatorOffBoard(int count, String suffix) {
     return new ElevatorOffboardVertex(