Respect floating DATE-TIME values during parsing#52
Conversation
Stop treating the struct fallback timezone as an implicit calendar timezone. Calendars now default to no timezone, so DATE-TIME values without TZID or a trailing Z are preserved as NaiveDateTime unless the calendar declares a default timezone via X-WR-TIMEZONE. This also keeps UTC values explicit, applies declared calendar defaults to floating values, and makes unrecognized TZID values fall back to the calendar default when available.
| is_nil(default) -> | ||
| nil | ||
|
|
||
| true -> | ||
| default |
There was a problem hiding this comment.
If default is nil, then returning default will in the true branch will return nil. So I think this change can be removed?
| @spec to_date(String.t() | nil, map, ICal.t()) :: | ||
| Date.t() | DateTime.t() | NaiveDateTime.t() | nil | ||
| def to_date(nil, _params, _calendar), do: nil |
There was a problem hiding this comment.
This is a breaking change to the API. which will break code that does not currently expect / handle NaiveDateTimes.
This would mean either bumping the major version of the library (which can be done), or finding a way to avoid introducing a new incompatible data type. It probably requires the new type, so this will also need a bump to 3.0 in the mix.exs.
| ics = """ | ||
| BEGIN:VCALENDAR | ||
| X-WR-TIMEZONE:Europe/Zurich | ||
| BEGIN:VEVENT | ||
| DTSTAMP;TZID=Garbage:22221224T083000 | ||
| END:VEVENT | ||
| END:VCALENDAR | ||
| """ |
There was a problem hiding this comment.
This should be put into a file in test/data/ and loaded with Helper.test_data/1
| ics = """ | ||
| BEGIN:VCALENDAR | ||
| X-WR-TIMEZONE:Europe/Zurich | ||
| BEGIN:VEVENT | ||
| UID:1 | ||
| DTSTART:20151224T080000 | ||
| END:VEVENT | ||
| END:VCALENDAR | ||
| """ |
There was a problem hiding this comment.
This should be put into a file in test/data/ and loaded with Helper.test_data/1
| dtstamp: DateTime.t() | NaiveDateTime.t() | nil, | ||
| created: DateTime.t() | NaiveDateTime.t() | nil, | ||
| dtstart: Date.t() | DateTime.t() | NaiveDateTime.t() | nil, | ||
| dtend: Date.t() | DateTime.t() | NaiveDateTime.t() | nil, | ||
| modified: Date.t() | DateTime.t() | NaiveDateTime.t() | nil, | ||
| recurrence_id: Date.t() | DateTime.t() | NaiveDateTime.t() | nil, | ||
| exdates: [Date.t() | DateTime.t() | NaiveDateTime.t()], | ||
| rdates: [Date.t() | DateTime.t() | NaiveDateTime.t() | ICal.period()], |
There was a problem hiding this comment.
Seeing all these types added, it would probably make sense to introduce custom types in ical.ex such as:
@type rfc5455_datetime :: DateTime.t() | NativeDateTime.t()
@type maybe_rfc5455_datetime :: nil | rfc5455_datetime
@type rfc5455_date :: Date.t() | rfc5455_datetime
@type maybe_rfc5455_date :: nil | rfc5455_dateThis would make things more succinct and less prone to accidental omissions, as we could then right this as:
dtstamp: ICal.maybe_rfc5455_datetime(),
created: ICal.maybe_rfc5455_datetime(),
dtstart: ICal.maybe_rfc5455_date(),
exdates: [ICal.rfc5455_date],wdyt?
| if timezone == nil do | ||
| to_local_date(date_string) | ||
| else | ||
| to_date_in_timezone(date_string, timezone) | ||
| end | ||
| end | ||
|
|
||
| def to_date(<<_::binary-size(15), "Z">> = date_string, _params, _calendar) do | ||
| to_date_in_timezone(date_string, "Etc/UTC") | ||
| end | ||
|
|
||
| def to_date(date_string, _params, %ICal{default_timezone: nil}) do | ||
| to_local_date(date_string) |
There was a problem hiding this comment.
It would probably be easier to add this to to_date_int_timezone as it already parses the trailing Z for UTC. so that could be checked there to set the timezone to Etc/UTC if the timezone passed in as the second parameter is nil. Perhaps something like:
def to_date_in_timezone(date_string, timezone) do
# datetime in the form "{YYYY}{0M}{0D}T{h24}{m}{s}[Z]"
with <<y::binary-size(4), m::binary-size(2), d::binary-size(2), ?T, t_h::binary-size(2),
t_m::binary-size(2), t_s::binary-size(2), rest::binary>>
when rest == "" or rest == "Z" <- date_string,
{year, ""} <- Integer.parse(y),
{month, ""} <- Integer.parse(m),
{day, ""} <- Integer.parse(d),
{hour, ""} <- Integer.parse(t_h),
{minute, ""} <- Integer.parse(t_m),
{second, ""} <- Integer.parse(t_s) do
to_date_in_timezone(year, month, day, hour, minute, second, rest, timezone)
else
_ -> nil
end
end
defp to_date_in_timezone(year, month, day, hour, minute, second, "Z", _timezone) do
to_date_in_timezone(year, month, day, hour, minute, second, "Z", "Etc/UTC")
end
defp to_date_in_timezone(year, month, day, hour, minute, second, _suffix, nil) do
case NaiveDateTime.new(year, month, day, hour, minute, second) do
{:ok, datetime} -> datetime
_ -> nil
end
end
defp to_date_in_timezone(year, month, day, hour, minute, second, _suffix, timezone) do
with {:ok, date} <- Date.new(year, month, day),
{:ok, time} <- Time.new(hour, minute, second) do
ICal.as_valid_datetime(date, time, timezone)
else
_ -> nil
end
end
@doc "Parses a local date string as a NaiveDatetime, returning `nil` on failure"
@spec to_local_date(String.t()) :: NaiveDateTime.t() | nil
def to_local_date(date_string) do
# Ensure there is no trailing 'Z' by only passing in the first 15 characters.
to_date_in_timezone(String.slice(date_string, 0, 15), nil)
end
This is a big one, I know 😅, will sure have breaking changes. But I felt it was important.
Summary
This PR makes iCalendar date-time parsing explicit about timezone intent instead of treating the library's internal fallback timezone as calendar-provided meaning.
Previously,
%ICal{}defaulted todefault_timezone: "Etc/UTC". As a result, floatingDATE-TIMEvalues like:were parsed as UTC
DateTimes, even though they have neither a trailingZnor aTZID.This PR changes the calendar default timezone to
nil, so the parser can distinguish between:X-WR-TIMEZONEZTZIDWhy
RFC 5545 distinguishes three
DATE-TIMEforms:Floating local time.
UTC time.
Local time in a named timezone.
The previous implementation collapsed these meanings because all bare date-times were resolved through
calendar.default_timezone, which was always"Etc/UTC"unless overwritten. That made it impossible to reliably distinguish a true UTC value from a floating value that merely lacked timezone metadata.Behavior After This Change
VALUE=DATEparses asDateDATE-TIMEwith trailingZparses as UTCDateTimeDATE-TIMEwith validTZIDparses asDateTimein that timezoneDATE-TIMEwithout calendar default timezone parses asNaiveDateTimeDATE-TIMEwithX-WR-TIMEZONEparses in that calendar default timezoneTZIDfalls back to the calendar default timezone if present, otherwise preserves the value asNaiveDateTimeExample
Given an event with one floating
DATE-TIMEand one UTCDATE-TIME:Before
Both values were parsed as UTC
DateTimes because the calendar struct defaulted todefault_timezone: "Etc/UTC":This loses the distinction between:
and:
Now
The floating value is preserved as a
NaiveDateTime, while the UTC value remains a UTCDateTime:So timezone intent is explicit:
Z, noTZID-> floating local timeZ-> UTC instantNotes
This keeps the library's support for
X-WR-TIMEZONE, but avoids treating an internal fallback as an implicit calendar declaration. The change prefers explicit timezone information over implicit assumptions.