Always generate DTEND for compatibility (#157)

rfc2822 · web-flow · commit 2cc77a5701f5 · 2025-12-05T14:15:49.000+01:00
* Always generate DTEND for compatibility

- Update `AndroidTimeField` to handle null time zones
- Add `calculateFromDefault` method in `EndTimeHandler`
- Modify `DurationHandler` to skip if DTEND is present

* Tests for explicit negative durations

* Comments

* Update tests

* Test comments
diff --git a/lib/src/main/kotlin/at/bitfire/synctools/mapping/calendar/handler/AndroidTimeField.kt b/lib/src/main/kotlin/at/bitfire/synctools/mapping/calendar/handler/AndroidTimeField.kt
@@ -17,9 +17,9 @@ import java.time.ZoneId
  * Converts timestamps from the [android.provider.CalendarContract.Events.DTSTART] or [android.provider.CalendarContract.Events.DTEND]
  * fields into other representations.
  *
- * @param timestamp     value of the DTSTART/DTEND field (timestamp in milliseconds)
- * @param timeZone      value of the respective timezone field ([android.provider.CalendarContract.Events.EVENT_TIMEZONE] / [android.provider.CalendarContract.Events.EVENT_END_TIMEZONE])
- * @param allDay        whether [android.provider.CalendarContract.Events.ALL_DAY] is non-null and not zero
+ * @param timestamp     value of the Android `DTSTART`/`DTEND` field (timestamp in milliseconds)
+ * @param timeZone      value of the respective Android timezone field or `null` for system default time zone
+ * @param allDay        whether Android `ALL_DAY` field is non-null and not zero
  */
 class AndroidTimeField(
     private val timestamp: Long,
diff --git a/lib/src/main/kotlin/at/bitfire/synctools/mapping/calendar/handler/DurationHandler.kt b/lib/src/main/kotlin/at/bitfire/synctools/mapping/calendar/handler/DurationHandler.kt
@@ -8,6 +8,7 @@ package at.bitfire.synctools.mapping.calendar.handler
 
 import android.content.Entity
 import android.provider.CalendarContract.Events
+import at.bitfire.ical4android.util.TimeApiExtensions.abs
 import at.bitfire.ical4android.util.TimeApiExtensions.toIcal4jDate
 import at.bitfire.ical4android.util.TimeApiExtensions.toIcal4jDateTime
 import at.bitfire.ical4android.util.TimeApiExtensions.toZonedDateTime
@@ -16,30 +17,33 @@ import net.fortuna.ical4j.model.DateTime
 import net.fortuna.ical4j.model.TimeZoneRegistry
 import net.fortuna.ical4j.model.component.VEvent
 import net.fortuna.ical4j.model.property.DtEnd
-import java.time.Duration
 import java.time.Instant
-import java.time.Period
 import java.time.ZoneOffset
 
+/**
+ * Maps a potentially present [Events.DURATION] to a VEvent [DtEnd] property.
+ *
+ * Does nothing when:
+ *
+ * - [Events.DTEND] is present / not null (because DTEND then takes precedence over DURATION), and/or
+ * - [Events.DURATION] is null / not present.
+ */
 class DurationHandler(
     private val tzRegistry: TimeZoneRegistry
 ): AndroidEventFieldHandler {
 
     override fun process(from: Entity, main: Entity, to: VEvent) {
         val values = from.entityValues
 
-        /* Skip if:
-        - DTEND is set – we don't need to process DURATION anymore.
-        - DURATION is not set – then usually DTEND is set; however it's also OK to have neither DTEND nor DURATION in a VEVENT. */
+        /* Skip if DTEND is set and/or DURATION is not set. In both cases EndTimeHandler is
+        responsible for generating the DTEND property. */
         if (values.getAsLong(Events.DTEND) != null)
             return
-        val durStr = values.getAsString(Events.DURATION) ?: return
-        val duration = AndroidTimeUtils.parseDuration(durStr)
+        val durationStr = values.getAsString(Events.DURATION) ?: return
 
-        // Skip in case of zero or negative duration (analogous to DTEND being before DTSTART).
-        if ((duration is Duration && (duration.isZero || duration.isNegative)) ||
-            (duration is Period && (duration.isZero || duration.isNegative)))
-            return
+        // parse duration and invert in case of negative value (events can't go back in time)
+        val parsedDuration = AndroidTimeUtils.parseDuration(durationStr)
+        val duration = parsedDuration.abs()
 
         /* Some servers have problems with DURATION. For maximum compatibility, we always generate DTEND instead of DURATION.
         (After all, the constraint that non-recurring events have a DTEND while recurring events use DURATION is Android-specific.)
diff --git a/lib/src/main/kotlin/at/bitfire/synctools/mapping/calendar/handler/EndTimeHandler.kt b/lib/src/main/kotlin/at/bitfire/synctools/mapping/calendar/handler/EndTimeHandler.kt
@@ -8,11 +8,22 @@ package at.bitfire.synctools.mapping.calendar.handler
 
 import android.content.Entity
 import android.provider.CalendarContract.Events
+import androidx.annotation.VisibleForTesting
 import net.fortuna.ical4j.model.TimeZoneRegistry
 import net.fortuna.ical4j.model.component.VEvent
 import net.fortuna.ical4j.model.property.DtEnd
+import java.time.Duration
+import java.time.Instant
 import java.util.logging.Logger
 
+/**
+ * Maps a potentially present [Events.DTEND] to a VEvent [DtEnd] property.
+ *
+ * If [Events.DTEND] is null / not present:
+ *
+ * - If [Events.DURATION] is present / not null, [DurationHandler] is responsible for generating the VEvent's [DtEnd].
+ * - If [Events.DURATION] is null / not present, this class is responsible for generating the VEvent's [DtEnd].
+ */
 class EndTimeHandler(
     private val tzRegistry: TimeZoneRegistry
 ): AndroidEventFieldHandler {
@@ -24,15 +35,21 @@ class EndTimeHandler(
         val values = from.entityValues
         val allDay = (values.getAsInteger(Events.ALL_DAY) ?: 0) != 0
 
-        // Skip if DTEND is not set – then usually DURATION is set; however it's also OK to have neither DTEND nor DURATION in a VEVENT.
-        val tsEnd = values.getAsLong(Events.DTEND) ?: return
-
-        // Also skip if DTEND is not after DTSTART (not allowed in iCalendar)
+        // Skip if DTSTART is not present (not allowed in iCalendar)
         val tsStart = values.getAsLong(Events.DTSTART) ?: return
-        if (tsEnd <= tsStart) {
-            logger.warning("Ignoring DTEND=$tsEnd that is not after DTSTART=$tsStart")
-            return
-        }
+
+        val tsEndOrNull = values.getAsLong(Events.DTEND)
+        val durationStr = values.getAsString(Events.DURATION)
+
+        if (tsEndOrNull == null && durationStr != null) // DTEND not present, but DURATION is present:
+            return                                      // DurationHandler is responsible for generating the DTEND property
+
+        /* Make sure that there's always a DTEND for compatibility. While it's allowed in RFC 5545
+        to omit DTEND, this causes problems with some servers (notably iCloud). See also:
+        https://github.com/bitfireAT/davx5-ose/issues/1859 */
+        val tsEnd = tsEndOrNull
+            ?.takeUnless { it < tsStart }               // only use DTEND if it's not before DTSTART
+            ?: calculateFromDefault(tsStart, allDay)    // always provide DTEND for compatibility
 
         // DATE or DATE-TIME according to allDay
         val end = AndroidTimeField(
@@ -46,4 +63,16 @@ class EndTimeHandler(
         to.properties += DtEnd(end)
     }
 
+    @VisibleForTesting
+    internal fun calculateFromDefault(tsStart: Long, allDay: Boolean): Long =
+        if (allDay) {
+            // all-day: default duration is PT1D; all-day events are always in UTC time zone
+            val start = Instant.ofEpochMilli(tsStart)
+            val end = start + Duration.ofDays(1)
+            end.toEpochMilli()
+        } else {
+            // non-all-day: default duration is PT0S; end time = start time
+            tsStart
+        }
+
 }
diff --git a/lib/src/test/kotlin/at/bitfire/synctools/mapping/calendar/handler/DurationHandlerTest.kt b/lib/src/test/kotlin/at/bitfire/synctools/mapping/calendar/handler/DurationHandlerTest.kt
@@ -43,6 +43,19 @@ class DurationHandlerTest {
         assertNull(result.duration)
     }
 
+    @Test
+    fun `All-day event with negative all-day duration`() {
+        val result = VEvent()
+        val entity = Entity(contentValuesOf(
+            Events.ALL_DAY to 1,
+            Events.DTSTART to 1592733600000L,   // 21/06/2020 10:00 UTC
+            Events.DURATION to "P-4D"
+        ))
+        handler.process(entity, entity, result)
+        assertEquals(DtEnd(Date("20200625")), result.endDate)
+        assertNull(result.duration)
+    }
+
     @Test
     fun `All-day event with non-all-day duration`() {
         val result = VEvent()
@@ -56,6 +69,19 @@ class DurationHandlerTest {
         assertNull(result.duration)
     }
 
+    @Test
+    fun `All-day event with negative non-all-day duration`() {
+        val result = VEvent()
+        val entity = Entity(contentValuesOf(
+            Events.ALL_DAY to 1,
+            Events.DTSTART to 1760486400000L,   // Wed Oct 15 2025 00:00:00 GMT+0000
+            Events.DURATION to "PT-24H"
+        ))
+        handler.process(entity, entity, result)
+        assertEquals(DtEnd(Date("20251016")), result.endDate)
+        assertNull(result.duration)
+    }
+
     @Test
     fun `Non-all-day event with all-day duration`() {
         val result = VEvent()
@@ -72,56 +98,72 @@ class DurationHandlerTest {
     }
 
     @Test
-    fun `Non-all-day event with non-all-day duration`() {
+    fun `Non-all-day event with negative all-day duration`() {
         val result = VEvent()
         val entity = Entity(contentValuesOf(
             Events.ALL_DAY to 0,
             Events.DTSTART to 1761433200000L,  // Sun Oct 26 2025 01:00:00 GMT+0200
             Events.EVENT_TIMEZONE to "Europe/Vienna",
-            Events.DURATION to "P24H"
+            Events.DURATION to "P-1D",
         ))
         // DST transition at 03:00, clock is set back to 02:00 → P1D = PT25H
         handler.process(entity, entity, result)
-        assertEquals(DtEnd(DateTime("20251027T000000", tzVienna)), result.endDate)
+        assertEquals(DtEnd(DateTime("20251027T010000", tzVienna)), result.endDate)
         assertNull(result.duration)
     }
 
-
-    // skip conditions
-
     @Test
-    fun `Skip if DTSTART is not set`() {
+    fun `Non-all-day event with non-all-day duration`() {
         val result = VEvent()
         val entity = Entity(contentValuesOf(
-            Events.DURATION to "PT1H"
+            Events.ALL_DAY to 0,
+            Events.DTSTART to 1761433200000L,  // Sun Oct 26 2025 01:00:00 GMT+0200
+            Events.EVENT_TIMEZONE to "Europe/Vienna",
+            Events.DURATION to "PT24H"
         ))
+        // DST transition at 03:00, clock is set back to 02:00 → PT24H goes one hour back
         handler.process(entity, entity, result)
+        assertEquals(DtEnd(DateTime("20251027T000000", tzVienna)), result.endDate)
         assertNull(result.duration)
     }
 
     @Test
-    fun `Skip if DURATION is negative all-day duration`() {
+    fun `Non-all-day event with negative non-all-day duration`() {
         val result = VEvent()
         val entity = Entity(contentValuesOf(
+            Events.ALL_DAY to 0,
             Events.DTSTART to 1761433200000L,  // Sun Oct 26 2025 01:00:00 GMT+0200
             Events.EVENT_TIMEZONE to "Europe/Vienna",
-            Events.DURATION to "P-1D"
+            Events.DURATION to "PT-24H"     // will be converted to PT24H
         ))
+        // DST transition at 03:00, clock is set back to 02:00 → PT24H goes one hour back
         handler.process(entity, entity, result)
-        assertNull(result.endDate)
+        assertEquals(DtEnd(DateTime("20251027T000000", tzVienna)), result.endDate)
         assertNull(result.duration)
     }
 
+
+    // skip conditions
+
     @Test
-    fun `Skip if DURATION is zero non-all-day duration`() {
+    fun `Skip if DTSTART is not set`() {
         val result = VEvent()
         val entity = Entity(contentValuesOf(
-            Events.DTSTART to 1761433200000L,  // Sun Oct 26 2025 01:00:00 GMT+0200
-            Events.EVENT_TIMEZONE to "Europe/Vienna",
-            Events.DURATION to "PT0S"
+            Events.DURATION to "PT1H"
+        ))
+        handler.process(entity, entity, result)
+        assertNull(result.duration)
+    }
+
+    @Test
+    fun `Skip if DTEND and DURATION are set`() {
+        val result = VEvent()
+        val entity = Entity(contentValuesOf(
+            Events.DTSTART to 1761433200000L,
+            Events.DTEND to 1761433200000L,
+            Events.DURATION to "P1D"
         ))
         handler.process(entity, entity, result)
-        assertNull(result.endDate)
         assertNull(result.duration)
     }
 
diff --git a/lib/src/test/kotlin/at/bitfire/synctools/mapping/calendar/handler/EndTimeHandlerTest.kt b/lib/src/test/kotlin/at/bitfire/synctools/mapping/calendar/handler/EndTimeHandlerTest.kt
@@ -21,7 +21,9 @@ import org.junit.Assume
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.robolectric.RobolectricTestRunner
+import java.time.OffsetDateTime
 import java.time.ZoneId
+import java.time.ZoneOffset
 
 @RunWith(RobolectricTestRunner::class)
 class EndTimeHandlerTest {
@@ -45,6 +47,17 @@ class EndTimeHandlerTest {
         assertEquals(DtEnd(Date("20200621")), result.endDate)
     }
 
+    @Test
+    fun `All-day event with empty DTEND`() {
+        val result = VEvent()
+        val entity = Entity(contentValuesOf(
+            Events.ALL_DAY to 1,
+            Events.DTSTART to 1592697600000L    // 21/06/2020; DTSTART is required for DTEND to be processed
+        ))
+        handler.process(entity, entity, result)
+        assertEquals(DtEnd(Date("20200622")), result.endDate)
+    }
+
     @Test
     fun `Non-all-day event with end timezone`() {
         val result = VEvent()
@@ -88,38 +101,62 @@ class EndTimeHandlerTest {
         assertEquals(defaultTz, (result.endDate?.date as? DateTime)?.timeZone)
     }
 
-
-    // skip conditions
-
     @Test
-    fun `Skip if DTEND is not after DTSTART`() {
+    fun `Non-all-day event with empty DTEND`() {
         val result = VEvent()
         val entity = Entity(contentValuesOf(
-            Events.DTSTART to 1592733500000L,
-            Events.DTEND to 1592733500000L
+            Events.ALL_DAY to 0,
+            Events.DTSTART to 1592733600000L,           // 21/06/2020 12:00 +0200; DTSTART is required for DTEND to be processed
+            Events.EVENT_TIMEZONE to "Europe/Vienna"    // will be used as end time zone
         ))
         handler.process(entity, entity, result)
-        assertNull(result.endDate)
+        assertEquals(DtEnd(DateTime("20200621T120000", tzVienna)), result.endDate)
     }
 
+
+    // skip conditions
+
     @Test
-    fun `Skip if DTEND is not set`() {
+    fun `Skip if DTSTART is not set`() {
         val result = VEvent()
         val entity = Entity(contentValuesOf(
-            Events.DTSTART to 1592733500000L
+            Events.DTEND to 1592733500000L
         ))
         handler.process(entity, entity, result)
         assertNull(result.endDate)
     }
 
     @Test
-    fun `Skip if DTSTART is not set`() {
+    fun `Skip if DURATION is set`() {
         val result = VEvent()
         val entity = Entity(contentValuesOf(
-            Events.DTEND to 1592733500000L
+            Events.DTSTART to 1592733500000L,
+            Events.DURATION to "PT1H"
         ))
         handler.process(entity, entity, result)
         assertNull(result.endDate)
     }
 
+
+    // calculateFromDefault
+
+    @Test
+    fun `calculateFromDefault (all-day)`() {
+        val start = OffsetDateTime.of(2025, 12, 5, 0, 0, 0, 0, ZoneOffset.UTC)
+        val tsStart = start.toInstant().toEpochMilli()
+
+        val result = handler.calculateFromDefault(tsStart, allDay = true)
+
+        val expectedEnd = OffsetDateTime.of(2025, 12, 6, 0, 0, 0, 0, ZoneOffset.UTC)
+        val expectedEndTs = expectedEnd.toInstant().toEpochMilli()
+        assertEquals(expectedEndTs, result)
+    }
+
+    @Test
+    fun `calculateFromDefault (non-all-day)`() {
+        val start = System.currentTimeMillis()
+        val result = handler.calculateFromDefault(start, allDay = false)
+        assertEquals(start, result)
+    }
+
 }
