Convert tranferSlack to duration or int

leonardehrenfried · leonardehrenfried · commit 208a6fee90d8 · 2024-06-07T12:52:05.000+02:00
diff --git a/docs/RouteRequest.md b/docs/RouteRequest.md
@@ -38,7 +38,7 @@ and in the [transferRequests in build-config.json](BuildConfiguration.md#transfe
 | [searchWindow](#rd_searchWindow)                                                                             |       `duration`       | The duration of the search-window.                                                                                                             | *Optional* |                  |  2.0  |
 | [streetRoutingTimeout](#rd_streetRoutingTimeout)                                                             |       `duration`       | The maximum time a street routing request is allowed to take before returning the results.                                                     | *Optional* | `"PT5S"`         |  2.2  |
 | [transferPenalty](#rd_transferPenalty)                                                                       |        `integer`       | An additional penalty added to boardings after the first.                                                                                      | *Optional* | `0`              |  2.0  |
-| [transferSlack](#rd_transferSlack)                                                                           |        `integer`       | The extra time needed to make a safe transfer in seconds.                                                                                      | *Optional* | `120`            |  2.0  |
+| [transferSlack](#rd_transferSlack)                                                                           |       `duration`       | The extra time needed to make a safe transfer in seconds.                                                                                      | *Optional* | `"PT2M"`         |  2.0  |
 | turnReluctance                                                                                               |        `double`        | Multiplicative factor on expected turning time.                                                                                                | *Optional* | `1.0`            |  2.0  |
 | [unpreferredCost](#rd_unpreferredCost)                                                                       | `cost-linear-function` | A cost function used to calculate penalty for an unpreferred route.                                                                            | *Optional* | `"0s + 1.00 t"`  |  2.2  |
 | waitReluctance                                                                                               |        `double`        | How much worse is waiting for a transit vehicle than being on a transit vehicle, as a multiplier.                                              | *Optional* | `1.0`            |  2.0  |
@@ -351,7 +351,7 @@ significant time or walking will still be taken.
 
 <h3 id="rd_transferSlack">transferSlack</h3>
 
-**Since version:** `2.0` ∙ **Type:** `integer` ∙ **Cardinality:** `Optional` ∙ **Default value:** `120`   
+**Since version:** `2.0` ∙ **Type:** `duration` ∙ **Cardinality:** `Optional` ∙ **Default value:** `"PT2M"`   
 **Path:** /routingDefaults 
 
 The extra time needed to make a safe transfer in seconds.
@@ -1181,7 +1181,7 @@ include stairs as a last result.
     },
     "waitReluctance" : 1.0,
     "otherThanPreferredRoutesPenalty" : 300,
-    "transferSlack" : 120,
+    "transferSlack" : "2m",
     "boardSlackForMode" : {
       "AIRPLANE" : "35m"
     },
diff --git a/docs/RouterConfiguration.md b/docs/RouterConfiguration.md
@@ -522,7 +522,7 @@ Used to group requests when monitoring OTP.
     },
     "waitReluctance" : 1.0,
     "otherThanPreferredRoutesPenalty" : 300,
-    "transferSlack" : 120,
+    "transferSlack" : "2m",
     "boardSlackForMode" : {
       "AIRPLANE" : "35m"
     },
diff --git a/src/ext/java/org/opentripplanner/ext/restapi/resources/RequestToPreferencesMapper.java b/src/ext/java/org/opentripplanner/ext/restapi/resources/RequestToPreferencesMapper.java
@@ -1,6 +1,7 @@
 package org.opentripplanner.ext.restapi.resources;
 
 import jakarta.validation.constraints.NotNull;
+import java.time.Duration;
 import java.util.function.Consumer;
 import org.opentripplanner.ext.restapi.mapping.LegacyVehicleRoutingOptimizeType;
 import org.opentripplanner.framework.lang.ObjectUtils;
@@ -151,7 +152,7 @@ private void mapTransfer(BoardAndAlightSlack boardAndAlightSlack) {
             "Invalid parameters: 'minTransferTime' must be greater than or equal to board slack plus alight slack"
           );
         }
-        transfer.withSlack(req.minTransferTime - boardAndAlightSlack.value);
+        transfer.withSlack(Duration.ofSeconds(req.minTransferTime - boardAndAlightSlack.value));
       }
 
       setIfNotNull(req.nonpreferredTransferPenalty, transfer::withNonpreferredCost);
diff --git a/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/request/RaptorRoutingRequestTransitData.java b/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/request/RaptorRoutingRequestTransitData.java
@@ -109,7 +109,7 @@ public RaptorRoutingRequestTransitData(
 
     this.slackProvider =
       new SlackProvider(
-        request.preferences().transfer().slack(),
+        (int) request.preferences().transfer().slack().toSeconds(),
         request.preferences().transit().boardSlack(),
         request.preferences().transit().alightSlack()
       );
diff --git a/src/main/java/org/opentripplanner/routing/api/request/preference/TransferPreferences.java b/src/main/java/org/opentripplanner/routing/api/request/preference/TransferPreferences.java
@@ -4,10 +4,12 @@
 import static org.opentripplanner.framework.lang.DoubleUtils.doubleEquals;
 
 import java.io.Serializable;
+import java.time.Duration;
 import java.util.Objects;
 import java.util.function.Consumer;
 import org.opentripplanner.framework.model.Cost;
 import org.opentripplanner.framework.model.Units;
+import org.opentripplanner.framework.time.DurationUtils;
 import org.opentripplanner.framework.tostring.ToStringBuilder;
 import org.opentripplanner.routing.algorithm.transferoptimization.api.TransferOptimizationParameters;
 
@@ -23,7 +25,7 @@ public final class TransferPreferences implements Serializable {
   private static final int MAX_NUMBER_OF_TRANSFERS = 30;
 
   private final Cost cost;
-  private final int slack;
+  private final Duration slack;
   private final double waitReluctance;
   private final int maxTransfers;
   private final int maxAdditionalTransfers;
@@ -32,7 +34,7 @@ public final class TransferPreferences implements Serializable {
 
   private TransferPreferences() {
     this.cost = Cost.ZERO;
-    this.slack = 120;
+    this.slack = Duration.ofMinutes(2);
     this.waitReluctance = 1.0;
     this.maxTransfers = 12;
     this.maxAdditionalTransfers = 5;
@@ -42,7 +44,7 @@ private TransferPreferences() {
 
   private TransferPreferences(Builder builder) {
     this.cost = builder.cost;
-    this.slack = Units.duration(builder.slack);
+    this.slack = DurationUtils.requireNonNegative(builder.slack);
     this.waitReluctance = Units.reluctance(builder.waitReluctance);
     this.maxTransfers = Units.count(builder.maxTransfers, MAX_NUMBER_OF_TRANSFERS);
     this.maxAdditionalTransfers =
@@ -89,7 +91,7 @@ public int cost() {
    * <p>
    * Unit is seconds. Default value is 2 minutes.
    */
-  public int slack() {
+  public Duration slack() {
     return slack;
   }
 
@@ -183,7 +185,7 @@ public String toString() {
     return ToStringBuilder
       .of(TransferPreferences.class)
       .addObj("cost", cost, DEFAULT.cost)
-      .addNum("slack", slack, DEFAULT.slack)
+      .addDuration("slack", slack, DEFAULT.slack)
       .addNum("waitReluctance", waitReluctance, DEFAULT.waitReluctance)
       .addNum("maxTransfers", maxTransfers, DEFAULT.maxTransfers)
       .addNum("maxAdditionalTransfers", maxAdditionalTransfers, DEFAULT.maxAdditionalTransfers)
@@ -196,7 +198,7 @@ public static class Builder {
 
     private final TransferPreferences original;
     private Cost cost;
-    private int slack;
+    private Duration slack;
     private Integer maxTransfers;
     private Integer maxAdditionalTransfers;
     private double waitReluctance;
@@ -223,7 +225,7 @@ public Builder withCost(int cost) {
       return this;
     }
 
-    public Builder withSlack(int slack) {
+    public Builder withSlack(Duration slack) {
       this.slack = slack;
       return this;
     }
diff --git a/src/main/java/org/opentripplanner/standalone/config/framework/json/ParameterBuilder.java b/src/main/java/org/opentripplanner/standalone/config/framework/json/ParameterBuilder.java
@@ -389,6 +389,19 @@ public Duration asDuration() {
     return ofRequired(DURATION, node -> parseDuration(node.asText()));
   }
 
+  public Duration asDurationOrSeconds(Duration dflt) {
+    // we claim that this only accepts durations but in reality it also accepts number of seconds
+    // we don't want to advertise this fact though
+    info.withType(DURATION);
+    setInfoOptional(dflt.toString());
+    var node = build();
+    if (node.isTextual()) {
+      return asDuration(dflt);
+    } else {
+      return Duration.ofSeconds((long) asDouble(dflt.toSeconds()));
+    }
+  }
+
   public List<Duration> asDurations(List<Duration> defaultValues) {
     return ofArrayAsList(DURATION, defaultValues, node -> parseDuration(node.asText()));
   }
diff --git a/src/main/java/org/opentripplanner/standalone/config/routerequest/TransferConfig.java b/src/main/java/org/opentripplanner/standalone/config/routerequest/TransferConfig.java
@@ -46,7 +46,7 @@ static void mapTransferPreferences(NodeAdapter c, TransferPreferences.Builder tx
             `alightSlack`.
             """
           )
-          .asInt(dft.slack())
+          .asDurationOrSeconds(dft.slack())
       )
       .withWaitReluctance(
         c
diff --git a/src/test/java/org/opentripplanner/GtfsTest.java b/src/test/java/org/opentripplanner/GtfsTest.java
@@ -128,7 +128,7 @@ public Itinerary plan(
     // Init preferences
     routingRequest.withPreferences(preferences -> {
       preferences.withTransfer(tx -> {
-        tx.withSlack(0);
+        tx.withSlack(Duration.ZERO);
         tx.withWaitReluctance(1);
         tx.withCost(preferLeastTransfers ? 300 : 0);
       });
diff --git a/src/test/java/org/opentripplanner/routing/api/request/preference/TransferPreferencesTest.java b/src/test/java/org/opentripplanner/routing/api/request/preference/TransferPreferencesTest.java
@@ -4,12 +4,13 @@
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.opentripplanner.routing.api.request.preference.ImmutablePreferencesAsserts.assertEqualsAndHashCode;
 
+import java.time.Duration;
 import org.junit.jupiter.api.Test;
 
 class TransferPreferencesTest {
 
   private static final int COST = 200;
-  private static final int SLACK = 150;
+  private static final Duration SLACK = Duration.ofSeconds(150);
   private static final double WAIT_RELUCTANCE = 0.95;
   private static final int MAX_TRANSFERS = 17;
   private static final int MAX_ADDITIONAL_TRANSFERS = 7;
diff --git a/src/test/java/org/opentripplanner/routing/core/RoutingPreferencesTest.java b/src/test/java/org/opentripplanner/routing/core/RoutingPreferencesTest.java
@@ -3,6 +3,7 @@
 import static org.junit.jupiter.api.Assertions.assertNotSame;
 import static org.junit.jupiter.api.Assertions.assertSame;
 
+import java.time.Duration;
 import org.junit.jupiter.api.Test;
 import org.opentripplanner.routing.api.request.preference.RoutingPreferences;
 
@@ -71,7 +72,7 @@ public void copyOfWithWalkChanges() {
   @Test
   public void copyOfWithTransferChanges() {
     var pref = new RoutingPreferences();
-    var copy = pref.copyOf().withTransfer(t -> t.withSlack(2)).build();
+    var copy = pref.copyOf().withTransfer(t -> t.withSlack(Duration.ofSeconds(2))).build();
 
     assertNotSame(pref, copy);
     assertNotSame(pref.transfer(), copy.transfer());
diff --git a/src/test/java/org/opentripplanner/standalone/config/routerequest/RouteRequestConfigTest.java b/src/test/java/org/opentripplanner/standalone/config/routerequest/RouteRequestConfigTest.java
@@ -5,7 +5,10 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.opentripplanner.standalone.config.framework.json.JsonSupport.newNodeAdapterForTest;
 
+import java.time.Duration;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.opentripplanner.routing.api.request.StreetMode;
 
 class RouteRequestConfigTest {
@@ -96,13 +99,14 @@ public void testAccessEgressPenalty() {
     );
   }
 
-  @Test
-  public void transferSlackAsInt() {
-    var slack = mapSlack("99");
-    assertEquals(99, slack);
+  @ParameterizedTest
+  @ValueSource(strings = { "99", "\"99s\"", "\"1m39s\"", "\"PT1m39s\"" })
+  public void transferSlackAsIntOrDuration(String input) {
+    var slack = mapSlack(input);
+    assertEquals(Duration.ofSeconds(99), slack);
   }
 
-  private static int mapSlack(String input) {
+  private static Duration mapSlack(String input) {
     var nodeAdapter = newNodeAdapterForTest(
       """
       {
diff --git a/src/test/resources/standalone/config/router-config.json b/src/test/resources/standalone/config/router-config.json
@@ -79,7 +79,7 @@
     },
     "waitReluctance": 1.0,
     "otherThanPreferredRoutesPenalty": 300,
-    "transferSlack": 120,
+    "transferSlack": "2m",
     // Default slack for any mode is 0 (zero)
     "boardSlackForMode": {
       "AIRPLANE": "35m"

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,7 @@`
`1`	`1`	`package org.opentripplanner.ext.restapi.resources;`
`2`	`2`
`3`	`3`	`import jakarta.validation.constraints.NotNull;`
	`4`	`+import java.time.Duration;`
`4`	`5`	`import java.util.function.Consumer;`
`5`	`6`	`import org.opentripplanner.ext.restapi.mapping.LegacyVehicleRoutingOptimizeType;`
`6`	`7`	`import org.opentripplanner.framework.lang.ObjectUtils;`
`@@ -151,7 +152,7 @@ private void mapTransfer(BoardAndAlightSlack boardAndAlightSlack) {`
`151`	`152`	`"Invalid parameters: 'minTransferTime' must be greater than or equal to board slack plus alight slack"`
`152`	`153`	`);`
`153`	`154`	`}`
`154`		`- transfer.withSlack(req.minTransferTime - boardAndAlightSlack.value);`
	`155`	`+ transfer.withSlack(Duration.ofSeconds(req.minTransferTime - boardAndAlightSlack.value));`
`155`	`156`	`}`
`156`	`157`
`157`	`158`	`setIfNotNull(req.nonpreferredTransferPenalty, transfer::withNonpreferredCost);`
Original file line number	Diff line number	Diff line change
`@@ -46,7 +46,7 @@ static void mapTransferPreferences(NodeAdapter c, TransferPreferences.Builder tx`
`46`	`46`	`alightSlack`.
`47`	`47`	`"""`
`48`	`48`	`)`
`49`		`- .asInt(dft.slack())`
	`49`	`+ .asDurationOrSeconds(dft.slack())`
`50`	`50`	`)`
`51`	`51`	`.withWaitReluctance(`
`52`	`52`	`c`