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

Fix Period and PeriodList to work with EXDATE and RDATE #684

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

axunonb
Copy link
Collaborator

@axunonb axunonb commented Dec 29, 2024

Improve reliability and usability for Period and PeriodList to become less error-prone by enforcing timezones being used consistently

Period

  • Make parameterless CTOR internal to ensure proper initialization by users
  • Add internal enum PeriodKind to identify the period kind as Undefined, DateTime, DateOnly or Period and add method PeriodKind GetPeriodKind()
  • For consistency, either the EndTime or the Duration can be set at a time. The last one set with a value not null will prevail, while the other will become null.
  • Timezones of StartTimeand (optional) EndTime must be the same
  • CompareTo uses AsUtc for comparing the StartTime
  • Remove returning a "magic" duration of 1 day, if EndTime is null and StartTime is date-only. This broke EXDATE and RDATE of an event, when only a start time exists. Full day is handled by PeriodSerializer.
  • Added EffectiveEndTime and EffectiveDuration properties to provide calculated values based on the set values.
  • Update the EndTime and Duration properties to directly return the set values.
  • Change the access modifiers of GetEffectiveDuration() and GetEffectiveEndTime methods from internal to private.

PeriodList

  • TzId: Removed, as the timezone is enforced to be the TzId of the first Period added.
  • EnsureConsistentTimezoneAndPeriodKind: The first period determines the TzId and PeriodKind of the PeriodList and all other Periods added must these set as well.
  • Add SetService(new PeriodListEvaluator(this)) for StringReader CTOR overload
  • Add static PeriodList FromStringReader(StringReader)
  • Add Add(IDateTime)
  • nullable enable

EventEvaluator:

  • EventEvaluator.WithEndTime(Period) only sets the EndTime, as Period.EffectiveDuration returns the duration.

TodoEvaluator:

  • Remove method PeriodWithDuration(Period) as it became redudant with the refactored Period class.

DataTypeSerializer and other serializers, CalendarObjectBase:

  • Activator.CreateInstance(TargetType, true) allows for not public CTORs, so that parameterless CTORs can be excluded from the public API, if proper initialization can't be assured (like with Period).

