AndroidRecurringCalendar: make validation / cleaning up more relaxed

Author: rfc2822

lib/src/androidTest/kotlin/at/bitfire/synctools/storage/calendar/AndroidRecurringCalendarTest.kt

@@ -18,18 +18,26 @@ import androidx.test.platform.app.InstrumentationRegistry
 import androidx.test.rule.GrantPermissionRule
 import at.bitfire.ical4android.impl.TestCalendar
 import at.bitfire.ical4android.util.MiscUtils.closeCompat
+import at.bitfire.synctools.test.assertContentValuesEqual
 import at.bitfire.synctools.test.assertEventAndExceptionsEqual
 import at.bitfire.synctools.test.withId
+import io.mockk.junit4.MockKRule
+import io.mockk.spyk
+import io.mockk.verify
 import net.fortuna.ical4j.util.TimeZones
 import org.junit.After
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertNull
+import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Rule
 import org.junit.Test
 
 class AndroidRecurringCalendarTest {
 
+    @get:Rule
+    val mockkRule = MockKRule(this)
+
     @get:Rule
     val permissonRule = GrantPermissionRule.grant(Manifest.permission.READ_CALENDAR, Manifest.permission.WRITE_CALENDAR)
 
@@ -38,6 +46,7 @@ class AndroidRecurringCalendarTest {
     lateinit var client: ContentProviderClient
     lateinit var provider: AndroidCalendarProvider
     lateinit var calendar: AndroidCalendar
+
     lateinit var recurringCalendar: AndroidRecurringCalendar
 
     @Before
@@ -46,7 +55,7 @@ class AndroidRecurringCalendarTest {
         client = context.contentResolver.acquireContentProviderClient(CalendarContract.AUTHORITY)!!
         provider = AndroidCalendarProvider(testAccount, client)
         calendar = TestCalendar.findOrCreate(testAccount, client)
-        recurringCalendar = AndroidRecurringCalendar(calendar)
+        recurringCalendar = spyk(AndroidRecurringCalendar(calendar))
     }
 
     @After
@@ -85,6 +94,11 @@ class AndroidRecurringCalendarTest {
         val mainEventId = recurringCalendar.addEventAndExceptions(event)
         val addedWithId = event.withId(mainEventId)
 
+        // verify that cleanUp was called
+        verify(exactly = 1) {
+            recurringCalendar.cleanUp(event)
+        }
+
         // verify
         val event2 = recurringCalendar.getById(mainEventId)
         assertEventAndExceptionsEqual(addedWithId, event2!!, onlyFieldsInExpected = true)
@@ -142,6 +156,11 @@ class AndroidRecurringCalendarTest {
         val updatedEventId = recurringCalendar.updateEventAndExceptions(addedEventId, updatedEventAndExceptions)
         assertEquals(updatedEventId, addedEventId)
 
+        // verify that cleanUp was called
+        verify(exactly = 1) {
+            recurringCalendar.cleanUp(updatedEventAndExceptions)
+        }
+
         // Verify update
         val event2 = recurringCalendar.getById(addedEventId)
         assertEventAndExceptionsEqual(
@@ -229,4 +248,89 @@ class AndroidRecurringCalendarTest {
         assertNull(deletedEvent)
     }
 
+
+    @Test
+    fun testCleanUp_Recurring_Exceptions_NoSyncId() {
+        val cleaned = recurringCalendar.cleanUp(EventAndExceptions(
+            main = Entity(contentValuesOf(
+                Events.TITLE to "Recurring Main Event",
+                Events.RRULE to "Some RRULE"
+            )),
+            exceptions = listOf(
+                Entity(contentValuesOf(
+                    Events.TITLE to "Exception"
+                ))
+            )
+        ))
+
+        // verify that exceptions were dropped (because the provider wouldn't be able to associate them without SYNC_ID)
+        assertTrue(cleaned.exceptions.isEmpty())
+    }
+
+    @Test
+    fun testCleanUp_Recurring_Exceptions_WithSyncId() {
+        val original = EventAndExceptions(
+            main = Entity(contentValuesOf(
+                Events._SYNC_ID to "SomeSyncId",
+                Events.TITLE to "Recurring Main Event",
+                Events.RRULE to "Some RRULE"
+            )),
+            exceptions = listOf(
+                Entity(contentValuesOf(
+                    Events.TITLE to "Exception",
+                    Events.ORIGINAL_SYNC_ID to "SomeSyncId"
+                ))
+            )
+        )
+        val cleaned = recurringCalendar.cleanUp(original)
+
+        // verify that cleanUp didn't modify anything
+        assertEventAndExceptionsEqual(original, cleaned)
+    }
+
+    @Test
+    fun testCleanUp_NotRecurring_Exceptions() {
+        val cleaned = recurringCalendar.cleanUp(EventAndExceptions(
+            main = Entity(contentValuesOf(
+                Events._SYNC_ID to "SomeSyncID",
+                Events.TITLE to "Non-Recurring Main Event"
+            )),
+            exceptions = listOf(
+                Entity(contentValuesOf(
+                    Events.TITLE to "Exception"
+                ))
+            )
+        ))
+
+        // verify that exceptions were dropped (because the main event is not recurring)
+        assertTrue(cleaned.exceptions.isEmpty())
+    }
+
+    @Test
+    fun testCleanMainEvent_RemovesOriginalFields() {
+        val result = recurringCalendar.cleanMainEvent(Entity(contentValuesOf(
+            Events.ORIGINAL_ID to "SomeValue",
+            Events.ORIGINAL_SYNC_ID to "SomeValue",
+            Events.ORIGINAL_INSTANCE_TIME to "SomeValue",
+            Events.ORIGINAL_ALL_DAY to "SomeValue"
+        )))
+        assertTrue(result.entityValues.isEmpty)
+    }
+
+    @Test
+    fun testCleanException_RemovesRecurrenceFields_AddsSyncId() {
+        val result = recurringCalendar.cleanException(Entity(contentValuesOf(
+            Events.RRULE to "SomeValue",
+            Events.RDATE to "SomeValue",
+            Events.EXRULE to "SomeValue",
+            Events.EXDATE to "SomeValue"
+        )), "SomeSyncID")
+
+        // all fields should have been dropped, but ORIGINAL_SYNC_ID should have been added
+        assertContentValuesEqual(
+            contentValuesOf(Events.ORIGINAL_SYNC_ID to "SomeSyncID"),
+            result.entityValues
+        )
+    }
+
 }

lib/src/main/kotlin/at/bitfire/synctools/storage/ContentValuesHelpers.kt

@@ -14,6 +14,9 @@ operator fun ContentValues.plusAssign(other: ContentValues) {
     putAll(other)
 }
 
+fun ContentValues.containsNotNull(field: String): Boolean =
+    getAsString(field) != null
+
 /**
  * Removes blank (empty or only white-space) [String] values from [ContentValues].
  *

lib/src/main/kotlin/at/bitfire/synctools/storage/calendar/AndroidCalendar.kt

@@ -97,8 +97,7 @@ class AndroidCalendar(
 
     fun addEvent(entity: Entity, batch: CalendarBatchOperation) {
         // insert main row
-        batch += CpoBuilder.newInsert(eventsUri)
-            .withValues(entity.entityValues)
+        batch += CpoBuilder.newInsert(eventsUri).withValues(entity.entityValues)
 
         // insert data rows (with reference to main row ID)
         for (row in entity.subValues)

lib/src/main/kotlin/at/bitfire/synctools/storage/calendar/AndroidRecurringCalendar.kt

@@ -7,37 +7,61 @@
 package at.bitfire.synctools.storage.calendar
 
 import android.content.ContentUris
+import android.content.ContentValues
+import android.content.Entity
 import android.os.RemoteException
 import android.provider.CalendarContract.Events
+import androidx.annotation.VisibleForTesting
 import at.bitfire.synctools.storage.BatchOperation.CpoBuilder
 import at.bitfire.synctools.storage.LocalStorageException
+import at.bitfire.synctools.storage.containsNotNull
 
 /**
- * Decorator for [AndroidCalendar] that adds support for [EventAndExceptions]
- * data objects.
+ * Adds support for [EventAndExceptions] data objects to [AndroidCalendar].
+ *
+ * There are basically two methods for inserting an exception event:
+ *
+ * 1. Insert it using [Events.CONTENT_EXCEPTION_URI] – then the calendar provider will take care
+ * of validating and cleaning up various fields, however it's then not possible to set some
+ * sync fields (all sync fields but [Events.SYNC_DATA1], [Events.SYNC_DATA3] and [Events.SYNC_DATA7] are filtered
+ * for some reason.) It also supports splitting the main event ("exception from this date"). Usually this method
+ * is used by calendar apps.
+ * 2. Insert it directly as normal event (using [Events.CONTENT_URI]). In this case [Events.ORIGINAL_SYNC_ID]
+ * must be set to the [Events._SYNC_ID] of the original event so that the calendar provider can associate the
+ * exception with the main event. It's not enough to set [Events.ORIGINAL_ID]!
+ *
+ * This class only uses the second method because it needs to support all sync fields.
  */
 class AndroidRecurringCalendar(
     val calendar: AndroidCalendar
 ) {
 
     /**
-     * Adds an event and all its exceptions.
+     * Inserts an event and all its exceptions. Input data is first cleaned up using [cleanUp].
+     *
+     * @param eventAndExceptions    event and exceptions to insert)
      *
      * @return ID of the resulting main event
+     *
+     * @throws IllegalArgumentException if [eventAndExceptions] has exceptions, but doesn't have a [Events._SYNC_ID]
      */
     fun addEventAndExceptions(eventAndExceptions: EventAndExceptions): Long {
         try {
             val batch = CalendarBatchOperation(calendar.client)
-            calendar.addEvent(eventAndExceptions.main, batch)
 
-            /* Add exceptions. We don't have to set ORIGINAL_ID of each exception to the ID of
-            the main event because the content provider associates events with their exceptions
-            using _SYNC_ID / ORIGINAL_SYNC_ID. */
-            for (exception in eventAndExceptions.exceptions)
+            // validate / clean up input
+            val cleaned = cleanUp(eventAndExceptions)
+
+            // add main event
+            calendar.addEvent(cleaned.main, batch)
+
+            // add exceptions
+            for (exception in cleaned.exceptions)
                 calendar.addEvent(exception, batch)
 
             batch.commit()
 
+            // main event was created as first row (index 0), return its insert result (= ID)
             val uri = batch.getResult(0)?.uri ?: throw LocalStorageException("Content provider returned null on insert")
             return ContentUris.parseId(uri)
         } catch (e: RemoteException) {
@@ -54,7 +78,8 @@ class AndroidRecurringCalendar(
     }
 
     /**
-     * Updates an event and all its exceptions.
+     * Updates an event and all its exceptions. Input data is first cleaned up using
+     * [cleanMainEvent] and [cleanException].
      *
      * @param id                    ID of the main event row
      * @param eventAndExceptions    new event (including exceptions)
@@ -69,13 +94,20 @@ class AndroidRecurringCalendar(
                 return addEventAndExceptions(eventAndExceptions)
             }
 
-            // update main event
+            // validate / clean up input
+            val cleaned = cleanUp(eventAndExceptions)
+
             val batch = CalendarBatchOperation(calendar.client)
-            calendar.updateEvent(id, eventAndExceptions.main, batch)
 
-            // remove and add exceptions again
-            batch += CpoBuilder.newDelete(calendar.eventsUri).withSelection("${Events.ORIGINAL_ID}=?", arrayOf(id.toString()))
-            for (exception in eventAndExceptions.exceptions)
+            // remove old exceptions (because they may be invalid for the updated event)
+            batch += CpoBuilder.newDelete(calendar.eventsUri)
+                .withSelection("${Events.ORIGINAL_ID}=?", arrayOf(id.toString()))
+
+            // update main event
+            calendar.updateEvent(id, cleaned.main, batch)
+
+            // add updated exceptions
+            for (exception in cleaned.exceptions)
                 calendar.addEvent(exception, batch)
 
             batch.commit()
@@ -110,4 +142,87 @@ class AndroidRecurringCalendar(
         }
     }
 
+
+    // validation / clean-up logic
+
+    @VisibleForTesting
+    internal fun cleanUp(original: EventAndExceptions): EventAndExceptions {
+        val main = cleanMainEvent(original.main)
+
+        val mainValues = main.entityValues
+        val syncId = mainValues.getAsString(Events._SYNC_ID)
+        val recurring = mainValues.containsNotNull(Events.RRULE) || mainValues.containsNotNull(Events.RDATE)
+
+        if (syncId == null || !recurring) {
+            // no sync id or main event not recurring → we can't / need to insert exceptions, so ignore them
+            return EventAndExceptions(main = main, exceptions = emptyList())
+        }
+
+        return EventAndExceptions(
+            main = main,
+            exceptions = original.exceptions.map { originalException ->
+                cleanException(originalException, syncId)
+            }
+        )
+    }
+
+    /**
+     * Prepares a main event for insertion into the calendar provider by making sure it
+     * doesn't have fields that a main event shouldn't have (original_...).
+     *
+     * @param original  original event to insert
+     *
+     * @return cleaned event that can actually be inserted
+     */
+    @VisibleForTesting
+    internal fun cleanMainEvent(original: Entity): Entity {
+        // make a copy (don't modify original entity / values)
+        val values = ContentValues(original.entityValues)
+
+        // remove values that a main event shouldn't have
+        val originalFields = arrayOf(
+            Events.ORIGINAL_ID, Events.ORIGINAL_SYNC_ID,
+            Events.ORIGINAL_INSTANCE_TIME, Events.ORIGINAL_ALL_DAY
+        )
+        for (field in originalFields)
+            values.remove(field)
+
+        // create new result with subvalues
+        val result = Entity(values)
+        for (subValue in original.subValues)
+            result.addSubValue(subValue.uri, subValue.values)
+        return result
+    }
+
+    /**
+     * Prepares an exception for insertion into the calendar provider:
+     *
+     * - Removes values that an exception shouldn't have (`RRULE`, `RDATE`, `EXRULE`, `EXDATE`).
+     * - Makes sure that the `ORIGINAL_SYNC_ID` is set to [syncId].
+     *
+     * @param original  original exception
+     * @param syncId    [Events._SYNC_ID] of the main event
+     *
+     * @return cleaned exception that can actually be inserted
+     */
+    @VisibleForTesting
+    internal fun cleanException(original: Entity, syncId: String): Entity {
+        // make a copy (don't modify original entity / values)
+        val values = ContentValues(original.entityValues)
+
+        // remove values that an exception shouldn't have
+        val recurrenceFields = arrayOf(Events.RRULE, Events.RDATE, Events.EXRULE, Events.EXDATE)
+        for (field in recurrenceFields)
+            values.remove(field)
+
+        // make sure that ORIGINAL_SYNC_ID is set so that the exception can be associated to the main event
+        values.put(Events.ORIGINAL_SYNC_ID, syncId)
+
+        // create new result with subvalues
+        val result = Entity(values)
+        for (subValue in original.subValues)
+            result.addSubValue(subValue.uri, subValue.values)
+        return result
+    }
+
 }