collective / icalendar

icalendar parser library for Python
https://icalendar.readthedocs.io
Other
1k stars 172 forks source link

datetime values of length 15 fail to parse #188

Closed oppianmatt closed 7 years ago

oppianmatt commented 8 years ago

in prop.py:

    @classmethod
    def from_ical(cls, ical, timezone=None):
        if isinstance(ical, cls):
            return ical.dt
        u = ical.upper()
        if u.startswith(('P', '-P', '+P')):
            return vDuration.from_ical(ical)

        if len(ical) in (15, 16):
            return vDatetime.from_ical(ical, timezone=timezone)
        elif len(ical) == 8:
            return vDate.from_ical(ical)
        elif len(ical) in (6, 7):
            return vTime.from_ical(ical)
        else:
            raise ValueError(
                "Expected datetime, date, or time, got: '%s'" % ical
            )

If the len of ical is 15 like with DTSTART:20150804T00000Z it fails in:

    @staticmethod
    def from_ical(ical, timezone=None):
        tzinfo = None
        if timezone:
            try:
                tzinfo = pytz.timezone(timezone)
            except pytz.UnknownTimeZoneError:
                tzinfo = _timezone_cache.get(timezone, None)

        try:
            timetuple = (
                int(ical[:4]),  # year
                int(ical[4:6]),  # month
                int(ical[6:8]),  # day
                int(ical[9:11]),  # hour
                int(ical[11:13]),  # minute
                int(ical[13:15]),  # second
            )
            if tzinfo:
                return tzinfo.localize(datetime(*timetuple))
            elif not ical[15:]:
                return datetime(*timetuple)
            elif ical[15:16] == 'Z':
                return pytz.utc.localize(datetime(*timetuple))
            else:
                raise ValueError(ical)
        except:
            raise ValueError('Wrong datetime format: %s' % ical)

It raises a ValueError when it tries to parse the seconds int(ical[13:15]), # second from the value 0Z.

stlaz commented 8 years ago

Seems like a proper behavior to me. Time format requires 6 digits.

geier commented 8 years ago

I agree, this is working as intended, as specified in RFC 5545 Section 3.3.12 seconds have 2 digits.

oppianmatt commented 8 years ago

Whilst yes that's in the spec then why the check for 15 digits?

I guess it's 15 digits without the Z.

But there are feeds out in the wild that have time component with 5 digits.

We should stick to the spec when creating a feed but be liberal in accepting possibly not up to spec feeds.

Be conservative in what you do, be liberal in what you accept from others.

I am getting the feed from http://www.availabilitycalendar.com/ical/tyXNOLc7mQRPM0IfpPwb.ics

stlaz commented 8 years ago

Why 15 digits? The standard says so. It is probably wrong to have a magic constant there so you can create a pull request where you can call it DATETIME_LENGTH, for example.

The thing here is - the standards exist so that you may expect certain behavior at certain situations and then be able to represent the information you learnt because you know what it is. This is where your quote simply does now hold - standards tell you exactly what you should accept from the other and that you can simply ignore everything else because the one offering you your input does not respect the rules you both agreed on.

After all - what is 5 digits time string? Do 2 digits go for hours, 2 for minutes and 1 for approximate tens of seconds? Or is it 3 for hours and 2 for minutes and it represents a time from a certain epoch?

If you ask me, I would personally say that the error is actually on the availabilitycalendar.com side and you may need to contact them first. It is a common mistake when concatenating hours/minutes/seconds and forgetting to check whether the certain part is lower than 10.

untitaker commented 7 years ago

Closing as not being a bug in icalendar.