Abstraction layer to create EXDATE and RDATE

  • Introduced ExceptionDateCollection, RecurrencePeriodCollection and PeriodCollectionBase` classes.
  • Updated serializers to handle the modified PeriodList implementation.

These classes are an abstraction layer for creating List<PeriodList> objects.
Periods and CalDateTimes are accepted without having to care for RFC 5545 rules regarding serialization:

ToRecurrenceDates and ToRecurrenceDates methods aggregate and convert the exception or recurrence CalDateTime and Period objects into a list of PeriodList objects. Periods are grouped by their timezone IDs and period kinds in a way, that each PeriodList contains only distinct periods ready to serialize.

PeriodSerializer, DateTimeSerializer:

  • Never set the UTC timezone ID (use appending 'Z') (Section 3.2.19: The "TZID" property parameter MUST NOT be applied to DATE properties and DATE-TIME or TIME properties whose time values are specified in UTC.)

Resolves #590
Resolves #591
Resolves #614
Resolves #676

@axunonb axunonb force-pushed the pr/exdate-rdate branch 3 times, most recently from 9d19aa8 to f09b049 Compare December 29, 2024 20:40
@axunonb
Copy link
Collaborator Author

axunonb commented Dec 29, 2024

@minichma The code changes became more than expected, because it was necessary to revisit Period and PeriodList.
I feel like EXDATE and RDATE should have more unit tests with different scenarios than those in RecurrenceWithRDateTests and RecurrenceWithExDateTests. If so, let me know.
Using the List<PeriodList> for EXDATE brings a disadvantage: Adding a Period with Duration will not throw. But there is a "how to" in the xmldoc, and we have specific methods to add an IDateIme to a PeriodList. This way we don't need extra classes just for EXDATE. Would this be acceptable?

@minichma
Copy link
Collaborator

Using the List<PeriodList> for EXDATE brings a disadvantage: Adding a Period with Duration will not throw. But there is a "how to" in the xmldoc, and we have specific methods to add an IDateIme to a PeriodList. This way we don't need extra classes just for EXDATE. Would this be acceptable?

I'm pretty sure it is. I see the challenges that come with introducing separate types for lists of Periods and IDateTimes - redundant code, challenges with type safety, inheritance. At the same time I remember the challenges I had when starting with ical.net and not knowing much about the details of RFC 5545. The ExDate property being a list of Periods was not exactly easy to understand at that time. So yes, doc comments will certainly be helpful in this respect. We can still continue thinking about alternatives in the future (maybe not v5 though once it has been released).

@axunonb axunonb marked this pull request as ready for review December 30, 2024 10:14
@axunonb axunonb requested a review from minichma December 30, 2024 10:14
@axunonb
Copy link
Collaborator Author

axunonb commented Dec 30, 2024

Oops, just noticed that
RDATE:20231002/P1D,20231003T100000
is currently possible with PeriodList, but not allowed. The VALUE type of containing elements must be the same - as with timezone. Will fix that.

@axunonb axunonb marked this pull request as draft December 30, 2024 17:33
@axunonb axunonb force-pushed the pr/exdate-rdate branch 2 times, most recently from 76a047e to 9eef103 Compare January 2, 2025 09:13
@axunonb
Copy link
Collaborator Author

axunonb commented Jan 2, 2025

The ExDate property being a list of Periods was not exactly easy to understand

Creating EXDATE and RDATE can indeed be a pain for users. Now there is an abstraction layer that makes this a breeze.

@axunonb axunonb marked this pull request as ready for review January 2, 2025 09:24
@minichma
Copy link
Collaborator

minichma commented Jan 2, 2025

@axunonb Wish you a great 2025! Hope I will find some time for a review this afternoon when I'll be on a train or otherwise tomorrow.

@axunonb
Copy link
Collaborator Author

axunonb commented Jan 2, 2025

@minichma Happy New Year and thank you!
Just noticed that as of section 3.8.5.1 for day-only EXDATE;VALUE=DATE: must be serialized...
We could start the new year with a first v5 pre release, couldn't we?

Copy link
Collaborator

@minichma minichma left a comment

Choose a reason for hiding this comment

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

This is a challenging one! Left a few comments but they might not be fully consistent. Just take what you think makes sense. Unfortunately I don't have the time right now to go into all the detail that would be required.

Ical.Net/DataTypes/Period.cs Outdated Show resolved Hide resolved
Ical.Net/DataTypes/Period.cs Outdated Show resolved Hide resolved
Ical.Net/DataTypes/Period.cs Outdated Show resolved Hide resolved
Ical.Net/DataTypes/PeriodList.cs Show resolved Hide resolved
Ical.Net/DataTypes/PeriodList.cs Show resolved Hide resolved
Ical.Net/DataTypes/Period.cs Outdated Show resolved Hide resolved
Ical.Net/Collections/PeriodCollectionBase.cs Outdated Show resolved Hide resolved
Ical.Net/Collections/ExceptionDateCollection.cs Outdated Show resolved Hide resolved
Ical.Net/DataTypes/PeriodList.cs Show resolved Hide resolved
Ical.Net/Serialization/DataTypes/PeriodSerializer.cs Outdated Show resolved Hide resolved
@axunonb
Copy link
Collaborator Author

axunonb commented Jan 7, 2025

@minichma Looks like the failing tests for .Net Framework go back to GitHub upgrading to ubuntu 24.04 as "latest", which is missing mono. Downgrading to ubuntu-22.04 might help: microsoft/sbom-tool#855

@axunonb axunonb force-pushed the pr/exdate-rdate branch 2 times, most recently from 0842c10 to e190317 Compare January 8, 2025 23:20
Improve reliability and usability for `Period` and `PeriodList` to become less error-prone
by enforcing timezones being used consistently

Period
- Make parameterless CTOR `internal` to ensure proper initialization by users
- A period can be defined
  1. by a start time and an end time,
  2. by a start time and a duration,
  3. by a start time only, with the duration unspecified. This is for EXDATE and RDATE date-only and date/time.
- For cosistency, either the `EndTime` or the `Duration` can be set at a time. The last one set with a value not `null` will prevail, while the other will become `null`.
- Timezones of `StartTime`and (optional) `EndTime` must be the same
- `CompareTo` uses `AsUtc` for comparing the `StartTime`
- Remove returning a "magic" duration of 1 day, if `EndTime` is null and `StartTime` is date-only. This broke `EXDATE` and `RDATE` of an event, when the only a start time exists.
- Added `EffectiveEndTime` and `EffectiveDuration` properties to provide calculated values based on the set values.
- Update the `EndTime` and `Duration` properties to directly return the set values.
- Change the access modifiers of `GetEffectiveDuration()` and `GetEffectiveEndTime` methods from internal to private.

PeriodList
- `TzId`: `public` setter changed to `private`
- `EnsureConsistentTimezones`: The first period determines the timezone of the `PeriodList` and all other `Period`s added must have the same timezone
- Add `SetService(new PeriodListEvaluator(this))` for `StringReader` CTOR overload
- Add `static PeriodList FromStringReader(StringReader)`
- Add `static PeriodList FromDateTime(IDateTime)`
- Add `PeriodList AddPeriod(Period)` for chaining
- Add `PeriodList Add(IDateTime)` for chaining
- nullable enable

EventEvaluator:
- `EventEvaluator.WithEndTime(Period)` only sets the `EndTime`, as `Period.EffectiveDuration` returns the duration.

TodoEvaluator:
- Remove method `PeriodWithDuration(Period)` as it became redudant with the refactored `Period` class.

DataTypeSerializer and other serializers, CalendarObjectBase:
- `Activator.CreateInstance(TargetType, true)` allows for not `public` CTORs, so that parameterless CTORs can be excluded from the public API, if proper initialization can't be assured (like with `Period`).

PropertySerializer:
- TBD: Never set the UTC timezone ID (use appending 'Z')

- Resolves ical-org#590
- Resolves ical-org#591
- Resolves ical-org#614
- Resolves ical-org#676
PeriodKind
- Add new enum

Period:
- Add internal `property string TzId`
- Add internal method `PeriodKind GetPeriodKind()`
- Ensure timeone consistence for setters of StartTime / EndTime

PeriodList:
- Add internal `property string TzId`
- Add internal property ``PeriodKind PeriodListKind`m
- Ensure added `Period`s have the same `TzId`and `PeriodKind` as the first one added

