Refactor to use DateOnly and TimeOnly for date/time handling #658

axunonb · 2024-11-30T21:27:34Z

Increased CalDateTime immutability

CalDateTime:

Date is of type DateOnly (breaking vs. v4)
Make UtzTzId a public string
Remove DateOnlyValue
Remove HasDate (always true) (breaking vs. v4)
Remove Millisecond (breaking vs. v4)
Remove Ticks (breaking vs. v4)
Remove overload CalDateTime(int year, int month, int day, string? tzId) (breaking vs. v4)
HasTime only has a getter (breaking vs. v4)
Time is a getter of type TimeOnly. Values are always rounded to the nearest second (breaking vs. v4)
AddMilliseconds takes double as argument, like DateTime.AddMilliseconds (breaking vs. v4)
Remove TimeOnlyValue
Value only has a getter (breaking vs. v4)
TzId only has a getter (breaking vs. v4)
Remove AsSystemLocal (breaking vs. v4)
Remove AsDateTimeOffset (breaking vs. v4)
Remove AddMilliseconds (breaking vs. v4)
Remove AddTicks (breaking vs. v4)
Remove unused TimeSpan? operator -(CalDateTime? left, IDateTime? right) (breaking vs. v4)
Update Equality operators for comparing to floating date/time (breaking vs. v4)

IDateTime (breaking vs. v4):

Remove AsSystemLocal (breaking vs. v4)`
Remove AsDateTimeOffset (breaking vs. v4)`
Remove AddMilliseconds (breaking vs. v4)
Remove AddTicks (breaking vs. v4)
Remove Millisecond (breaking vs. v4)
Remove Ticks (breaking vs. v4)
Add explicit bool IsFloating (implicit TzId == null)
Date is a getter of type DateOnly
Time is a getter of type TimeOnly
HasTime only has a getter
Remove HasDate (always true) (breaking vs. v4)
Value only has a getter
TzId only has a getter
ToTimeZone(string?) argument is a NRT

Miscellaneous:

CTORs with DateTime arguments persist. Actually they are also comfortable, and now consistently processed
Keep IDateTime - maybe in another PR
Transitioned from DateTime to DateOnly and TimeOnly across the codebase for improved date and time handling.
Updated methods, properties, and tests to ensure compatibility with the new approach.
Updated xmldoc

Resolves #656
Resolves #662

Increased `CalDateTime` immutability CalDateTime: * `Date` is of type `DateOnly` (breaking vs. v4) * Make `UtzTzId` a public string * Remove `DateOnlyValue` * Remove `HasDate` (always true) (breaking vs. v4) * Remove overload CalDateTime(int year, int month, int day, string? tzId) (breaking vs. v4) * `HasTime` only has a getter (breaking vs. v4) * `Time` is a getter of type `TimeOnly`. Values are always rounded to the nearest second (breaking vs. v4) * Remove `TimeOnlyValue` * `Value` only has a getter (breaking vs. v4) * `TzId` only has a getter (breaking vs. v4) * Remove `AsSystemLocal` (breaking vs. v4) * Remove unused `TimeSpan? operator -(CalDateTime? left, IDateTime? right)` (breaking vs. v4) * Update Equality operators for comparing to floating date/time (breaking vs. v4) IDateTime (breaking vs. v4): * Remove `AsSystemLocal` * Add explicit bool IsFloating (implicit TzId == null) * `Date` is a getter of type `DateOnly` * `Time` is a getter of type `TimeOnly` * `HasTime` only has a getter * Remove `HasDate` * `Value` only has a getter * `TzId` only has a getter * `ToTimeZone(string?)` argument is a NRT Miscellaneous: * CTORs with `DateTime` arguments persist. Actually they are also comfortable, and now consistently processed * Keep `IDateTime` - maybe in another PR * Transitioned from DateTime to DateOnly and TimeOnly across the codebase for improved date and time handling. * Updated methods, properties, and tests to ensure compatibility with the new approach. * Updated xmldoc

Ical.Net.Tests/CalDateTimeTests.cs

Ical.Net.Tests/ComponentTest.cs

Ical.Net.Tests/ProgramTest.cs

Ical.Net.Tests/RecurrenceTests.cs

…anged Remove fallback to system's local timezone for floating date/time

