Handling period durations of days with `T` prefix

[ArnyminerZ]

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

[ArnyminerZ]

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

[devvv4ever]

(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

[rfc2822]

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

[ArnyminerZ]

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

[ArnyminerZ]

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

[rfc2822]

@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…

[ArnyminerZ]

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

[rfc2822]

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

[devvv4ever]

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.