Fix mishandling of exceptions in UTC#2268
Conversation
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
There was a problem hiding this comment.
Pull request overview
Fixes an iCal generation crash when recurrence exceptions use a UTC Instant for DTSTART and the main event uses a ZonedDateTime, by avoiding ical4j’s problematic cross-type comparison and adding a regression test.
Changes:
- Add a regression test that writes an event with a UTC
InstantexceptionDTSTARTand verifies generation succeeds. - Update
ICalendarGenerator.writeto compare exception start times by convertingTemporalvalues toInstantbefore ordering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| synctools/src/main/kotlin/at/bitfire/synctools/icalendar/ICalendarGenerator.kt | Prevents the crash by comparing exception/main start times via Instant conversion instead of TemporalAdapter.isBefore. |
| synctools/src/test/kotlin/at/bitfire/synctools/icalendar/ICalendarGeneratorTest.kt | Adds coverage for the UTC-Instant exception DTSTART scenario to prevent regressions. |
| // Convert both Temporals to Instants for comparison, because they may be in different time zones. If conversion fails, ignore the start date of this exception. | ||
| val startInstant = runCatching { start.toInstant() }.getOrNull() ?: return@let | ||
| val earliestInstant = earliestStart?.let { runCatching { it.toInstant() }.getOrNull() } | ||
| if (earliestInstant == null || startInstant < earliestInstant) |
There was a problem hiding this comment.
thought: Maybe explicitly check for the expected types, handle them and throw a specific Exception if it's not one of the supported types? I'd find it easier to understand if it would be clear when toInstant is expected to work and when not, instead of using a somehow intransparent runCatching.
But didn't look in depth, just an idea.
Also have unassigned myself (not available tomorrow)
rfc2822
added
pr-bugfix
sunkup
left a comment
sunkup
left a comment
There was a problem hiding this comment.
Works well, but I second what Ricki says.
This comment was marked as duplicate.
This comment was marked as duplicate.
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
|
Superseded by #2280 |
Purpose
Closes https://github.com/bitfireAT/davx5/issues/898
The TemporalAdapter.isBefore call in ICalendarGenerator.write crashed when comparing a UTC Instant (for example 2025-08-22T03:30:00Z from an exception's DTSTART) against a ZonedDateTime (from the main event's DTSTART), because ical4j internally tries ZonedDateTime.from(Instant) which always throws DateTimeException.
Short description
Added a failing test that showcases the issue, and a fix for it.
Copy of bitfireAT/synctools#442
Checklist