Skip to content

Bugs related to DST and service date - #5004

Merged
t2gran merged 2 commits into
opentripplanner:dev-2.xfrom
entur:otp2_fix_service_date
Apr 11, 2023
Merged

t2gran merged 2 commits into
opentripplanner:dev-2.xfrom
entur:otp2_fix_service_date

Conversation

@t2gran

@t2gran t2gran commented Mar 28, 2023 •

Copy link
Copy Markdown
Member

Summary

This PR fixes 2 errors related to DST adjustments.

Fix empty arriveBy search

When selecting trip-pattern-for-day otp used ended up using the previous day for arrive-by search. This again cased OTP to search an empty set of patterns.

fix SIRI alerts operatingDay resolving

In some legacy cases the operatingDay is passed into OTP with time and zoneID. To get
the correct operating day we simply ignore time and zone-id.  The existing code was
wrong for days with DST forward adjustments. This only apply to the SIRI updaters.

Issue

No issue for this.

Unit tests

Unit test for the ServiceDateUtils is updated, but no module/integration test is added. The plan is to delete this code, and we can not reuse the tests.

Documentation

No.

It turns out that the error was in the trip-pattern-for-day extract,
selecting the wrong range of days.
@t2gran t2gran added the !Bug Apply to issues describing a bug and PRs witch fixes it. label Mar 28, 2023
@t2gran t2gran added this to the 2.3 milestone Mar 28, 2023
@t2gran
t2gran requested a review from a team as a code owner March 28, 2023 13:04
Comment on lines -46 to +47
this.departureDate = transitSearchTimeZero.toLocalDate();
this.departureDate = ServiceDateUtils.asServiceDay(transitSearchTimeZero);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a bit complex, since we already know the date for the trip search one frame up in the call chain. Would it make sense to pass it separately, rather than compute it again from a computed value?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or rather we could pass in the date here, and compute the transitSearchTimeZero from the date and transitLayer timezone

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is a bugfix and I do not want to take any risk. So, I do not want to include a refactoring. I plan to clean-up this as part of the #4002.

@t2gran t2gran changed the title Otp2 fix service date Bugs related to DST and service date Mar 30, 2023
@t2gran
t2gran marked this pull request as draft March 30, 2023 22:34
@codecov

codecov Bot commented Mar 30, 2023 •

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.09 🎉

Comparison is base (1b1f3c0) 64.07% compared to head (45b2ee7) 64.16%.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5004      +/-   ##
=============================================
+ Coverage      64.07%   64.16%   +0.09%     
- Complexity     13588    13724     +136     
=============================================
  Files           1675     1687      +12     
  Lines          66261    66738     +477     
  Branches        7149     7187      +38     
=============================================
+ Hits           42456    42823     +367     
- Misses         21429    21516      +87     
- Partials        2376     2399      +23     
Impacted Files Coverage Δ
...a/org/opentripplanner/ext/siri/EntityResolver.java 15.92% <100.00%> (-2.18%) ⬇️
...entripplanner/framework/time/ServiceDateUtils.java 91.42% <100.00%> (ø)
...equest/RaptorRoutingRequestTransitDataCreator.java 90.36% <100.00%> (ø)

... and 113 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

In some legacy cases the operatingDay is passed into OTP with time and zoneID. To get
the correct operating day we simply ignore time and zone-id.  The existing code was
wrong for days with DST forward adjustments.
@t2gran
t2gran force-pushed the otp2_fix_service_date branch from d60c9b9 to 45b2ee7 Compare March 31, 2023 08:51
@t2gran
t2gran marked this pull request as ready for review March 31, 2023 08:51
@vpaturet

vpaturet commented Apr 3, 2023

Copy link
Copy Markdown
Contributor

@t2gran Not completely obvious from reading the PR description, but as I understand:

  • the start of service date is defined according to the GTFS convention "noon-12hours"
  • on a normal day, it is 00:00 local time
  • when transitioning from standard time to DST it is 23:00 local time, the day before.
  • when transitioning from DST to standard time it is 01:00 local time, the same day.
  • thus to ensure you get the right LocalDate for a given start of service date, you need first to shift the start of service date by 12 hours and then extract the LocalDate.

Correct?

@t2gran

t2gran commented Apr 7, 2023 •

Copy link
Copy Markdown
Member Author

@vpaturet, your summary of ServiceDay/OperatingDay is correct.

thus to ensure you get the right LocalDate for a given start of service date, you need first to shift the start of service date by 12 hours and then extract the LocalDate.

To be clear, the LocalDate is defined by the LocalDate at noon, not when the service day starts(midnight). In most cases this is the same day, but not "when transitioning from standard time to DST it is 23:00 local time".

@t2gran
t2gran merged commit 796c893 into opentripplanner:dev-2.x Apr 11, 2023
@t2gran
t2gran deleted the otp2_fix_service_date branch April 11, 2023 15:20
t2gran pushed a commit that referenced this pull request Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Bug Apply to issues describing a bug and PRs witch fixes it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants