Add validation for visit via locations with coordinates when attempting to link them to graph #6704

optionsome · 2025-06-17T20:36:59Z

Summary

Temporary vertex creation is moved to routing worker so we don't do it separately for access/egress and direct searches. TemporaryVerticesContainer is now used for coordinate visit via locations as well.

Functional changes:

Names for unlabeled visit via location coordinate vertices have now translatable default name
visit via location coordinate locations are now validated similarly as origin and destination and if they cannot be connected to street network, a routing error will be returned

Issue

Relates to #6501

Unit tests

Will add new tests and there is one existing test failing (haven't had time yet to figure out why).

Documentation

Not needed

Changelog

Skipped

codecov · 2025-06-19T18:12:08Z

Codecov Report

❌ Patch coverage is 87.86280% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.21%. Comparing base (1ea783b) to head (e2e597b).
⚠️ Report is 220 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...entripplanner/routing/algorithm/RoutingWorker.java	75.51%	9 Missing and 3 partials ⚠️
...a/service/DefaultViaCoordinateTransferFactory.java	0.00%	12 Missing ⚠️
...reet/search/TemporaryVerticesContainerBuilder.java	96.24%	2 Missing and 6 partials ⚠️
...oradapter/transit/mappers/RaptorRequestMapper.java	64.70%	4 Missing and 2 partials ⚠️
...rg/opentripplanner/visualizer/GraphVisualizer.java	0.00%	5 Missing ⚠️
...va/org/opentripplanner/apis/gtfs/GraphQLUtils.java	0.00%	2 Missing ⚠️
...ntripplanner/apis/gtfs/generated/GraphQLTypes.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6704      +/-   ##
=============================================
+ Coverage      72.08%   72.21%   +0.12%     
- Complexity     19512    19597      +85     
=============================================
  Files           2106     2108       +2     
  Lines          78936    79136     +200     
  Branches        8000     8012      +12     
=============================================
+ Hits           56902    57147     +245     
+ Misses         19220    19173      -47     
- Partials        2814     2816       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…sContainer

… points

...va/org/opentripplanner/routing/algorithm/raptoradapter/router/street/DirectStreetRouter.java

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

leonardehrenfried · 2025-07-01T11:27:36Z

application/src/main/java/org/opentripplanner/routing/linking/SameEdgeAdjuster.java


  /**
-   * Utility class. If the from and to vertices are generated and lie along some of the same edges,
+   * Utility class. If the from, to or via vertices are generated and lie along some of the same edges,


This class is only used by TemporarVerticesContainerBuilder. We can move it into its package and make it package-private.

It is used by TestHalfEdges but that's a pretty old and very unfocussed test. I would be ok if you deleted the two test cases that use it.

However, I can also do it in one of my PRs.

I'm a bit hesitant on removing the TestHalfEdges test because there probably isn't a test anywhere else that tests this functionality. I think we should create unit tests for the adjuster itself and remove the TestHalfEdges test, but I think this pr is big enough already so I'm not going to do this refactoring at least in this pr.

leonardehrenfried · 2025-07-01T11:48:45Z

application/src/test/java/org/opentripplanner/street/search/TemporaryVerticesContainerTest.java

+  public void locationNotFoundException() {
+    // Stops not found
+    try (
+      var container = TemporaryVerticesContainer.of(g)


I think assertThrows is more appropriate here.

There are a few more places in this class where this is the case.

assertThrows doesn't call the close() on the AutoCloseable by default, but there is a workaround junit-team/junit-framework#2189 (comment). I'm not sure which solution is prettier.

While thinking about this, I realized there was memory leak when the builder threw an exception. I ended up using assertThrows as suggested.

leonardehrenfried

I think this is a solid refactoring where lots of duplicate instantiation of the temp vertices container is removed. Nevertheless, I have a few requests.

…-refactor

t2gran · 2025-09-25T10:27:28Z

application/src/main/java/org/opentripplanner/street/search/LinkingContextBuilder.java

+ * This class is responsible for linking the RouteRequest origin, destination and visit via
+ * locations that contain coordinates to the Graph used in the A-Star search. This builder also
+ * validates that it was possible to link the locations to the graph. The responsibility of cleaning
+ * up the temporary vertices and edges is on the {@link TemporaryVerticesContainer}.
+ */


This is looks like the responsibilities of the domain class, not the builder. We only document if a builder needs to do something none standard stuff on builders. All domain documentatio should be on the domain class.

t2gran · 2025-10-02T12:01:45Z

application/src/main/java/org/opentripplanner/routing/algorithm/RoutingWorker.java

This class suddenly have to deal with a lot of detailed access/egress mode business logic. This does not belong in this class.

Creating and passing in a TemporaryVertexContainer is ok (there is not many better options), but the container should be clean - no routing/domain logic - pure framework. If we do that then the logic in createTemporaryVerticesContainer can be moved. Not sure where, but to a mapper or pushed down.

…mplementation

…cesContainer into LinkingContext

…rtices management

…xt for temporary vertices management

optionsome · 2025-10-14T16:16:46Z

I'll close this and open another pr from upstream repo with these changes + more.

Refactor how temporary vertices are created for a routing request

ee8bc2d

optionsome added !Technical Debt Improve code quality, no functional changes. +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR +Skip Changelog This is not a relevant change for a product owner since last release. labels Jun 17, 2025

Refactor more and fix missing use of stop vertices

ccbff2f

optionsome added 10 commits June 23, 2025 13:28

Move Exception handling where it belongs after refactoring

60433ff

Refactor how coordinate visit via location vertices/edges are created

6ba2932

Set inputField as VIA for routing errors related to via points

5e1834d

Move FromToViaVertexRequest and create a builder for TemporaryVertice…

445ba25

…sContainer

Adjust edges related to via points as well

b89d493

Remove unused constructor

0403a3a

Only adjust edges between adjacent edges and adjust edges between via…

c83c04c

… points

Filter which modes are used for temporary edges based on request

2ccfc1a

Move test to correct package

ce298b5

Add tests

6bcbf19

optionsome marked this pull request as ready for review July 1, 2025 07:26

optionsome requested a review from a team as a code owner July 1, 2025 07:26

optionsome requested a review from leonardehrenfried July 1, 2025 08:36

optionsome changed the title ~~Refactor how temporary vertices are created for a routing request~~ Add validation for visit via locations with coordinates when attempting to link them to graph Jul 1, 2025

optionsome removed the +Skip Changelog This is not a relevant change for a product owner since last release. label Jul 1, 2025

Add javadoc

beb3499

leonardehrenfried reviewed Jul 1, 2025

View reviewed changes

...va/org/opentripplanner/routing/algorithm/raptoradapter/router/street/DirectStreetRouter.java Show resolved Hide resolved

leonardehrenfried reviewed Jul 1, 2025

View reviewed changes

application/src/main/java/org/opentripplanner/routing/api/request/RouteRequest.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed Jul 1, 2025

View reviewed changes

leonardehrenfried requested changes Jul 1, 2025

View reviewed changes

optionsome added 2 commits July 1, 2025 22:46

Rename via getters to follow naming conventions

06f208e

Fix memory leak when builder throws exception

0c11058

leonardehrenfried previously approved these changes Jul 2, 2025

View reviewed changes

habrahamsson-skanetrafiken mentioned this pull request Aug 18, 2025

Restore lookup of multi-modal stations and group of stations #6805

Merged

optionsome requested a review from leonardehrenfried August 19, 2025 08:37

leonardehrenfried previously approved these changes Aug 21, 2025

View reviewed changes

t2gran modified the milestones: 2.8, 2.9 (next release) Sep 10, 2025

Merge remote-tracking branch 'upstream/dev-2.x' into temporary-vertex…

43fceb9

…-refactor

optionsome dismissed leonardehrenfried’s stale review via 43fceb9 September 22, 2025 11:17

Ensure that stop's coordinate is not using for non-car routing

e2e597b

optionsome requested a review from leonardehrenfried September 22, 2025 11:51

leonardehrenfried previously approved these changes Sep 24, 2025

View reviewed changes

t2gran added the +Skip Changelog This is not a relevant change for a product owner since last release. label Oct 2, 2025

t2gran requested changes Oct 2, 2025

View reviewed changes

t2gran and others added 6 commits October 7, 2025 13:51

temp

33991c7

Refactor LinkingContext to remove temporary edges and AutoCloseable i…

6585c80

…mplementation

Refactor RoutingWorker and LinkingContext to integrate TemporaryVerti…

5d10429

…cesContainer into LinkingContext

Rename builder

0493509

Refactor StreetGraphFinder to utilize LinkingContext for temporary ve…

778a1b2

…rtices management

Add temporary edges to container

3a498ce

optionsome dismissed leonardehrenfried’s stale review via 3a498ce October 7, 2025 12:11

t2gran and others added 8 commits October 7, 2025 14:29

Refactor GraphVisualizer to integrate LinkingContext for temporary ve…

3055e61

…rtices management

Refactor ScheduledDeviatedTripIntegrationTest to utilize LinkingConte…

ed4b8c5

…xt for temporary vertices management

RoutingWorker.java

6fc1974

LinkingContext

ecf7f6a

LinkingContextBuilder

69d19f2

GraphVisualizer

3440194

Fix WalkRoutingTest

e7f5d4b

Update some more tests

815b750

optionsome closed this Oct 14, 2025

optionsome mentioned this pull request Oct 14, 2025

Add validation for visit via locations with coordinates when attempting to link them to graph #6972

Open

Add validation for visit via locations with coordinates when attempting to link them to graph #6704

Add validation for visit via locations with coordinates when attempting to link them to graph #6704

Uh oh!

Conversation

optionsome commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Unit tests

Documentation

Changelog

Uh oh!

codecov bot commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optionsome Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optionsome commented Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

optionsome commented Jun 17, 2025 •

edited

Loading

codecov bot commented Jun 19, 2025 •

edited

Loading

optionsome Jul 2, 2025 •

edited

Loading