Remove unused Siri code

[leonardehrenfried]

Summary

While I was investigating how to handle cancellations for Siri and GTFS (for later unification) I stumbled over a peculiar piece of complicated logic that appears to be completely unused: keeping the pattern -> stop relationship of realtime-added patterns in SiriTripPatternCache.

It seems that once upon a time the API was directly requesting some information from the cache but the method getAddedTripPatternsForStop is now completely unused.

I believe that it can be removed.

The best way to review this, would be to look at the code yourself and start removing the dead code to see what else can also be removed.

Issue

#4816

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.64%. Comparing base (0a58d0f) to head (bd35ec5).
Report is 1 commits behind head on dev-2.x.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5893      +/-   ##
=============================================
+ Coverage      68.62%   68.64%   +0.02%     
+ Complexity     16824    16821       -3     
=============================================
  Files           1927     1927              
  Lines          72929    72878      -51     
  Branches        7475     7469       -6     
=============================================
- Hits           50044    50028      -16     
+ Misses         20312    20277      -35     
  Partials        2573     2573              

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

[abyrd]

I began review by removing the method getAddedTripPatternsForStop which is clearly unused. With that method removed, the field patternsForStop is solely accessed by SiriTripPatternCache#getOrCreateTripPattern. The same is true for the field updatedTripPatternsForTripCache. Both of these fields are removed by this PR.

The only remaining accesses to either of these fields were in method getOrCreateTripPattern, from lines 167 to 195 (the same large block of code removed in this PR).

The question is then whether the Map operations in this code block existed solely to maintain information whose only escape path was the unused getAddedTripPatternsForStop method, or if they have side effects on the details of shared objects (specifically the TripPattern Map values) which remain visible by other reference chains and methods.

Reviewing that code block, it looks like the only operations performed on the TripPatterns is adding or removing them from the Maps. The rest of the code is just for logging these actions. The long block comment before this code seems to describe its purpose and confirm that it only adds or removes the instance as a whole to indexes.

So it does appear that all this code can be removed without any impact on the rest of the system, which also brings this SiriTripPatternCache closer to the non-Siri/GTFS version.

[abyrd]

Altough git shows me as the last author of the long block comment in the code changed by this PR, this is only due to some recent reformatting. I did not write that comment, so needed to interpret it myself while reviewing. A few words and ideas were unclear to me. I'll summarize in case it helps later improve other documentation.

The first term I didn't understand is "departureRow searches", which are apparently the original purpose of the indexes and methods being removed. An explanation is found at GraphQLDataFetchers.GraphQLDepartureRow, but that comment is still difficult to understand for someone not already familiar with the subject. A departure row is said to be a "combination of a pattern and a stop of that pattern", but it's not clear what it means that they are combined. I suppose this means something like a compound key, representing a particular stop on a particular pattern. The next line says "they are de-duplicated so for each pattern there will only be a single departure row." This probably isn't correct - there must be more than one departure row per pattern. I'd guess there are the same number of departure rows as there are stops on a given pattern. The comment might mean that the departure rows have semantic equality, so a departure row with a given stop ID will only appear once within a given pattern. So maybe: "departure rows are de-duplicated, such that for each pattern there is only be a single departure row per stop on the pattern".

Second, the use of the word "cache" throughout the realtime code always sows some uncertainty when I'm reading it. I am not sure whether there's a true intent or design for these Maps to serve as "caches" for object instances, rather than just indexes or views of information organized differently in other fields. The comments seem to be using the verb "cache" as a synonym for "to record" or "to index", without the conventional emphasis on semantic deduplication and reuse of instances.

[vpaturet]

Same conclusion for me, this code is not in use anymore and does not have any side-effect besides logging.