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 94.26230% with 7 lines in your changes missing coverage. Please review.

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

Files with missing lines	Patch %	Lines
...er/routing/algorithm/mapping/AlertToLegMapper.java	77.77%	2 Missing and 2 partials ⚠️
...ntripplanner/ext/fares/FaresToItineraryMapper.java	60.00%	1 Missing and 1 partial ⚠️
...terchain/filters/transit/DecorateTransitAlert.java	83.33%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6386      +/-   ##
=============================================
+ Coverage      69.72%   69.75%   +0.03%     
- Complexity     18016    18051      +35     
=============================================
  Files           2057     2059       +2     
  Lines          76967    77024      +57     
  Branches        7844     7842       -2     
=============================================
+ Hits           53666    53731      +65     
+ Misses         20550    20543       -7     
+ Partials        2751     2750       -1     

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

[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]

Why is this an AtomicBoolean ? Consider changing it to a 'boolean'.

[leonardehrenfried]

It needs to be an atomic boolean because you cannot use a primitive value in a lambda.

[t2gran]

This and the if (!leg.isTransitLeg()) is fragile. Another way to solve it is to use an interface:

interface AlertsAware<T extends AlertsAware<T>> { <access alert methods>; T decorateWithAlerts(alerts); }

The if would then be if(!(leg instanceof AlertsAware legWithAlerts)) ... and the implementation of decorateWithAlerts will copy, add and build. The nice thing about using interfaces is that any class can implement it, and having a list of Alerts is no longer tied to the Leg type hierarchy.Later we can add TaxiLeg extends CarLeg implements WithAlerts.

The minimum change is to throw an exception in the default case.

[t2gran]

An alternative here is to not pass in Leg at all, just the fields that the mapper need and return a list of Allerts. Then there is not need to create the leg, and rebuild it with alerts as the next step.

[t2gran]

There is a logical fragile dependency on the values for date here. DateTimes.ZONED_DATE_TIME_1 should only be used in cases where you do not care if what the date is - if someone changes the date in DateTimes your test should stil run green - if not define the testdata inside the test.

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

[t2gran]

Same comment as for the Alerts, maybe use interface ProductAware