mangstadt / biweekly

biweekly is an iCalendar library written in Java.
BSD 2-Clause "Simplified" License
323 stars 44 forks source link

Timezone issue in com.google.ical.compat.javautil.DateIteratorFactory #16

Closed jkwatson closed 8 years ago

jkwatson commented 8 years ago

The method:

  public static DateIterator createDateIterator(
      String rdata, Date start, TimeZone tzid, boolean strict)
      throws ParseException {
    return new RecurrenceIteratorWrapper(
        RecurrenceIteratorFactory.createRecurrenceIterator(
            rdata, dateToDateValue(start, true),
            tzid, strict));
  }

claims it will use the timezone to interpret the start parameter, but it does not actually do that. The GregorianCalendar that is created in dateToDateValue is hard-coded to UTC, and thus the start parameter is not properly handled if the tzid is not UTC.

The following unit test shows the issue:

    @Test
    public void testTimeZoneHandling() throws Exception {
        TimeZone timeZone = TimeZone.getTimeZone("America/Los_Angeles");
        Calendar calendar = Calendar.getInstance(timeZone, Locale.US);
        calendar.clear();
        calendar.set(2015, Calendar.JANUARY, 1, 0, 0, 0);
        Date start = calendar.getTime();
        DateIterator dateIterator = DateIteratorFactory.createDateIterator("RRULE:FREQ=DAILY;INTERVAL=1", start, timeZone, true);
        Date next = dateIterator.next();
        assertEquals(start, next);
    }

I believe that the RecurrenceIteratorWrapper and the RecurrenceIterableWrapper need to store the timezone, and use it when converting between internal date formats and java.util.Dates. The only method I'm not sure about is public static DateIterator createDateIterator(RecurrenceIterator rit), which would then require a timezone parameter, although adding one does not work properly.

As a work-around, I'm explicitly using createDateIterator(createRecurrenceIterator(rrule, start, timeZone, true)) and manually building the DateValue using the correct time zone for the start date.

Note: This unit test can be used to verify that public static DateIterator createDateIterator(RecurrenceIterator rit) continues to work correctly, with any changes that are made.

    @Test
    public void testTimeZones_viaRecurrenceIterator() throws Exception {
        TimeZone timeZone = TimeZone.getTimeZone("America/Los_Angeles");
        RecurrenceIterator recurrenceIterator = RecurrenceIteratorFactory.createRecurrenceIterator("RRULE:FREQ=DAILY;INTERVAL=1", new DateTimeValueImpl(2015, 1, 1, 0, 0, 1), timeZone, true);
        DateIterator dateIterator = DateIteratorFactory.createDateIterator(recurrenceIterator);
        Date next = dateIterator.next();

        Calendar calendar = Calendar.getInstance(timeZone, Locale.US);
        calendar.clear();
        calendar.set(2015, Calendar.JANUARY, 1, 0, 0, 1);
        Date start = calendar.getTime();
        assertEquals(start, next);
    }
mangstadt commented 8 years ago

Hi John,

Thanks for posting this. biweekly only uses the createDateIterator(RecurrenceIterator rit) method (line 96 of the RecurrenceProperty class), so it looks like there's nothing to worry about from biweekly's perspective. Thanks for alerting me to the problem though.

The code in the com.google.ical package comes from the google-rfc-2445 project, so you may want to report the issue to them. However, I'm pretty sure the project is inactive :( . The project is not hosted on Maven Central, which is why I put its source code in with biweekly's source code.

jkwatson commented 8 years ago

I think you might be the sole maintainer of the com.google.ical package now. :) Thanks for the response. In my project, I just need the recurrence rule processing, rather than the whole iCal processing, so I've only been using the stuff in com.google.ical. I'm working around this issue now, just fine, so if you want to leave the com.google.ical sleeping dog lie, then that's definitely your prerogative.

I may take some time to get this fixed, and if I do, I will definitely provide a PR for you.

mangstadt commented 8 years ago

I think you might be the sole maintainer of the com.google.ical package now. :)

Gee thanks, I'm honored. -_-

I may take some time to get this fixed, and if I do, I will definitely provide a PR for you.

I would appreciate it, thank you.