Make parsing and assumptions about UID/RECURRENCE-ID explicit

gradle/libs.versions.toml

@@ -26,6 +26,7 @@ guava = { module = "com.google.guava:guava", version.ref = "guava" }
 ical4j = { module = "org.mnode.ical4j:ical4j", version.ref = "ical4j" }
 junit = { module = "junit:junit", version.ref = "junit" }
 kotlin-stdlib = { module = "org.jetbrains.kotlin:kotlin-stdlib", version.ref = "kotlin" }
+mockk = { module = "io.mockk:mockk", version.ref = "mockk" }
 mockk-android = { module = "io.mockk:mockk-android", version.ref = "mockk" }
 slf4j-jdk = { module = "org.slf4j:slf4j-jdk14", version.ref = "slf4j" }
 

lib/build.gradle.kts

@@ -135,4 +135,5 @@ dependencies {
 
     // unit tests
     testImplementation(libs.junit)
+    testImplementation(libs.mockk)
 }

lib/src/main/kotlin/at/bitfire/ical4android/AndroidEvent.kt

@@ -38,6 +38,7 @@ import at.bitfire.ical4android.util.TimeApiExtensions.toLocalDate
 import at.bitfire.ical4android.util.TimeApiExtensions.toLocalTime
 import at.bitfire.ical4android.util.TimeApiExtensions.toRfc5545Duration
 import at.bitfire.ical4android.util.TimeApiExtensions.toZonedDateTime
+import at.bitfire.synctools.exception.InvalidLocalResourceException
 import at.bitfire.synctools.storage.BatchOperation.CpoBuilder
 import at.bitfire.synctools.storage.CalendarBatchOperation
 import at.bitfire.synctools.storage.LocalStorageException
@@ -215,7 +216,7 @@ class AndroidEvent(
         }
 
         val allDay = (row.getAsInteger(Events.ALL_DAY) ?: 0) != 0
-        val tsStart = row.getAsLong(Events.DTSTART) ?: throw LocalStorageException("Found event without DTSTART")
+        val tsStart = row.getAsLong(Events.DTSTART) ?: throw InvalidLocalResourceException("Found event without DTSTART")
 
         var tsEnd = row.getAsLong(Events.DTEND)
         var duration =   // only use DURATION of DTEND is not defined
