Missing commits from: Improve paging opentripplanner#5551 (chery-pick…

…ed all commits including a5caefb)

src/main/java/org/opentripplanner/framework/collection/ListUtils.java

@@ -15,15 +15,15 @@ public class ListUtils {
    * null or empty.
    */
   public static <T> T first(List<T> list) {
-    return list == null || list.isEmpty() ? null : list.get(0);
+    return list == null || list.isEmpty() ? null : list.getFirst();
   }
 
   /**
    * Return the last element in the list. {@code null} is returned if the list is
    * null or empty.
    */
   public static <T> T last(List<T> list) {
-    return list == null || list.isEmpty() ? null : list.get(list.size() - 1);
+    return list == null || list.isEmpty() ? null : list.getLast();
   }
 
   /**

src/main/java/org/opentripplanner/framework/text/CharacterEscapeFormatter.java

@@ -8,7 +8,7 @@
  * does not contain the special character anymore. The original string can be computed by
  * reversing the process.
  * <p>
- * A "special-characters" is removed from a text using an escape character and
+ * A "special-character" is removed from a text using an escape character and
  * a substitution character. For example, if:
  * <ul>
  *   <li>the escape char is '\'</li>
@@ -96,10 +96,10 @@ public String decode(String encodedText) {
           );
         }
         prevEsc = false;
-      } else if (ch != escapeChar) {
-        buf.append(ch);
-      } else {
+      } else if (ch == escapeChar) {
         prevEsc = true;
+      } else {
+        buf.append(ch);
       }
     }
     return buf.toString();

src/main/java/org/opentripplanner/framework/token/Deserializer.java

@@ -9,14 +9,14 @@
 class Deserializer {
 
   private static final Pattern SPLIT_PATTERN = Pattern.compile(
-    "[" + Character.toString(TokenFormat.FIELD_SEPARATOR) + "]"
+    "[" + TokenFormatterConfiguration.fieldSeparator() + "]"
   );
 
   private final List<String> values;
 
   Deserializer(String token) {
     byte[] bytes = Base64.getUrlDecoder().decode(token);
-    var tokenFormatter = TokenFormat.tokenFormatter();
+    var tokenFormatter = TokenFormatterConfiguration.tokenFormatter();
     this.values =
       Stream.of(SPLIT_PATTERN.split(new String(bytes), -1)).map(tokenFormatter::decode).toList();
   }

src/main/java/org/opentripplanner/framework/token/Serializer.java

@@ -8,7 +8,7 @@ class Serializer {
   private final TokenDefinition definition;
   private final Object[] values;
   private final StringBuilder buf = new StringBuilder();
-  private final CharacterEscapeFormatter tokenFormatter = TokenFormat.tokenFormatter();
+  private final CharacterEscapeFormatter tokenFormatter = TokenFormatterConfiguration.tokenFormatter();
 
   private Serializer(TokenDefinition definition, Object[] values) {
     this.definition = definition;
@@ -45,6 +45,6 @@ private void writeString(String value) {
     if (value != null) {
       buf.append(tokenFormatter.encode(value));
     }
-    buf.append(TokenFormat.FIELD_SEPARATOR);
+    buf.append(TokenFormatterConfiguration.fieldSeparator());
   }
 }

src/main/java/org/opentripplanner/framework/token/Token.java

@@ -49,13 +49,13 @@ public String getString(String fieldName) {
    * Be careful with enums. If values are added or deleted the backward/forward compatibility
    * is compromised. This method return an empty value if the enum does not exist.
    * <p>
-   * To keep enum values forward compatible the value must first be added, and then it
+   * To ensure that enum values are forward compatible the value must first be added, and then it
    * can not be used in a token before OTP is released and deployed. Then when the enum value
-   * exit in the deployed server, then a new version of OTP can be rolled out witch now can use
+   * exist in the deployed server, then a new version of OTP can be rolled out which now can use
    * the new value.
    * <p>
-   * To keep backwards compatible enum values should be **deprecated**, not removed. The enum
-   * value can only be deleted, when all tokens with the value is expired (depend on use-case).
+   * To ensure backwards compatibility, enum values should be **deprecated**, not removed. The enum
+   * value can only be deleted, when all tokens with the value has expired (depends on use-case).
    */
   public <T extends Enum<T>> Optional<T> getEnum(String fieldName, Class<T> enumClass) {
     try {

src/main/java/org/opentripplanner/framework/token/TokenFormatterConfiguration.java

@@ -0,0 +1,25 @@
+package org.opentripplanner.framework.token;
+
+import org.opentripplanner.framework.text.CharacterEscapeFormatter;
+
+class TokenFormatterConfiguration {
+
+  private static final char TOKEN_ESCAPE = '\\';
+  private static final char TOKEN_SUBSTITUTION = '+';
+  private static final char FIELD_SEPARATOR = '|';
+
+  /** Prevent instantiation - this is a utility class. */
+  private TokenFormatterConfiguration() {}
+
+  /**
+   * We use the pipe '|' for field separations. The IDs included in the token frequently use
+   * ':' so the visual difference is better than the alternatives like ',' ';' and TAB.
+   */
+  static char fieldSeparator() {
+    return FIELD_SEPARATOR;
+  }
+
+  static CharacterEscapeFormatter tokenFormatter() {
+    return new CharacterEscapeFormatter(TOKEN_ESCAPE, FIELD_SEPARATOR, TOKEN_SUBSTITUTION);
+  }
+}

src/main/java/org/opentripplanner/framework/tostring/ToStringBuilder.java

@@ -61,7 +61,7 @@ public static ToStringBuilder of(Class<?> clazz) {
   }
 
   /**
-   * Create a ToStringBuilder for a "named" type. The prefered method is {@link #of(Class)},
+   * Create a ToStringBuilder for a "named" type. The preferred method is {@link #of(Class)},
    * but this can be used if the type is unknown or irrelevant.
    */
   public static ToStringBuilder of(String name) {

src/main/java/org/opentripplanner/model/plan/paging/cursor/PageCursorInput.java

@@ -12,15 +12,15 @@
  */
 public interface PageCursorInput {
   /**
-   * The earliest-removed-departure define the start of the search-window following the
+   * The earliest-removed-departure defines the start of the search-window following the
    * current window. To include this removed itinerary (and all other removed itineraries)
    * in the next-page search the search windows must overlap.
    */
   Instant earliestRemovedDeparture();
   Instant latestRemovedDeparture();
 
   /**
-   * In case the result have too many results; The {@code numberOfItineraries} request parameter
+   * In case the result has too many results: The {@code numberOfItineraries} request parameter
    * is less than the number of itineraries found, then we keep the last itinerary kept and
    * returned as part of the result. The sort vector will be included in the page-cursor and
    * used in the next/previous page to filter away duplicates.

src/main/java/org/opentripplanner/model/plan/paging/cursor/PageCursorSerializer.java

@@ -73,7 +73,7 @@ public static PageCursor decode(String cursor) {
       ItinerarySortKey itineraryPageCut = null;
       var token = SCHEMA_TOKEN.decode(cursor);
 
-      // This throws an exception if an enum is serialized witch is not in the code.
+      // This throws an exception if an enum is serialized which is not in the code.
       // This is a forward compatibility issue. To avoid this, add the value enum, role out.
       // Start using the enum, roll out again.
       PageType type = token.getEnum(TYPE_FIELD, PageType.class).orElseThrow();

src/main/java/org/opentripplanner/routing/algorithm/filterchain/ItineraryListFilterChainBuilder.java

@@ -445,8 +445,8 @@ public ItineraryListFilterChain build() {
 
     // Paging related filters - these filters are run after group-by filters to allow a result
     // outside the page to also take effect inside the window. This is debatable but lead to less
-    // noise, but is not deterministic because the result depend on the search-window size and
-    // where "cut" between each page is
+    // noise, however it is not deterministic because the result depends on the size of the search-window and
+    // where the "cut" between each page is located.
     {
       // Limit to search-window
       if (earliestDepartureTime != null) {

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

@@ -183,9 +183,9 @@ public void applyPageCursor() {
 
   /**
    * When paging we must crop the list of itineraries in the right end according to the sorting of
-   * the original search and according to the paging direction(next or previous). We always
+   * the original search and according to the paging direction (next or previous). We always
    * crop at the end of the initial search. This is a utility function delegating to the
-   * pageCursor, fi available.
+   * pageCursor, if available.
    */
   public ListSection cropItinerariesAt() {
     return pageCursor == null ? ListSection.TAIL : pageCursor.cropItinerariesAt();

src/main/java/org/opentripplanner/routing/api/response/TripSearchMetadata.java

@@ -48,30 +48,38 @@ private TripSearchMetadata(
   }
 
   public static TripSearchMetadata createForArriveBy(
-    Instant reqTime,
+    Instant earliestDepartureTimeUsed,
     Duration searchWindowUsed,
-    @Nullable Instant previousTimeInclusive
+    @Nullable Instant firstDepartureTime
   ) {
-    Instant prevDateTime = previousTimeInclusive == null
-      ? reqTime.minus(searchWindowUsed)
-      // Round up to closest minute, to meet the _inclusive_ requirement
-      : previousTimeInclusive.minusSeconds(1).truncatedTo(ChronoUnit.MINUTES).plusSeconds(60);
+    Instant actualEdt = firstDepartureTime == null
+      ? earliestDepartureTimeUsed
+      // Round down to the minute before to avoid duplicates. This may cause missed itineraries.
+      : firstDepartureTime.minusSeconds(60).truncatedTo(ChronoUnit.MINUTES);
 
-    return new TripSearchMetadata(searchWindowUsed, prevDateTime, reqTime.plus(searchWindowUsed));
+    return new TripSearchMetadata(
+      searchWindowUsed,
+      actualEdt.minus(searchWindowUsed),
+      earliestDepartureTimeUsed.plus(searchWindowUsed)
+    );
   }
 
   public static TripSearchMetadata createForDepartAfter(
-    Instant reqTime,
+    Instant requestDepartureTime,
     Duration searchWindowUsed,
-    Instant nextDateTimeExclusive
+    Instant lastDepartureTime
   ) {
-    Instant nextDateTime = nextDateTimeExclusive == null
-      ? reqTime.plus(searchWindowUsed)
+    Instant nextDateTime = lastDepartureTime == null
+      ? requestDepartureTime.plus(searchWindowUsed)
       // There is no way to make this work properly. If we round down we get duplicates, if we
-      // round up we skip itineraries.
-      : nextDateTimeExclusive.plusSeconds(60).truncatedTo(ChronoUnit.MINUTES);
+      // round up we might skip itineraries.
+      : lastDepartureTime.plusSeconds(60).truncatedTo(ChronoUnit.MINUTES);
 
-    return new TripSearchMetadata(searchWindowUsed, reqTime.minus(searchWindowUsed), nextDateTime);
+    return new TripSearchMetadata(
+      searchWindowUsed,
+      requestDepartureTime.minus(searchWindowUsed),
+      nextDateTime
+    );
   }
 
   @Override

src/main/java/org/opentripplanner/service/paging/PagingService.java

@@ -46,7 +46,11 @@ public PagingService(
     List<Itinerary> itineraries
   ) {
     this.searchWindowUsed = searchWindowUsed;
-    this.earliestDepartureTime = earliestDepartureTime;
+    // EDT is required if search-window is set
+    this.earliestDepartureTime =
+      searchWindowUsed == null
+        ? earliestDepartureTime
+        : Objects.requireNonNull(earliestDepartureTime);
     this.latestArrivalTime = latestArrivalTime;
     this.itinerariesSortOrder = Objects.requireNonNull(itinerariesSortOrder);
     this.arriveBy = arriveBy;
@@ -73,15 +77,15 @@ public PageCursor previousPageCursor() {
 
   @Nullable
   public TripSearchMetadata createTripSearchMetadata() {
-    if (searchWindowUsed == null) {
+    if (noTransitSearchPerformed()) {
       return null;
     }
 
     if (arriveBy) {
       return TripSearchMetadata.createForArriveBy(
-        latestArrivalTime,
+        earliestDepartureTime,
         searchWindowUsed,
-        firstRemovedArrivalTime()
+        firstKeptDepartureTime()
       );
     } else {
       return TripSearchMetadata.createForDepartAfter(
@@ -93,8 +97,7 @@ public TripSearchMetadata createTripSearchMetadata() {
   }
 
   private Duration calculateSearchWindowNextSearch() {
-    // No transit search performed
-    if (searchWindowUsed == null) {
+    if (noTransitSearchPerformed()) {
       return null;
     }
 
@@ -131,10 +134,10 @@ private Instant lastKeptDepartureTime() {
       : numItinerariesFilterResults.pageCut().startTimeAsInstant();
   }
 
-  private Instant firstRemovedArrivalTime() {
+  private Instant firstKeptDepartureTime() {
     return numItinerariesFilterResults == null
       ? null
-      : numItinerariesFilterResults.pageCut().endTimeAsInstant();
+      : numItinerariesFilterResults.pageCut().startTimeAsInstant();
   }
 
   private PagingSearchWindowAdjuster createSearchWindowAdjuster(
@@ -173,11 +176,9 @@ private PageCursorFactory pageCursorFactory() {
   }
 
   private PageCursorFactory mapIntoPageCursorFactory(@Nullable PageType currentPageType) {
-    Objects.requireNonNull(itinerariesSortOrder);
     var searchWindowNextSearch = calculateSearchWindowNextSearch();
     var factory = new PageCursorFactory(itinerariesSortOrder, searchWindowNextSearch);
 
-    // No transit search performed
     if (noTransitSearchPerformed()) {
       return factory;
     }
@@ -199,7 +200,7 @@ private PageCursorFactory mapIntoPageCursorFactory(@Nullable PageType currentPag
   }
 
   private void assertRequestPrerequisites() {
-    if (searchWindowUsed == null) {
+    if (noTransitSearchPerformed()) {
       throw new IllegalStateException("SearchWindow not set");
     }
     if (earliestDepartureTime == null) {

src/test/java/org/opentripplanner/_support/debug/TestDebug.java

@@ -12,39 +12,39 @@
  * or
  * $ java -DtestDebug ..
  * </pre>
- * In IntelliJ I recommend adding the system property in the <em>JUnit template</em>. In the test
+ * In IntelliJ it's recommended to add the system property in the <em>JUnit template</em>. In the test
  * drop down, choose <b>Edit Configuration...</b> then <b>Edit Configuration Templates..</b> and
  * choose <b>JUnit</b>.
  */
 public class TestDebug {
 
-  private static Boolean enabled = enabled();
+  private static final Boolean ENABLED = enabled();
 
   /** This is a utility class - only static methods */
   private TestDebug() {}
 
   public static boolean on() {
-    return enabled;
+    return ENABLED;
   }
 
   public static boolean off() {
-    return !enabled;
+    return !ENABLED;
   }
 
   public static void print(Object value) {
-    if (enabled()) {
+    if (ENABLED) {
       System.err.print(value);
     }
   }
 
   public static void println() {
-    if (enabled()) {
+    if (ENABLED) {
       System.err.println();
     }
   }
 
   public static void println(Object value) {
-    if (enabled()) {
+    if (ENABLED) {
       System.err.println(value);
     }
   }