Merge pull request opentripplanner#6621 from HSLdevcom/fix-paging-first-search-previous-page-edge-case

Fix issue with paging when going to the next page after initial arrive by search

Author: VillePihlava

application/src/main/java/org/opentripplanner/model/plan/SortOrder.java

@@ -32,7 +32,7 @@ public enum SortOrder {
   STREET_AND_DEPARTURE_TIME;
 
   /**
-   * The itineraries are sorted with by arrival time with the earliest arrival time first. When
+   * The itineraries are sorted by arrival time with the earliest arrival time first. When
    * paging we need to know which end of the list of itineraries we should crop. This method is used
    * to decide that together with the current page type (next/previous).
    * <p>

application/src/main/java/org/opentripplanner/model/plan/paging/cursor/PageCursorFactory.java

@@ -13,6 +13,15 @@
 
 public class PageCursorFactory {
 
+  /**
+   * The search-window start and end is [inclusive, exclusive], so to calculate the start of the
+   * search-window from the last time included in the search window we need to include one extra
+   * minute at the end.
+   * <p>
+   * The value is set to a minute because raptor operates in one minute increments.
+   */
+  private static final Duration SEARCH_WINDOW_END_EXCLUSIVITY_TIME_ADDITION = Duration.ofMinutes(1);
+
   private final SortOrder sortOrder;
   private final Duration newSearchWindow;
   private PageType currentPageType;
@@ -22,6 +31,7 @@ public class PageCursorFactory {
   private boolean wholeSearchWindowUsed = true;
   private ItinerarySortKey itineraryPageCut = null;
   private PageCursorInput pageCursorInput = null;
+  private Instant firstSearchLatestItineraryDeparture = null;
 
   private PageCursor nextCursor = null;
   private PageCursor prevCursor = null;
@@ -33,17 +43,24 @@ public PageCursorFactory(SortOrder sortOrder, Duration newSearchWindow) {
 
   /**
    * Set the original search earliest-departure-time({@code edt}), latest-arrival-time ({@code lat},
-   * optional) and the search-window used.
+   * optional) and the search-window used. Also resolve the page-type and
+   * first-search-latest-itinerary-departure.
    */
   public PageCursorFactory withOriginalSearch(
     @Nullable PageType pageType,
+    @Nullable Instant firstItineraryResultDeparture,
     Instant edt,
     Instant lat,
     Duration searchWindow
   ) {
     this.currentPageType = pageType == null
       ? resolvePageTypeForTheFirstSearch(sortOrder)
       : pageType;
+    this.firstSearchLatestItineraryDeparture = resolveFirstSearchLatestItineraryDeparture(
+      pageType,
+      firstItineraryResultDeparture,
+      edt
+    );
 
     this.currentEdt = edt;
     this.currentLat = lat;
@@ -109,6 +126,26 @@ private static PageType resolvePageTypeForTheFirstSearch(SortOrder sortOrder) {
     return sortOrder.isSortedByAscendingArrivalTime() ? NEXT_PAGE : PREVIOUS_PAGE;
   }
 
+  /**
+   * If the first search is an arrive by search (PREVIOUS_PAGE type), the current search window is
+   * misleading. Instead of using the current search window to set the page cursor of the next
+   * page, the departure time of the latest itinerary result is used to avoid missing itineraries.
+   */
+  private Instant resolveFirstSearchLatestItineraryDeparture(
+    @Nullable PageType pageType,
+    @Nullable Instant firstItineraryResultDeparture,
+    Instant edt
+  ) {
+    if (pageType == null && resolvePageTypeForTheFirstSearch(sortOrder) == PREVIOUS_PAGE) {
+      if (firstItineraryResultDeparture != null) {
+        return firstItineraryResultDeparture;
+      } else {
+        return edt;
+      }
+    }
+    return null;
+  }
+
   /** Create page cursor pair (next and previous) */
   private void createPageCursors() {
     if (currentEdt == null || nextCursor != null || prevCursor != null) {
@@ -128,10 +165,10 @@ private void createPageCursors() {
         prevEdt = edtBeforeNewSw();
         nextEdt = pageCursorInput.earliestRemovedDeparture();
       } else {
-        // The search-window start and end is [inclusive, exclusive], so to calculate the start of the
-        // search-window from the last time included in the search window we need to include one extra
-        // minute at the end.
-        prevEdt = pageCursorInput.latestRemovedDeparture().minus(newSearchWindow).plusSeconds(60);
+        prevEdt = pageCursorInput
+          .latestRemovedDeparture()
+          .minus(newSearchWindow)
+          .plus(SEARCH_WINDOW_END_EXCLUSIVITY_TIME_ADDITION);
         nextEdt = edtAfterUsedSw();
       }
     }
@@ -163,6 +200,15 @@ private Instant edtBeforeNewSw() {
   }
 
   private Instant edtAfterUsedSw() {
-    return currentEdt.plus(currentSearchWindow);
+    Instant defaultEdt = currentEdt.plus(currentSearchWindow);
+    if (firstSearchLatestItineraryDeparture != null) {
+      Instant edtFromLatestItineraryDeparture = firstSearchLatestItineraryDeparture.plus(
+        SEARCH_WINDOW_END_EXCLUSIVITY_TIME_ADDITION
+      );
+      if (edtFromLatestItineraryDeparture.isBefore(defaultEdt)) {
+        return edtFromLatestItineraryDeparture;
+      }
+    }
+    return defaultEdt;
   }
 }

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

@@ -183,6 +183,7 @@ private PageCursorFactory mapIntoPageCursorFactory(@Nullable PageType currentPag
 
     factory = factory.withOriginalSearch(
       currentPageType,
+      itineraries.size() > 0 ? itineraries.get(0).startTimeAsInstant() : null,
       earliestDepartureTime,
       latestArrivalTime,
       searchWindowUsed

application/src/test/java/org/opentripplanner/model/plan/paging/cursor/PageCursorFactoryTest.java

@@ -27,15 +27,15 @@ class PageCursorFactoryTest implements PlanTestConstants {
   private static final Instant T10_30 = time("10:30");
   private static final Instant T11_01 = time("11:01");
   private static final Instant T12_00 = time("12:00");
-  private static final Instant T12_10 = time("12:10");
+  private static final Instant T12_01 = time("12:01");
   private static final Instant T12_30 = time("12:30");
   private static final Instant T13_00 = time("13:00");
   private static final Instant T13_30 = time("13:30");
 
   @Test
   public void sortArrivalAscending() {
     var factory = new PageCursorFactory(STREET_AND_ARRIVAL_TIME, D90M)
-      .withOriginalSearch(null, T12_00, null, D1H)
+      .withOriginalSearch(null, null, T12_00, null, D1H)
       .withPageCursorInput(new TestPageCursorInput());
 
     var nextPage = factory.nextPageCursor();
@@ -48,7 +48,7 @@ public void sortArrivalAscending() {
   @Test
   public void sortArrivalAscendingCropSearchWindow() {
     var factory = new PageCursorFactory(STREET_AND_ARRIVAL_TIME, D90M)
-      .withOriginalSearch(NEXT_PAGE, T12_00, null, D1H)
+      .withOriginalSearch(NEXT_PAGE, null, T12_00, null, D1H)
       .withPageCursorInput(
         new TestPageCursorInput(
           newItinerary(A).bus(65, timeAsSeconds(T12_30), timeAsSeconds(T13_30), B).build(),
@@ -66,7 +66,7 @@ public void sortArrivalAscendingCropSearchWindow() {
   @Test
   public void sortArrivalAscendingPreviousPage() {
     var factory = new PageCursorFactory(STREET_AND_ARRIVAL_TIME, D90M)
-      .withOriginalSearch(PREVIOUS_PAGE, T12_00, null, D1H)
+      .withOriginalSearch(PREVIOUS_PAGE, null, T12_00, null, D1H)
       .withPageCursorInput(new TestPageCursorInput());
 
     var nextPage = factory.nextPageCursor();
@@ -79,7 +79,7 @@ public void sortArrivalAscendingPreviousPage() {
   @Test
   public void sortArrivalAscendingCropSearchWindowPreviousPage() {
     var factory = new PageCursorFactory(STREET_AND_ARRIVAL_TIME, D90M)
-      .withOriginalSearch(PREVIOUS_PAGE, T12_00, null, D1H)
+      .withOriginalSearch(PREVIOUS_PAGE, null, T12_00, null, D1H)
       .withPageCursorInput(
         new TestPageCursorInput(
           newItinerary(A).bus(65, timeAsSeconds(T12_30), timeAsSeconds(T13_30), B).build(),
@@ -97,11 +97,11 @@ public void sortArrivalAscendingCropSearchWindowPreviousPage() {
   @Test
   public void sortDepartureDescending() {
     var factory = new PageCursorFactory(STREET_AND_DEPARTURE_TIME, D90M)
-      .withOriginalSearch(null, T12_00, T13_30, D1H)
+      .withOriginalSearch(null, null, T12_00, T13_30, D1H)
       .withPageCursorInput(new TestPageCursorInput());
 
     var nextPage = factory.nextPageCursor();
-    assertPageCursor(nextPage, T13_00, null, D90M, NEXT_PAGE, false, false);
+    assertPageCursor(nextPage, T12_01, null, D90M, NEXT_PAGE, false, false);
 
     var prevPage = factory.previousPageCursor();
     assertPageCursor(prevPage, T10_30, T13_30, D90M, PREVIOUS_PAGE, false, false);
@@ -110,7 +110,7 @@ public void sortDepartureDescending() {
   @Test
   public void sortDepartureDescendingCropSearchWindow() {
     var factory = new PageCursorFactory(STREET_AND_DEPARTURE_TIME, D90M)
-      .withOriginalSearch(PREVIOUS_PAGE, T12_00, T13_30, D1H)
+      .withOriginalSearch(PREVIOUS_PAGE, null, T12_00, T13_30, D1H)
       .withPageCursorInput(
         new TestPageCursorInput(
           newItinerary(A).bus(65, timeAsSeconds(T12_30), timeAsSeconds(T13_00), B).build(),
@@ -128,7 +128,7 @@ public void sortDepartureDescendingCropSearchWindow() {
   @Test
   public void sortDepartureDescendingNextPage() {
     var factory = new PageCursorFactory(STREET_AND_DEPARTURE_TIME, D90M)
-      .withOriginalSearch(NEXT_PAGE, T12_00, T13_30, D1H)
+      .withOriginalSearch(NEXT_PAGE, null, T12_00, T13_30, D1H)
       .withPageCursorInput(new TestPageCursorInput());
 
     var nextPage = factory.nextPageCursor();
@@ -141,7 +141,7 @@ public void sortDepartureDescendingNextPage() {
   @Test
   public void sortDepartureDescendingCropSearchWindowNextPage() {
     var factory = new PageCursorFactory(STREET_AND_DEPARTURE_TIME, D90M)
-      .withOriginalSearch(NEXT_PAGE, T12_00, T13_30, D1H)
+      .withOriginalSearch(NEXT_PAGE, null, T12_00, T13_30, D1H)
       .withPageCursorInput(
         new TestPageCursorInput(
           newItinerary(A).bus(65, timeAsSeconds(T12_30), timeAsSeconds(T13_00), B).build(),
@@ -159,7 +159,7 @@ public void sortDepartureDescendingCropSearchWindowNextPage() {
   @Test
   public void testGeneralizedCostMaxLimit() {
     var factory = new PageCursorFactory(STREET_AND_DEPARTURE_TIME, D90M)
-      .withOriginalSearch(NEXT_PAGE, T12_00, T13_30, D1H)
+      .withOriginalSearch(NEXT_PAGE, null, T12_00, T13_30, D1H)
       .withPageCursorInput(
         new TestPageCursorInput(
           newItinerary(A).bus(65, timeAsSeconds(T12_30), timeAsSeconds(T13_00), B).build(),

application/src/test/java/org/opentripplanner/service/paging/PS1_LegacyMetaDataTest.java

@@ -51,16 +51,10 @@ void testCreateTripSearchMetadataDepartAfterNormalSearchWindow() {
     var subject = testDriver.pagingService();
 
     assertEquals(D30m, subject.createTripSearchMetadata().searchWindowUsed);
-    assertEquals(
-      "08:15",
-      TestPagingUtils.cleanStr(subject.createTripSearchMetadata().prevDateTime)
-    );
+    assertEquals("8:15", TestPagingUtils.cleanStr(subject.createTripSearchMetadata().prevDateTime));
     // 12:11 will drop results, the solution is to use the complete sort-vector.
     // The cursor implementation does that
-    assertEquals(
-      "09:15",
-      TestPagingUtils.cleanStr(subject.createTripSearchMetadata().nextDateTime)
-    );
+    assertEquals("9:15", TestPagingUtils.cleanStr(subject.createTripSearchMetadata().nextDateTime));
   }
 
   @Test
@@ -89,13 +83,7 @@ void testCreateTripSearchMetadataArriveByWithNormalSearchWindow() {
     var subject = testDriver.pagingService();
 
     assertEquals(D30m, subject.createTripSearchMetadata().searchWindowUsed);
-    assertEquals(
-      "08:45",
-      TestPagingUtils.cleanStr(subject.createTripSearchMetadata().prevDateTime)
-    );
-    assertEquals(
-      "09:45",
-      TestPagingUtils.cleanStr(subject.createTripSearchMetadata().nextDateTime)
-    );
+    assertEquals("8:45", TestPagingUtils.cleanStr(subject.createTripSearchMetadata().prevDateTime));
+    assertEquals("9:45", TestPagingUtils.cleanStr(subject.createTripSearchMetadata().nextDateTime));
   }
 }