mangstadt / biweekly

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

EXDATE parser shifts time by 2 hours #73

Closed gangeli closed 7 years ago

gangeli commented 7 years ago

The EXDATE parser seems to be shifting parsed times over by 2 hours -- perhaps ignoring daylight savings time somewhere? The following is a minimal test-case to illustrate the error:

String exdate =                  "19971011T090000Z";
Instant expected = Instant.parse("1997-10-11T09:00:00Z");

ICalParameters params = new ICalParameters();
ParseContext context = new ParseContext();
context.setVersion(ICalVersion.V2_0);
ExceptionDatesScribe exdateScribe = new ExceptionDatesScribe();

ExceptionDates dates = exdateScribe.parseText(exdate, null, params, context);
biweekly.util.ICalDate date = dates.getValues().get(0);

assertEquals(expected, date.toInstant());

The output of the test case is:

java.lang.AssertionError: 
Expected :1997-10-11T09:00:00Z
Actual   :1997-10-11T07:00:00Z
 <Click to see difference>

Let me know if I can help! If nothing else, I can help write fuzz tests :)

mangstadt commented 7 years ago

Thanks for posting. It's producing that output because a dataType is not being passed into the parseText method. If the dataType is null, then it does not process the time component of the timestamp string.

Below is the corrected unit test:

String exdate =                  "19971011T090000Z";
Instant expected = Instant.parse("1997-10-11T09:00:00Z");

ICalParameters params = new ICalParameters();
ParseContext context = new ParseContext();
context.setVersion(ICalVersion.V2_0);
ExceptionDatesScribe exdateScribe = new ExceptionDatesScribe();

ICalDataType dataType = params.getValue();
if (dataType == null) {
  dataType = exdateScribe.defaultDataType(context.getVersion());
}

ExceptionDates dates = exdateScribe.parseText(exdate, dataType, params, context);
biweekly.util.ICalDate date = dates.getValues().get(0);

assertEquals(expected, date.toInstant());

Everything works OK when you construct a complete iCalendar object:

String str = "BEGIN:VCALENDAR\r\nVERSION:2.0\r\nEXDATE:19971011T090000Z\r\nEND:VCALENDAR";
ICalendar ical = Biweekly.parse(str).first();
System.out.println(ical.getProperty(ExceptionDates.class).getValues().get(0).toInstant());
gangeli commented 7 years ago

Is this for compatibility issues? Would it make sense to make this work with default behavior?

mangstadt commented 7 years ago

Certain properties can take on different data types. If a property doesn't specify a data type, then every property has a default data type (according to the iCal specs). In addition, the default data type varies based on the iCalendar version.

For example, the EXDATE specification states:

   Value Type:  The default value type for this property is DATE-TIME.
      The value type can be set to DATE.
gangeli commented 7 years ago

Wait, unless I'm mistaken the default currently is DATE rather than DATE-TIME? I would expect that with a default of DATE-TIME parsing an EXDATE should be identical to the reference Instant. Perhaps I'm misunderstanding the code.

mangstadt commented 7 years ago

The line in question is:

boolean hasTime = (dataType == DATE_TIME);

Which is why it treats a null dataType as a DATE value. The method's not written to explicitly handle a null dataType value. The scribe classes aren't meant to be directly called by the programmer, so they don't check their inputs as vigorously.