github-advanced-security · 2024-12-02T13:48:25Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Ical.Net/DataTypes/IDateTime.cs

axunonb · 2024-12-02T14:38:19Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

@@ -988,6 +990,6 @@ private static IDateTime MatchTimeZone(IDateTime dt1, IDateTime dt2)

        return dt1.IsUtc
            ? new CalDateTime(copy.AsUtc)
-            : new CalDateTime(copy.AsSystemLocal);


Isn't the code in MatchTimeZone(IDateTime dt1, IDateTime dt2) completely useless?

Its not the most elegant code, but it aims to adjust UNTIL, which usually is specified in UTC, to DTSTART's time zone, which usually isn't in UTC. This allows checking comparing the adjusted UNTIL with the generated recurrences without considering the time zone again. So the functionality is needed one way or the other.

That's what I thought, but the way it is implemented looks like a noop. Then I removed the method invocation temporarily with no effect on unit tests. So this is just a hint re recurrences.

I could easily be wrong, or relevant tests are missing. We'd need a test with UNTIL exactly at the last recurrence and then run the test with a TZ with positive offset and with one with a negative offset. In any case the last recurrence should be returned.

The 2 arguments to MatchTimeZone(IDateTime dt1, IDateTime dt2) always have the same timezone, so there is nothing to adjust? [Edit] The method in effect is r.Until = r.Until.

if (r.Until != DateTime.MinValue) { r.Until = MatchTimeZone(referenceDate, new CalDateTime(r.Until, referenceDate.TzId)).Value; }

You are fully right, its a noop. But I strongly believe that something's wrong there, because the conversion needs to be done somewhere. Should be investigated separately. Maybe the translation between UTC and tzid happened in the constructor of CalDateTime and was lost in one of the previous PRs?

Created #663 to track

Resolves ical-org#660

…t null Reasoning: DATE cannot have a timezone

Ical.Net/Calendar.cs

minichma · 2024-12-02T19:45:02Z

Just noted, that I didn't submit my review comments from 2d ago, so please read my comments in the context of last Sat.

Ical.Net/DataTypes/Trigger.cs

Ical.Net/DataTypes/CalDateTime.cs

minichma · 2024-11-30T23:01:51Z

Ical.Net/DataTypes/CalDateTime.cs

+            return false;
+        }
+
+        if (left.IsFloating || right.IsFloating)


Suggest to add a check if IsFloating equals. I.e.

if (left.IsFloating != right.IsFloating) return false;

On the first glance I thought the proposal worked, but it doesn't.

If we don't add this check, we could consider instances as equal even if one is floating and the other has a TZ set, which seems wrong. What would be the reason for that behavior?

Ical.Net/DataTypes/CalDateTime.cs

minichma · 2024-11-30T23:22:25Z

Ical.Net/Evaluation/RecurrenceUtil.cs

