Skip to content

Add cursor-based paging - #3759

Merged
hannesj merged 25 commits into
opentripplanner:dev-2.xfrom
entur:otp2_cusor_based_scrolling
Dec 21, 2021
Merged

Add cursor-based paging#3759
hannesj merged 25 commits into
opentripplanner:dev-2.xfrom
entur:otp2_cusor_based_scrolling

Conversation

@hannesj

@hannesj hannesj commented Nov 29, 2021

Copy link
Copy Markdown
Contributor

Summary

This PR adds cursor-based scrolling. It adds a new class, PageCursor, which encapsulates and encodes the necessary metadata to allow for paging. Currently paging is only supported in one direction, forwards if arriveBy is false and backwards if arriveBy is true. It adds this information to all APIs, both as input and output. Also, a button for fetching the next page is added to the debug client,

Unit tests

Added unit test for paging data.

Code style

Code style followed.

Documentation

No updates. The field documentation was updated for the GraphQL APIs.

@hannesj
hannesj requested a review from a team as a code owner November 29, 2021 18:19
@hannesj
hannesj force-pushed the otp2_cusor_based_scrolling branch from 8840579 to b52a657 Compare November 29, 2021 20:21
@t2gran t2gran added this to the 2.1 milestone Nov 30, 2021
@t2gran t2gran added the !Improvement A functional improvement or micro feature label Nov 30, 2021
@t2gran
t2gran requested review from optionsome and t2gran November 30, 2021 09:22
@hannesj

hannesj commented Nov 30, 2021

Copy link
Copy Markdown
Contributor Author

Need to add bidirectional paging, at least in the API layer, but it might return nulls for now.

@t2gran
t2gran marked this pull request as draft December 3, 2021 09:31
@hannesj
hannesj force-pushed the otp2_cusor_based_scrolling branch from edf6e65 to e6b326a Compare December 9, 2021 13:04
@t2gran
t2gran marked this pull request as ready for review December 9, 2021 15:11
@hannesj hannesj added the Entur Test This is currently being tested at Entur label Dec 14, 2021
Comment thread src/ext/java/org/opentripplanner/ext/transmodelapi/model/plan/TripQuery.java Outdated
Comment thread src/main/java/org/opentripplanner/api/common/RoutingResource.java Outdated
Comment thread src/main/java/org/opentripplanner/api/resource/TripPlannerResponse.java Outdated
Comment thread src/main/java/org/opentripplanner/api/resource/TripPlannerResponse.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/RoutingService.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/algorithm/RoutingWorker.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/algorithm/RoutingWorker.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/algorithm/RoutingWorker.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/algorithm/mapping/PageCursorMapper.java Outdated

@optionsome optionsome left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including nextPageCursor or previousPageCursor fields in the query at least in the legacy graphQL API leads to NPE if no public transportation modes are included. I think in that situation those fields should just return null.

Another note, at least arriveBy=true search can include same itinerary in the previous page as in the current page. I remember some known issue was discussed in a developer meeting but was it this?

Comment thread src/ext/resources/legacygraphqlapi/schema.graphqls Outdated
Comment thread src/ext/resources/legacygraphqlapi/schema.graphqls
Comment thread src/ext/resources/legacygraphqlapi/schema.graphqls
Comment thread src/main/java/org/opentripplanner/routing/api/request/RoutingRequest.java Outdated
hannesj and others added 4 commits December 16, 2021 12:39
Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>
Co-authored-by: Thomas Gran <t2gran@gmail.com>
@hannesj

hannesj commented Dec 16, 2021

Copy link
Copy Markdown
Contributor Author

Including nextPageCursor or previousPageCursor fields in the query at least in the legacy graphQL API leads to NPE if no public transportation modes are included. I think in that situation those fields should just return null.

This has now been fixed in all APIs

@optionsome optionsome left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be ok now apart from the known problems with the itineraries being served in next/previous pages.

@hannesj
hannesj merged commit efb9ea2 into opentripplanner:dev-2.x Dec 21, 2021
@hannesj
hannesj deleted the otp2_cusor_based_scrolling branch December 21, 2021 16:39
t2gran pushed a commit that referenced this pull request Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Entur Test This is currently being tested at Entur !Improvement A functional improvement or micro feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants