Improve paging - avoid duplicates and missed itineraries when paging #5551

t2gran · 2023-12-06T13:44:55Z

Summary

There was a few smaller issues with the paging. This PR clean up the code and fixes a few bugs. It also improve the token serialization framework. Ideally this PR could be split in at least 3 PRs, but that would lead to problems with the forward and backward compatibility.

NOTE! This PR breaks the forward and backward compatibility for paging. It would be possible to stay backwards compatible, but the cost is probably higher than the gain. Hence; we advice to roll out this when traffic is low - minimising the impact for end users.

Overview

Add support for boolean and enums in tokens
Make tokens human-readable after base64 decode
Make paging a isolated component with PagingServicethe entry point
- This enable us to write a clean module test on the paging testing all paging logic in isolation.
- There is also almost 100% branch coverage on each unit.
Use the last kept itinerary as page-cut, not first removed
Make search-window [inclusive, exclusive] - this fixes a bug
The latest-arrival-time should not change for arriveBy search when paging back

Issue

Closes #5040

Unit tests

✅ Unit-test
✅ Module test

Documentation

The JavaDoc is updated, but the paging design doc is not - we will do that in another PR.

Changelog

✅

Bumping the serialization version id

Not requiered.

Removing itineraries outside the search-window is the responsibility of another filter.

…he paging token

� Conflicts: � src/test/java/org/opentripplanner/model/plan/pagecursor/DeduplicationPageCutTest.java

This will allow us to write a proper unit test on the paging logic.

This is used to allow a function can save a result in the box.

…ting.

…tests. Turn on debug using the system property on the cmd line: "-DtestDebug"

…ng into PagingServiceFactory This will enable us to write unit test on the mapping and module test on the paging.

Co-authored-by: Leonard Ehrenfried <[email protected]>

leonardehrenfried · 2023-12-08T07:13:22Z

Can you resolve the conflicts?

jtorin

I've not finished the whole PR yet, will continue in another session.

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

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

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

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

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

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

.../opentripplanner/routing/algorithm/filterchain/deletionflagger/ItineraryDeletionFlagger.java

Co-authored-by: Johan Torin <[email protected]>

jtorin

Second and last batch of review comments.

(It was like being on a quest with Falkor... 😅)

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

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

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

src/test/java/org/opentripplanner/framework/text/CharacterEscapeFormatterTest.java

src/test/java/org/opentripplanner/framework/token/TokenTypeTest.java

...java/org/opentripplanner/routing/algorithm/filterchain/deletionflagger/PagingFilterTest.java

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

The unit-test for CharacterEscapeFormatter is also improved.

Co-authored-by: Johan Torin <[email protected]>

This also address a few comments from the review. More unit-tests are added and a few is cleaned. # Conflicts: # src/test/java/org/opentripplanner/framework/token/TokenTypeTest.java

….java Co-authored-by: Johan Torin <[email protected]>

t2gran · 2023-12-12T16:41:37Z

@jtorin Thank you for a very good review! I think I have fixed all issues now, except one comment I did not understand.

…ed all commits including a5caefb)

- plus a small cleanup in PagingServiceFactory.

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

...opentripplanner/routing/algorithm/filterchain/deletionflagger/OutsideSearchWindowFilter.java

leonardehrenfried

I must admit that this code goes a little over my head but the code quality is solid. I've picked up on a typo but overall this looks good.

…n/deletionflagger/OutsideSearchWindowFilter.java Co-authored-by: Leonard Ehrenfried <[email protected]>

t2gran added 30 commits November 22, 2023 17:26

feature: Add support for enums in tokens

c0fc502

feature: Remove sort-order duplicate from ItineraryPageCut

09f15d9

refactor: Add test that test all combinations for sorting itineraries

50a414d

refactor: Remove search-window pruning from PagingFilter

5229361

Removing itineraries outside the search-window is the responsibility of another filter.

refactor: Move ListSection to framework, so it can be reused

bbe8b22

refactor: Cleanup code: PageType and RouteRequest

e2a1246

refactor: Calculate deduplicationSection instead of including it in t…

f97861c

…he paging token

refactor: Cleanup PageCursor, do not repeat ItinerarySortKey

f08a7ad

refactor: Rename ItineraryPageCut to DeduplicationPageCut

043c512

refactor: Make DeduplicationPageCut package local

f636e66

feature: Add boolean type to token serializer

28e7466

feature: Add CharacterEscapeFormatter to escape a special character.

6461290

feature: Make tokens readable after Base64 decode

180243f

refactor: Use Token framwork to serialize page-cursor

2b50ac7

� Conflicts: � src/test/java/org/opentripplanner/model/plan/pagecursor/DeduplicationPageCutTest.java

refactor: Cleanup NumItinerariesFilterResults

cf7f5ed

refactor: Move PagingSearchWindowAdjuster to paging package

595dc2d

� Conflicts: � src/test/java/org/opentripplanner/model/plan/pagecursor/DeduplicationPageCutTest.java

refactor: Move 'pagecursor' to 'paging.cursor' package

f07a15e

� Conflicts: � src/test/java/org/opentripplanner/model/plan/pagecursor/DeduplicationPageCutTest.java

refactor: Improve naming: withMaxNumberOfItinerariesCropSection(..)

