From fd236aa219ec145dddfa4938df87760412ec2035 Mon Sep 17 00:00:00 2001 From: Thomas Gran Date: Tue, 12 Dec 2023 18:05:47 +0100 Subject: [PATCH] Missing commits from: Improve paging #5551 (chery-picked all commits including a5caefb4) --- .../framework/collection/ListUtils.java | 4 +- .../text/CharacterEscapeFormatter.java | 8 +-- .../framework/token/Deserializer.java | 4 +- .../framework/token/Serializer.java | 4 +- .../framework/token/Token.java | 8 +-- .../framework/token/TokenFormat.java | 13 ---- .../token/TokenFormatterConfiguration.java | 25 ++++++++ .../framework/tostring/ToStringBuilder.java | 2 +- .../plan/paging/cursor/PageCursorInput.java | 4 +- .../paging/cursor/PageCursorSerializer.java | 2 +- .../ItineraryListFilterChainBuilder.java | 4 +- .../routing/api/request/RouteRequest.java | 4 +- .../api/response/TripSearchMetadata.java | 36 ++++++----- .../service/paging/PagingService.java | 23 ++++---- .../_support/debug/TestDebug.java | 14 ++--- .../text/CharacterEscapeFormatterTest.java | 35 +++++++++-- .../framework/token/TokenTypeTest.java | 6 -- .../model/routing/TripSearchMetadataTest.java | 20 +++---- .../deletionflagger/PagingFilterTest.java | 2 +- .../paging/PS1_LegacyMetaDataTest.java | 59 +++++++++++++++---- .../service/paging/TestDriver.java | 5 ++ 21 files changed, 182 insertions(+), 100 deletions(-) delete mode 100644 src/main/java/org/opentripplanner/framework/token/TokenFormat.java create mode 100644 src/main/java/org/opentripplanner/framework/token/TokenFormatterConfiguration.java diff --git a/src/main/java/org/opentripplanner/framework/collection/ListUtils.java b/src/main/java/org/opentripplanner/framework/collection/ListUtils.java index 204bb1249a0..513c0bcc0d3 100644 --- a/src/main/java/org/opentripplanner/framework/collection/ListUtils.java +++ b/src/main/java/org/opentripplanner/framework/collection/ListUtils.java @@ -15,7 +15,7 @@ public class ListUtils { * null or empty. */ public static T first(List list) { - return list == null || list.isEmpty() ? null : list.get(0); + return list == null || list.isEmpty() ? null : list.getFirst(); } /** @@ -23,7 +23,7 @@ public static T first(List list) { * null or empty. */ public static T last(List list) { - return list == null || list.isEmpty() ? null : list.get(list.size() - 1); + return list == null || list.isEmpty() ? null : list.getLast(); } /** diff --git a/src/main/java/org/opentripplanner/framework/text/CharacterEscapeFormatter.java b/src/main/java/org/opentripplanner/framework/text/CharacterEscapeFormatter.java index c9fc180ee0b..2bd5bf14b71 100644 --- a/src/main/java/org/opentripplanner/framework/text/CharacterEscapeFormatter.java +++ b/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. *

