Simplify timezone initialization #6965

habrahamsson-skanetrafiken · 2025-10-10T13:31:55Z

Summary

The initialization logic for the timetableRepository is unnecessarily complex. This PR tries to improve on this code and make it less of a tangle. This is not a complete solution to the problem but just one step in the right direction.

Change 1: The main thing tackled in this PR is to make the timeZone initialization logic less messy. The timeZone is now initialized either explicitly or when the first agency is added. If we use the timeZone from an agency we will require all subsequent agencies to have the same timeZone. If there are no agencies we fall back to GMT. In practice this is the same behavior as previously, but the new logic is less fragile and should be much easier to follow.

Change 2: The transitServiceStarts and transitServiceEnds now only resides in the CalendarService instead of being duplicated in the TimetableReposity. Both of these values default to EPOCH if there is no transit data instead of LocalDate.MAX and LocalDate.MIN.

There are also some pure refactorings to the mapping logic from OtpTransitService to TimetableRepository. The commits are clearly marked as refactorings. They should be pretty simple to review commit by commit.

Unit tests

Added testing for netex DatedServiceJourney.

Documentation

Added some relevant javadoc.

Bumping the serialization version id

Yes, this removes some fields in the TimetableRepository.

codecov · 2025-10-10T13:39:14Z

Codecov Report

❌ Patch coverage is 99.02913% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.20%. Comparing base (9966fd4) to head (4b119b3).
⚠️ Report is 13 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...builder/module/TransitWithFutureDateValidator.java	94.11%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6965      +/-   ##
=============================================
+ Coverage      72.18%   72.20%   +0.02%     
- Complexity     19838    19851      +13     
=============================================
  Files           2155     2157       +2     
  Lines          80048    80042       -6     
  Branches        8082     8076       -6     
=============================================
+ Hits           57786    57798      +12     
+ Misses         19415    19403      -12     
+ Partials        2847     2841       -6

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

t2gran · 2025-10-13T16:16:35Z

Have you checked that the time-zone is correctly serialized after the refactor?

Does this take us on step closer to set the time-zone at the first graph-build startup, and keep the value during street-graph-build, transit-graph-build and serving route requests? I want the time-zone to be explicit set either as a cli parameter to OTP or in otp-config.json, abort if there is a conflict. Setting it based on input data should not be an option.

application/src/main/java/org/opentripplanner/transit/service/TimetableRepository.java

habrahamsson-skanetrafiken · 2025-10-14T10:43:41Z

@t2gran The time zone is still stored in the TimetableRepository in the same way as previously so serialization works the same.

This absolutely takes us closer to initializing the timezone at startup. But this PR tries to minimize any change in the current behaviour.

...on/src/main/java/org/opentripplanner/graph_builder/module/AddTransitEntitiesToTimetable.java

leonardehrenfried

This makes a very confusing API a little less confusing - well done.

I tested this with GTFS and a NeTEx feed and in both cases the time zone was set correctly.

I made one comment about naming, but it's private method, so it's not very important.

t2gran · 2025-10-15T19:46:33Z

I like that you prefix the commits, it makes it easier to review each commit. But, please use fix:, feat:, doc:, refactor:, and test: witch is already in use in OTP.

application/src/main/java/org/opentripplanner/apis/gtfs/datafetchers/serviceTimeRangeImpl.java

application/src/main/java/org/opentripplanner/transit/service/TimetableRepository.java

t2gran · 2025-10-20T09:43:42Z

I made an isse for how I want the time-zone #6985

Do you want to merge this PR, and then implement the issue or just do the issue?

t2gran · 2025-10-20T10:07:26Z

application/src/main/java/org/opentripplanner/model/OtpTransitService.java


  Collection<Trip> getAllTrips();

+  Collection<TripOnServiceDate> getTripOnServiceDates();


JAvaDoc: Does this contains syntetic TripOnServiceDate or just NeTEx DatedServiceJourneys?

Well in practice it only contains netex DatedServiceJourneys and is unused by the gtfs importer. But in theory I guess it could be populated by the gtfs importer if there was something that mapped to a TripOnServiceDate in gtfs.

Wouldn't it be inconsistent to talk about DatedServiceJourneys in the context of the OtpTransitService?

As said in the dev meeting, this OtpTransitService is just an intemediate builder for a TimetableRepository, in the long run I think we should get rid of it in favor of a real builder for the TimetableRepository.

t2gran · 2025-10-20T10:14:12Z

application/src/main/java/org/opentripplanner/transit/service/TimetableRepository.java

+    return calendarServiceData
+      .getFirstDate()
+      .map(serviceDate -> ServiceDateUtils.asStartOfService(serviceDate, getTimeZone()).toInstant())
+      .orElse(Instant.EPOCH);


This looks like an error to me, consider throwing an exception

This will be the case when there are no imported services. I don't think it's an error really. It seems like a reasonable default to say that the service period is EPOCH-EPOCH when we don't have transit data. But I could return an optional instead. Do you think that would be preferable?

t2gran · 2025-10-20T10:14:27Z

application/src/main/java/org/opentripplanner/transit/service/TimetableRepository.java

+      .map(serviceDate ->
+        ServiceDateUtils.asStartOfService(serviceDate.plusDays(1), getTimeZone()).toInstant()
+      )
+      .orElse(Instant.EPOCH);


t2gran · 2025-10-20T10:40:13Z

application/src/main/java/org/opentripplanner/transit/service/TimetableRepository.java

+    return calendarServiceData
+      .getLastDate()


I do not understand how this could be correct. The transitServiceEnd and transitServiceEnd are configured, while the calendar start/end reflects the data. What happens if the data period is shorter than the configured?

By the way, the CalendarServiceData is a ghost of the past and something I want to replace.

The configured transitServiceStart and transitServiceEnd is used as a filter in the netex and gtfs importers. But the TimetableRepository doesn't know anything about the configured values. It calculates these values based on the running serivces. This behaviour isn't changed, before this PR it is calculated here:

OpenTripPlanner/application/src/main/java/org/opentripplanner/transit/service/TimetableRepository.java

Line 636 in 1b81110

this.transitServiceStarts = t;

habrahamsson-skanetrafiken · 2025-10-21T07:29:25Z

I can update this to align with the issue you created. So you think I should rename the transitModelTimeZone parameter to timeZone in build-config.json?

habrahamsson-skanetrafiken added 4 commits October 10, 2025 15:17

[Refactor] Separate adding entities to transitservice

cc9a8ee

[Refactor] Separate class for adding to timetable

709ef80

[Refactor] Add netex entities in AddTransitEntitiesToTimetable

330f61c

Simplify timeZone initialization and serviceDate handling

bc46d02

habrahamsson-skanetrafiken added 2 commits October 13, 2025 13:11

[Refactor] Extract netex test zipfile

cb49600

Test for DatedServiceJourney

2626d63

habrahamsson-skanetrafiken marked this pull request as ready for review October 13, 2025 12:26

habrahamsson-skanetrafiken requested a review from a team as a code owner October 13, 2025 12:26

optionsome requested a review from leonardehrenfried October 14, 2025 09:06

leonardehrenfried reviewed Oct 14, 2025

View reviewed changes

application/src/main/java/org/opentripplanner/transit/service/TimetableRepository.java Outdated Show resolved Hide resolved

[Refactor] Fix formatting.

e1ba0f7

habrahamsson-skanetrafiken requested a review from leonardehrenfried October 14, 2025 09:58

leonardehrenfried reviewed Oct 14, 2025

View reviewed changes

...on/src/main/java/org/opentripplanner/graph_builder/module/AddTransitEntitiesToTimetable.java Outdated Show resolved Hide resolved

leonardehrenfried previously approved these changes Oct 14, 2025

View reviewed changes

[Refactor] Rename some methods

8780927

habrahamsson-skanetrafiken dismissed leonardehrenfried’s stale review via 8780927 October 14, 2025 14:37

habrahamsson-skanetrafiken requested a review from leonardehrenfried October 14, 2025 14:37

leonardehrenfried previously approved these changes Oct 14, 2025

View reviewed changes

habrahamsson-skanetrafiken changed the title ~~Simplify timezone~~ Simplify timezone initialization Oct 15, 2025

t2gran reviewed Oct 16, 2025

View reviewed changes

application/src/main/java/org/opentripplanner/apis/gtfs/datafetchers/serviceTimeRangeImpl.java Show resolved Hide resolved

application/src/main/java/org/opentripplanner/transit/service/TimetableRepository.java Show resolved Hide resolved

Merge branch 'dev-2.x' into simplify-timezone

4b119b3

habrahamsson-skanetrafiken dismissed leonardehrenfried’s stale review via 4b119b3 October 17, 2025 08:17

habrahamsson-skanetrafiken requested a review from leonardehrenfried October 17, 2025 08:17

habrahamsson-skanetrafiken requested a review from t2gran October 17, 2025 08:17

t2gran reviewed Oct 20, 2025

View reviewed changes

t2gran requested changes Oct 20, 2025

View reviewed changes


		Collection<Trip> getAllTrips();

		Collection<TripOnServiceDate> getTripOnServiceDates();

Simplify timezone initialization #6965

Are you sure you want to change the base?

Simplify timezone initialization #6965

Conversation

habrahamsson-skanetrafiken commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Unit tests

Documentation

Bumping the serialization version id

Uh oh!

codecov bot commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

t2gran commented Oct 13, 2025

Uh oh!

Uh oh!

habrahamsson-skanetrafiken commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

t2gran commented Oct 15, 2025

Uh oh!

Uh oh!

Uh oh!

t2gran commented Oct 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

habrahamsson-skanetrafiken commented Oct 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

habrahamsson-skanetrafiken commented Oct 10, 2025 •

edited

Loading

codecov bot commented Oct 10, 2025 •

edited

Loading

habrahamsson-skanetrafiken commented Oct 14, 2025 •

edited

Loading