Workaround: don't UPDATE eventStatus to null (#85)

* Add behavior test for updating eventStatus from null to null

* Add issue reference

* [WIP] Another test

* Provide two workarounds and tests

* Fix AndroidRecurringCalendar

Author: rfc2822

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

@@ -55,8 +55,11 @@ class AndroidCalendarProviderBehaviorTest {
     }
 
 
+    /**
+     * Reported as https://issuetracker.google.com/issues/446730408.
+     */
     @Test(expected = NullPointerException::class)
-    fun testUpdateEventStatusToNull() {
+    fun testUpdateEventStatusFromNonNullToNull() {
         val id = calendar.addEvent(Entity(contentValuesOf(
             Events.CALENDAR_ID to calendar.id,
             Events.DTSTART to System.currentTimeMillis(),
@@ -71,4 +74,40 @@ class AndroidCalendarProviderBehaviorTest {
         ))
     }
 
+    @Test
+    fun testUpdateEventStatusFromNullToNotPresent() {
+        val id = calendar.addEvent(Entity(contentValuesOf(
+            Events.CALENDAR_ID to calendar.id,
+            Events.DTSTART to System.currentTimeMillis(),
+            Events.DTEND to System.currentTimeMillis() + 3600000,
+            Events.TITLE to "Some Event (Status tentative)",
+            Events.STATUS to null
+        )))
+
+        // No problem because STATUS is not explicitly set.
+        calendar.updateEventRow(id, contentValuesOf(
+            //Events.STATUS to null,
+            Events.TITLE to "Some Event (Status null)"
+        ))
+    }
+
+    /**
+     * Reported as https://issuetracker.google.com/issues/446730408.
+     */
+    @Test(expected = NullPointerException::class)
+    fun testUpdateEventStatusFromNullToNull() {
+        val id = calendar.addEvent(Entity(contentValuesOf(
+            Events.CALENDAR_ID to calendar.id,
+            Events.DTSTART to System.currentTimeMillis(),
+            Events.DTEND to System.currentTimeMillis() + 3600000,
+            Events.TITLE to "Some Event (Status tentative)",
+            Events.STATUS to null
+        )))
+
+        calendar.updateEventRow(id, contentValuesOf(
+            Events.STATUS to null,      // updating status to null causes NullPointerException
+            Events.TITLE to "Some Event (Status null)"
+        ))
+    }
+
 }

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

@@ -246,6 +246,52 @@ class AndroidCalendarTest {
         )
     }
 
+    @Test
+    fun testGetStatusUpdateWorkaround_NoStatusUpdate() {
+        assertEquals(
+            AndroidCalendar.StatusUpdateWorkaround.NO_WORKAROUND,
+            calendar.getStatusUpdateWorkaround(0, ContentValues())
+        )
+    }
+
+    @Test
+    fun testGetStatusUpdateWorkaround_UpdateStatusToNonNull() {
+        assertEquals(
+            AndroidCalendar.StatusUpdateWorkaround.NO_WORKAROUND,
+            calendar.getStatusUpdateWorkaround(0, contentValuesOf(Events.STATUS to Events.STATUS_TENTATIVE))
+        )
+    }
+
+    @Test
+    fun testGetStatusUpdateWorkaround_UpdateStatusFromNullToNull() {
+        val id = calendar.addEvent(Entity(contentValuesOf(
+            Events.CALENDAR_ID to calendar.id,
+            Events.DTSTART to now,
+            Events.DTEND to now + 3600000,
+            Events.TITLE to "Event without status",
+            Events.STATUS to null
+        )))
+        assertEquals(
+            AndroidCalendar.StatusUpdateWorkaround.DONT_UPDATE_STATUS,
+            calendar.getStatusUpdateWorkaround(id, contentValuesOf(Events.STATUS to null))
+        )
+    }
+
+    @Test
+    fun testGetStatusUpdateWorkaround_UpdateStatusFromNonNullToNull() {
+        val id = calendar.addEvent(Entity(contentValuesOf(
+            Events.CALENDAR_ID to calendar.id,
+            Events.DTSTART to now,
+            Events.DTEND to now + 3600000,
+            Events.TITLE to "Event without status",
+            Events.STATUS to Events.STATUS_TENTATIVE
+        )))
+        assertEquals(
+            AndroidCalendar.StatusUpdateWorkaround.REBUILD_EVENT,
+            calendar.getStatusUpdateWorkaround(id, contentValuesOf(Events.STATUS to null))
+        )
+    }
+
     @Test
     fun testUpdateEventRow() {
         val id = calendar.addEvent(Entity(contentValuesOf(
@@ -266,7 +312,8 @@ class AndroidCalendarTest {
             Events.CALENDAR_ID to calendar.id,
             Events.DTSTART to now,
             Events.DTEND to now + 3600000,
-            Events.TITLE to "Some Event"
+            Events.TITLE to "Some Event",
+            Events.STATUS to null
         )
         val entity = Entity(values)
         val reminder = contentValuesOf(

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

@@ -27,6 +27,7 @@ import io.mockk.verify
 import net.fortuna.ical4j.util.TimeZones
 import org.junit.After
 import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNotEquals
 import org.junit.Assert.assertNull
 import org.junit.Assert.assertTrue
 import org.junit.Before
@@ -190,7 +191,8 @@ class AndroidRecurringCalendarTest {
                 Events.ORIGINAL_SYNC_ID to "recur3",
                 Events.DTSTART to now + 86400000,
                 Events.DTEND to now + 86400000 + 2*3600000,
-                Events.TITLE to "Initial Exception"
+                Events.TITLE to "Initial Exception",
+                Events.STATUS to Events.STATUS_CONFIRMED
             ))
         )
         val initialEventAndExceptions = EventAndExceptions(main = initialEvent, exceptions = initialExceptions)
@@ -199,19 +201,22 @@ class AndroidRecurringCalendarTest {
         // Create updated event with null STATUS (requires rebuild)
         val updatedEvent = Entity(initialEvent.entityValues.apply {
             put(Events.TITLE, "Updated Event")
-            remove(Events.STATUS)
+            putNull(Events.STATUS)
         })
         val updatedEventAndExceptions = EventAndExceptions(main = updatedEvent, exceptions = initialExceptions)
 
         // Update event (should trigger rebuild)
         val updatedEventId = recurringCalendar.updateEventAndExceptions(mainEventId, updatedEventAndExceptions)
+        assertNotEquals(mainEventId, updatedEventId)
 
         // Verify update = deletion + re-creation
         assertNull(recurringCalendar.getById(mainEventId))
-        val updatedEvent2 = recurringCalendar.getById(updatedEventId)
+        val updatedEvent2 = recurringCalendar.getById(updatedEventId)!!
+        if (!updatedEvent2.main.entityValues.containsKey(Events.STATUS))      // STATUS will not be returned if it's null
+            updatedEvent2.main.entityValues.putNull(Events.STATUS)      // add for equality check
         assertEventAndExceptionsEqual(
             updatedEventAndExceptions.withId(updatedEventId),
-            updatedEvent2!!,
+            updatedEvent2,
             onlyFieldsInExpected = true
         )
     }

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

@@ -295,7 +295,7 @@ class AndroidCalendar(
      * Updates a specific event's main row with the given values. Doesn't influence data rows.
      *
      * This method always uses the update method of the content provider and does not
-     * re-create rows, as it is required for some operations (see [updateEvent] and [eventUpdateNeedsRebuild]
+     * re-create rows, as it is required for some operations (see [updateEvent] and [getStatusUpdateWorkaround]
      * for more information).
      *
      * @param id        event ID
@@ -311,40 +311,81 @@ class AndroidCalendar(
         }
     }
 
+    /**
+     * Updates an event and applies the eventStatus=null workaround, if necessary.
+     *
+     * While the event row can be updated, sub-values (data rows) are always deleted and created from scratch.
+     *
+     * @param id        ID of the event to update
+     * @param entity    new values of the event
+     *
+     * @return ID of the updated event (not necessarily the same as the original event)
+     */
     fun updateEvent(id: Long, entity: Entity): Long {
         try {
-            val rebuild = eventUpdateNeedsRebuild(id, entity.entityValues) ?: true
-            if (rebuild) {
-                deleteEvent(id)
-                return addEvent(entity)
-            }
-
-            // remove existing data rows which are created by us (don't touch 3rd-party calendar apps rows)
             val batch = CalendarBatchOperation(client)
-            updateEvent(id, entity, batch)
+            val newEventIdIdx = updateEvent(id, entity, batch)
             batch.commit()
 
-            return id
+            if (newEventIdIdx == null)
+                // event was updated
+                return id
+            else {
+                // event was re-built
+                val result = batch.getResult(newEventIdIdx)
+                val newEventUri = result?.uri ?: throw LocalStorageException("Content provider returned null on insert")
+                return ContentUris.parseId(newEventUri)
+            }
         } catch (e: RemoteException) {
             throw LocalStorageException("Couldn't update event $id", e)
         }
     }
 
-    internal fun updateEvent(id: Long, entity: Entity, batch: CalendarBatchOperation) {
+    /**
+     * Enqueues an update of an event and applies the eventStatus=null workaround, if necessary.
+     *
+     * While the event row can be updated, sub-values (data rows) are always deleted and created from scratch.
+     *
+     * @param id        ID of the event to update
+     * @param batch     batch operation in which the update is enqueued
+     * @param entity    new values of the event
+     *
+     * @return `null` if an event update was enqueued so that its ID won't change;
+     * otherwise (if re-build is needed) the result index of the new event ID.
+     */
+    internal fun updateEvent(id: Long, entity: Entity, batch: CalendarBatchOperation): Int? {
+        val workaround = getStatusUpdateWorkaround(id, entity.entityValues)
+        if (workaround == StatusUpdateWorkaround.REBUILD_EVENT) {
+            deleteEvent(id, batch)
+
+            val idx = batch.nextBackrefIdx()
+            addEvent(entity, batch)
+            return idx
+        }
+
+        // remove existing data rows which are created by us (don't touch 3rd-party calendar apps rows)
         deleteDataRows(id, batch)
 
         // update main row
+        val newValues = ContentValues(entity.entityValues).apply {
+            // don't update event ID
+            remove(Events._ID)
+
+            // don't update status if that is our required workaround
+            if (workaround == StatusUpdateWorkaround.DONT_UPDATE_STATUS)
+                remove(Events.STATUS)
+        }
         batch += CpoBuilder.newUpdate(eventUri(id))
-            .withValues(ContentValues(entity.entityValues).apply {
-                remove(Events._ID)  // don't update ID
-            })
+            .withValues(newValues)
 
         // insert data rows (with reference to main row ID)
         for (row in entity.subValues)
             batch += CpoBuilder.newInsert(row.uri.asSyncAdapter(account))
                 .withValues(ContentValues(row.values).apply {
                     put(AndroidEvent2.DATA_ROW_EVENT_ID, id)      // always keep reference to main row ID
                 })
+
+        return null
     }
 
     /**
@@ -375,7 +416,7 @@ class AndroidCalendar(
 
     /**
      * There is a bug in the calendar provider that prevent events from being updated from a non-null STATUS value
-     * to STATUS=null (see AndroidCalendarProviderBehaviorTest.testUpdateEventStatusToNull).
+     * to STATUS=null (see `AndroidCalendarProviderBehaviorTest` test class).
      *
      * In that case we can't update the event, so we completely re-create it.
      *
@@ -384,9 +425,20 @@ class AndroidCalendar(
      *
      * @return whether the event can't be updated/needs to be re-created; or `null` if existing values couldn't be determined
      */
-    internal fun eventUpdateNeedsRebuild(id: Long, newValues: ContentValues): Boolean? {
-        val existingValues = getEventRow(id, arrayOf(Events.STATUS)) ?: return null
-        return existingValues.getAsInteger(Events.STATUS) != null && newValues.getAsInteger(Events.STATUS) == null
+    internal fun getStatusUpdateWorkaround(id: Long, newValues: ContentValues): StatusUpdateWorkaround {
+        // No workaround needed if STATUS is a) not updated at all, or b) updated to a non-null value.
+        if (!newValues.containsKey(Events.STATUS) || newValues.getAsInteger(Events.STATUS) != null)
+            return StatusUpdateWorkaround.NO_WORKAROUND
+        // We're now sure that STATUS shall be updated to null.
+
+        // If STATUS is null before the update, just don't include the STATUS in the update.
+        // In case that the old values can't be determined, rebuild the row to be on the safe side.
+        val existingValues = getEventRow(id, arrayOf(Events.STATUS)) ?: return StatusUpdateWorkaround.REBUILD_EVENT
+        if (existingValues.getAsInteger(Events.STATUS) == null)
+            return StatusUpdateWorkaround.DONT_UPDATE_STATUS
+
+        // Update from non-null to null → rebuild (delete/insert) event instead of updating it.
+        return StatusUpdateWorkaround.REBUILD_EVENT
     }
 
     /**
@@ -422,6 +474,10 @@ class AndroidCalendar(
         }
     }
 
+    internal fun deleteEvent(id: Long, batch: CalendarBatchOperation) {
+        batch += CpoBuilder.newDelete(eventUri(id))
+    }
+
 
 
     // event instances (these methods operate directly with event IDs and without the events
@@ -516,6 +572,15 @@ class AndroidCalendar(
 
     // helpers
 
+    enum class StatusUpdateWorkaround {
+        /** no workaround needed */
+        NO_WORKAROUND,
+        /** don't update eventStatus (no need to change value) */
+        DONT_UPDATE_STATUS,
+        /** rebuild event (delete+insert instead of update) */
+        REBUILD_EVENT
+    }
+
     val account
         get() = provider.account
 

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

@@ -93,32 +93,32 @@ class AndroidRecurringCalendar(
      */
     fun updateEventAndExceptions(id: Long, eventAndExceptions: EventAndExceptions): Long {
         try {
-            val rebuild = calendar.eventUpdateNeedsRebuild(id, eventAndExceptions.main.entityValues) ?: true
-            if (rebuild) {
-                deleteEventAndExceptions(id)
-                return addEventAndExceptions(eventAndExceptions)
-            }
-
             // validate / clean up input
             val cleaned = cleanUp(eventAndExceptions)
 
-            val batch = CalendarBatchOperation(calendar.client)
-
             // remove old exceptions (because they may be invalid for the updated event)
+            val batch = CalendarBatchOperation(calendar.client)
             batch += CpoBuilder.newDelete(calendar.eventsUri)
                 .withSelection("${Events.ORIGINAL_ID}=?", arrayOf(id.toString()))
 
-            // update main event
-            calendar.updateEvent(id, cleaned.main, batch)
+            // update main event (also applies eventStatus workaround, if needed)
+            val newEventIdIdx = calendar.updateEvent(id, cleaned.main, batch)
 
             // add updated exceptions
             for (exception in cleaned.exceptions)
                 calendar.addEvent(exception, batch)
 
             batch.commit()
 
-            // original row was updated, so return original ID
-            return id
+            if (newEventIdIdx == null) {
+                // original row was updated, so return original ID
+                return id
+            } else {
+                // event was re-built
+                val result = batch.getResult(newEventIdIdx)
+                val newEventUri = result?.uri ?: throw LocalStorageException("Content provider returned null on insert")
+                return ContentUris.parseId(newEventUri)
+            }
         } catch (e: RemoteException) {
             throw LocalStorageException("Couldn't update event/exceptions", e)
         }