FixInvalidDayOffsetPreprocessor: Allow DURATION as value (#182)

* Add test

* Allow duration as value

* Restrict the regex to properties that can have DURATION as a value

* Tighten regex further

* Convert negative asserts (not equals) to positive asserts (test for what is expected)

* Test KDoc

* Use capturing group; Add comments

* Use replaceRange

---------

Co-authored-by: Ricki Hirner <[email protected]>

Author: sunkup

lib/src/main/kotlin/at/bitfire/ical4android/validation/FixInvalidDayOffsetPreprocessor.kt

@@ -14,25 +14,27 @@ object FixInvalidDayOffsetPreprocessor : StreamPreprocessor() {
         // Examples:
         // TRIGGER:-P2DT
         // TRIGGER:-PT2D
-        "^(DURATION|TRIGGER):-?P((T-?\\d+D)|(-?\\d+DT))\$",
+        // REFRESH-INTERVAL;VALUE=DURATION:-PT1D
+        "(?:^|^(?:DURATION|REFRESH-INTERVAL|RELATED-TO|TRIGGER);VALUE=)(?:DURATION|TRIGGER):(-?P((T-?\\d+D)|(-?\\d+DT)))$",
         setOf(RegexOption.MULTILINE, RegexOption.IGNORE_CASE)
     )
 
     override fun fixString(original: String): String {
-        var s: String = original
+        var iCal: String = original
 
         // Find all instances matching the defined expression
-        val found = regexpForProblem().findAll(s)
+        val found = regexpForProblem().findAll(iCal).toList()
 
-        // ..and repair them
-        for (match in found) {
-            val matchStr = match.value
-            val fixed = matchStr
-                .replace("PT", "P")
-                .replace("DT", "D")
-            s = s.replace(matchStr, fixed)
+        // ... and repair them. Use reversed order so that already replaced occurrences don't interfere with the following matches.
+        for (match in found.reversed()) {
+            match.groups[1]?.let { duration ->      // first capturing group is the duration value, for instance: "-PT1D"
+                val fixed = duration.value     // fixed is then for instance: "-P1D"
+                    .replace("PT", "P")
+                    .replace("DT", "D")
+                iCal = iCal.replaceRange(duration.range, fixed)
+            }
         }
-        return s
+        return iCal
     }
 
 }

lib/src/main/kotlin/at/bitfire/ical4android/validation/StreamPreprocessor.kt

@@ -13,6 +13,12 @@ abstract class StreamPreprocessor {
 
     abstract fun regexpForProblem(): Regex?
 
+    /**
+     * Fixes an iCalendar string.
+     *
+     * @param original The complete iCalendar string
+     * @return The complete iCalendar string, but fixed
+     */
     abstract fun fixString(original: String): String
 
     fun preprocess(reader: Reader): Reader {
@@ -21,7 +27,7 @@ abstract class StreamPreprocessor {
         val resetSupported = try {
             reader.reset()
             true
-        } catch(e: IOException) {
+        } catch(_: IOException) {
             false
         }
 

lib/src/test/kotlin/at/bitfire/ical4android/validation/FixInvalidDayOffsetPreprocessorTest.kt

@@ -12,16 +12,23 @@ import java.time.Duration
 
 class FixInvalidDayOffsetPreprocessorTest {
 
-    private fun fixAndAssert(expected: String, testValue: String) {
-
+    /**
+     * Calls [FixInvalidDayOffsetPreprocessor.fixString] and asserts the result is equal to [expected].
+     *
+     * @param expected      The expected result
+     * @param testValue     The value to test
+     * @param parseDuration If `true`, [Duration.parse] is called on the fixed value to make sure it's a valid duration
+     */
+    private fun assertFixedEquals(expected: String, testValue: String, parseDuration: Boolean = true) {
         // Fix the duration string
         val fixed = FixInvalidDayOffsetPreprocessor.fixString(testValue)
 
         // Test the duration can now be parsed
-        for (line in fixed.split('\n')) {
-            val duration = line.substring(line.indexOf(':') + 1)
-            Duration.parse(duration)
-        }
+        if (parseDuration)
+            for (line in fixed.split('\n')) {
+                val duration = line.substring(line.indexOf(':') + 1)
+                Duration.parse(duration)
+            }
 
         // Assert
         assertEquals(expected, fixed)
@@ -35,36 +42,57 @@ class FixInvalidDayOffsetPreprocessorTest {
         )
     }
 
+    @Test
+    fun test_FixString_SucceedsAsValueOnCorrectProperties() {
+        // By RFC 5545 the only properties allowed to hold DURATION as a VALUE are:
+        // DURATION, REFRESH, RELATED, TRIGGER
+        assertFixedEquals("DURATION;VALUE=DURATION:P1D", "DURATION;VALUE=DURATION:PT1D")
+        assertFixedEquals("REFRESH-INTERVAL;VALUE=DURATION:P1D", "REFRESH-INTERVAL;VALUE=DURATION:PT1D")
+        assertFixedEquals("RELATED-TO;VALUE=DURATION:P1D", "RELATED-TO;VALUE=DURATION:PT1D")
+        assertFixedEquals("TRIGGER;VALUE=DURATION:P1D", "TRIGGER;VALUE=DURATION:PT1D")
+    }
+
+    @Test
+    fun test_FixString_FailsAsValueOnWrongProperty() {
+        // The update from RFC 2445 to RFC 5545 disallows using DURATION as a VALUE in FREEBUSY
+        assertFixedEquals("FREEBUSY;VALUE=DURATION:PT1D", "FREEBUSY;VALUE=DURATION:PT1D", parseDuration = false)
+    }
+
+    @Test
+    fun test_FixString_FailsIfNotAtStartOfLine() {
+        assertFixedEquals("xxDURATION;VALUE=DURATION:PT1D", "xxDURATION;VALUE=DURATION:PT1D", parseDuration = false)
+    }
+
     @Test
     fun test_FixString_DayOffsetFrom_Invalid() {
-        fixAndAssert("DURATION:-P1D", "DURATION:-PT1D")
-        fixAndAssert("TRIGGER:-P2D", "TRIGGER:-PT2D")
+        assertFixedEquals("DURATION:-P1D", "DURATION:-PT1D")
+        assertFixedEquals("TRIGGER:-P2D", "TRIGGER:-PT2D")
 
-        fixAndAssert("DURATION:-P1D", "DURATION:-P1DT")
-        fixAndAssert("TRIGGER:-P2D", "TRIGGER:-P2DT")
+        assertFixedEquals("DURATION:-P1D", "DURATION:-P1DT")
+        assertFixedEquals("TRIGGER:-P2D", "TRIGGER:-P2DT")
     }
 
     @Test
     fun test_FixString_DayOffsetFrom_Valid() {
-        fixAndAssert("DURATION:-PT12H", "DURATION:-PT12H")
-        fixAndAssert("TRIGGER:-PT12H", "TRIGGER:-PT12H")
+        assertFixedEquals("DURATION:-PT12H", "DURATION:-PT12H")
+        assertFixedEquals("TRIGGER:-PT12H", "TRIGGER:-PT12H")
     }
 
     @Test
     fun test_FixString_DayOffsetFromMultiple_Invalid() {
-        fixAndAssert("DURATION:-P1D\nTRIGGER:-P2D", "DURATION:-PT1D\nTRIGGER:-PT2D")
-        fixAndAssert("DURATION:-P1D\nTRIGGER:-P2D", "DURATION:-P1DT\nTRIGGER:-P2DT")
+        assertFixedEquals("DURATION:-P1D\nTRIGGER:-P2D", "DURATION:-PT1D\nTRIGGER:-PT2D")
+        assertFixedEquals("DURATION:-P1D\nTRIGGER:-P2D", "DURATION:-P1DT\nTRIGGER:-P2DT")
     }
 
     @Test
     fun test_FixString_DayOffsetFromMultiple_Valid() {
-        fixAndAssert("DURATION:-PT12H\nTRIGGER:-PT12H", "DURATION:-PT12H\nTRIGGER:-PT12H")
+        assertFixedEquals("DURATION:-PT12H\nTRIGGER:-PT12H", "DURATION:-PT12H\nTRIGGER:-PT12H")
     }
 
     @Test
     fun test_FixString_DayOffsetFromMultiple_Mixed() {
-        fixAndAssert("DURATION:-P1D\nDURATION:-PT12H\nTRIGGER:-P2D", "DURATION:-PT1D\nDURATION:-PT12H\nTRIGGER:-PT2D")
-        fixAndAssert("DURATION:-P1D\nDURATION:-PT12H\nTRIGGER:-P2D", "DURATION:-P1DT\nDURATION:-PT12H\nTRIGGER:-P2DT")
+        assertFixedEquals("DURATION:-P1D\nDURATION:-PT12H\nTRIGGER:-P2D", "DURATION:-PT1D\nDURATION:-PT12H\nTRIGGER:-PT2D")
+        assertFixedEquals("DURATION:-P1D\nDURATION:-PT12H\nTRIGGER:-P2D", "DURATION:-P1DT\nDURATION:-PT12H\nTRIGGER:-P2DT")
     }
 
     @Test