Cleanup itinerary, make it immutable #6535

t2gran · 2025-03-12T10:28:56Z

Summary

Cleanup Itinerary code. This started with an few errors in our logs at entur, see #6534. The error happens in the mapping in the GraphQL code, but the a negative cost should not be allowed on the itinerary. This PR clean up the Itinerary and
make it immutable except the lifecycle systemNotices field.

I have added a few TODOs for further cleanup. Also the builder setters and Itinerary getter for generalizedCost should use the type-safe Cost type - I changed the internal representation from int to Cost, but has not converted the external representation everywhere.

Notable changes

Itinerary used the Cost type internally
The elevation gained/lost is computed in two phases and is fragile to changes in leg
Itinerary is now immutable except for the systemNotices - which is used for life-cycle in filter-chain. This is intended.
All Itinerary field and method names are updated to follow the naming conventions

Review

Esay to review commit by commit, but as a whole works too.
I will fix all minor comments in a follow-up PR to make the review of this go fast - there are a lot of changes and I do not want to have this PR pending for a long time.

Issue

This is related to #6534

Unit tests

✅ I have extracted some code and added unit-test for it, other than that I relay on existing tests.

Documentation

✅ The JavaDoc is updated in a few places.

Changelog

🟥 Not relevant

Bumping the serialization version id

✅ The Cost class is changed

To avoid rounding errors we should use centi-seconds in cost. This is the same resolution used in Raptor.

When legs are modified the totals are recalculated, when this happen we need to add the original elevation edge calculation together with the new totals from elevation profile.

…lty, not int

codecov · 2025-03-12T10:35:21Z

Codecov Report

Attention: Patch coverage is 88.53119% with 57 lines in your changes missing coverage. Please review.

Project coverage is 70.24%. Comparing base (8a7a685) to head (867ff36).
Report is 47 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...iority/TransitGroupPriorityItineraryDecorator.java	7.14%	12 Missing and 1 partial ⚠️
...entripplanner/routing/algorithm/RoutingWorker.java	59.09%	9 Missing ⚠️
...r/routing/algorithm/mapping/ItinerariesHelper.java	11.11%	8 Missing ⚠️
...ssibilityscore/DecorateWithAccessibilityScore.java	82.60%	3 Missing and 1 partial ⚠️
...planner/ext/realtimeresolver/RealtimeResolver.java	78.94%	2 Missing and 2 partials ⚠️
...opconsolidation/DecorateConsolidatedStopNames.java	71.42%	2 Missing and 2 partials ⚠️
...g/opentripplanner/model/plan/ItineraryBuilder.java	94.80%	4 Missing ⚠️
...algorithm/mapping/RaptorPathToItineraryMapper.java	89.28%	1 Missing and 2 partials ⚠️
...rg/opentripplanner/ext/fares/DecorateWithFare.java	66.66%	1 Missing and 1 partial ⚠️
...tripplanner/ext/fares/ItineraryFaresDecorator.java	88.88%	1 Missing and 1 partial ⚠️
... and 4 more

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6535      +/-   ##
=============================================
+ Coverage      70.22%   70.24%   +0.02%     
- Complexity     18331    18367      +36     
=============================================
  Files           2085     2087       +2     
  Lines          77308    77383      +75     
  Branches        7840     7840              
=============================================
+ Hits           54286    54359      +73     
+ Misses         20250    20249       -1     
- Partials        2772     2775       +3

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

…TheWay

leonardehrenfried

I read through the code and even though I could find a few typos I chose to leave them as is and we will clean the up later.

I also ran the IBI smoke test suite on this PR and it passed so I'm pretty confident that no bugs were introduced.

And there is a fix for the tests that you had to disable: leonardehrenfried@61eab43

You may want to cherry-pick (not merge) it. If not we can do it afterwards.

habrahamsson-skanetrafiken

Nice job. This is a bit too big to do a thorough review but I commented on a few minor things that can be addressed later.

habrahamsson-skanetrafiken · 2025-03-13T15:00:16Z

...ication/src/ext-test/java/org/opentripplanner/ext/realtimeresolver/RealtimeResolverTest.java


    assertEquals(1, itineraries.size());
+    itinerary = itineraries.getFirst();


Nit: I think it would be nicer to use separate variables for the original and the modified itineraries

