Apply review feedback

src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/Transfer.java

@@ -16,25 +16,7 @@
 
 public class Transfer {
 
-  /**
-   * Since transfer costs are not computed through a full A* with pruning they can incur an
-   * absurdly high cost that overflows the integer cost inside RAPTOR
-   * (https://github.com/opentripplanner/OpenTripPlanner/issues/5509).
-   * <p>
-   * An example would be a transfer using lots of stairs being used on a wheelchair when no
-   * wheelchair-specific one has been generated.
-   * (see https://docs.opentripplanner.org/en/dev-2.x/Accessibility/).
-   * <p>
-   * For this reason there is this sanity limit that makes sure that the transfer cost stays below a
-   * limit that is still very high (several days of transit-equivalent cost) but far away from the
-   * integer overflow.
-   * <p>
-   * The unit is in RAPTOR cost, so it's centiseconds.
-   *
-   * @see EdgeTraverser
-   * @see RaptorCostConverter
-   */
-  private static final int MAX_TRANSFER_RAPTOR_COST = Integer.MAX_VALUE / 30;
+  protected static final int MAX_TRANSFER_COST = 2_000_000;
 
   private final int toStop;
 
@@ -80,19 +62,15 @@ public List<Edge> getEdges() {
   }
 
   public Optional<RaptorTransfer> asRaptorTransfer(StreetSearchRequest request) {
-    return toRaptor(request)
-      .filter(s -> s.generalizedCost() < MAX_TRANSFER_RAPTOR_COST && s.generalizedCost() >= 0);
-  }
-
-  private Optional<RaptorTransfer> toRaptor(StreetSearchRequest request) {
     WalkPreferences walkPreferences = request.preferences().walk();
     if (edges == null || edges.isEmpty()) {
       double durationSeconds = distanceMeters / walkPreferences.speed();
+      final double domainCost = costLimitSanityCheck(durationSeconds * walkPreferences.reluctance());
       return Optional.of(
         new DefaultRaptorTransfer(
           this.toStop,
           (int) Math.ceil(durationSeconds),
-          RaptorCostConverter.toRaptorCost(durationSeconds * walkPreferences.reluctance()),
+          RaptorCostConverter.toRaptorCost(domainCost),
           this
         )
       );
@@ -107,12 +85,32 @@ private Optional<RaptorTransfer> toRaptor(StreetSearchRequest request) {
       new DefaultRaptorTransfer(
         this.toStop,
         (int) s.getElapsedTimeSeconds(),
-        RaptorCostConverter.toRaptorCost(s.getWeight()),
+        RaptorCostConverter.toRaptorCost(costLimitSanityCheck(s.getWeight())),
         this
       )
     );
   }
 
+  /**
+   * Since transfer costs are not computed through a full A* with pruning they can incur an
+   * absurdly high cost that overflows the integer cost inside RAPTOR
+   * (https://github.com/opentripplanner/OpenTripPlanner/issues/5509).
+   * <p>
+   * An example would be a transfer using lots of stairs being used on a wheelchair when no
+   * wheelchair-specific one has been generated.
+   * (see https://docs.opentripplanner.org/en/dev-2.x/Accessibility/).
+   * <p>
+   * For this reason there is this sanity limit that makes sure that the transfer cost stays below a
+   * limit that is still very high (several days of transit-equivalent cost) but far away from the
+   * integer overflow.
+   *
+   * @see EdgeTraverser
+   * @see RaptorCostConverter
+   */
+  private static int costLimitSanityCheck(double cost) {
+    return (cost >= 0 && cost <= MAX_TRANSFER_COST) ? (int) cost : MAX_TRANSFER_COST;
+  }
+
   @Override
   public String toString() {
     return ToStringBuilder

src/test/java/org/opentripplanner/_support/geometry/Coordinates.java

@@ -8,4 +8,5 @@ public class Coordinates {
   public static final Coordinate BERLIN_BRANDENBURG_GATE = new Coordinate(52.51627, 13.37770);
   public static final Coordinate HAMBURG = new Coordinate(53.5566, 10.0003);
   public static final Coordinate KONGSBERG_PLATFORM_1 = new Coordinate(59.67216, 9.65107);
+  public static final Coordinate BOSTON = new Coordinate(42.36541, -71.06129);
 }

src/test/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/TransferTest.java

@@ -1,5 +1,6 @@
 package org.opentripplanner.routing.algorithm.raptoradapter.transit;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.opentripplanner.street.model._data.StreetModelForTest.intersectionVertex;
 
@@ -9,6 +10,7 @@
 import org.junit.jupiter.api.Test;
 import org.opentripplanner._support.geometry.Coordinates;
 import org.opentripplanner.raptor.api.model.RaptorTransfer;
+import org.opentripplanner.routing.algorithm.raptoradapter.transit.cost.RaptorCostConverter;
 import org.opentripplanner.street.model._data.StreetModelForTest;
 import org.opentripplanner.street.model.vertex.IntersectionVertex;
 import org.opentripplanner.street.search.request.StreetSearchRequest;
@@ -19,22 +21,22 @@ class TransferTest {
   private static final IntersectionVertex BRANDENBURG_GATE_V = intersectionVertex(
     Coordinates.BERLIN_BRANDENBURG_GATE
   );
-  private static final IntersectionVertex KONGSBERG_V = intersectionVertex(
-    Coordinates.KONGSBERG_PLATFORM_1
+  private static final IntersectionVertex BOSTON_V = intersectionVertex(
+    Coordinates.BOSTON
   );
 
   @Nested
   class WithEdges {
 
     @Test
     void limitMaxCost() {
-      // very long edge from Berlin to Kongsberg, Norway that has of course a huge cost to traverse
-      var edge = StreetModelForTest.streetEdge(BERLIN_V, KONGSBERG_V);
+      // very long edge from Berlin to Boston that has of course a huge cost to traverse
+      var edge = StreetModelForTest.streetEdge(BERLIN_V, BOSTON_V);
 
       var veryLongTransfer = new Transfer(0, List.of(edge));
-      assertTrue(veryLongTransfer.getDistanceMeters() > 800_000);
+      assertTrue(veryLongTransfer.getDistanceMeters() > 1_000_000);
       // cost would be too high, so it should not be included in RAPTOR search
-      assertTrue(veryLongTransfer.asRaptorTransfer(StreetSearchRequest.of().build()).isEmpty());
+      assertMaxCost(veryLongTransfer.asRaptorTransfer(StreetSearchRequest.of().build()).get());
     }
 
     @Test
@@ -50,26 +52,27 @@ void allowLowCost() {
     }
   }
 
+
   @Nested
   class WithoutEdges {
 
     @Test
     void overflow() {
       var veryLongTransfer = new Transfer(0, Integer.MAX_VALUE);
-      assertTrue(veryLongTransfer.asRaptorTransfer(StreetSearchRequest.of().build()).isEmpty());
+      assertMaxCost(veryLongTransfer.asRaptorTransfer(StreetSearchRequest.of().build()).get());
     }
 
     @Test
     void negativeCost() {
       var veryLongTransfer = new Transfer(0, -5);
-      assertTrue(veryLongTransfer.asRaptorTransfer(StreetSearchRequest.of().build()).isEmpty());
+      assertMaxCost(veryLongTransfer.asRaptorTransfer(StreetSearchRequest.of().build()).get());
     }
 
     @Test
     void limitMaxCost() {
-      var veryLongTransfer = new Transfer(0, 800_000);
+      var veryLongTransfer = new Transfer(0, 8_000_000);
       // cost would be too high, so it should not be included in RAPTOR search
-      assertTrue(veryLongTransfer.asRaptorTransfer(StreetSearchRequest.of().build()).isEmpty());
+      assertMaxCost(veryLongTransfer.asRaptorTransfer(StreetSearchRequest.of().build()).get());
     }
 
     @Test
@@ -82,4 +85,9 @@ void allowLowCost() {
       assertTrue(raptorTransfer.isPresent());
     }
   }
+
+  private static void assertMaxCost(RaptorTransfer transfer) {
+    assertEquals(RaptorCostConverter.toRaptorCost(Transfer.MAX_TRANSFER_COST), transfer.generalizedCost());
+  }
+
 }