PeriodListSerialilzer
- Serializes TZID
- Serializes value type PERIOD
- Introduced `ExceptionDateCollection`, `RecurrencePeriodCollection` and PeriodCollectionBase` classes.
- Updated serializers to handle the modified `PeriodList` implementation.

These classes are an abstraction layer for creating `List<PeriodList>` objects.
`Period`s and `CalDateTime`s are accepted without having to care for RFC 5545 rules regarding serialization:

`ToRecurrenceDates` and `ToRecurrenceDates` methods aggregate and convert the exception or recurrence `CalDateTime` and `Period` objects into a list of `PeriodList` objects. Periods are grouped by their timezone IDs and period kinds in a way, that each `PeriodList` contains only distinct periods ready to serialize.
- Adjust code in ' EventEvaluator` and `PeriodSerializer`
- Method `GetPeriodKind()` is replace with internal property `PeriodKind`
- Add internal `Period.Create` to centralize logic if end time and duration exist
- Modified unit test
Remove solution for RDATE/ESDATE introduced earlier in this PE

The commit removes several test and source files related to period and exception date collections in calendar events. Specifically, the following files have been completely removed:
- `PeriodCollectionBase.cs`
- `ExceptionDateCollection.cs`
- `RecurrencePeriodCollection.cs`
- `PeriodCollectionTests.cs`
- `RecurrenceWithExDateTests.cs`
- `RecurrenceWithRDateTests.cs`
@axunonb axunonb force-pushed the pr/exdate-rdate branch 3 times, most recently from 66a1611 to fca144a Compare January 9, 2025 00:02
- Add internal properties `PeriodKind` and `TzId` to `PeriodList`.
- Modify `Period` indexer and `Insert` to ensure consistency for timezone and `PeriodKind`
- Prepare for `PeriodList` to become `internal`
- Update `PeriodListSerializer` and `PeriodSerializer` classes for `Period` serialization
- Introduce `ExceptionDates`, `RecurrenceDates`, and `PeriodListWrapperBase` to become wrappers around these properties of `RecurringComponent`
- Add unit tests in `PeriodListWrapperTests` for `ExceptionDates` and `RecurrenceDates`.

Note: The wrappers are not yet implemented for `IRecurringComponent`s.
…nceDates`

`IRecurrable`
- Change the type of `ExceptionDates` from `IList<PeriodList>` to `ExceptionDates`.
- Change the type of `RecurrenceDates` from `IList<PeriodList>` to `RecurrenceDates`.

`RecurringComponent`, `VTimeZoneInfo`
- Rename `IList<PeriodList> ExceptionDates` to `ExceptionDatesPeriodLists`. This serves as the backing storage for the new `ExceptionDates` class/property.
- Rename `IList<PeriodList> RecurrenceDates` to `RecurrenceDatesPeriodLists`. This serves as the backing storage for the new `RecurrenceDates` class/property.
- Add the creation of instances of `ExceptionDates` and `RecurrenceDates` to the `Initialize()` method.

`PeriodList` and `PeriodListEvaluator`
- Change the access modifier to `internal`.

`RecurringEvaluator` and `TodoEvaluator`
- Implement the new `ExceptionDates` and `RecurrenceDates` classes.

`VTimeZone`
- Enhanced serialization:
  - Before: Each `RDATE` date/time was serialized as a separate `RDATE` propery
  - Now: `RDATE` date/times which can be clustered go into one single `RDATE` property

Unit tests
- Refactor tests, using `ExceptionDates` and `RecurrenceDates` instead of `IList<PeriodList>`
- Remove the duplicate `SimpleDeserializationTests.RecurrenceDates1()` method, which exists in `DeserializationTests.RecurrenceDates1()`.

Todo:
- Replace the use of `PeriodList.GetGroupedPeriods` in `CalendarEvent.Equals()`.
@axunonb
Copy link
Collaborator Author

axunonb commented Jan 10, 2025

@minichma Also a long story has end...
The last step was to migrate usage of IList<PeriodList> to ExceptionDates and RecurrenceDates.
Finally PeriodList and PeriodListEvaluator have the internal access modifier
I'll do my own review for all changes tomorrow. Could you have another look at the PR (at least at most relevant parts like ExceptionDates, RecurrenceDates, Period and the serializers? I tried to separate migration steps in the latest 4 commits.

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