Encapsulate transit model index

[vpaturet]

Summary

This PR encapsulates the TransitModelIndex so that it is accessed only through the TransitService/TransitEditorService interfaces.

This prepares for further refactoring steps where the mutable state that is currently stored in the TransitModelIndex, the TimetableSnapshot and the TransitLayer will be handled in a more consistent and unified way.

This also brings the code closer to the target design (immutable transit model) where all transit model updates are performed through the TransitEditorService.

Note:

• The TransitService service should not provide direct read/write access to the underlying collections in the index. Ideally, all accessors should return an unmodifiable view of these collections. This is currently not always the case. This PR fixes this issue in a couple of accessor methods where it is easy to demonstrate that the access is indeed read-only. More accessors should be fixed in a follow-up PR.

• The content and semantic of TransitLayer, TimetableSnapshot, TransitModelIndex are not explicit in the current design, but it appears that:

  • TransitLayer is used only in Raptor and reflects all real-time updates
  • TimetableSnapshot is used in API calls and reflects all real-time updates
  • TransitModelIndex is used in API calls and reflects scheduled data + initial state of extra journeys
    (i.e: the only updates to the index are additions of new trips, no removal/modification. Subsequent changes in added trips are not applied to the index)

Issue

No

Unit tests

Updated unit tests

Documentation

No

[codecov]

Codecov Report

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

Project coverage is 69.66%. Comparing base (5991d60) to head (fc53d94).
Report is 25 commits behind head on dev-2.x.

Files	Patch %	Lines
...ehicle_position/PollingVehiclePositionUpdater.java	0.00%	6 Missing ⚠️
...oradapter/transit/mappers/TransitLayerUpdater.java	83.33%	0 Missing and 1 partial ⚠️
...ner/standalone/configure/ConstructApplication.java	0.00%	0 Missing and 1 partial ⚠️
.../opentripplanner/transit/service/TransitModel.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5973      +/-   ##
=============================================
+ Coverage      69.64%   69.66%   +0.02%     
- Complexity     17139    17165      +26     
=============================================
  Files           1937     1942       +5     
  Lines          73747    73784      +37     
  Branches        7546     7550       +4     
=============================================
+ Hits           51362    51403      +41     
  Misses         19752    19752              
+ Partials        2633     2629       -4     

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

[leonardehrenfried]

This looks like a good PR. Since it's touching a few tests, would it be possible to wait it until I managed to get #5916 through code review?

In fact this PR has had 2 approvals but because of Henrik going away and merge conflicts, it could not be merged yet. We can also decide that it has passed review even though not all requirements were met in the correct order.

[vpaturet]

I will review your PR, at this stage I guess that is sufficient to get it approved and merged.

[leonardehrenfried]

I think treating the index as an implementation detail is a great idea and will make accessing this information a lot more sane. Nevertheless, I think we want to hear @t2gran's take on it.

[abyrd]

The changes look good to me. Your explanation of the current role of each class, and of how this PR moves toward the target design for TransitModel seems consistent. It's noteworthy that TransitService already had most of the necessary methods, with DefaultTransitService reading through to the TransitModelIndex. In many places this PR is just using this existing publicly visible service, allowing the index to become package-private.

[t2gran]

Overall this looks ready to merge, but a few JavaDoc comments would be nice.