Make ScheduledTransitLeg, FlexibleTransitLeg fully immutable

[leonardehrenfried]

Summary

Right now the ScheduledTransitLeg is mostly immutable apart from a few fields

• distanceMeters due to testing
• alerts because they are added later
• fareProducts they are also added later

Because Aracadis has a few sandbox features that manipulate the legs over the years I had to fix many bugs because of the leg is half immutable and half mutable. I would like to fix all of them at once and therefore I'm making the ScheduledTransitLeg and FlexibleTransitLeg immutable and all fields must now be set via a builder.

There is also the special sandbox leg ConsolidatedStopLeg which also required some special handling but it is now also fully immutable and has a builder.

Unit tests

Lots of tests added and some re-written.

Documentation

Javadoc.

[codecov]

Codecov Report

Attention: Patch coverage is 93.02326% with 9 lines in your changes missing coverage. Please review.

Project coverage is 69.76%. Comparing base (448ad8f) to head (5bf1072).
Report is 13 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...r/ext/fares/impl/CombinedInterlinedTransitLeg.java	0.00%	3 Missing ⚠️
...ntripplanner/ext/fares/FaresToItineraryMapper.java	50.00%	1 Missing and 1 partial ⚠️
...ntripplanner/model/plan/UnknownTransitPathLeg.java	0.00%	2 Missing ⚠️
...terchain/filters/transit/DecorateTransitAlert.java	83.33%	1 Missing ⚠️
...er/routing/algorithm/mapping/AlertToLegMapper.java	92.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6386      +/-   ##
=============================================
+ Coverage      69.72%   69.76%   +0.03%     
- Complexity     18016    18055      +39     
=============================================
  Files           2057     2059       +2     
  Lines          76967    77027      +60     
  Branches        7844     7841       -3     
=============================================
+ Hits           53666    53735      +69     
+ Misses         20550    20544       -6     
+ Partials        2751     2748       -3     

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

[habrahamsson-skanetrafiken]

This is great work. Making these model classes fully immutable makes stuff so much simpler to reason about.

[habrahamsson-skanetrafiken]

I think there is a potential problem with the overrideDistanceMeters thing. If someone were to do this:

// Given an existing leg1 with some geometry
var leg2 = leg1.copy().withAlightStopIndexInPattern(newIndex).build();
// Now leg2 will have another geometry but the same distance as leg1

It's probably not an issue right now but there is a potential for bugs. One solution would be to remove the builder.setOverrideDistanceMeters() and rewrite the test that uses it to generate some actual coordinates.

[leonardehrenfried]

You're right, the override is ugly and dangerous. I will try to get rid of it.

[leonardehrenfried]

There is only a single test GroupByDistanceTest that actually requires the ability to directly set the distance. It will be a bit of work to re-write it. Let's discuss in the meeting.

[t2gran]

I think Legs should be reduced to a DTO - carrying data - not responsible for data consistency. For values calculated e.g. in routing we want them to propagate to the output - not recomputing them even if they are slightly off due to some optimization. It is ok to fallback to some default algorithm to compute unset values. The pattern I used in builders to achieve this is like this (pseudo code), b can be derived from a:

class A {
  a, b : AType;  
}

class ABuilder {
  A original;
  a = NOT_SET, b=NOT_SET;  
  withA(...)
  withB(...)
  computeB() {
    if(b != NOT_SET) return b;
    if(a != NOT_SET) return <compute b based on a>;
    return original.b();
  }
}

computeB() is called in the build() method or As constructor. The trick is to keep a ref to the original so you know if a and b is changed or not.

[leonardehrenfried]

I don't follow everything here. Let's find a way forward in the dev meeting.

[habrahamsson-skanetrafiken]

I think it's a good idea to make the Legs be more of a value object. My suggestion would be to handle the distance calculation in a setter in the builder. That way you will get consistent behavior regardless of whether you copy() an existing leg or create one from scratch. Something like this:

class ScheduletTransitLegBuilder {

  ...

  /**
   * Note: This will override the distance if already set.
   */
  public withPatternAndBoardAlightIndices(Pattern pattern, int boardIndex, int alightIndex) {
    this.pattern = pattern;
    this.boardIndex = boardIndex;
    this.alightIndex = alightIndex;

    var coords = getCoords(pattorn, boardIndex, alightIndex);
    this.legGeometry = createGeometry(coords);
    this.distance = calculateDistance(coords);
  }

  public withOverrideDistanceMeters(double d) {
    this.distance = d;
  }
}

[leonardehrenfried]

We discussed today that we will remove the logic to compute the distance from the leg/builder itself. It needs to be done by the callers.

[habrahamsson-skanetrafiken]

We decided on the developer meeting to accept the issue with the overrideDistance for now and solve it in an upcoming PR.

With that caveat, I'm fine with this PR.

[t2gran]

I think Legs should be reduced to a DTO - carrying data - not responsible for data consistency. For values calculated e.g. in routing we want them to propagate to the output - not recomputing them even if they are slightly off due to some optimization. It is ok to fallback to some default algorithm to compute unset values. The pattern I used in builders to achieve this is like this (pseudo code), b can be derived from a:

class A {
  a, b : AType;  
}

class ABuilder {
  A original;
  a = NOT_SET, b=NOT_SET;  
  withA(...)
  withB(...)
  computeB() {
    if(b != NOT_SET) return b;
    if(a != NOT_SET) return <compute b based on a>;
    return original.b();
  }
}

computeB() is called in the build() method or As constructor. The trick is to keep a ref to the original so you know if a and b is changed or not.