Move PathTransfer to new transfer module #7115

jessicaKoehnke · 2025-12-03T16:39:48Z

Summary

This is a refactoring to enable the runtime modification of transfers. PathTransfers are pulled out of the TransitRepository and moved into their own repository. Constrained transfers will be moved into the same module in a next step.

Issue

It's the first step for the plan detailed in issue #7074

Unit tests

No new features and no new tests

Documentation

Bumping the serialization version id

yes

t2gran · 2025-12-03T16:55:01Z

This is close, but only the root access points (TransferServiceand TransferRepository) should be visible in the org.opentripplanner.transfer pacage. I prefer creating interfaces for these. Look at org.opentripplanner.service.worldenvelope for how it is done.

codecov · 2025-12-08T09:48:00Z

Codecov Report

❌ Patch coverage is 65.97938% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.08%. Comparing base (6d59a52) to head (9342dc7).
⚠️ Report is 49 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...ner/standalone/configure/ConstructApplication.java	12.50%	7 Missing ⚠️
...rg/opentripplanner/ext/flex/FlexTransferIndex.java	77.27%	4 Missing and 1 partial ⚠️
...r/transfer/configure/TransferRepositoryModule.java	0.00%	5 Missing ⚠️
...entripplanner/apis/gtfs/datafetchers/StopImpl.java	0.00%	3 Missing ⚠️
...n/java/org/opentripplanner/standalone/OTPMain.java	0.00%	2 Missing ⚠️
...nner/transfer/configure/TransferServiceModule.java	0.00%	2 Missing ⚠️
...nner/transfer/internal/DefaultTransferService.java	75.00%	2 Missing ⚠️
...entripplanner/transfer/internal/TransferIndex.java	60.00%	2 Missing ⚠️
.../java/org/opentripplanner/ext/flex/FlexRouter.java	66.66%	1 Missing ⚠️
...entripplanner/apis/gtfs/GraphQLRequestContext.java	0.00%	1 Missing ⚠️
... and 3 more

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #7115      +/-   ##
=============================================
+ Coverage      72.05%   72.08%   +0.02%     
- Complexity     20679    20709      +30     
=============================================
  Files           2242     2250       +8     
  Lines          83801    83964     +163     
  Branches        8348     8347       -1     
=============================================
+ Hits           60385    60526     +141     
- Misses         20488    20510      +22     
  Partials        2928     2928

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

application/src/main/java/org/opentripplanner/standalone/configure/ConstructApplication.java

application/src/main/java/org/opentripplanner/transit/service/TimetableRepository.java

application/src/main/java/org/opentripplanner/transfer/internal/DefaultTransferRepository.java

application/src/main/java/org/opentripplanner/transfer/TransferRepository.java

habrahamsson-skanetrafiken · 2025-12-11T12:15:38Z

application/src/main/java/org/opentripplanner/transfer/internal/TransferIndex.java

+    if (OTPFeature.FlexRouting.isOn()) {
+      // Flex transfers should only use WALK mode transfers.
+      for (PathTransfer transfer : transferRepository.findTransfers(StreetMode.WALK)) {
+        transfersToStop.put(transfer.to, transfer);
+        transfersFromStop.put(transfer.from, transfer);
+      }
+    }
+    LOG.info("Transfer repository index init complete.");


If the indices in this class are specific to flex I think that should be indicated in the names of the collections and the accessor methods. Now it looks like these are indices of all transfers to and from a specific stop.

I was a bit hesitant on how to refactor this part. I wanted to make sure that the index is only created when flex is on, so that the memory is conserved when it's off. But you do make a good point. I tried doing it with another abstraction layer now. I also moved the flex index code back to the sandbox package, I do get some Class xxx is not exported from module 'org.opentripplanner.otp' warnings from IntelliJ now though. I'm not a hundred percent sure how the dependencies between the sandbox and the core features should be managed best. Is what I did now in line with what we want to aim for in the future?

Using inheritance instead of an interface might be better for future proving this, I can change it if you think that is a better idea

I'm not sure what the cleanest way would be to handle the flex specific indices and I don't really have a strong opinion. We do something similar for the timetable repository with FlexIndex, maybe you could mimic that? Perhaps @t2gran has an opinion?

I have now changed it to using inheritance. This way any future additions to the normal TransferIndex will be automatically adopted by the FlexTransferIndex. Additionally I put the flex feature toggle condition into the application construction code. The business logic is now free of any sandbox dependencies. I think this is a good goal to aim for, that way we can create an application construction module, a sandbox module and several business logic modules in the future and we'd have no circular dependencies.

The inheritance design looks good for now. It would be nice if we could document why we have this.

I am not sure why an extra index for the Flex is needed. The transit code have a similar index in the Raptor adaptor layer. It might be that the flex index support more stop types - if so this should be documented. Another difference is that the transit uses stopIndex for faster, more compact access. We need to decide what we do with the index in the Raptor adaptor layer as well. So, we can address both of these at the same time. I know there are differences for these indexes, but there is no documentation on "why" - this should be written in the JavaDoc on TransferIndex.

…refactoring # Conflicts: # application/src/ext-test/java/org/opentripplanner/ext/flex/trip/ScheduledDeviatedTripIntegrationTest.java # application/src/ext/java/org/opentripplanner/ext/flex/FlexIndex.java # application/src/test/java/org/opentripplanner/transit/model/_data/TransitTestEnvironmentBuilder.java

… index from normal index

…dex support any future additions to TransferIndex automatically

…not business logic

t2gran · 2025-12-19T14:58:48Z

application/src/main/java/org/opentripplanner/transfer/TransferService.java