- * 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: *

    *
  • the escape char is '\'
    • @@ -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(); diff --git a/src/main/java/org/opentripplanner/framework/token/Deserializer.java b/src/main/java/org/opentripplanner/framework/token/Deserializer.java index aa730f6d404..c48723d2e69 100644 --- a/src/main/java/org/opentripplanner/framework/token/Deserializer.java +++ b/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 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(); } diff --git a/src/main/java/org/opentripplanner/framework/token/Serializer.java b/src/main/java/org/opentripplanner/framework/token/Serializer.java index 4fa1502c763..2d01a0ebf5e 100644 --- a/src/main/java/org/opentripplanner/framework/token/Serializer.java +++ b/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()); } } diff --git a/src/main/java/org/opentripplanner/framework/token/Token.java b/src/main/java/org/opentripplanner/framework/token/Token.java index b7ed72a61d4..2dc02169271 100644 --- a/src/main/java/org/opentripplanner/framework/token/Token.java +++ b/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. *

    - * 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. *

    - * 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 > Optional getEnum(String fieldName, Class enumClass) { try { diff --git a/src/main/java/org/opentripplanner/framework/token/TokenFormat.java b/src/main/java/org/opentripplanner/framework/token/TokenFormat.java deleted file mode 100644 index 33b8115f66e..00000000000 --- a/src/main/java/org/opentripplanner/framework/token/TokenFormat.java +++ /dev/null @@ -1,13 +0,0 @@ -package org.opentripplanner.framework.token; - -import org.opentripplanner.framework.text.CharacterEscapeFormatter; - -interface TokenFormat { - char FIELD_SEPARATOR = '|'; - char TOKEN_ESCAPE = '\\'; - char TOKEN_SUBSTITUTION = '+'; - - static CharacterEscapeFormatter tokenFormatter() { - return new CharacterEscapeFormatter(TOKEN_ESCAPE, FIELD_SEPARATOR, TOKEN_SUBSTITUTION); - } -} diff --git a/src/main/java/org/opentripplanner/framework/token/TokenFormatterConfiguration.java b/src/main/java/org/opentripplanner/framework/token/TokenFormatterConfiguration.java new file mode 100644 index 00000000000..3945014dad5 --- /dev/null +++ b/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); + } +} diff --git a/src/main/java/org/opentripplanner/framework/tostring/ToStringBuilder.java b/src/main/java/org/opentripplanner/framework/tostring/ToStringBuilder.java index 1b98e37ba53..cb5d26294db 100644 --- a/src/main/java/org/opentripplanner/framework/tostring/ToStringBuilder.java +++ b/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) { diff --git a/src/main/java/org/opentripplanner/model/plan/paging/cursor/PageCursorInput.java b/src/main/java/org/opentripplanner/model/plan/paging/cursor/PageCursorInput.java index 7277d688cc5..a9bef266739 100644 --- a/src/main/java/org/opentripplanner/model/plan/paging/cursor/PageCursorInput.java +++ b/src/main/java/org/opentripplanner/model/plan/paging/cursor/PageCursorInput.java @@ -12,7 +12,7 @@ */ 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. */ @@ -20,7 +20,7 @@ public interface PageCursorInput { 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. diff --git a/src/main/java/org/opentripplanner/model/plan/paging/cursor/PageCursorSerializer.java b/src/main/java/org/opentripplanner/model/plan/paging/cursor/PageCursorSerializer.java index 9eae6af5768..98f64c68062 100644 --- a/src/main/java/org/opentripplanner/model/plan/paging/cursor/PageCursorSerializer.java +++ b/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(); diff --git a/src/main/java/org/opentripplanner/routing/algorithm/filterchain/ItineraryListFilterChainBuilder.java b/src/main/java/org/opentripplanner/routing/algorithm/filterchain/ItineraryListFilterChainBuilder.java index a1f0bd35026..872bad6b4ae 100644 --- a/src/main/java/org/opentripplanner/routing/algorithm/filterchain/ItineraryListFilterChainBuilder.java +++ b/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) { diff --git a/src/main/java/org/opentripplanner/routing/api/request/RouteRequest.java b/src/main/java/org/opentripplanner/routing/api/request/RouteRequest.java index ea9f53a6505..ce2fdb31c44 100644 --- a/src/main/java/org/opentripplanner/routing/api/request/RouteRequest.java +++ b/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(); diff --git a/src/main/java/org/opentripplanner/routing/api/response/TripSearchMetadata.java b/src/main/java/org/opentripplanner/routing/api/response/TripSearchMetadata.java index 76e0417774b..a5eb5842f23 100644 --- a/src/main/java/org/opentripplanner/routing/api/response/TripSearchMetadata.java +++ b/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 diff --git a/src/main/java/org/opentripplanner/service/paging/PagingService.java b/src/main/java/org/opentripplanner/service/paging/PagingService.java index 458b12251dd..1f035cadafc 100644 --- a/src/main/java/org/opentripplanner/service/paging/PagingService.java +++ b/src/main/java/org/opentripplanner/service/paging/PagingService.java @@ -46,7 +46,11 @@ public PagingService( List 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) { diff --git a/src/test/java/org/opentripplanner/_support/debug/TestDebug.java b/src/test/java/org/opentripplanner/_support/debug/TestDebug.java index ecba6244f1d..df08e373ffa 100644 --- a/src/test/java/org/opentripplanner/_support/debug/TestDebug.java +++ b/src/test/java/org/opentripplanner/_support/debug/TestDebug.java @@ -12,39 +12,39 @@ * or * $ java -DtestDebug .. * - * In IntelliJ I recommend adding the system property in the JUnit template. In the test + * In IntelliJ it's recommended to add the system property in the JUnit template. In the test * drop down, choose Edit Configuration... then Edit Configuration Templates.. and * choose JUnit. */ 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); } } diff --git a/src/test/java/org/opentripplanner/framework/text/CharacterEscapeFormatterTest.java b/src/test/java/org/opentripplanner/framework/text/CharacterEscapeFormatterTest.java index 235b76badd9..b33b06babb6 100644 --- a/src/test/java/org/opentripplanner/framework/text/CharacterEscapeFormatterTest.java +++ b/src/test/java/org/opentripplanner/framework/text/CharacterEscapeFormatterTest.java @@ -2,19 +2,44 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; -import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; class CharacterEscapeFormatterTest { - @Test - public void encodeDecode() { - var subject = new CharacterEscapeFormatter('\\', ';', '|'); + @ParameterizedTest + @ValueSource( + strings = { + "This;is;a:text", + " ; ", + " ;", + "; ", + ";;;", + "^", + " ^ ", + " ^", + "^ ", + ";^;", + "^;", + ";^", + "%^%", + "^%", + "%^", + ";^;", + "^;", + ";^", + } + ) + public void encodeDecode(String original) { + var subject = new CharacterEscapeFormatter('^', ';', '%'); - var original = "This;is;a:text: ;\\;, \\; ;\\ |\\|, \\| |\\"; var escapedText = subject.encode(original); var result = subject.decode(escapedText); + assertNotEquals(escapedText, original); + assertEquals(original, result); assertFalse(escapedText.contains(";"), escapedText); } diff --git a/src/test/java/org/opentripplanner/framework/token/TokenTypeTest.java b/src/test/java/org/opentripplanner/framework/token/TokenTypeTest.java index 1dc83414b7e..e4ba897928b 100644 --- a/src/test/java/org/opentripplanner/framework/token/TokenTypeTest.java +++ b/src/test/java/org/opentripplanner/framework/token/TokenTypeTest.java @@ -52,10 +52,4 @@ void stringToValue() { assertNull(TokenType.STRING.stringToValue("")); assertNull(TokenType.TIME_INSTANT.stringToValue("")); } - - @Test - void values() {} - - @Test - void valueOf() {} } diff --git a/src/test/java/org/opentripplanner/model/routing/TripSearchMetadataTest.java b/src/test/java/org/opentripplanner/model/routing/TripSearchMetadataTest.java index 6897c896e37..fce79cd2d4c 100644 --- a/src/test/java/org/opentripplanner/model/routing/TripSearchMetadataTest.java +++ b/src/test/java/org/opentripplanner/model/routing/TripSearchMetadataTest.java @@ -27,25 +27,25 @@ void createMetadataForArriveWithSearchWindowOnly() { void createMetadataForArriveByWithTimeGiven() { TripSearchMetadata subject; - // New arrival-time with seconds, 10:05:15, should be rounded up to 10:06:00 + // New arrival-time with seconds, 10:35:01, should be rounded up to 10:36:00 subject = TripSearchMetadata.createForArriveBy( Instant.parse("2020-05-17T10:20:00Z"), SEARCH_WINDOW_USED, - Instant.parse("2020-05-17T10:05:15Z") + Instant.parse("2020-05-17T10:35:00Z") ); assertEquals(SEARCH_WINDOW_USED, subject.searchWindowUsed); - assertEquals("2020-05-17T10:06:00Z", subject.prevDateTime.toString()); + assertEquals("2020-05-17T10:04:00Z", subject.prevDateTime.toString()); assertEquals("2020-05-17T10:50:00Z", subject.nextDateTime.toString()); - // New arrival-time without seconds, 10:05:00, should stay the same: 10:05:00 + // New arrival-time without seconds, 10:36:00, should stay the same: 10:36:00 subject = TripSearchMetadata.createForArriveBy( Instant.parse("2020-05-17T11:20:00Z"), SEARCH_WINDOW_USED, - Instant.parse("2020-05-17T11:05:00Z") + Instant.parse("2020-05-17T11:35:59Z") ); - assertEquals("2020-05-17T11:05:00Z", subject.prevDateTime.toString()); + assertEquals("2020-05-17T11:04:00Z", subject.prevDateTime.toString()); assertEquals("2020-05-17T11:50:00Z", subject.nextDateTime.toString()); } @@ -65,23 +65,23 @@ void createMetadataForDepartAfterWithSearchWindowOnly() { void createMetadataForDepartAfterWithTimeGiven() { TripSearchMetadata subject; - // New departure-time with seconds, 10:35:15, should be rounded down to 10:35:00 + // New departure-time, 10:35:00, should be rounded up to 10:36:00 subject = TripSearchMetadata.createForDepartAfter( Instant.parse("2020-05-17T10:20:00Z"), SEARCH_WINDOW_USED, - Instant.parse("2020-05-17T10:35:15Z") + Instant.parse("2020-05-17T10:35:00Z") ); assertEquals(SEARCH_WINDOW_USED, subject.searchWindowUsed); assertEquals("2020-05-17T09:50:00Z", subject.prevDateTime.toString()); assertEquals("2020-05-17T10:36:00Z", subject.nextDateTime.toString()); - // New departure-time without seconds, 11:35:00, should stay the same: 11:35:00 + // New departure-time, 11:35:59, should be rounded up to 11:36:00 subject = TripSearchMetadata.createForDepartAfter( Instant.parse("2020-05-17T11:20:00Z"), SEARCH_WINDOW_USED, - Instant.parse("2020-05-17T11:35:00Z") + Instant.parse("2020-05-17T11:35:59Z") ); assertEquals("2020-05-17T10:50:00Z", subject.prevDateTime.toString()); assertEquals("2020-05-17T11:36:00Z", subject.nextDateTime.toString()); diff --git a/src/test/java/org/opentripplanner/routing/algorithm/filterchain/deletionflagger/PagingFilterTest.java b/src/test/java/org/opentripplanner/routing/algorithm/filterchain/deletionflagger/PagingFilterTest.java index fdb56751815..965889bdefc 100644 --- a/src/test/java/org/opentripplanner/routing/algorithm/filterchain/deletionflagger/PagingFilterTest.java +++ b/src/test/java/org/opentripplanner/routing/algorithm/filterchain/deletionflagger/PagingFilterTest.java @@ -58,7 +58,7 @@ public class PagingFilterTest implements PlanTestConstants { *

  • cost: 5 or 7
    • *
  • mode: car or transit
    • * - * There is 8 car itineraries and 16 transit (car do not have transfers) = 24 itineraries + * There are 8 car itineraries and 16 transit (car do not have transfers) = 24 itineraries */ private final List allItineraries = allPossibleSortingCombinationsOfItineraries(); diff --git a/src/test/java/org/opentripplanner/service/paging/PS1_LegacyMetaDataTest.java b/src/test/java/org/opentripplanner/service/paging/PS1_LegacyMetaDataTest.java index 896f9338f05..609607d84ff 100644 --- a/src/test/java/org/opentripplanner/service/paging/PS1_LegacyMetaDataTest.java +++ b/src/test/java/org/opentripplanner/service/paging/PS1_LegacyMetaDataTest.java @@ -4,30 +4,29 @@ import static org.opentripplanner.service.paging.TestPagingModel.D30m; import java.time.Duration; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.opentripplanner.framework.time.TimeUtils; /** - * This test the entire paging service module. + * This tests the entire paging service module. *

    - * To debug this test, set the + * To debug this test, set either the system property 'testDebug' or the environment variable 'testDebug' to 'true'. */ @SuppressWarnings("DataFlowIssue") class PS1_LegacyMetaDataTest { + private static final int T08_45 = TimeUtils.time("08:45"); + private static final int T09_15 = TimeUtils.time("09:15"); + private static final int T12_00 = TimeUtils.time("12:00"); - private static final int T12_30 = TimeUtils.time("12:30"); private static final int T13_00 = TimeUtils.time("13:00"); - private static final int T13_30 = TimeUtils.time("13:30"); private static final Duration SEARCH_WINDOW_USED = D30m; - private final TestPagingModel model = TestPagingModel.testDataWithManyItinerariesCaseA(); - @Test @SuppressWarnings("deprecation") - void testCreateTripSearchMetadataDepartAfter() { + void testCreateTripSearchMetadataDepartAfterWithPageCut() { + var model = TestPagingModel.testDataWithManyItinerariesCaseA(); var testDriver = model.departAfterDriver(T12_00, SEARCH_WINDOW_USED, 3); var subject = testDriver.pagingService(); @@ -46,14 +45,34 @@ void testCreateTripSearchMetadataDepartAfter() { @Test @SuppressWarnings("deprecation") - @Disabled("FIX: This test should not fail!") - void testCreateTripSearchMetadataArriveBy() { + void testCreateTripSearchMetadataDepartAfterNormalSearchWindow() { + var model = TestPagingModel.testDataWithFewItinerariesCaseB(); + var testDriver = model.departAfterDriver(T08_45, SEARCH_WINDOW_USED, 3); + var subject = testDriver.pagingService(); + + assertEquals(D30m, subject.createTripSearchMetadata().searchWindowUsed); + assertEquals( + "08: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) + ); + } + + @Test + @SuppressWarnings("deprecation") + void testCreateTripSearchMetadataArriveByWithPageCut() { + var model = TestPagingModel.testDataWithManyItinerariesCaseA(); var testDriver = model.arriveByDriver(T12_00, T13_00, SEARCH_WINDOW_USED, 3); var subject = testDriver.pagingService(); assertEquals(D30m, subject.createTripSearchMetadata().searchWindowUsed); assertEquals( - "11:40", + "11:39", TestPagingUtils.cleanStr(subject.createTripSearchMetadata().prevDateTime) ); assertEquals( @@ -61,4 +80,22 @@ void testCreateTripSearchMetadataArriveBy() { TestPagingUtils.cleanStr(subject.createTripSearchMetadata().nextDateTime) ); } + + @Test + @SuppressWarnings("deprecation") + void testCreateTripSearchMetadataArriveByWithNormalSearchWindow() { + var model = TestPagingModel.testDataWithFewItinerariesCaseB(); + var testDriver = model.arriveByDriver(T09_15, T12_00, SEARCH_WINDOW_USED, 3); + 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) + ); + } } diff --git a/src/test/java/org/opentripplanner/service/paging/TestDriver.java b/src/test/java/org/opentripplanner/service/paging/TestDriver.java index 4bed648c613..0b40b140139 100644 --- a/src/test/java/org/opentripplanner/service/paging/TestDriver.java +++ b/src/test/java/org/opentripplanner/service/paging/TestDriver.java @@ -151,6 +151,11 @@ private static TestDriver createNewDriver( var swFilter = new OutsideSearchWindowFilter(edt, searchWindow); kept = swFilter.removeMatchesForTest(kept); + // Simulate Raptor - apply LAT filtering done by raptor + if (lat != null) { + kept = kept.stream().filter(it -> !lat.isBefore(it.endTime().toInstant())).toList(); + } + //Page filter if (pageCut != null) { var pageFilter = new PagingFilter(sortOrder, cropItineraries.invert(), pageCut);