c5afe6c

refactor: Extract paging logic into PagingService

5db6775

This will allow us to write a proper unit test on the paging logic.

refactor: Cleanup PagingFilterTest

e52c370

feature: Add a mutable Box to pass a reference to a method.

1618774

This is used to allow a function can save a result in the box.

refactor: Push logic from request class to PageCursor(feature envy)

bce86f1

feature: Add a utility method to simplify using remove-filters in tes…

23f6b43

…ting.

feature: Add a TestDebug class to simplify "System.out" debugging in …

0fbfbcc

…tests. Turn on debug using the system property on the cmd line: "-DtestDebug"

refactor: Extract Paging functionality into a PagingService and mappi…

a5037ca

…ng into PagingServiceFactory This will enable us to write unit test on the mapping and module test on the paging.

feature: Add a paging module test.

cf6a299

refactor: Remove duplicate code in ItineraryListFilterChainBuilder

28e473f

refactor: Do paging related filtering at the end of the filter process.

9bbee78

feature: Add 'first()' and 'last()' function to ListUtils.

044af91

refactor: SortOrderComparator switch

2109205

leonardehrenfried requested a review from jtorin December 7, 2023 15:03

Apply suggestions from code review

65801fa

Co-authored-by: Leonard Ehrenfried <[email protected]>

Merge remote-tracking branch 'otp/dev-2.x' into serialize_page_token

bdebc41

jtorin suggested changes Dec 8, 2023

View reviewed changes

Apply suggestions from code review

081fc0c

Co-authored-by: Johan Torin <[email protected]>

t2gran requested a review from jtorin December 11, 2023 15:17

jtorin suggested changes Dec 11, 2023

View reviewed changes

t2gran and others added 5 commits December 12, 2023 14:43

review: Rename TokenFormat to TokenFormatterConfiguration

ddac7a7

The unit-test for CharacterEscapeFormatter is also improved.

review: Improve TestDebug implementation

dadbd04

Apply suggestions from code review

2b2ab93

Co-authored-by: Johan Torin <[email protected]>

review: Fix bugs in legacy calculation of prev/next request-time

251a85c

This also address a few comments from the review. More unit-tests are added and a few is cleaned. # Conflicts: # src/test/java/org/opentripplanner/framework/token/TokenTypeTest.java

Update src/main/java/org/opentripplanner/service/paging/PagingService…

a5caefb

….java Co-authored-by: Johan Torin <[email protected]>

t2gran requested a review from jtorin December 12, 2023 16:39

t2gran added a commit to entur/OpenTripPlanner-LegacyHSLFork that referenced this pull request Dec 12, 2023

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

fd236aa

…ed all commits including a5caefb)

t2gran added 2 commits December 13, 2023 13:45

fix: Fix NPE in DefaultRoutingService debug logging.

81cbba3

- plus a small cleanup in PagingServiceFactory.

fix: NPE in PagingService

ad2e35d

t2gran force-pushed the serialize_page_token branch from 74156d0 to ad2e35d Compare December 13, 2023 12:46

jtorin reviewed Dec 13, 2023

View reviewed changes

src/main/java/org/opentripplanner/routing/api/response/TripSearchMetadata.java Show resolved Hide resolved

t2gran requested a review from jtorin December 13, 2023 15:55

leonardehrenfried reviewed Dec 13, 2023

View reviewed changes

...opentripplanner/routing/algorithm/filterchain/deletionflagger/OutsideSearchWindowFilter.java Outdated Show resolved Hide resolved

leonardehrenfried previously approved these changes Dec 13, 2023

View reviewed changes

t2gran dismissed leonardehrenfried’s stale review via e4b1fd0 December 14, 2023 00:49

Update src/main/java/org/opentripplanner/routing/algorithm/filterchai…

e4b1fd0

…n/deletionflagger/OutsideSearchWindowFilter.java Co-authored-by: Leonard Ehrenfried <[email protected]>

leonardehrenfried approved these changes Dec 14, 2023

View reviewed changes

jtorin approved these changes Dec 14, 2023

View reviewed changes

t2gran merged commit e478e32 into opentripplanner:dev-2.x Dec 14, 2023
5 checks passed

t2gran deleted the serialize_page_token branch December 14, 2023 10:11

t2gran pushed a commit that referenced this pull request Dec 14, 2023

Add changelog entry for #5551 [ci skip]

5ab5fb1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve paging - avoid duplicates and missed itineraries when paging #5551

Improve paging - avoid duplicates and missed itineraries when paging #5551

t2gran commented Dec 6, 2023

leonardehrenfried commented Dec 8, 2023

jtorin left a comment

jtorin left a comment

t2gran commented Dec 12, 2023

leonardehrenfried left a comment

Improve paging - avoid duplicates and missed itineraries when paging #5551

Improve paging - avoid duplicates and missed itineraries when paging #5551

Conversation

t2gran commented Dec 6, 2023

Summary

Issue

Unit tests

Documentation

Changelog

Bumping the serialization version id

leonardehrenfried commented Dec 8, 2023

jtorin left a comment

Choose a reason for hiding this comment

jtorin left a comment

Choose a reason for hiding this comment

t2gran commented Dec 12, 2023

leonardehrenfried left a comment

Choose a reason for hiding this comment