Skip to content

Conversation

@leonardehrenfried
Copy link
Member

@leonardehrenfried leonardehrenfried commented Jan 26, 2026

Summary

The GTFS-RT adapter has accumulated tech debt some of which this PR reduces. In particular it:

  • moves the validation of the trip id and start/service date into a single place. If the TripUpdate is contructed it is validated and calls to tripId() and serviceDate() are safe and non-null.
  • because trip updates are now validated, it's no longer necessary to pass along the id and service date with all method calls
  • it flattens the deep nesting in some of the methods.
  • timeZone and deduplicator, which never change, are passed to the contructor of TripTimesUpdater

Unit tests

One added.

Documentation

Javadoc.

Changelog

Skip.

Bumping the serialization version id

No.

@leonardehrenfried leonardehrenfried requested a review from a team as a code owner January 26, 2026 10:20
@leonardehrenfried leonardehrenfried added !Technical Debt Improve code quality, no functional changes. +Real-Time The issue/PR is related to RealTime updates +Skip Changelog This is not a relevant change for a product owner since last release. labels Jan 26, 2026
@codecov
Copy link

codecov bot commented Jan 26, 2026

Codecov Report

❌ Patch coverage is 88.35979% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.16%. Comparing base (593abac) to head (5992956).
⚠️ Report is 23 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...dater/trip/gtfs/GtfsRealTimeTripUpdateAdapter.java 79.74% 11 Missing and 5 partials ⚠️
...ripplanner/updater/trip/gtfs/model/TripUpdate.java 91.30% 4 Missing ⚠️
...ripplanner/updater/trip/gtfs/TripTimesUpdater.java 90.00% 2 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #7238      +/-   ##
=============================================
+ Coverage      72.13%   72.16%   +0.02%     
+ Complexity     21087    21085       -2     
=============================================
  Files           2297     2298       +1     
  Lines          85233    85184      -49     
  Branches        8489     8482       -7     
=============================================
- Hits           61484    61474      -10     
+ Misses         20770    20736      -34     
+ Partials        2979     2974       -5     

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

@leonardehrenfried leonardehrenfried force-pushed the gtfs-rt-cleanup branch 2 times, most recently from 9343c38 to 0bb8ffa Compare January 26, 2026 14:21
@optionsome optionsome self-assigned this Jan 27, 2026
@optionsome optionsome self-requested a review January 27, 2026 09:35
@optionsome optionsome requested a review from vpaturet January 27, 2026 09:36
@leonardehrenfried leonardehrenfried force-pushed the gtfs-rt-cleanup branch 2 times, most recently from 9fe021d to 0a89aaf Compare January 29, 2026 11:57
Copy link
Member

@optionsome optionsome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes are a bit hard to read, but I think I'll test locally a bit when this is otherwise ready.

Comment on lines 170 to 176
if (failuresByRelationship.containsKey(scheduleRelationship)) {
var c = failuresByRelationship.get(scheduleRelationship);
failuresByRelationship.put(scheduleRelationship, ++c);
} else {
failuresByRelationship.put(scheduleRelationship, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic could be "simplified" to be failuresByRelationship.merge(scheduleRelationship, 1, (a, b) -> a + b), but I don't know if it's more readable or not :D.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe failuresByRelationship.merge(scheduleRelationship, 1, Integer::sum) is even simpler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was the one that originally added this logging but I actually don't think it's useful anymore. I can remove it altogether if you allow me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine with removing it. We still have the logging based on error types, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And thanks for this tip - I didn't know about merge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will remove. Yes, we have logging based on the error type.

@leonardehrenfried
Copy link
Member Author

I also moved the validation of the stop sequence to the validate() method of TripUpdate.

@leonardehrenfried leonardehrenfried added this pull request to the merge queue Feb 2, 2026
Merged via the queue into opentripplanner:dev-2.x with commit 8a88111 Feb 2, 2026
8 checks passed
@leonardehrenfried leonardehrenfried deleted the gtfs-rt-cleanup branch February 2, 2026 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

+Real-Time The issue/PR is related to RealTime updates +Skip Changelog This is not a relevant change for a product owner since last release. !Technical Debt Improve code quality, no functional changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants