niccokunzmann/python-recurring-ical-events

bug: Crash with X-WR-TIMEZONE and VEVENT with DTSTART without TZID

franc6 opened this issue · 13 comments

franc6 commented

Describe the bug
When parsing a calendar that contains X-WR-TIMEZONE and a VEVENT with a DTSTART that does not include a TZID, the code will crash in x_wr_timezone.py at line 114. This happens because the dt value passed doesn't have timezone information.

To Reproduce

# content should be the ICS file data
calendar = Calendar.from_ical(content)
if calendar is not None:
  rie_events = rie.of(calendar).between(dateutil.parser.parse("2021-09-16T00:00:00"), dateutil.parser.parse(2021-09-16T23:59:59")
  for event in rie_events:
    print(event.get("SUMMARY"))
    print(event.get("DTSTART").dt.astimezone())

ICS file

BEGIN:VCALENDAR
VERSION:2.0
PRODID:manually_generated
X-WR-TIMEZONE:Europe/Brussels
X-WR-CALNAME:MyCalendar
X-WR-CALDESC:Description of calendar
BEGIN:VEVENT
UID:match_1025179
DTSTAMP:20210911T014015Z
DESCRIPTION:Summary
DTSTART:20210916T210000
DTEND:20210916T224500
SUMMARY:Summary
END:VEVENT
END:VCALENDAR

Expected behavior
Output should be:

Summary
2021-09-16 21:00:00+02:00

Console output

venv/lib/python3.9/site-packages/recurring_ical_events.py:468: in of
    a_calendar = x_wr_timezone.to_standard(a_calendar)
venv/lib/python3.9/site-packages/x_wr_timezone.py:149: in to_standard
    return walker.walk(calendar)
venv/lib/python3.9/site-packages/x_wr_timezone.py:64: in walk
    subcomponent = self.walk_event(subcomponent)
venv/lib/python3.9/site-packages/x_wr_timezone.py:74: in walk_event
            calendar_data,
    attributes[name] = self.walk_value(value)
venv/lib/python3.9/site-packages/x_wr_timezone.py:85: in walk_value
    return walk(value)
venv/lib/python3.9/site-packages/x_wr_timezone.py:103: in walk_value_vDDDTypes
    dt = self.walk_value(value.dt)
venv/lib/python3.9/site-packages/x_wr_timezone.py:85: in walk_value
    return walk(value)
            calendar_data,
venv/lib/python3.9/site-packages/x_wr_timezone.py:126: in walk_value_datetime
    if self.is_UTC(dt):
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <x_wr_timezone.UTCChangingWalker object at 0x104b429a0>
dt = datetime.datetime(2021, 9, 16, 21, 0)

    def is_UTC(self, dt):
        """Return whether the time zone is a UTC time zone."""
>       return dt.tzname().upper() == "UTC"
E       AttributeError: 'NoneType' object has no attribute 'upper'

venv/lib/python3.9/site-packages/x_wr_timezone.py:114: AttributeError

Version:
recurring-ical-events 1.0.1b0
x-wr-timezone 0.0.4

Additional context
This problem did not exist prior to the use of x_wr_timzone.py. My test case works as I expect (i.e. DTSTART value is interpreted as being in the local time zone) if the X-WR-TIMEZONE entry is removed.

Suggested implementation
I'm assuming the bug is actually in x_wr_timezone.py, but I didn't take time to look up how to limit it to that project. Since I'm not sure what x_wr_timezone.py is intended to do in this case, I don't have a suggested fix. The simple "fix" of returning False from is_UTC if dt.tzname() returns None will avoid the crash, but might not be the right fix.

  • add an ICS file
  • add a test
  • implement the test
  • fix the test
niccokunzmann commented
franc6 commented

I'm not sure if I've got the behavior correct, but it does what I expect now, based on what I've read elsewhere. New tests were added, and the existing tests still pass. See niccokunzmann/x-wr-timezone#7

niccokunzmann commented

Thanks for the fix! I wonder. What do you think would close this Issue?
I have these suggestions:

  • At least, a new release that claims x-wr-timezone >= 0.0.5 with the added change in the changelog.
  • Maybe: tests for the file that breaks. I think that would be good. The reason is that it must run with the new version of x-wr-timezone and the tests must be still all ok. Best would be if x-wr-timezone v0.0.4 breaks the new tests.
franc6 commented

Your suggestions sound good to me. I'd also be content with you closing it as not a bug in this project; if anyone else runs into this before you get around to updating tests and requirements, the comments here should guide them to a fix (require x-wr-timezone >= 0.0.5). I shouldn't have time to add tests for you, but I can suggest the above ICS file and the returned start time as 2021-09-16 21:00:00+02:00.

For my own purposes, I'm happy with adding the x-wr-timezone requirement bumped to >=0.0.5, since I had a user report this problem to me the day after I posted this issue. :)

niccokunzmann commented

@franc6, thanks for all the info - this was a fast fix.
I created #87 and merged it since all tests run.
The new version v1.0.2b will contain all the fixes you provided with the dependency x-wr-timezone updated.

  • 👍1
NicoHood commented

Hi,
I just upgraded your library. Just to recap: x-wr timezone is now a hard dependency and always applies. No further interaction required?

On the top of the readme you have a feature list, I suggest to add the information/feature there as well.

niccokunzmann commented

Thanks, yes that sounds good.

niccokunzmann commented

@NicoHood have a look, your PR is also welcome.

NicoHood commented

Love it!

NicoHood commented

Is it possible to get rid of the pytz dependency? Why dont we track it in an issue here?

niccokunzmann commented

Yep, you can open an issue. As long as icalendar depends on pytz, we cannot eradicate it. They also have an issue to link as dependency and even a pull request If you like to help them, I believe. Did we have one open already?

NicoHood commented

Didnt the upstream fix the pytz issue, so we could get rid of that dependency now?

niccokunzmann commented

collective/icalendar#294 is there as a PR. Still. icalendar depends on pytz. It might be possible to make this optional. I do not know how, yet. see also: #58 #78