-        new CalDateTime(dt.Date), new CalDateTime(dt.Date.AddDays(1).AddSeconds(-1)), includeReferenceDateInResults);
+    public static HashSet<Occurrence> GetOccurrences(IRecurrable recurrable, IDateTime dt, bool includeReferenceDateInResults)
+    {
+        var endTimeOnly = dt.Time?.Add(TimeSpan.FromSeconds(-1));


Be careful, Time could be 0:00 in which case we'd end up with a negative time.

InvalidOperationException: see below

Not sure, I understand. I think the problem still exists. If someone specifies a dt e.g. 20241203T000000, which is legit, then this code would lead to 20241203 T -000001.

I think, we would introduce a new bug here and should consider fixing it right away.

minichma · 2024-12-02T19:39:45Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

@@ -988,6 +990,6 @@ private static IDateTime MatchTimeZone(IDateTime dt1, IDateTime dt2)

        return dt1.IsUtc
            ? new CalDateTime(copy.AsUtc)
-            : new CalDateTime(copy.AsSystemLocal);


Its not the most elegant code, but it aims to adjust UNTIL, which usually is specified in UTC, to DTSTART's time zone, which usually isn't in UTC. This allows checking comparing the adjusted UNTIL with the generated recurrences without considering the time zone again. So the functionality is needed one way or the other.

Ical.Net/Calendar.cs

minichma · 2024-12-02T19:47:01Z

This is a great improvement regarding readability and maintainability!

* Updated CalDateTime to truncate time to seconds instead of rounding. * Removed AddMilliseconds and AddTicks methods and related test cases. * Fix: minus and plus operators for cases where time parts are equal * Adjusted EndOfDay method to use AddSeconds(-1) instead of AddTicks(-1) * Removed GetOccurrenceShouldExcludeDtEndZoned test.

minichma · 2024-12-03T13:09:23Z

Ical.Net/Calendar.cs

@@ -205,11 +205,17 @@ public VTimeZone AddTimeZone(VTimeZone tz)
    /// <param name="dt">The date for which to return occurrences. Time is ignored on this parameter.</param>
    /// <returns>A list of occurrences that occur on the given date (<paramref name="dt"/>).</returns>
    public virtual HashSet<Occurrence> GetOccurrences(IDateTime dt)
-        => GetOccurrences<IRecurringComponent>(new CalDateTime(dt.Date), new CalDateTime(dt.Date.AddDays(1).AddSeconds(-1)));
+    {
+        var endTimeOnly = dt.Time?.Add(TimeSpan.FromSeconds(-1));


If Time is 0:00, you'll end up with a negative time.

Track in #662

This is a new issue introduced in this PR, so shouldn't we fix it right away? The behavior is also somewhat different from the previous one. In the previous version we just truncated the time, in the new one we keep the time, if set, and observe the period until the same time next day. Is this intentional?

minichma · 2024-12-03T13:13:17Z

Ical.Net/CalendarComponents/CalendarEvent.cs

@@ -152,15 +152,19 @@ public virtual bool IsAllDay
        get => !Start.HasTime;
        set
        {
-            // Set whether or not the start date/time
+            // Set whether the start date/time


I don't think this property should be editable. Whether the event is all-day should be specified via DTSTART.

I just changed the wording of the comment :)

But yes, this one was easy. I removed the setter that was used only in 1 unit test. AllDay can simply be set with a CalDateTime without time.

minichma · 2024-12-03T13:17:04Z

Ical.Net/DataTypes/CalDateTime.cs

-        _tzId = string.Equals(UtcTzId, tzId, StringComparison.OrdinalIgnoreCase) ? UtcTzId : tzId;
+        _tzId = tzId switch
+        {
+            _ when !timeOnly.HasValue && string.Equals(UtcTzId, tzId, StringComparison.OrdinalIgnoreCase) => UtcTzId,


Why does UTC receive special handling here?

thanks, that's redundant

minichma · 2024-12-03T13:20:12Z

Ical.Net/DataTypes/CalDateTime.cs

+            return false;
+        }
+
+        if (left.IsFloating || right.IsFloating)


If we don't add this check, we could consider instances as equal even if one is floating and the other has a TZ set, which seems wrong. What would be the reason for that behavior?

minichma · 2024-12-03T13:26:39Z

Ical.Net/DataTypes/CalDateTime.cs

+        }
+        else
+        {
+            copy._dateOnly = DateOnly.FromDateTime(newValue);


I think we shouldn't silently convert a date-only value to date-time but rather throw an exception. This is tricky in multiple ways. In particular, if DST changes. Maybe related to #660. Do we need those operators at all? If yes, we might want to be more explicit, i.e. have a 'add/subtract nominal' vs 'add/subtract accurate' in correspondence with how RFC5545 distinguishes those cases.

Created #661 for tracking a separate PR

minichma · 2024-12-03T14:17:58Z

Ical.Net/DataTypes/IDateTime.cs

    /// </summary>
    int Second { get; }

    /// <summary>
-    /// Gets the millisecond for this date/time value.
+    /// Gets the millisecond that applies to the <see cref="Value"/>.
    /// </summary>
    int Millisecond { get; }


minichma · 2024-12-03T14:18:04Z

Ical.Net/DataTypes/IDateTime.cs

    /// </summary>
    int Millisecond { get; }

    /// <summary>
-    /// Gets the ticks for this date/time value.
+    /// Gets the ticks that applies to the <see cref="Value"/>.
    /// </summary>
    long Ticks { get; }


The question is applicable to other properties, too: Day, Month, Year...
All can be retrieved from the Value property. We could clean-up, when this would bring any advantage. But not in this PR.

Sure, my thought on Milliseconds and Ticks is just, that they are truncated starting with this PR, so providing getters for them could be confusing and considered inconsistent.

minichma · 2024-12-03T15:06:11Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

@@ -988,6 +990,6 @@ private static IDateTime MatchTimeZone(IDateTime dt1, IDateTime dt2)

        return dt1.IsUtc
            ? new CalDateTime(copy.AsUtc)
-            : new CalDateTime(copy.AsSystemLocal);


You are fully right, its a noop. But I strongly believe that something's wrong there, because the conversion needs to be done somewhere. Should be investigated separately. Maybe the translation between UTC and tzid happened in the constructor of CalDateTime and was lost in one of the previous PRs?

minichma · 2024-12-03T15:08:46Z

Ical.Net/Evaluation/RecurrenceUtil.cs

-        new CalDateTime(dt.Date), new CalDateTime(dt.Date.AddDays(1).AddSeconds(-1)), includeReferenceDateInResults);
+    public static HashSet<Occurrence> GetOccurrences(IRecurrable recurrable, IDateTime dt, bool includeReferenceDateInResults)
+    {
+        var endTimeOnly = dt.Time?.Add(TimeSpan.FromSeconds(-1));


Not sure, I understand. I think the problem still exists. If someone specifies a dt e.g. 20241203T000000, which is legit, then this code would lead to 20241203 T -000001.

minichma · 2024-12-03T15:10:41Z

Ical.Net/Utility/DateUtil.cs

@@ -19,7 +19,7 @@ public static IDateTime StartOfDay(IDateTime dt)
        => dt.AddHours(-dt.Hour).AddMinutes(-dt.Minute).AddSeconds(-dt.Second);

    public static IDateTime EndOfDay(IDateTime dt)


We should probably get rid of that. Calculations should be done precisely, adding -1 whatever shouldn't be necessary.

Created issue #662 for a separate PR

minichma · 2024-12-03T15:12:00Z

Sorry for the many comments. You're working on the big ones here, so many things to consider.

axunonb · 2024-12-03T19:56:27Z

No need for excuses :) - comments are all very welcome.
There is one more commit with 2 issues from your latest comments.
For other comments we have new issues to track them - so they'll not go into this PR, which is huge already.
One important topic is the discussion re comparing floating to zoned date/time, Let's resolve the topic there.

* Made `IsAllDay` in `CalendarEvent.cs` read-only, * Updated `Issue432_AllDay` unit test in `RecurrenceTests.cs` to remove time component from `Start` property and eliminate setting the `IsAllDay` property. (The only place where the setter was used.) * Simplified `CalDateTime.cs` initialization by removing condition related to `UtcTzId` when `timeOnly` is not provided.

* `operator ==`: if (left.IsFloating != right.IsFloating) return false; * Add unit test

axunonb · 2024-12-04T23:46:20Z

@minichma PR comments should all all be resolved or addressed

minichma · 2024-12-05T05:44:49Z

I'm on a business trip until Sat. Not sure, whether I'll find the time until then.

minichma · 2024-12-05T15:35:56Z

Ical.Net/Calendar.cs

@@ -205,11 +205,17 @@ public VTimeZone AddTimeZone(VTimeZone tz)
    /// <param name="dt">The date for which to return occurrences. Time is ignored on this parameter.</param>
    /// <returns>A list of occurrences that occur on the given date (<paramref name="dt"/>).</returns>
    public virtual HashSet<Occurrence> GetOccurrences(IDateTime dt)
-        => GetOccurrences<IRecurringComponent>(new CalDateTime(dt.Date), new CalDateTime(dt.Date.AddDays(1).AddSeconds(-1)));
+    {
+        var endTimeOnly = dt.Time?.Add(TimeSpan.FromSeconds(-1));


This is a new issue introduced in this PR, so shouldn't we fix it right away? The behavior is also somewhat different from the previous one. In the previous version we just truncated the time, in the new one we keep the time, if set, and observe the period until the same time next day. Is this intentional?

Ical.Net/DataTypes/CalDateTime.cs

minichma · 2024-12-05T15:56:34Z

Ical.Net/DataTypes/CalDateTime.cs

-        if (!dt.HasTime && hours % 24 > 0)
+        var copy = Copy<CalDateTime>();
+        var newValue = copy.Value.AddHours(hours);
+        if (!copy.HasTime && (hours % 24 == 0))


We should consider (in a subsequent PR) to remove the special handling of multiples of 24. This is the kind of magic we had in v4 with midnight. A user wouldn't understand, why adding multiple of 24h behaves differently than adding other values. I suggest to either disallow adding hours to date-only instances altogether (preferred) , or, to always convert to date-time.

issues #661 and #662 already exists since last review

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

minichma · 2024-12-05T16:05:32Z

Ical.Net/Evaluation/RecurrenceUtil.cs

-        new CalDateTime(dt.Date), new CalDateTime(dt.Date.AddDays(1).AddSeconds(-1)), includeReferenceDateInResults);
+    public static HashSet<Occurrence> GetOccurrences(IRecurrable recurrable, IDateTime dt, bool includeReferenceDateInResults)
+    {
+        var endTimeOnly = dt.Time?.Add(TimeSpan.FromSeconds(-1));


I think, we would introduce a new bug here and should consider fixing it right away.

axunonb · 2024-12-05T18:34:18Z

introduce a new bug here and should consider fixing it right away

~~Agree about this should be fixed (#662) with another PR.~~
The fix was trivial, so it's done in 5956fb9

It's existing code logic though:

ical.net/Ical.Net/Evaluation/RecurrenceUtil.cs

Lines 16 to 17 in add58a9

    
           public static HashSet<Occurrence> GetOccurrences(IRecurrable recurrable, IDateTime dt, bool includeReferenceDateInResults) => GetOccurrences(recurrable, 
        
               new CalDateTime(dt.Date), new CalDateTime(dt.Date.AddDays(1).AddSeconds(-1)), includeReferenceDateInResults);

[Edit]
The remaining time adjustments were only found in class Calendar.
The adjustments are redundant, because
public static HashSet<Occurrence> RecurrenceUtil.GetOccurrences(IRecurrable recurrable, IDateTime periodStart, IDateTime periodEnd, bool includeReferenceDateInResults)
uses precise "LessThan" for comparing end date/times
This is now also cleaned-up with 5956fb9

Removed the `endTimeOnly` variable and its usage in the `GetOccurrences` method. The code now directly creates the `CalDateTime` object for the end of the period without adjusting the time component. This change streamlines the code and avoids potential issues related to time adjustments.

axunonb · 2024-12-05T19:33:56Z

Comments resolved or tracked by issues

The `GetOccurrences` methods in `Calendar.cs` have been simplified by removing the code that adjusted the end time by subtracting one second or one tick. The methods now directly use the start date and the date one day after the start date without adjusting the time component. This change simplifies the logic and removes unnecessary time adjustments. The adjustments are redudant, because public static HashSet<Occurrence> RecurrenceUtil.GetOccurrences(IRecurrable recurrable, IDateTime periodStart, IDateTime periodEnd, bool includeReferenceDateInResults) uses precise "LessThan" for comparing end date/times Resolves ical-org#662

sonarqubecloud · 2024-12-05T22:49:52Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

axunonb · 2024-12-05T23:01:19Z

The remaining time adjustments were only found in class Calendar.

The adjustments are redundant, because calls finally go to
public static HashSet<Occurrence> RecurrenceUtil.GetOccurrences(IRecurrable recurrable, IDateTime periodStart, IDateTime periodEnd, bool includeReferenceDateInResults)
which uses precise "LessThan" for comparing end date/times.

minichma · 2024-12-06T16:53:30Z

The fix was trivial, so it's done in 5956fb9

It's existing code logic though:

Great, thank you! The difference to the previous code is, that the previous version calculated 23:59:59 the previous day, but the new code calculated -0:00:01 the same day. So now its certainly better.

minichma

Great improvement!

axunonb force-pushed the pr/refactor-idatetime branch 3 times, most recently from 39c43e4 to 7a72ba1 Compare November 30, 2024 22:56

axunonb force-pushed the pr/refactor-idatetime branch from 7a72ba1 to 7463f67 Compare December 1, 2024 21:01

Remove AsDateTimeOffset from IDateTime and CalDateTime

67cda7c

axunonb force-pushed the pr/refactor-idatetime branch from ba7eb3e to 67cda7c Compare December 2, 2024 09:02