Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panic when shifting to a DST transition #11

Open
ebegumisa opened this issue Sep 27, 2023 · 5 comments
Open

Panic when shifting to a DST transition #11

ebegumisa opened this issue Sep 27, 2023 · 5 comments
Milestone

Comments

@ebegumisa
Copy link
Contributor

ebegumisa commented Sep 27, 2023

Firstly, thanks for the library.

Problem

I'm in a timezone which regrettably uses daylight saving tIme.

When taking a chrono DST timezone datetime and attempting to apply chronoutils shift utility functions (either directly or indirectly via a relative addition), chronoutils panics if the computed datetime lands on a DST transition.

Examples

    use chrono::{TimeZone, LocalResult};

    #[test] 
    fn test_shift_months_datetime_to_dst_backward_transition() {
        
        let dst_tz = &chrono_tz::Australia::Melbourne;
        
        // On Apr 5th 2020 after 02:59:59, clocks were wound back to 02:00:00 making 02:00::00 to
        // 02:59:59 ambiguous.
        // <https://www.timeanddate.com/time/change/australia/melbourne?year=2020>
        
        base = dst_tz.with_ymd_and_hms(2020, 3, 5, 2, 00, 0).single().unwrap();
        shift_months(base, 1); 
     // ^^^^^^^^^^^^^^^^^^^^^ panics due to shift to DST ambiguous datetime
    }

    #[test]
    fn test_shift_months_datetime_to_dst_forward_transition() {
        
        let dst_tz = &chrono_tz::Australia::Melbourne;

        // On Oct 4th 2020 after 01:59:59, clocks were advanced to 03:00:00 making 02:00:00 to 
        // 02:59:59 non-existent.
        // <https://www.timeanddate.com/time/change/australia/melbourne?year=2020>   
        
        let base = dst_tz.with_ymd_and_hms(2020, 9, 4, 2, 00, 0).single().unwrap();
        shift_months(base, 1); 
     // ^^^^^^^^^^^^^^^^^^^^^ panics due to shift to DST non-existent datetime
    }

Cause

These panics are due to the shift utils unwrapping calls to the chrono::Datelike::with_* functions, with the latter's doc currently stating:

Returns None when:
  • The resulting date does not exist (for example day(31) in April).
  • In case of DateTime<Tz> if the resulting date and time fall within a timezone transition such as from DST to standard time.
  • The value for day is out of range.
pub fn shift_months<D: Datelike>(date: D, months: i32) -> D {

...

   if day <= 28 {
        date.with_day(day)
            .unwrap() 
          // ^^^^^^^^ could panic due to DST
            .with_month(month as u32)
            .unwrap() // <<< Ditto
            .with_year(year)
            .unwrap() // <<< Ditto
    } else {
        date.with_day(1)
            .unwrap() // <<< Ditto
            .with_month(month as u32)
            .unwrap() // <<< Ditto
            .with_year(year)
            .unwrap() // <<< Ditto
            .with_day(day)
            .unwrap() // <<< Ditto
    }
...

Proposed Solution

It would be helpful to have *_opt versions of the shift utils which return an Option rather than a Datelike, for example

pub fn shift_months_opt( .. ) -> Option<Datelike>

This way, the caller can choose how to handle DST issues. A returned None won't help the caller distinguish nonsensical input args from args which result in a DST transition, but callers should be able to determine that for themselves.

@ebegumisa
Copy link
Contributor Author

I am working an a pull request to resolve this issue.

ebegumisa added a commit to ebegumisa/chronoutil that referenced this issue Sep 27, 2023
ebegumisa added a commit to ebegumisa/chronoutil that referenced this issue Sep 27, 2023
• `delta::shift_months`, `delta::shift_years` and `delta::with_month` all now
  have opt counterparts. `delta::day` doesn't need any counterpart as it already
  behaves as olliemath#11 proposes.

• Add a few test assertions for checking `delta::with_month_opt` which, unlike
  the other new functions, isn't already fully covered by the existing tests.
ebegumisa added a commit to ebegumisa/chronoutil that referenced this issue Sep 27, 2023
ebegumisa added a commit to ebegumisa/chronoutil that referenced this issue Sep 27, 2023
@olliemath
Copy link
Owner

olliemath commented Sep 27, 2023

Hey, thanks for the comprehensive issue.

Ah, that's annoying that the fold is not handled by chrono (usually the way you resolve this with datetime types is by having a "fold" attribute which is either 0 or 1, depending on if it's the first or second 2am).

A shift_months_opt sounds like a good start. The larger solution will probably be to emulate the way that chrono handles the addition of regular duration to a datetime around the fold. If you can get shift_months_opt looking good I'm happy to add them and will investigate investigate improving the RelativeDuration for the 0.3 release

@ebegumisa
Copy link
Contributor Author

Thanks @olliemath,

I've sent the PR #12 adding the shift_months_opt and related functions.

ebegumisa added a commit to ebegumisa/chronoutil that referenced this issue Sep 29, 2023
olliemath pushed a commit that referenced this issue Sep 29, 2023
* Issue #11: Add panicking test cases illustrating DST transition issue.

* Issue #11: Add `_opt` versions of shift utils.

• `delta::shift_months`, `delta::shift_years` and `delta::with_month` all now
  have opt counterparts. `delta::day` doesn't need any counterpart as it already
  behaves as #11 proposes.

• Add a few test assertions for checking `delta::with_month_opt` which, unlike
  the other new functions, isn't already fully covered by the existing tests.

* Issue #11: Add non-panicking DST transition test cases illustrating that issue is resolved.

* Issue #11: Document new `*_opt` shfit utils.

* Issue #11 -- Satisfy CI linting checks.
@olliemath
Copy link
Owner

shift_months_opt is now published under 0.2.6 - I'm going to keep this issue open to track progress on RelativeDelta - which I'll plan for 0.3.0

@olliemath olliemath added this to the 0.3 milestone Sep 29, 2023
@ebegumisa
Copy link
Contributor Author

shift_months_opt is now published under 0.2.6 - I'm going to keep this issue open to track progress on RelativeDelta - which I'll plan for 0.3.0

Great @olliemath . Much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants