Handling period durations of days with `T` prefix

Question

Handling period durations of days with `T` prefix

ArnyminerZ opened this issue 2 years ago · 12 comments

There has been detected some instances where the ICS file has an invalid duration, for example this trigger:

TRIGGER:-P2DT

Trying to parse it in Kotlin Playground shows that is not a valid duration, which is what ICSx5 complains about when trying to load those kind of URLs.

The Java Documentation is not very clear about the use of the T prefix. However, this StackOverflow post doesn't mention D as a valid unit for periods with the T prefix, which this is the case. To be clearer:

TRIGGER:-PT2H    <- VALID
TRIGGER:-PT2D    <- INVALID

Answer 1 · 2023-01-16T12:02:07.000Z

This must be fixed by ical4j, but a possible fix is to find matches of [-]?PT\d*D, and replace them by the corresponding duration in hours, for example, so this conversion should be made:

TRIGGER:-PT2D
<>
TRIGGER:-PT48H

Answer 2 · 2023-01-16T12:22:37.000Z

(For internal reference)

The ICS file where this popped up was: https://ics.ecal.com/ecal-sub/63b0e388811993000d5f2064/Collingwood%20Magpies%20Football%20Club.ics

Zammad ticket: #234

Answer 3 · 2023-01-16T13:00:16.000Z

(Ok please forget that… guess my brain was somewhere else, have to check again.)

Answer 4 · 2023-01-17T07:32:50.000Z

A fix can be to take the same approach as with fixInvalidUtcOffset. This code fixes the offset in days, and converts it to hours (you can take a look at the Kotlin Playground):

import java.time.Duration;

val INVALID_DAY_PERIOD_REGEX = Regex("-?PT-?\\d+D", setOf(RegexOption.MULTILINE, RegexOption.IGNORE_CASE))

fun fixStringFromReader(original: String): String {
    // Convert the reader to a string
    var str = original
    val found = INVALID_DAY_PERIOD_REGEX.findAll(str)
    for (match in found) {
        val range = match.range
        val start = range.first
        val end = range.last
        val numPos = str.indexOf("PT", start) + 2
        val number = str.substring(numPos, end).toLongOrNull()
        if (number != null) {
            val newPiece = str.substring(start, numPos) + (number*24) + "H"
            str = str.replaceRange(IntRange(start, end), newPiece)
        }
    }
    return str
}

fun main() {
    val original = "-PT2D"
    println("Original: $original")
    try {
        Duration.parse(original)
        println("Duration was ok")
    } catch (_: Exception) {
        println("Original duration is not valid")
    }
    val fixed = fixStringFromReader(original)
    println("Fixed: $fixed")
    try {
        Duration.parse(fixed)
        println("New Duration is ok")
    } catch (_: Exception) {
        println("Could not fix duration")
    }
}

Then the proper function for ICalPreprocessor:

/**
 * Fixes durations with day offsets with the 'T' prefix.
 * @param reader Reader that reads the potentially broken iCalendar
 * @see <a href="https://github.com/bitfireAT/ical4android/issues/77">GitHub</a>
 */
fun fixInvalidDayOffset(reader: Reader): Reader {
    fun fixStringFromReader(): String {
        // Convert the reader to a string
        var str = IOUtils.toString(reader)
        val found = INVALID_DAY_PERIOD_REGEX.findAll(str)
        // Find all matches for the expression
        for (match in found) {
            // Get the range of the match
            val range = match.range
            // Get the start position of the match
            val start = range.first
            // And the end position
            val end = range.last
            // Get the position of the number inside str (without the prefix)
            val numPos = str.indexOf("PT", start) + 2
            // And get the number, converting it to long
            val number = str.substring(numPos, end).toLongOrNull()
            // If the number has been converted to long correctly
            if (number != null) {
                // Build a new string with the prefix given, and the number converted to hours
                val newPiece = str.substring(start, numPos) + (number*24) + "H"
                // Replace the range found with the new piece
                str = str.replaceRange(IntRange(start, end), newPiece)
            }
        }
        return str
    }

    var result: String? = null

    val resetSupported = try {
        reader.reset()
        true
    } catch (e: IOException) {
        false
    }

    if (resetSupported) {
        // reset is supported, no need to copy the whole stream to another String (unless we have to fix the period)
        val horizonFind = Scanner(reader)
            .findWithinHorizon(INVALID_DAY_PERIOD_REGEX.toPattern(), 0)
        if (horizonFind != null) {
            reader.reset()
            result = fixStringFromReader()
        }
    } else
    // If reset is not supported, always try to fix the issue by copying the string
        result = fixStringFromReader()

    if (result != null)
        return StringReader(result)

    // not modified, return original iCalendar
    reader.reset()
    return reader
}

This does the trick, but also means more overload when parsing long icals... What do you think @rfc2822 ? Should we implement this?

Edit: I was thinking that maybe we could only target the tags that use Java Durations, otherwise unexpected replacements may be made, in the description, for example, or something.

Edit 2: If I'm not wrong, the only ical elements that make use of the Duration type are DURATION and TRIGGER

Answer 5 · 2023-01-17T07:44:14.000Z

My bad, this is something for ical4android. Maybe we should move this there?

Answer 6 · 2023-01-19T21:41:49.000Z

@devvv4ever The invalid iCalendar seems to be generated by "E-DIARY 1.0". I couldn't find such a product… can you please ask the customer which software was used? So that it can be fixed at the source, regardless of whether/how we work around it.

@ArnyminerZ Thanks, I'll have a look at at it. It's however complicated code just to support an invalid iCalendar, I don't know whether it's worth to lug along the maintenance work for this special case…

Answer 7 · 2023-01-19T21:58:15.000Z

Yup, it's quite a lot of code for such an specific case...

Answer 8 · 2023-01-19T22:45:52.000Z

But looks fine, I think it's a good approach and we should do it. I'll review soon :)

Answer 9 · 2023-02-03T13:33:56.000Z

I just noted that the defect line is

TRIGGER:-P2DT

but the fix is for

TRIGGER:-PT2D

isn't it?

So as I understand it, the fix wouldn't allow to process the original file.

Answer 10 · 2023-02-07T09:54:20.000Z

I've fixed that, and improved the tests so we are sure that the strings are parseable to Duration. See #80

Answer 11 · 2023-03-06T12:27:06.000Z

I can confirm the issue is still there, but not sure why. I've tried copying the pre-processor code into a Kotlin snippet:

fun regexpForProblem() = Regex(
    // Examples:
    // TRIGGER:-P2DT
    // TRIGGER:-PT2D
    "^(DURATION|TRIGGER):-?P((T-?\\d+D)|(-?\\d+DT))\$",
    setOf(RegexOption.MULTILINE, RegexOption.IGNORE_CASE)
)

fun fixString(original: String): String {
    var s: String = original

    // Find all matches for the expression
    val found = regexpForProblem().find(s) ?: return s
    for (match in found.groupValues) {
        val fixed = match
            .replace("PT", "P")
            .replace("DT", "D")
        s = s.replace(match, fixed)
    }
    return s
}

fun main() {
    val cases = listOf("TRIGGER:-P2DT", "TRIGGER:-PT15M", "TRIGGER:-P5DT")
    for (str in cases) {
      val fixed = fixString(str).let { it.substring(it.indexOf(':') + 1) }
      println(fixed)
      println(java.time.Duration.parse(fixed))
    }
}

The cases provided have been taken from the url shared by @devvv4ever (https://ics.ecal.com/ecal-sub/63b0e388811993000d5f2064/Collingwood%20Magpies%20Football%20Club.ics).

When running in ICSx⁵, the error is:

Couldn't validate calendar
       at.bitfire.ical4android.InvalidCalendarException: Couldn't parse iCalendar
       at at.bitfire.ical4android.ICalendar$Companion.fromReader(ICalendar.kt:91)
       at at.bitfire.ical4android.Event$Companion.eventsFromReader(Event.kt:79)
       at at.bitfire.icsdroid.ui.AddCalendarValidationFragment$ValidationModel$validate$downloader$1.onSuccess(AddCalendarValidationFragment.kt:126)
       at at.bitfire.icsdroid.CalendarFetcher.fetchNetwork$icsx5_72_2_1_1_gplayDebug(CalendarFetcher.kt:157)
       at at.bitfire.icsdroid.CalendarFetcher$fetchNetwork$1.invokeSuspend(Unknown Source:14)
       at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
       at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
       at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570)
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750)
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677)
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664)
      Caused by: net.fortuna.ical4j.data.ParserException: Error at line 63:Text cannot be parsed to a Duration
       at net.fortuna.ical4j.data.CalendarParserImpl.parse(CalendarParserImpl.java:172)
       at net.fortuna.ical4j.data.CalendarBuilder.build(CalendarBuilder.java:197)
       at net.fortuna.ical4j.data.CalendarBuilder.build(CalendarBuilder.java:185)
       at at.bitfire.ical4android.ICalendar$Companion.fromReader(ICalendar.kt:89)
       at at.bitfire.ical4android.Event$Companion.eventsFromReader(Event.kt:79) 
       at at.bitfire.icsdroid.ui.AddCalendarValidationFragment$ValidationModel$validate$downloader$1.onSuccess(AddCalendarValidationFragment.kt:126) 
       at at.bitfire.icsdroid.CalendarFetcher.fetchNetwork$icsx5_72_2_1_1_gplayDebug(CalendarFetcher.kt:157) 
       at at.bitfire.icsdroid.CalendarFetcher$fetchNetwork$1.invokeSuspend(Unknown Source:14) 
       at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33) 
       at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106) 
       at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570) 
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750) 
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677) 
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664) 
      Caused by: j$.time.format.DateTimeParseException: Text cannot be parsed to a Duration
       at j$.time.Duration.parse(Duration.java:417)
       at net.fortuna.ical4j.model.TemporalAmountAdapter.parse(TemporalAmountAdapter.java:145)
       at net.fortuna.ical4j.model.TemporalAmountAdapter.parse(TemporalAmountAdapter.java:132)
       at net.fortuna.ical4j.model.property.Trigger.setValue(Trigger.java:253)
       at net.fortuna.ical4j.model.property.Trigger.<init>(Trigger.java:166)
       at net.fortuna.ical4j.model.property.Trigger$Factory.createProperty(Trigger.java:301)
       at net.fortuna.ical4j.model.property.Trigger$Factory.createProperty(Trigger.java:291)
       at net.fortuna.ical4j.model.PropertyBuilder.build(PropertyBuilder.java:76)
       at net.fortuna.ical4j.data.DefaultContentHandler.endProperty(DefaultContentHandler.java:159)
       at net.fortuna.ical4j.data.CalendarParserImpl$PropertyParser.parse(CalendarParserImpl.java:309)
       at net.fortuna.ical4j.data.CalendarParserImpl$PropertyParser.access$1100(CalendarParserImpl.java:241)
       at net.fortuna.ical4j.data.CalendarParserImpl$PropertyListParser.parse(CalendarParserImpl.java:226)
       at net.fortuna.ical4j.data.CalendarParserImpl$ComponentParser.parse(CalendarParserImpl.java:446)
       at net.fortuna.ical4j.data.CalendarParserImpl$ComponentParser.access$900(CalendarParserImpl.java:422)
       at net.fortuna.ical4j.data.CalendarParserImpl$PropertyListParser.parse(CalendarParserImpl.java:224)
       at net.fortuna.ical4j.data.CalendarParserImpl$ComponentParser.parse(CalendarParserImpl.java:446)
       at net.fortuna.ical4j.data.CalendarParserImpl$ComponentParser.access$900(CalendarParserImpl.java:422)
       at net.fortuna.ical4j.data.CalendarParserImpl$PropertyListParser.parse(CalendarParserImpl.java:224)
       at net.fortuna.ical4j.data.CalendarParserImpl.parseCalendar(CalendarParserImpl.java:128)
       at net.fortuna.ical4j.data.CalendarParserImpl.parseCalendarList(CalendarParserImpl.java:194)
       at net.fortuna.ical4j.data.CalendarParserImpl.parse(CalendarParserImpl.java:163)
       at net.fortuna.ical4j.data.CalendarBuilder.build(CalendarBuilder.java:197) 
       at net.fortuna.ical4j.data.CalendarBuilder.build(CalendarBuilder.java:185) 
       at at.bitfire.ical4android.ICalendar$Companion.fromReader(ICalendar.kt:89) 
       at at.bitfire.ical4android.Event$Companion.eventsFromReader(Event.kt:79) 
       at at.bitfire.icsdroid.ui.AddCalendarValidationFragment$ValidationModel$validate$downloader$1.onSuccess(AddCalendarValidationFragment.kt:126) 
       at at.bitfire.icsdroid.CalendarFetcher.fetchNetwork$icsx5_72_2_1_1_gplayDebug(CalendarFetcher.kt:157) 
       at at.bitfire.icsdroid.CalendarFetcher$fetchNetwork$1.invokeSuspend(Unknown Source:14) 
       at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33) 
       at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106) 
       at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570) 
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750) 
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677) 
       at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664)

Which shows the error at line 63, which is TRIGGER:-P5DT, that is parsing correctly at the test snippet.

Answer 12 · 2023-03-06T12:32:59.000Z

I've printed the reader that is being passed for parsing the calendar:

BEGIN:VCALENDAR
PRODID:-//E-DIARY//E-DIARY 1.0//EN
VERSION:2.0
METHOD:PUBLISH
X-WR-CALNAME:Collingwood Magpies Football Club
X-Built-On-Cache-Miss:true
BEGIN:VEVENT
LAST-MODIFIED:20230223T030355Z
DTSTAMP:20230223T030355Z
LOCATION:MCG
X-ECAL-SCHEDULE:508efe5dfb0615bb30000001
DTSTART:20230317T084000Z
DTEND:20230317T114000Z
SUMMARY:Round 1: Geelong Cats V Collingwood
TRANSP:TRANSPARENT
SEQUENCE:0
UID:63945a154c410f17fb7528a7
PRIORITY:5
X-MICROSOFT-CDO-IMPORTANCE:1
CLASS:PUBLIC
DESCRIPTION:Round 1 | Watch live on Kayo | Join the conversation @Collingwo
    odFC #GoPies \n\nCan't get the game? Follow the action live at the Colling
    wood Match Centre\n\nMatch Centre\nhttps://ecal.ai/f/jS6Wx/TS6N\n\nBuy Tic
    kets\nhttps://ecal.ai/f/kcCl6/TS6N\n\nVisit our Superstore\nhttps://ecal.a
    i/f/jS6Wy/TS6N\n\nDownload our Official Club App\nhttps://ecal.ai/f/jS6Wz/
    TS6N\n\nFacebook\nhttps://ecal.ai/f/jS6WB/TS6N\n\nTwitter\nhttps://ecal.ai
    /f/jS6WC/TS6N\n\nYouTube\nhttps://ecal.ai/f/jS6WD/TS6N\n\nInstagram\nhttps
    ://ecal.ai/f/jS6WF/TS6N\n\nManage my ECAL\nhttps://support.ecal.com
BEGIN:VALARM
TRIGGER:-P2D
ACTION:DISPLAY
DESCRIPTION:Reminder
END:VALARM
BEGIN:VALARM
TRIGGER:-PT15M
ACTION:DISPLAY
DESCRIPTION:Reminder
END:VALARM
END:VEVENT
BEGIN:VEVENT
LAST-MODIFIED:20230223T030355Z
DTSTAMP:20230223T030355Z
LOCATION:MCG
X-ECAL-SCHEDULE:508efe5dfb0615bb30000001
DTSTART:20230325T024500Z
DTEND:20230325T054500Z
SUMMARY:Round 2: Collingwood V Port Adelaide
TRANSP:TRANSPARENT
SEQUENCE:0
UID:63945a154c410f17fb7528a8
PRIORITY:5
X-MICROSOFT-CDO-IMPORTANCE:1
CLASS:PUBLIC
DESCRIPTION:Round 2 | Watch live on Kayo | Join the conversation @Collingwo
    odFC #GoPies\n\nGet to the game! Purchase your tickets now!\n\nBuy Tickets
    \nhttps://ecal.ai/f/kcCl7/TS6N\n\nJoin the Magpie Army\nhttps://ecal.ai/f/
    jS6WG/TS6N\n\nVisit our Superstore\nhttps://ecal.ai/f/jS6WH/TS6N\n\nDownlo
    ad our Official Club App\nhttps://ecal.ai/f/jS6WJ/TS6N\n\nFacebook\nhttps:
    //ecal.ai/f/jS6WK/TS6N\n\nTwitter\nhttps://ecal.ai/f/jS6WL/TS6N\n\nYouTube
    \nhttps://ecal.ai/f/jS6WM/TS6N\n\nInstagram\nhttps://ecal.ai/f/jS6WN/TS6N\
    n\nManage my ECAL\nhttps://support.ecal.com
BEGIN:VALARM
TRIGGER:-P5DT
ACTION:DISPLAY
DESCRIPTION:Reminder
END:VALARM
BEGIN:VALARM
TRIGGER:-P2D
ACTION:DISPLAY
DESCRIPTION:Reminder
END:VALARM
END:VEVENT

Line 63 is: TRIGGER:-P5DT, which is in fact a non-valid duration. Therefore ICalPreprocessor is not working correctly.