habrahamsson-skanetrafiken · 2025-03-13T15:13:44Z

application/src/main/java/org/opentripplanner/model/plan/Itinerary.java

@@ -428,8 +445,8 @@ public void setElevationGained(Double elevationGained) {
   * <p>
   * -1 indicate that the cost is not set/computed.


This comment should be updated

habrahamsson-skanetrafiken · 2025-03-13T15:17:27Z

application/src/main/java/org/opentripplanner/model/plan/Itinerary.java

-
-  public List<ScheduledTransitLeg> getScheduledTransitLegs() {
-    return getLegs()
+  public List<ScheduledTransitLeg> findScheduledTransitLegs() {


Using "find" here seems strange to me. Shouldn't it rather be listScheduledTransitLegs()?

Agree, this was a mistake.

habrahamsson-skanetrafiken · 2025-03-13T15:25:42Z

application/src/ext/java/org/opentripplanner/ext/ridehailing/DecorateWithRideHailing.java

        .parallelStream()
        .map(leg -> decorateLegWithRideEstimate(i, leg, service))
        .toList();
-
-      i.setLegs(legs);
+      return builder.withLegs(ignore -> legs).build();


Why don't we run the decoration in this lambda?

Because we are updating the existing legs, not setting them. You always start with creating a builder passing in the legs. So, I wanted to make sure the developer setting the legs we careful - not just replacing the new legs, witch may cause information loss.

I will add a comment on it.

habrahamsson-skanetrafiken · 2025-03-13T15:48:57Z

application/src/main/java/org/opentripplanner/model/plan/Itinerary.java

+  /**
+   * The fare products of this itinerary.
+   */
+  public ItineraryFares fare() {


Shouldn't this be called fares() to be consistent with the type name?

The typename is wrong - it is not a collection of fares it is THE itinerary fare. I can change the typename in a followup PR.

Sometimes 'Information' or 'Info' is added to the class name in cases where the plural is used in this way make it easier to read/talk in natural english. E.g. ItineraryFareInfo

habrahamsson-skanetrafiken · 2025-03-13T16:08:22Z

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

+  }
+
+  static RoutingResult empty() {
+    return new RoutingResult(null, null);


I think this class would be simpler to follow if you used Collections.emptyList() instead of null parameters.

t2gran · 2025-03-14T10:11:36Z

I read through the code and even though I could find a few typos I chose to leave them as is and we will clean the up later.

@leonardehrenfried Feel free to comment on them, then I can fix them in a follow up PR.

t2gran · 2025-03-14T10:14:40Z

@habrahamsson-skanetrafiken thank you for the feedback, I will make the fixes in a follwup PR as soon as this is merged.

There was a merge conflict so I need the approvals again :-)

Fix NPE in RoutingWorker after #6535 was merged

t2gran added 26 commits March 7, 2025 17:00

refactor: Use centi-seconds in Cost

ddfe74d

To avoid rounding errors we should use centi-seconds in cost. This is the same resolution used in Raptor.

refactor: Use Cost internally in Itinerary

0c13d54

refactor: Encapsulate access/egress penalty in Itinerary

3c2474f

refactor: Use StreetLeg#copyOf, not builder constructor

5a1e45c

refactor: StreetLeg#of(), not create()

59349b7

refactor: Create ItineraryBuilder - initial version

2dac019

refactor: Make Itinerary 'generalizedCost2' immutable.

80414fe

refactor: Add doc and sort fields in Itinerary and builder

dc37598

refactor: Add SystemNotice to ItineraryBuilder

809565c

refactor: Rename RouteResult -> RoutingResult

bedf90d

refactor: Add unit-test to RoutingResult

a95b26f

refactor: make Itinerary elevation gained/lost immutable

c172d68

refactor: Make Itinerary maxSlope & tooSloped immutable

8e04f00

refactor: Support immutable itineraries for decorators in filter-chain

3ef8236

refactor: Rename FaresToItineraryMapper -> ItineraryFaresDecorator

ac5c2e2

refactor: Use ItineraryBuilder for fares

c6d1576

refactor: Fix elevation gained/lost calculation

5a4ffef

When legs are modified the totals are recalculated, when this happen we need to add the original elevation edge calculation together with the new totals from elevation profile.

refactor: Use ItineraryBuilder to apply fares on legs

1fd5cbc

refactor: Use ItineraryBuilder in DecorateConsolidatedStopNames

bd731cd

refactor: Use ItineraryBuilder in DecorateTransitAlert

3603f90

refactor: Use ItineraryBuilder in GraphQLIntegrationTest

29ffd3f

refactor: Use ItineraryBuilder in DecorateWithAccessibilityScore

5a37a36

refactor: Use ItineraryBuilder in DecorateWithRideHailing

c0683b2

refactor: Use ItineraryBuilder in RealtimeResolver

f7a289e

refactor: Remove the last Itinerary mutators

de20a81

refactor: Use Cost type for Itinerary#getGeneralizedCostIncludingPena…

470e402

…lty, not int

t2gran added Technical Debt bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Skip Changelog labels Mar 12, 2025

t2gran added this to the 2.7 (next release) milestone Mar 12, 2025

t2gran requested a review from a team as a code owner March 12, 2025 10:28

t2gran added the Entur Test This is currently being tested at Entur label Mar 12, 2025

t2gran added 2 commits March 12, 2025 11:39

refactor: Remove duplicate methods isWalkingAllTheWay & isOnStreetAll…

f65a0fd

…TheWay

refactor: Cleanup Itinerary method & field names

2a19cf9

t2gran modified the milestones: 2.7, 2.8 (next release) Mar 12, 2025

t2gran changed the title ~~Fix gen cost itinerary~~ Cleanup itinerary, make it immutable Mar 12, 2025

habrahamsson-skanetrafiken requested review from leonardehrenfried and habrahamsson-skanetrafiken March 13, 2025 14:33

leonardehrenfried previously approved these changes Mar 13, 2025

View reviewed changes

refactor: Enable tests in Sandbox

f46d68a

t2gran dismissed leonardehrenfried’s stale review via f46d68a March 14, 2025 09:35

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

195c46c

habrahamsson-skanetrafiken previously approved these changes Mar 14, 2025

View reviewed changes

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

867ff36

t2gran dismissed habrahamsson-skanetrafiken’s stale review via 867ff36 March 14, 2025 09:53

t2gran requested review from leonardehrenfried and habrahamsson-skanetrafiken March 14, 2025 10:12

habrahamsson-skanetrafiken approved these changes Mar 14, 2025

View reviewed changes

leonardehrenfried approved these changes Mar 14, 2025

View reviewed changes

t2gran merged commit 9e67f60 into opentripplanner:dev-2.x Mar 14, 2025
6 checks passed

t2gran deleted the fix_gen_cost_itinerary branch March 14, 2025 12:20

t2gran pushed a commit that referenced this pull request Mar 14, 2025

Bump serialization version id for #6535

52dc65f

t2gran mentioned this pull request Mar 14, 2025

Fix itinerary cost review #6539

Merged

t2gran added a commit to entur/OpenTripPlanner that referenced this pull request Mar 20, 2025

fix: Fix NPE in RoutingWorker after opentripplanner#6535 was merged

e780290

t2gran mentioned this pull request Mar 20, 2025

Fix NPE in RoutingWorker after #6535 was merged #6562

Merged

t2gran added a commit that referenced this pull request Mar 20, 2025

Merge pull request #6562 from entur/fix_via_results

38a90ca

Fix NPE in RoutingWorker after #6535 was merged


		assertEquals(1, itineraries.size());
		itinerary = itineraries.getFirst();

		@@ -428,8 +445,8 @@ public void setElevationGained(Double elevationGained) {
		* <p>
		* -1 indicate that the cost is not set/computed.

Cleanup itinerary, make it immutable #6535

Cleanup itinerary, make it immutable #6535

Uh oh!

Conversation

t2gran commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Notable changes

Review

Issue

Unit tests

Documentation

Changelog

Bumping the serialization version id

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

leonardehrenfried left a comment • 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!

habrahamsson-skanetrafiken 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!

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!

t2gran commented Mar 14, 2025

Uh oh!

t2gran commented Mar 14, 2025

Uh oh!

Uh oh!

Uh oh!

t2gran commented Mar 12, 2025 •

edited

Loading

codecov bot commented Mar 12, 2025 •

edited

Loading

leonardehrenfried left a comment •

edited

Loading