+ * stable over the course of a request.
+ */
+public interface TransferService {
+  Collection<PathTransfer> findTransfersByStop(StopLocation stop);


This method needs JavaDoc. It is not clear from the definition if the stop is the from or to stop, or both.

t2gran · 2025-12-23T09:43:11Z

application/src/ext/java/org/opentripplanner/ext/flex/FlexTransferIndex.java

+    if (!indexed) {
+      throw new IllegalStateException("The transfer index is needed but not initialized");
+    }


The if().. can be extracted into a private method and reused below.

t2gran · 2025-12-23T09:54:35Z

application/src/main/java/org/opentripplanner/transfer/internal/TransferIndex.java

+    if (OTPFeature.FlexRouting.isOn()) {
+      // Flex transfers should only use WALK mode transfers.
+      for (PathTransfer transfer : transferRepository.findTransfers(StreetMode.WALK)) {
+        transfersToStop.put(transfer.to, transfer);
+        transfersFromStop.put(transfer.from, transfer);
+      }
+    }
+    LOG.info("Transfer repository index init complete.");


The inheritance design looks good for now. It would be nice if we could document why we have this.

I am not sure why an extra index for the Flex is needed. The transit code have a similar index in the Raptor adaptor layer. It might be that the flex index support more stop types - if so this should be documented. Another difference is that the transit uses stopIndex for faster, more compact access. We need to decide what we do with the index in the Raptor adaptor layer as well. So, we can address both of these at the same time. I know there are differences for these indexes, but there is no documentation on "why" - this should be written in the JavaDoc on TransferIndex.

t2gran · 2025-12-23T10:49:16Z

application/src/main/java/org/opentripplanner/transfer/TransferRepository.java

+  /**
+   * @param toStop {@code StopLocation} that is set as a to-stop
+   * @return all walk mode {@code PathTransfer}s with the specified {@code StopLocation} as a
+   * to-stop
+   * @throws UnsupportedOperationException if flex routing is not activated
+   * @throws IllegalStateException         if the index was not initialized
+   */


Suggested change

/**

* @param toStop {@code StopLocation} that is set as a to-stop

* @return all walk mode {@code PathTransfer}s with the specified {@code StopLocation} as a

* to-stop

* @throws UnsupportedOperationException if flex routing is not activated

* @throws IllegalStateException if the index was not initialized

*/

This is not DRY, do not repeat information that is part of the method signature + return type. In this case I do not think there are any information in the doc at all.

In general we do not document "programming error" exceptions. The pre/post conditions are "runtime" documentation on how to use the methods. If you fail to do the index() the exception document the intention.

By the way, methods used as is by the service should be documented in the service interface, and it is not needed to document them twice - so leaving them without doc in the Repository is fine.

t2gran · 2025-12-23T10:52:08Z

application/src/main/java/org/opentripplanner/transfer/TransferRepository.java

+   * Initialize the index. This needs to be called before
+   * {@link #findWalkTransfersFromStop(StopLocation)} and {@link #findWalkTransfersToStop(StopLocation)}
+   */
+  void index();


Witch methods are backed by the the index is a implementation detail - an abstraction leak.

t2gran · 2025-12-23T11:23:48Z

application/src/main/java/org/opentripplanner/transfer/internal/TransferIndex.java

+import org.opentripplanner.transfer.TransferRepository;
+import org.opentripplanner.transit.model.site.StopLocation;
+
+public class TransferIndex {


I would like to see some doc on this interface:

Why do we needed it -> Flex only user

Why can we not reuse the Raptor Adaptor layer providing similar functionality?

If you find it hard to answer the above questions, you can just add TODOs instead.

jessicaKoehnke 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 Dec 3, 2025

jessicaKoehnke force-pushed the transfer-module-refactoring branch from 0abc62a to e1216dc Compare December 8, 2025 09:21

refactor: extract path transfers to new transfer module

4dc2ef3

jessicaKoehnke force-pushed the transfer-module-refactoring branch from f4e5ee6 to 4dc2ef3 Compare December 8, 2025 10:46

jessicaKoehnke marked this pull request as ready for review December 8, 2025 11:01

jessicaKoehnke requested a review from a team as a code owner December 8, 2025 11:01

optionsome assigned t2gran and habrahamsson-skanetrafiken Dec 9, 2025

optionsome requested a review from habrahamsson-skanetrafiken December 9, 2025 10:13

habrahamsson-skanetrafiken requested changes Dec 11, 2025

View reviewed changes

jessicaKoehnke added 4 commits December 11, 2025 17:39

review: undo using qualified name for DaggerConstructApplicationFactory

7747071

review: rename methods according to OTP naming conventions

f05d741

review: extract TransferIndex as an interface to better separate flex…

e7acdf7

… index from normal index

jessicaKoehnke requested a review from habrahamsson-skanetrafiken December 12, 2025 14:21

jessicaKoehnke added 2 commits December 18, 2025 17:15

refactor: use inheritance instead of interface to make FlexTransferIn…

f591748

…dex support any future additions to TransferIndex automatically

refactor: make only application construction depend on sandbox code, …

9342dc7

…not business logic

t2gran requested changes Dec 23, 2025

View reviewed changes

Move PathTransfer to new transfer module #7115

Are you sure you want to change the base?

Move PathTransfer to new transfer module #7115

Uh oh!

Conversation

jessicaKoehnke commented Dec 3, 2025

Summary

Issue

Unit tests

Documentation

Bumping the serialization version id

Uh oh!

t2gran commented Dec 3, 2025

Uh oh!

codecov bot commented Dec 8, 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!

Uh oh!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Dec 8, 2025 •

edited

Loading