From c7fe069b1fa82e5551c4be04632b9ef32485226c Mon Sep 17 00:00:00 2001 From: rfc2822 Date: Sat, 8 Feb 2014 18:53:31 +0100 Subject: [PATCH] Better DTSTART/DTEND handling * generalized InvalidResourceException for parsing errors * only iCals with both DtStart and DtEnd/Duration are processed (DtEnd will be derived by iCal4j when not present in .ics) * all-day events must last at least one day (fixes #166) * other DtEnd/Duration rewriting + tests --- .../strings-es.xml => values-es/strings.xml} | 0 src/at/bitfire/davdroid/resource/Event.java | 88 ++++++++++--------- .../resource/InvalidResourceException.java | 13 +++ .../davdroid/resource/LocalCalendar.java | 53 ++++------- .../davdroid/resource/RemoteCollection.java | 16 ++-- .../bitfire/davdroid/resource/Resource.java | 7 +- .../CalendarsSyncAdapterService.java | 3 +- test/assets/all-day-0sec.ics | 11 +++ test/assets/all-day-10days.ics | 11 +++ test/assets/all-day-1day.ics | 11 +++ test/assets/event-on-that-day.ics | 11 +++ .../davdroid/resource/test/EventTest.java | 83 +++++++++++++++++ .../bitfire/davdroid/test/CalendarTest.java | 42 --------- 13 files changed, 211 insertions(+), 138 deletions(-) rename res/{values/strings-es.xml => values-es/strings.xml} (100%) create mode 100644 src/at/bitfire/davdroid/resource/InvalidResourceException.java create mode 100644 test/assets/all-day-0sec.ics create mode 100644 test/assets/all-day-10days.ics create mode 100644 test/assets/all-day-1day.ics create mode 100644 test/assets/event-on-that-day.ics create mode 100644 test/src/at/bitfire/davdroid/resource/test/EventTest.java delete mode 100644 test/src/at/bitfire/davdroid/test/CalendarTest.java diff --git a/res/values/strings-es.xml b/res/values-es/strings.xml similarity index 100% rename from res/values/strings-es.xml rename to res/values-es/strings.xml diff --git a/src/at/bitfire/davdroid/resource/Event.java b/src/at/bitfire/davdroid/resource/Event.java index 06b719ee..344d8f78 100644 --- a/src/at/bitfire/davdroid/resource/Event.java +++ b/src/at/bitfire/davdroid/resource/Event.java @@ -122,11 +122,17 @@ public class Event extends Resource { @Override @SuppressWarnings("unchecked") - public void parseEntity(@NonNull InputStream entity) throws IOException, ParserException { - CalendarBuilder builder = new CalendarBuilder(); - net.fortuna.ical4j.model.Calendar ical = builder.build(entity); - if (ical == null) - return; + public void parseEntity(@NonNull InputStream entity) throws IOException, InvalidResourceException { + net.fortuna.ical4j.model.Calendar ical; + try { + CalendarBuilder builder = new CalendarBuilder(); + ical = builder.build(entity); + + if (ical == null) + throw new InvalidResourceException("No iCalendar found"); + } catch (ParserException e) { + throw new InvalidResourceException(e); + } // event ComponentList events = ical.getComponents(Component.VEVENT); @@ -141,10 +147,25 @@ public class Event extends Resource { generateUID(); } - dtStart = event.getStartDate(); validateTimeZone(dtStart); - dtEnd = event.getEndDate(); validateTimeZone(dtEnd); + if ((dtStart = event.getStartDate()) == null || (dtEnd = event.getEndDate()) == null) + throw new InvalidResourceException("Invalid start time/end time/duration"); + + if (hasTime(dtStart)) { + validateTimeZone(dtStart); + validateTimeZone(dtEnd); + } + + // all-day events and "events on that day": + // * related UNIX times must be in UTC + // * must have a duration (set to one day if missing) + if (!hasTime(dtStart) && !dtEnd.getDate().after(dtStart.getDate())) { + Log.i(TAG, "Repairing iCal: DTEND := DTSTART+1"); + Calendar c = Calendar.getInstance(TimeZone.getTimeZone(Time.TIMEZONE_UTC)); + c.setTime(dtStart.getDate()); + c.add(Calendar.DATE, 1); + dtEnd.setDate(new Date(c.getTimeInMillis())); + } - duration = event.getDuration(); rrule = (RRule)event.getProperty(Property.RRULE); rdate = (RDate)event.getProperty(Property.RDATE); exrule = (ExRule)event.getProperty(Property.EXRULE); @@ -180,7 +201,7 @@ public class Event extends Resource { @Override @SuppressWarnings("unchecked") - public ByteArrayOutputStream toEntity() throws IOException, ValidationException { + public ByteArrayOutputStream toEntity() throws IOException { net.fortuna.ical4j.model.Calendar ical = new net.fortuna.ical4j.model.Calendar(); ical.getProperties().add(Version.VERSION_2_0); ical.getProperties().add(new ProdId("-//bitfire web engineering//DAVdroid " + Constants.APP_VERSION + "//EN")); @@ -241,13 +262,17 @@ public class Event extends Resource { CalendarOutputter output = new CalendarOutputter(false); ByteArrayOutputStream os = new ByteArrayOutputStream(); - output.output(ical, os); + try { + output.output(ical, os); + } catch (ValidationException e) { + Log.e(TAG, "Generated invalid iCalendar"); + } return os; } public long getDtStartInMillis() { - return (dtStart != null && dtStart.getDate() != null) ? dtStart.getDate().getTime() : 0; + return dtStart.getDate().getTime(); } public String getDtStartTzID() { @@ -265,18 +290,7 @@ public class Event extends Resource { } - public Long getDtEndInMillis() { - if (hasNoTime(dtStart) && dtEnd == null) { // "event on that day" - // dtEnd = dtStart + 1 day - Calendar c = Calendar.getInstance(TimeZone.getTimeZone(Time.TIMEZONE_UTC)); - c.setTime(dtStart.getDate()); - c.add(Calendar.DATE, 1); - return c.getTimeInMillis(); - - } else if (dtEnd == null || dtEnd.getDate() == null) { // no DTEND provided (maybe DURATION instead) - return null; - } - + public long getDtEndInMillis() { return dtEnd.getDate().getTime(); } @@ -298,40 +312,28 @@ public class Event extends Resource { // helpers public boolean isAllDay() { - if (hasNoTime(dtStart)) { - // events on that day - if (dtEnd == null) - return true; - - // all-day events - if (hasNoTime(dtEnd)) - return true; - } - return false; + return !hasTime(dtStart); } - protected static boolean hasNoTime(DateProperty date) { - if (date == null) - return false; - return !(date.getDate() instanceof DateTime); + protected static boolean hasTime(DateProperty date) { + return date.getDate() instanceof DateTime; } protected static String getTzId(DateProperty date) { - if (date == null) - return null; - - if (hasNoTime(date) || date.isUtc()) + if (date.isUtc() || !hasTime(date)) return Time.TIMEZONE_UTC; else if (date.getTimeZone() != null) return date.getTimeZone().getID(); else if (date.getParameter(Value.TZID) != null) return date.getParameter(Value.TZID).getValue(); - return null; + + // fallback + return Time.TIMEZONE_UTC; } /* guess matching Android timezone ID */ protected static void validateTimeZone(DateProperty date) { - if (date == null || date.isUtc() || hasNoTime(date)) + if (date.isUtc() || !hasTime(date)) return; String tzID = getTzId(date); diff --git a/src/at/bitfire/davdroid/resource/InvalidResourceException.java b/src/at/bitfire/davdroid/resource/InvalidResourceException.java new file mode 100644 index 00000000..d5bb1fac --- /dev/null +++ b/src/at/bitfire/davdroid/resource/InvalidResourceException.java @@ -0,0 +1,13 @@ +package at.bitfire.davdroid.resource; + +public class InvalidResourceException extends Exception { + private static final long serialVersionUID = 1593585432655578220L; + + public InvalidResourceException(String message) { + super(message); + } + + public InvalidResourceException(Throwable throwable) { + super(throwable); + } +} diff --git a/src/at/bitfire/davdroid/resource/LocalCalendar.java b/src/at/bitfire/davdroid/resource/LocalCalendar.java index 6ae9c038..65c66320 100644 --- a/src/at/bitfire/davdroid/resource/LocalCalendar.java +++ b/src/at/bitfire/davdroid/resource/LocalCalendar.java @@ -13,7 +13,6 @@ package at.bitfire.davdroid.resource; import java.net.URI; import java.net.URISyntaxException; import java.text.ParseException; -import java.util.Date; import java.util.LinkedList; import java.util.List; import java.util.regex.Matcher; @@ -221,29 +220,25 @@ public class LocalCalendar extends LocalCollection { e.setLocation(cursor.getString(1)); e.setDescription(cursor.getString(2)); + boolean allDay = cursor.getInt(7) != 0; long tsStart = cursor.getLong(3), tsEnd = cursor.getLong(4); + String duration = cursor.getString(18); - String tzId; - if (cursor.getInt(7) != 0) { // ALL_DAY != 0 - tzId = null; // -> use UTC - } else { + String tzId = null; + if (!allDay) { // use the start time zone for the end time, too - // because the Samsung Planner UI allows the user to change the time zone - // but it will change the start time zone only + // because apps like Samsung Planner allow the user to change "the" time zone but change the start time zone only tzId = cursor.getString(5); - //tzIdEnd = cursor.getString(6); } e.setDtStart(tsStart, tzId); if (tsEnd != 0) e.setDtEnd(tsEnd, tzId); - + else if (!StringUtils.isEmpty(duration)) + e.setDuration(new Duration(new Dur(duration))); + // recurrence try { - String duration = cursor.getString(18); - if (!StringUtils.isEmpty(duration)) - e.setDuration(new Duration(new Dur(duration))); - String strRRule = cursor.getString(10); if (!StringUtils.isEmpty(strRRule)) e.setRrule(new RRule(strRRule)); @@ -436,30 +431,16 @@ public class LocalCalendar extends LocalCollection { if (event.getExdate() != null) builder = builder.withValue(Events.EXDATE, event.getExdate().getValue()); - // set DTEND for single-time events or DURATION for recurring events - // because that's the way Android likes it - if (!recurring) { - // not recurring: set DTEND - long dtEnd = 0; - String tzEnd = null; - if (event.getDtEndInMillis() != null) { - dtEnd = event.getDtEndInMillis(); - tzEnd = event.getDtEndTzID(); - } else if (event.getDuration() != null) { - Date dateEnd = event.getDuration().getDuration().getTime(event.getDtStart().getDate()); - dtEnd = dateEnd.getTime(); - } - builder = builder - .withValue(Events.DTEND, dtEnd) - .withValue(Events.EVENT_END_TIMEZONE, tzEnd); + // set either DTEND for single-time events or DURATION for recurring events + // because that's the way Android likes it (see docs) + if (recurring) { + // calculate DURATION from start and end date + Duration duration = new Duration(event.getDtStart().getDate(), event.getDtEnd().getDate()); + builder = builder.withValue(Events.DURATION, duration.getValue()); } else { - // recurring: set DURATION - String duration = null; - if (event.getDuration() != null) - duration = event.getDuration().getValue(); - else if (event.getDtEnd() != null) - duration = new Duration(event.getDtStart().getDate(), event.getDtEnd().getDate()).getValue(); - builder = builder.withValue(Events.DURATION, duration); + builder = builder + .withValue(Events.DTEND, event.getDtEndInMillis()) + .withValue(Events.EVENT_END_TIMEZONE, event.getDtEndTzID()); } if (event.getSummary() != null) diff --git a/src/at/bitfire/davdroid/resource/RemoteCollection.java b/src/at/bitfire/davdroid/resource/RemoteCollection.java index d26914e5..659ee285 100644 --- a/src/at/bitfire/davdroid/resource/RemoteCollection.java +++ b/src/at/bitfire/davdroid/resource/RemoteCollection.java @@ -21,7 +21,6 @@ import java.util.List; import lombok.Cleanup; import lombok.Getter; -import net.fortuna.ical4j.data.ParserException; import net.fortuna.ical4j.model.ValidationException; import org.apache.http.HttpException; @@ -33,7 +32,6 @@ import at.bitfire.davdroid.webdav.DavNoContentException; import at.bitfire.davdroid.webdav.HttpPropfind; import at.bitfire.davdroid.webdav.WebDavResource; import at.bitfire.davdroid.webdav.WebDavResource.PutMode; -import ezvcard.VCardException; public abstract class RemoteCollection { private static final String TAG = "davdroid.RemoteCollection"; @@ -98,18 +96,14 @@ public abstract class RemoteCollection { foundResources.add(resource); } else Log.e(TAG, "Ignoring entity without content"); - } catch (ParserException ex) { - Log.e(TAG, "Ignoring unparseable iCal in multi-response", ex); - } catch (VCardException ex) { - Log.e(TAG, "Ignoring unparseable vCard in multi-response", ex); + } catch (InvalidResourceException e) { + Log.e(TAG, "Ignoring unparseable entity in multi-response", e); } } return foundResources.toArray(new Resource[0]); - } catch (ParserException ex) { - Log.e(TAG, "Couldn't parse iCal from GET", ex); - } catch (VCardException ex) { - Log.e(TAG, "Couldn't parse vCard from GET", ex); + } catch (InvalidResourceException e) { + Log.e(TAG, "Couldn't parse entity from GET", e); } return new Resource[0]; @@ -118,7 +112,7 @@ public abstract class RemoteCollection { /* internal member operations */ - public Resource get(Resource resource) throws IOException, HttpException, ParserException, VCardException { + public Resource get(Resource resource) throws IOException, HttpException, InvalidResourceException { WebDavResource member = new WebDavResource(collection, resource.getName()); member.get(); diff --git a/src/at/bitfire/davdroid/resource/Resource.java b/src/at/bitfire/davdroid/resource/Resource.java index ff37b9fc..0b7e10be 100644 --- a/src/at/bitfire/davdroid/resource/Resource.java +++ b/src/at/bitfire/davdroid/resource/Resource.java @@ -17,9 +17,6 @@ import java.io.InputStream; import lombok.Getter; import lombok.Setter; import lombok.ToString; -import net.fortuna.ical4j.data.ParserException; -import net.fortuna.ical4j.model.ValidationException; -import ezvcard.VCardException; @ToString public abstract class Resource { @@ -42,6 +39,6 @@ public abstract class Resource { public abstract void generateUID(); public abstract void generateName(); - public abstract void parseEntity(InputStream entity) throws IOException, ParserException, VCardException; - public abstract ByteArrayOutputStream toEntity() throws IOException, ValidationException; + public abstract void parseEntity(InputStream entity) throws IOException, InvalidResourceException; + public abstract ByteArrayOutputStream toEntity() throws IOException; } diff --git a/src/at/bitfire/davdroid/syncadapter/CalendarsSyncAdapterService.java b/src/at/bitfire/davdroid/syncadapter/CalendarsSyncAdapterService.java index 6226d9d3..797e1f9c 100644 --- a/src/at/bitfire/davdroid/syncadapter/CalendarsSyncAdapterService.java +++ b/src/at/bitfire/davdroid/syncadapter/CalendarsSyncAdapterService.java @@ -57,7 +57,8 @@ public class CalendarsSyncAdapterService extends Service { Map, RemoteCollection> map = new HashMap, RemoteCollection>(); for (LocalCalendar calendar : LocalCalendar.findAll(account, provider)) { - URI uri = new URI(accountManager.getUserData(account, Constants.ACCOUNT_KEY_BASE_URL)).resolve(calendar.getPath()); + URI baseURI = new URI(accountManager.getUserData(account, Constants.ACCOUNT_KEY_BASE_URL)); + URI uri = baseURI.resolve(calendar.getPath()); RemoteCollection dav = new CalDavCalendar(uri.toString(), accountManager.getUserData(account, Constants.ACCOUNT_KEY_USERNAME), accountManager.getPassword(account), diff --git a/test/assets/all-day-0sec.ics b/test/assets/all-day-0sec.ics new file mode 100644 index 00000000..07679f29 --- /dev/null +++ b/test/assets/all-day-0sec.ics @@ -0,0 +1,11 @@ +BEGIN:VCALENDAR +VERSION:2.0 +PRODID:-//hacksw/handcal//NONSGML v1.0//EN +BEGIN:VEVENT +UID:all-day-0sec@example.com +DTSTAMP:20140101T000000Z +DTSTART;VALUE=DATE:19970714 +DTEND;VALUE=DATE:19970714 +SUMMARY:0 Sec Event +END:VEVENT +END:VCALENDAR \ No newline at end of file diff --git a/test/assets/all-day-10days.ics b/test/assets/all-day-10days.ics new file mode 100644 index 00000000..52e6dbdf --- /dev/null +++ b/test/assets/all-day-10days.ics @@ -0,0 +1,11 @@ +BEGIN:VCALENDAR +VERSION:2.0 +PRODID:-//hacksw/handcal//NONSGML v1.0//EN +BEGIN:VEVENT +UID:all-day-10days@example.com +DTSTAMP:20140101T000000Z +DTSTART;VALUE=DATE:19970714 +DTEND;VALUE=DATE:19970724 +SUMMARY:All-Day 10 Days +END:VEVENT +END:VCALENDAR \ No newline at end of file diff --git a/test/assets/all-day-1day.ics b/test/assets/all-day-1day.ics new file mode 100644 index 00000000..ead04361 --- /dev/null +++ b/test/assets/all-day-1day.ics @@ -0,0 +1,11 @@ +BEGIN:VCALENDAR +VERSION:2.0 +PRODID:-//hacksw/handcal//NONSGML v1.0//EN +BEGIN:VEVENT +UID:all-day-1day@example.com +DTSTAMP:20140101T000000Z +DTSTART;VALUE=DATE:19970714 +DTEND;VALUE=DATE:19970714 +SUMMARY:All-Day 1 Day +END:VEVENT +END:VCALENDAR \ No newline at end of file diff --git a/test/assets/event-on-that-day.ics b/test/assets/event-on-that-day.ics new file mode 100644 index 00000000..0ccbd4f3 --- /dev/null +++ b/test/assets/event-on-that-day.ics @@ -0,0 +1,11 @@ +BEGIN:VCALENDAR +VERSION:2.0 +PRODID:-//hacksw/handcal//NONSGML v1.0//EN +BEGIN:VEVENT +UID:event-on-that-day@example.com +DTSTAMP:19970714T170000Z +ORGANIZER;CN=John Doe:MAILTO:john.doe@example.com +DTSTART;VALUE=DATE:19970714 +SUMMARY:Bastille Day Party +END:VEVENT +END:VCALENDAR \ No newline at end of file diff --git a/test/src/at/bitfire/davdroid/resource/test/EventTest.java b/test/src/at/bitfire/davdroid/resource/test/EventTest.java new file mode 100644 index 00000000..c98d4f32 --- /dev/null +++ b/test/src/at/bitfire/davdroid/resource/test/EventTest.java @@ -0,0 +1,83 @@ +/******************************************************************************* + * Copyright (c) 2013 Richard Hirner (bitfire web engineering). + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the GNU Public License v3.0 + * which accompanies this distribution, and is available at + * http://www.gnu.org/licenses/gpl.html + ******************************************************************************/ +package at.bitfire.davdroid.resource.test; + +import java.io.IOException; +import java.io.InputStream; + +import lombok.Cleanup; +import net.fortuna.ical4j.data.ParserException; +import android.content.res.AssetManager; +import android.test.InstrumentationTestCase; +import android.text.format.Time; +import at.bitfire.davdroid.resource.Event; +import at.bitfire.davdroid.resource.InvalidResourceException; + +public class EventTest extends InstrumentationTestCase { + AssetManager assetMgr; + + Event eViennaEvolution, + eOnThatDay, eAllDay1Day, eAllDay10Days, eAllDay0Sec; + + public void setUp() throws IOException, InvalidResourceException { + assetMgr = getInstrumentation().getContext().getResources().getAssets(); + + eViennaEvolution = parseCalendar("vienna-evolution.ics"); + eOnThatDay = parseCalendar("event-on-that-day.ics"); + eAllDay1Day = parseCalendar("all-day-1day.ics"); + eAllDay10Days = parseCalendar("all-day-10days.ics"); + eAllDay0Sec = parseCalendar("all-day-0sec.ics"); + + //assertEquals("Test-Ereignis im schönen Wien", e.getSummary()); + } + + + public void testStartEndTimes() throws IOException, ParserException { + // event with start+end date-time + assertEquals(1381330800000L, eViennaEvolution.getDtStartInMillis()); + assertEquals("Europe/Vienna", eViennaEvolution.getDtStartTzID()); + assertEquals(1381334400000L, eViennaEvolution.getDtEndInMillis()); + assertEquals("Europe/Vienna", eViennaEvolution.getDtEndTzID()); + } + + public void testStartEndTimesAllDay() throws IOException, ParserException { + // event with start date only + assertEquals(868838400000L, eOnThatDay.getDtStartInMillis()); + assertEquals(Time.TIMEZONE_UTC, eOnThatDay.getDtStartTzID()); + // DTEND missing in VEVENT, must have been set to DTSTART+1 day + assertEquals(868838400000L + 86400000, eOnThatDay.getDtEndInMillis()); + assertEquals(Time.TIMEZONE_UTC, eOnThatDay.getDtEndTzID()); + + // event with start+end date for all-day event (one day) + assertEquals(868838400000L, eAllDay1Day.getDtStartInMillis()); + assertEquals(Time.TIMEZONE_UTC, eAllDay1Day.getDtStartTzID()); + assertEquals(868838400000L + 86400000, eAllDay1Day.getDtEndInMillis()); + assertEquals(Time.TIMEZONE_UTC, eAllDay1Day.getDtEndTzID()); + + // event with start+end date for all-day event (ten days) + assertEquals(868838400000L, eAllDay10Days.getDtStartInMillis()); + assertEquals(Time.TIMEZONE_UTC, eAllDay10Days.getDtStartTzID()); + assertEquals(868838400000L + 10*86400000, eAllDay10Days.getDtEndInMillis()); + assertEquals(Time.TIMEZONE_UTC, eAllDay10Days.getDtEndTzID()); + + // event with start+end date on some day (invalid 0 sec-event) + assertEquals(868838400000L, eAllDay0Sec.getDtStartInMillis()); + assertEquals(Time.TIMEZONE_UTC, eAllDay0Sec.getDtStartTzID()); + // DTEND invalid in VEVENT, must have been set to DTSTART+1 day + assertEquals(868838400000L + 86400000, eAllDay0Sec.getDtEndInMillis()); + assertEquals(Time.TIMEZONE_UTC, eAllDay0Sec.getDtEndTzID()); + } + + + protected Event parseCalendar(String fname) throws IOException, InvalidResourceException { + @Cleanup InputStream in = assetMgr.open(fname, AssetManager.ACCESS_STREAMING); + Event e = new Event(fname, null); + e.parseEntity(in); + return e; + } +} diff --git a/test/src/at/bitfire/davdroid/test/CalendarTest.java b/test/src/at/bitfire/davdroid/test/CalendarTest.java deleted file mode 100644 index b8b1d4b8..00000000 --- a/test/src/at/bitfire/davdroid/test/CalendarTest.java +++ /dev/null @@ -1,42 +0,0 @@ -/******************************************************************************* - * Copyright (c) 2013 Richard Hirner (bitfire web engineering). - * All rights reserved. This program and the accompanying materials - * are made available under the terms of the GNU Public License v3.0 - * which accompanies this distribution, and is available at - * http://www.gnu.org/licenses/gpl.html - ******************************************************************************/ -package at.bitfire.davdroid.test; - -import java.io.IOException; -import java.io.InputStream; - -import net.fortuna.ical4j.data.ParserException; -import android.content.res.AssetManager; -import android.test.InstrumentationTestCase; -import at.bitfire.davdroid.resource.Event; - -public class CalendarTest extends InstrumentationTestCase { - AssetManager assetMgr; - - public void setUp() { - assetMgr = getInstrumentation().getContext().getResources().getAssets(); - } - - - public void testTimeZonesByEvolution() throws IOException, ParserException { - Event e = parseCalendar("vienna-evolution.ics"); - assertEquals("Test-Ereignis im schönen Wien", e.getSummary()); - - //DTSTART;TZID=/freeassociation.sourceforge.net/Tzfile/Europe/Vienna:20131009T170000 - /*assertEquals(1381330800000L, e.getDtStartInMillis()); - assertEquals(1381334400000L, (long)e.getDtEndInMillis());*/ - } - - - protected Event parseCalendar(String fname) throws IOException, ParserException { - InputStream in = assetMgr.open(fname, AssetManager.ACCESS_STREAMING); - Event e = new Event(fname, null); - e.parseEntity(in); - return e; - } -}