Merge pull request #5293 from entur/limit_search_window

Add a configurable limit for the search window

docs/RouterConfiguration.md

@@ -50,6 +50,7 @@ A full list of them can be found in the [RouteRequest](RouteRequest.md).
 | [transit](#transit)                                                                       |        `object`       | Configuration for transit searches with RAPTOR.                                                       | *Optional* |               |   na  |
 |    [iterationDepartureStepInSeconds](#transit_iterationDepartureStepInSeconds)            |       `integer`       | Step for departure times between each RangeRaptor iterations.                                         | *Optional* | `60`          |   na  |
 |    [maxNumberOfTransfers](#transit_maxNumberOfTransfers)                                  |       `integer`       | This parameter is used to allocate enough memory space for Raptor.                                    | *Optional* | `12`          |   na  |
+|    [maxSearchWindow](#transit_maxSearchWindow)                                            |       `duration`      | Upper limit of the request parameter searchWindow.                                                    | *Optional* | `"PT24H"`     |  2.4  |
 |    [scheduledTripBinarySearchThreshold](#transit_scheduledTripBinarySearchThreshold)      |       `integer`       | This threshold is used to determine when to perform a binary trip schedule search.                    | *Optional* | `50`          |   na  |
 |    [searchThreadPoolSize](#transit_searchThreadPoolSize)                                  |       `integer`       | Split a travel search in smaller jobs and run them in parallel to improve performance.                | *Optional* | `0`           |   na  |
 |    [transferCacheMaxSize](#transit_transferCacheMaxSize)                                  |       `integer`       | The maximum number of distinct transfers parameters to cache pre-calculated transfers for.            | *Optional* | `25`          |   na  |
@@ -198,6 +199,22 @@ entire transit network. The memory overhead of setting this higher than the maxi
 transfers is very little so it is better to set it too high than to low.
 
 
+<h3 id="transit_maxSearchWindow">maxSearchWindow</h3>
+
+**Since version:** `2.4` ∙ **Type:** `duration` ∙ **Cardinality:** `Optional` ∙ **Default value:** `"PT24H"`   
+**Path:** /transit 
+
+Upper limit of the request parameter searchWindow.
+
+Maximum search window that can be set through the searchWindow API parameter.
+Due to the way timetable data are collected before a Raptor trip search,
+using a search window larger than 24 hours may lead to inconsistent search results.
+Limiting the search window prevents also potential performance issues.
+The recommended maximum value is 24 hours.
+This parameter does not restrict the maximum duration of a dynamic search window (use
+the parameter `transit.dynamicSearchWindow.maxWindow` to specify such a restriction).
+
+
 <h3 id="transit_scheduledTripBinarySearchThreshold">scheduledTripBinarySearchThreshold</h3>
 
 **Since version:** `na` ∙ **Type:** `integer` ∙ **Cardinality:** `Optional` ∙ **Default value:** `50`   

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

@@ -47,6 +47,11 @@ public int transferCacheMaxSize() {
       return 5;
     }
 
+    @Override
+    public Duration maxSearchWindow() {
+      return Duration.ofHours(24);
+    }
+
     @Override
     public List<Duration> pagingSearchWindowAdjustments() {
       return PAGING_SEARCH_WINDOW_ADJUSTMENTS;
@@ -77,6 +82,16 @@ public List<RouteRequest> transferCacheRequests() {
    */
   int transferCacheMaxSize();
 
+  /**
+   * The maximum search window that can be set through the searchWindow API parameter. Due to the
+   * way timetable data are collected before a Raptor trip search, using a search window larger than
+   * 24 hours may lead to inconsistent search results. Limiting the search window prevents also
+   * potential performance issues. The recommended maximum value is 24 hours.
+   * This parameter does not restrict the maximum duration of a dynamic search window (use
+   * the parameter transit.dynamicSearchWindow.maxWindow to specify such a restriction).
+   */
+  Duration maxSearchWindow();
+
   /**
    * This parameter is used to reduce the number of pages a client have to step through for a
    * journey where there are few alternatives/low frequency. This also work well to adjust for

src/main/java/org/opentripplanner/routing/api/request/RouteRequest.java

@@ -12,6 +12,7 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.function.Consumer;
+import javax.annotation.Nullable;
 import org.opentripplanner.framework.time.DateUtils;
 import org.opentripplanner.model.GenericLocation;
 import org.opentripplanner.model.plan.SortOrder;
@@ -58,6 +59,9 @@ public class RouteRequest implements Cloneable, Serializable {
 
   private Instant dateTime = Instant.now();
 
+  @Nullable
+  private Duration maxSearchWindow;
+
   private Duration searchWindow;
 
   private PageCursor pageCursor;
@@ -330,9 +334,29 @@ public Duration searchWindow() {
   }
 
   public void setSearchWindow(Duration searchWindow) {
+    if (searchWindow != null) {
+      if (hasMaxSearchWindow() && searchWindow.toSeconds() > maxSearchWindow.toSeconds()) {
+        throw new IllegalArgumentException("The search window cannot exceed " + maxSearchWindow);
+      }
+      if (searchWindow.isNegative()) {
+        throw new IllegalArgumentException("The search window must be a positive duration");
+      }
+    }
     this.searchWindow = searchWindow;
   }
 
+  private boolean hasMaxSearchWindow() {
+    return maxSearchWindow != null;
+  }
+
+  public Duration maxSearchWindow() {
+    return maxSearchWindow;
+  }
+
+  public void setMaxSearchWindow(@Nullable Duration maxSearchWindow) {
+    this.maxSearchWindow = maxSearchWindow;
+  }
+
   public Locale locale() {
     return locale;
   }

src/main/java/org/opentripplanner/standalone/config/RouterConfig.java

@@ -69,6 +69,7 @@ public RouterConfig(JsonNode node, String source, boolean logUnusedParams) {
     this.transmodelApi = new TransmodelAPIConfig("transmodelApi", root);
     this.routingRequestDefaults = mapDefaultRouteRequest("routingDefaults", root);
     this.transitConfig = new TransitRoutingConfig("transit", root, routingRequestDefaults);
+    this.routingRequestDefaults.setMaxSearchWindow(transitConfig.maxSearchWindow());
     this.updatersParameters = new UpdatersConfig(root);
     this.rideHailingConfig = new RideHailingServicesConfig(root);
     this.vectorTileLayers = VectorTileConfig.mapVectorTilesParameters(root, "vectorTileLayers");

src/main/java/org/opentripplanner/standalone/config/routerconfig/TransitRoutingConfig.java

@@ -5,6 +5,7 @@
 import static org.opentripplanner.standalone.config.framework.json.OtpVersion.V2_1;
 import static org.opentripplanner.standalone.config.framework.json.OtpVersion.V2_2;
 import static org.opentripplanner.standalone.config.framework.json.OtpVersion.V2_3;
+import static org.opentripplanner.standalone.config.framework.json.OtpVersion.V2_4;
 
 import java.time.Duration;
 import java.util.List;
@@ -31,6 +32,7 @@ public final class TransitRoutingConfig implements RaptorTuningParameters, Trans
   private final List<Duration> pagingSearchWindowAdjustments;
 
   private final Map<StopTransferPriority, Integer> stopTransferCost;
+  private final Duration maxSearchWindow;
   private final DynamicSearchWindowCoefficients dynamicSearchWindowCoefficients;
 
   public TransitRoutingConfig(
@@ -200,6 +202,24 @@ public TransitRoutingConfig(
         )
         .asDurations(PAGING_SEARCH_WINDOW_ADJUSTMENTS);
 
+    this.maxSearchWindow =
+      c
+        .of("maxSearchWindow")
+        .since(V2_4)
+        .summary("Upper limit of the request parameter searchWindow.")
+        .description(
+          """
+            Maximum search window that can be set through the searchWindow API parameter.
+            Due to the way timetable data are collected before a Raptor trip search,
+            using a search window larger than 24 hours may lead to inconsistent search results.
+            Limiting the search window prevents also potential performance issues.
+            The recommended maximum value is 24 hours.
+            This parameter does not restrict the maximum duration of a dynamic search window (use
+            the parameter `transit.dynamicSearchWindow.maxWindow` to specify such a restriction).
+            """
+        )
+        .asDuration(Duration.ofHours(24));
+
     this.dynamicSearchWindowCoefficients = new DynamicSearchWindowConfig("dynamicSearchWindow", c);
   }
 
@@ -248,6 +268,11 @@ public List<RouteRequest> transferCacheRequests() {
     return transferCacheRequests;
   }
 
+  @Override
+  public Duration maxSearchWindow() {
+    return maxSearchWindow;
+  }
+
   @Override
   public List<Duration> pagingSearchWindowAdjustments() {
     return pagingSearchWindowAdjustments;

src/test/java/org/opentripplanner/routing/core/RouteRequestTest.java

@@ -2,9 +2,11 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
+import java.time.Duration;
 import java.util.List;
 import org.junit.jupiter.api.Test;
 import org.opentripplanner.model.GenericLocation;
@@ -16,6 +18,12 @@
 
 class RouteRequestTest {
 
+  private static final Duration DURATION_24_HOURS = Duration.ofHours(24);
+  private static final Duration DURATION_24_HOURS_AND_ONE_MINUTE = DURATION_24_HOURS.plusMinutes(1);
+  private static final Duration DURATION_ZERO = Duration.ofMinutes(0);
+  private static final Duration DURATION_ONE_MINUTE = Duration.ofMinutes(1);
+  private static final Duration DURATION_MINUS_ONE_MINUTE = DURATION_ONE_MINUTE.negated();
+
   @Test
   public void testRequest() {
     // TODO VIA: looks like some parts of this test are obsolete since method no longer exist
@@ -110,6 +118,37 @@ void testValidateFromAndTo() {
     request.validateOriginAndDestination();
   }
 
+  @Test
+  void testValidSearchWindow() {
+    RouteRequest request = new RouteRequest();
+    request.setSearchWindow(DURATION_ONE_MINUTE);
+  }
+
+  @Test
+  void testZeroSearchWindow() {
+    RouteRequest request = new RouteRequest();
+    request.setSearchWindow(DURATION_ZERO);
+  }
+
+  @Test
+  void testTooLongSearchWindow() {
+    RouteRequest request = new RouteRequest();
+    request.setMaxSearchWindow(DURATION_24_HOURS);
+    assertThrows(
+      IllegalArgumentException.class,
+      () -> request.setSearchWindow(DURATION_24_HOURS_AND_ONE_MINUTE)
+    );
+  }
+
+  @Test
+  void testNegativeSearchWindow() {
+    RouteRequest request = new RouteRequest();
+    assertThrows(
+      IllegalArgumentException.class,
+      () -> request.setSearchWindow(DURATION_MINUS_ONE_MINUTE)
+    );
+  }
+
   private GenericLocation randomLocation() {
     return new GenericLocation(Math.random(), Math.random());
   }

src/test/java/org/opentripplanner/standalone/config/RouterConfigDocTest.java

@@ -0,0 +1,53 @@
+package org.opentripplanner.standalone.config;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.fail;
+import static org.opentripplanner.standalone.config.framework.json.JsonSupport.jsonNodeForTest;
+import static org.opentripplanner.standalone.config.framework.json.JsonSupport.jsonNodeFromResource;
+
+import org.junit.jupiter.api.Test;
+import org.opentripplanner.framework.application.OtpAppException;
+import org.opentripplanner.standalone.config.framework.json.NodeAdapter;
+
+class RouterConfigDocTest {
+
+  private static final String SOURCE = "RouterConfigTest";
+
+  /**
+   * Test that the router-config.json example used in documentation is valid.
+   */
+  @Test
+  void validateExample() {
+    var node = jsonNodeFromResource("standalone/config/router-config.json");
+
+    // Setup so we get access to the NodeAdapter
+    var a = new NodeAdapter(node, SOURCE);
+    var c = new RouterConfig(a, false);
+
+    // Test for unused parameters
+    var buf = new StringBuilder();
+    a.logAllWarnings(m -> buf.append("\n").append(m));
+    if (!buf.isEmpty()) {
+      fail(buf.toString());
+    }
+  }
+
+  @Test
+  void testSemanticValidation() {
+    // apiProcessingTimeout must be greater then streetRoutingTimeout
+    var root = createNodeAdaptor(
+      """
+      {
+        server: { apiProcessingTimeout : "1s" },
+        routingDefaults: { streetRoutingTimeout: "17s" }
+      }
+      """
+    );
+    //
+    assertThrows(OtpAppException.class, () -> new RouterConfig(root, false));
+  }
+
+  private static NodeAdapter createNodeAdaptor(String jsonText) {
+    return new NodeAdapter(jsonNodeForTest(jsonText), "Test");
+  }
+}

src/test/java/org/opentripplanner/standalone/config/RouterConfigTest.java

@@ -1,53 +1,34 @@
 package org.opentripplanner.standalone.config;
 
-import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.fail;
-import static org.opentripplanner.standalone.config.framework.json.JsonSupport.jsonNodeForTest;
-import static org.opentripplanner.standalone.config.framework.json.JsonSupport.jsonNodeFromResource;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.opentripplanner.standalone.config.framework.json.JsonSupport.newNodeAdapterForTest;
 
+import java.time.Duration;
 import org.junit.jupiter.api.Test;
-import org.opentripplanner.framework.application.OtpAppException;
-import org.opentripplanner.standalone.config.framework.json.NodeAdapter;
 
 class RouterConfigTest {
 
-  private static final String SOURCE = "RouterConfigTest";
-
-  /**
-   * Test that the router-config.json example used in documentation is valid.
-   */
   @Test
-  void validateExample() {
-    var node = jsonNodeFromResource("standalone/config/router-config.json");
-
-    // Setup so we get access to the NodeAdapter
-    var a = new NodeAdapter(node, SOURCE);
-    var c = new RouterConfig(a, false);
-
-    // Test for unused parameters
-    var buf = new StringBuilder();
-    a.logAllWarnings(m -> buf.append("\n").append(m));
-    if (!buf.isEmpty()) {
-      fail(buf.toString());
-    }
+  void defaultMaxSearchWindowIs24Hours() {
+    validateMaxSearchWindow("", Duration.ofHours(24));
   }
 
   @Test
-  void testSemanticValidation() {
-    // apiProcessingTimeout must be greater then streetRoutingTimeout
-    var root = createNodeAdaptor(
+  void maxSearchWindowIsOverriddenInRouterConfig() {
+    validateMaxSearchWindow(
       """
       {
-        server: { apiProcessingTimeout : "1s" },
-        routingDefaults: { streetRoutingTimeout: "17s" }
+        "transit": {
+          "maxSearchWindow": "48h"
+        }
       }
-      """
+      """,
+      Duration.ofHours(48)
     );
-    //
-    assertThrows(OtpAppException.class, () -> new RouterConfig(root, false));
   }
 
-  private static NodeAdapter createNodeAdaptor(String jsonText) {
-    return new NodeAdapter(jsonNodeForTest(jsonText), "Test");
+  private void validateMaxSearchWindow(String configuration, Duration expectedDuration) {
+    RouterConfig routerConfig = new RouterConfig(newNodeAdapterForTest(configuration), false);
+    assertEquals(expectedDuration, routerConfig.routingRequestDefaults().maxSearchWindow());
   }
 }