@@ -785,7 +786,7 @@ class AndroidEvent(
     private fun buildEvent(recurrence: Event?, builder: CpoBuilder) {
         val event = recurrence ?: requireNotNull(event)
 
-        val dtStart = event.dtStart ?: throw InvalidCalendarException("Events must have DTSTART")
+        val dtStart = event.dtStart ?: throw InvalidLocalResourceException("Events must have DTSTART")
         val allDay = DateUtils.isDate(dtStart)
 
         // make sure that time zone is supported by Android

lib/src/main/kotlin/at/bitfire/ical4android/Event.kt

@@ -9,6 +9,8 @@ package at.bitfire.ical4android
 import at.bitfire.ical4android.ICalendar.Companion.CALENDAR_NAME
 import at.bitfire.ical4android.util.DateUtils.isDateTime
 import at.bitfire.ical4android.validation.EventValidator
+import at.bitfire.synctools.exception.InvalidLocalResourceException
+import at.bitfire.synctools.icalendar.CalendarUidSplitter
 import net.fortuna.ical4j.data.CalendarOutputter
 import net.fortuna.ical4j.data.ParserException
 import net.fortuna.ical4j.model.Calendar
@@ -108,7 +110,7 @@ data class Event(
         ical.properties += Version.VERSION_2_0
         ical.properties += prodId.withUserAgents(userAgents)
 
-        val dtStart = dtStart ?: throw InvalidCalendarException("Won't generate event without start time")
+        val dtStart = dtStart ?: throw InvalidLocalResourceException("Won't generate event without start time")
 
         EventValidator.repair(this)     // repair this event before creating the VEVENT
 
@@ -258,85 +260,49 @@ data class Event(
             val ical = fromReader(reader, properties)
 
             // process VEVENTs
-            val vEvents = ical.getComponents<VEvent>(Component.VEVENT)
+            val splitter = CalendarUidSplitter<VEvent>()
+            val vEventsByUid = splitter.associateByUid(ical, Component.VEVENT)
 
-            // make sure every event has an UID
-            for (vEvent in vEvents)
-                if (vEvent.uid == null) {
-                    val uid = Uid(UUID.randomUUID().toString())
-                    logger.warning("Found VEVENT without UID, using a random one: ${uid.value}")
-                    vEvent.properties += uid
-                }
-
-            logger.fine("Assigning exceptions to main events")
-            val mainEvents = mutableMapOf<String /* UID */, VEvent>()
-            val exceptions = mutableMapOf<String /* UID */, MutableMap<String /* RECURRENCE-ID */, VEvent>>()
-            for (vEvent in vEvents) {
-                val uid = vEvent.uid.value
-                val sequence = vEvent.sequence?.sequenceNo ?: 0
-
-                if (vEvent.recurrenceId == null) {
-                    // main event (no RECURRENCE-ID)
-
-                    // If there are multiple entries, compare SEQUENCE and use the one with higher SEQUENCE.
-                    // If the SEQUENCE is identical, use latest version.
-                    val event = mainEvents[uid]
-                    if (event == null || (event.sequence != null && sequence >= event.sequence.sequenceNo))
-                        mainEvents[uid] = vEvent
-
-                } else {
-                    // exception (RECURRENCE-ID)
-                    var ex = exceptions[uid]
-                    // first index level: UID
-                    if (ex == null) {
-                        ex = mutableMapOf()
-                        exceptions[uid] = ex
-                    }
-                    // second index level: RECURRENCE-ID
-                    val recurrenceID = vEvent.recurrenceId.value
-                    val event = ex[recurrenceID]
-                    if (event == null || (event.sequence != null && sequence >= event.sequence.sequenceNo))
-                        ex[recurrenceID] = vEvent
-                }
-            }
-
-            /* There may be UIDs which have only RECURRENCE-ID entries and not a main entry (for instance, a recurring
+            /* Note: There may be UIDs which have only RECURRENCE-ID entries and not a main entry (for instance, a recurring
             event with an exception where the current user has been invited only to this exception. In this case,
             the UID will not appear in mainEvents but only in exceptions. */
 
-            val events = mutableListOf<Event>()
-            for ((uid, vEvent) in mainEvents) {
-                val event = fromVEvent(vEvent)
+            // make sure every event has an UID
+            vEventsByUid[null]?.let { withoutUid ->
+                val uid = Uid(UUID.randomUUID().toString())
+                logger.warning("Found VEVENT without UID, using a random one: ${uid.value}")
+                withoutUid.main?.properties?.add(uid)
+                withoutUid.exceptions.forEach { it.properties.add(uid) }
+            }
 
-                // assign exceptions to main event and then remove them from exceptions array
-                exceptions.remove(uid)?.let { eventExceptions ->
-                    event.exceptions.addAll(eventExceptions.values.map { fromVEvent(it) })
+            // convert into Events (data class)
+            val events = mutableListOf<Event>()
+            for (associatedEvents in vEventsByUid.values) {
+                val mainVEvent = associatedEvents.main ?:
+                    // no main event but only exceptions, create fake main event
+                    // FIXME: we should construct a proper recurring fake event, not just take first the exception
+                    associatedEvents.exceptions.first()
+
+                val event = fromVEvent(mainVEvent)
+                associatedEvents.exceptions.mapTo(event.exceptions) { exceptionVEvent ->
+                    fromVEvent(exceptionVEvent).also { exception ->
+                        // make sure that exceptions have at least a SUMMARY (if the main event does have one)
+                        if (exception.summary == null)
+                            exception.summary = event.summary
+                    }
                 }
 
-                // make sure that exceptions have at least a SUMMARY
-                event.exceptions.forEach { it.summary = it.summary ?: event.summary }
-
                 events += event
             }
 
-            for ((uid, onlyExceptions) in exceptions) {
-                logger.info("UID $uid doesn't have a main event but only exceptions: $onlyExceptions")
-
-                // create a fake main event from the first exception
-                val fakeEvent = fromVEvent(onlyExceptions.values.first())
-                fakeEvent.exceptions.addAll(onlyExceptions.values.map { fromVEvent(it) })
-
-                events += fakeEvent
-            }
-
             // Try to repair all events after reading the whole iCalendar
             for (event in events)
                 EventValidator.repair(event)
 
             return events
         }
 
-        private fun fromVEvent(event: VEvent): Event {
+        fun fromVEvent(event: VEvent): Event {
             val e = Event()
 
             // sequence must only be null for locally created, not-yet-synchronized events

lib/src/main/kotlin/at/bitfire/ical4android/ICalendar.kt

@@ -9,17 +9,16 @@ package at.bitfire.ical4android
 import at.bitfire.ical4android.ICalendar.Companion.CALENDAR_NAME
 import at.bitfire.ical4android.validation.ICalPreprocessor
 import at.bitfire.synctools.BuildConfig
+import at.bitfire.synctools.exception.InvalidRemoteResourceException
+import at.bitfire.synctools.icalendar.ICalendarParser
 import net.fortuna.ical4j.data.CalendarBuilder
-import net.fortuna.ical4j.data.CalendarParserFactory
-import net.fortuna.ical4j.data.ContentHandlerContext
 import net.fortuna.ical4j.data.ParserException
 import net.fortuna.ical4j.model.Calendar
 import net.fortuna.ical4j.model.ComponentList
 import net.fortuna.ical4j.model.Date
 import net.fortuna.ical4j.model.Parameter
 import net.fortuna.ical4j.model.Property
 import net.fortuna.ical4j.model.PropertyList
-import net.fortuna.ical4j.model.TimeZoneRegistryFactory
 import net.fortuna.ical4j.model.component.Daylight
 import net.fortuna.ical4j.model.component.Observance
 import net.fortuna.ical4j.model.component.Standard
@@ -89,35 +88,14 @@ open class ICalendar {
          * @param properties Known iCalendar properties (like [CALENDAR_NAME]) will be put into this map. Key: property name; value: property value
          *
          * @return parsed iCalendar resource
-         * @throws ParserException when the iCalendar can't be parsed
-         * @throws IllegalArgumentException when the iCalendar resource contains an invalid value
+         *
+         * @throws InvalidRemoteResourceException when the iCalendar can't be parsed
          */
+        @Deprecated("Use ICalendarParser directly")
         fun fromReader(reader: Reader, properties: MutableMap<String, String>? = null): Calendar {
             logger.fine("Parsing iCalendar stream")
 
-            // preprocess stream to work around some problems that can't be fixed later
-            val preprocessed = ICalPreprocessor.preprocessStream(reader)
-
-            // parse stream
-            val calendar: Calendar
-            try {
-                calendar = CalendarBuilder(
-                    CalendarParserFactory.getInstance().get(),
-                    ContentHandlerContext().withSupressInvalidProperties(true),
-                    TimeZoneRegistryFactory.getInstance().createRegistry()      // AndroidCompatTimeZoneRegistry
-                ).build(preprocessed)
-            } catch(e: ParserException) {
-                throw InvalidCalendarException("Couldn't parse iCalendar", e)
-            } catch(e: IllegalArgumentException) {
-                throw InvalidCalendarException("iCalendar contains invalid value", e)
-            }
-
-            // apply ICalPreprocessor for increased compatibility
-            try {
-                ICalPreprocessor.preprocessCalendar(calendar)
-            } catch (e: Exception) {
-                logger.log(Level.WARNING, "Couldn't pre-process iCalendar", e)
-            }
+            val calendar = ICalendarParser().parse(reader)
 
             // fill calendar properties
             properties?.let {

lib/src/main/kotlin/at/bitfire/ical4android/JtxICalObject.kt

@@ -14,13 +14,13 @@ import android.os.ParcelFileDescriptor
 import android.util.Base64
 import at.bitfire.ical4android.ICalendar.Companion.withUserAgents
 import at.bitfire.ical4android.util.MiscUtils.toValues
+import at.bitfire.synctools.exception.InvalidRemoteResourceException
 import at.bitfire.synctools.storage.BatchOperation
 import at.bitfire.synctools.storage.JtxBatchOperation
 import at.techbee.jtx.JtxContract
 import at.techbee.jtx.JtxContract.JtxICalObject.TZ_ALLDAY
 import at.techbee.jtx.JtxContract.asSyncAdapter
 import net.fortuna.ical4j.data.CalendarOutputter
-import net.fortuna.ical4j.data.ParserException
 import net.fortuna.ical4j.model.Calendar
 import net.fortuna.ical4j.model.ComponentList
 import net.fortuna.ical4j.model.Date
@@ -283,8 +283,7 @@ open class JtxICalObject(
          *
          * @return array of filled [JtxICalObject] data objects (may have size 0)
          *
-         * @throws ParserException when the iCalendar can't be parsed
-         * @throws IllegalArgumentException when the iCalendar resource contains an invalid value
+         * @throws InvalidRemoteResourceException when the iCalendar can't be parsed
          * @throws IOException on I/O errors
          */
         fun fromReader(

lib/src/main/kotlin/at/bitfire/ical4android/Task.kt

@@ -8,8 +8,8 @@ package at.bitfire.ical4android
 
 import androidx.annotation.IntRange
 import at.bitfire.ical4android.util.DateUtils
+import at.bitfire.synctools.exception.InvalidRemoteResourceException
 import net.fortuna.ical4j.data.CalendarOutputter
-import net.fortuna.ical4j.data.ParserException
 import net.fortuna.ical4j.model.Calendar
 import net.fortuna.ical4j.model.Component
 import net.fortuna.ical4j.model.DateTime
@@ -106,8 +106,7 @@ data class Task(
          *
          * @return array of filled [Task] data objects (may have size 0)
          *
-         * @throws ParserException when the iCalendar can't be parsed
-         * @throws IllegalArgumentException when the iCalendar resource contains an invalid value
+         * @throws InvalidRemoteResourceException when the iCalendar can't be parsed
          * @throws IOException on I/O errors
          */
         fun tasksFromReader(reader: Reader): List<Task> {

lib/src/main/kotlin/at/bitfire/synctools/exception/InvalidLocalResourceException.kt

@@ -0,0 +1,17 @@
+/*
+ * This file is part of bitfireAT/synctools which is released under GPLv3.
+ * Copyright © All Contributors. See the LICENSE and AUTHOR files in the root directory for details.
+ * SPDX-License-Identifier: GPL-3.0-or-later
+ */
+
+package at.bitfire.synctools.exception
+
+/**
+ * Represents an invalid local resource (for instance, an Android event).
+ */
+class InvalidLocalResourceException: InvalidResourceException {
+
+    constructor(message: String): super(message)
+    constructor(message: String, ex: Throwable): super(message, ex)
+
+}

lib/src/main/kotlin/at/bitfire/synctools/exception/InvalidRemoteResourceException.kt

@@ -0,0 +1,17 @@
+/*
+ * This file is part of bitfireAT/synctools which is released under GPLv3.
+ * Copyright © All Contributors. See the LICENSE and AUTHOR files in the root directory for details.
+ * SPDX-License-Identifier: GPL-3.0-or-later
+ */
+
+package at.bitfire.synctools.exception
+
+/**
+ * Represents an invalid remote resource (for instance, a calendar object resource).
+ */
+class InvalidRemoteResourceException: InvalidResourceException {
+
+    constructor(message: String): super(message)
+    constructor(message: String, ex: Throwable): super(message, ex)
+
+}

lib/src/main/kotlin/at/bitfire/ical4android/InvalidCalendarException.kt → lib/src/main/kotlin/at/bitfire/synctools/exception/InvalidResourceException.kt

@@ -4,9 +4,12 @@
  * SPDX-License-Identifier: GPL-3.0-or-later
  */
 
-package at.bitfire.ical4android
+package at.bitfire.synctools.exception
 
-class InvalidCalendarException: Exception {
+/**
+ * Represents an invalid resource.
+ */
+abstract class InvalidResourceException: Exception {
 
     constructor(message: String): super(message)
     constructor(message: String, ex: Throwable): super(message, ex)