Pre-process iCalendars line by line to avoid OOM on large files (#92)

* Fix OOM issues

* Remove reset

* Get rid of Android's Log

* Fix function calling

* Ignore exception in try-catch

* Fix tests

* Get rid of `regexpForProblem` in `StreamPreprocessor`

* Improve KDoc

* Rename argument

* Move warning

* Closing readers

* Make it clear why we use LF

* Use Guava's `CharSource` and get rid of `SequenceReader`

* Optimize imports

Signed-off-by: Arnau Mora <[email protected]>

* Disable inspection

Signed-off-by: Arnau Mora <[email protected]>

* Added Spotbugs annotations

Signed-off-by: Arnau Mora <[email protected]>

* Add annotations

Signed-off-by: Arnau Mora <[email protected]>

* [WIP] Process line by line

* KDoc

* Move ICalPreprocessorInstrumentedTest to unit tests

* KDoc

* Add test

* Make some fields internal

* KDoc

---------

Signed-off-by: Arnau Mora <[email protected]>
Co-authored-by: Ricki Hirner <[email protected]>

Author: ArnyminerZ

gradle/libs.versions.toml

@@ -15,7 +15,7 @@ kotlin = "2.2.21"
 mockk = "1.14.6"
 roboelectric = "4.16"
 slf4j = "2.0.17"
-spotbugs-annotations = "4.9.8"
+spotbugs = "4.9.8"
 
 [libraries]
 android-desugar = { module = "com.android.tools:desugar_jdk_libs", version.ref = "android-desugar" }
@@ -32,7 +32,7 @@ mockk = { module = "io.mockk:mockk", version.ref = "mockk" }
 mockk-android = { module = "io.mockk:mockk-android", version.ref = "mockk" }
 roboelectric = { module = "org.robolectric:robolectric", version.ref = "roboelectric" }
 slf4j-jdk = { module = "org.slf4j:slf4j-jdk14", version.ref = "slf4j" }
-spotbugs-annotations = { module = "com.github.spotbugs:spotbugs-annotations", version.ref = "spotbugs-annotations" }
+spotbugs-annotations = { module = "com.github.spotbugs:spotbugs-annotations", version.ref = "spotbugs" }
 
 [plugins]
 android-library = { id = "com.android.library", version.ref = "agp" }

lib/build.gradle.kts

@@ -131,6 +131,9 @@ dependencies {
     // synctools.test package also provide test rules
     implementation(libs.androidx.test.rules)
 
+    // Useful annotations
+    api(libs.spotbugs.annotations)
+
     // instrumented tests
     androidTestImplementation(libs.androidx.test.rules)
     androidTestImplementation(libs.androidx.test.runner)

lib/src/main/kotlin/at/bitfire/synctools/icalendar/validation/FixInvalidDayOffsetPreprocessor.kt

@@ -6,13 +6,16 @@
 
 package at.bitfire.synctools.icalendar.validation
 
+import androidx.annotation.VisibleForTesting
+
 /**
  * Fixes durations with day offsets with the 'T' prefix.
  * See also https://github.com/bitfireAT/ical4android/issues/77
  */
-class FixInvalidDayOffsetPreprocessor : StreamPreprocessor() {
+class FixInvalidDayOffsetPreprocessor : StreamPreprocessor {
 
-    override fun regexpForProblem() = Regex(
+    @VisibleForTesting
+    internal val regexpForProblem = Regex(
         // Examples:
         // TRIGGER:-P2DT
         // TRIGGER:-PT2D
@@ -21,11 +24,11 @@ class FixInvalidDayOffsetPreprocessor : StreamPreprocessor() {
         setOf(RegexOption.MULTILINE, RegexOption.IGNORE_CASE)
     )
 
-    override fun fixString(original: String): String {
-        var iCal: String = original
+    override fun repairLine(line: String): String {
+        var iCal: String = line
 
         // Find all instances matching the defined expression
-        val found = regexpForProblem().findAll(iCal).toList()
+        val found = regexpForProblem.findAll(iCal).toList()
 
         // ... and repair them. Use reversed order so that already replaced occurrences don't interfere with the following matches.
         for (match in found.reversed()) {

lib/src/main/kotlin/at/bitfire/synctools/icalendar/validation/FixInvalidUtcOffsetPreprocessor.kt

@@ -6,28 +6,28 @@
 
 package at.bitfire.synctools.icalendar.validation
 
+import androidx.annotation.VisibleForTesting
 import java.util.logging.Level
 import java.util.logging.Logger
 
 
 /**
  * Some servers modify UTC offsets in TZOFFSET(FROM,TO) like "+005730" to an invalid "+5730".
  *
- * Rewrites values of all TZOFFSETFROM and TZOFFSETTO properties which match [TZOFFSET_REGEXP]
+ * Rewrites values of all TZOFFSETFROM and TZOFFSETTO properties which match [regexpForProblem]
  * so that an hour value of 00 is inserted.
  */
-class FixInvalidUtcOffsetPreprocessor: StreamPreprocessor() {
+class FixInvalidUtcOffsetPreprocessor: StreamPreprocessor {
 
     private val logger
         get() = Logger.getLogger(javaClass.name)
 
-    private val TZOFFSET_REGEXP = Regex("^(TZOFFSET(FROM|TO):[+\\-]?)((18|19|[2-6]\\d)\\d\\d)$",
+    @VisibleForTesting
+    internal val regexpForProblem = Regex("^(TZOFFSET(FROM|TO):[+\\-]?)((18|19|[2-6]\\d)\\d\\d)$",
         setOf(RegexOption.MULTILINE, RegexOption.IGNORE_CASE))
 
-    override fun regexpForProblem() = TZOFFSET_REGEXP
-
-    override fun fixString(original: String) =
-        original.replace(TZOFFSET_REGEXP) {
+    override fun repairLine(line: String) =
+        line.replace(regexpForProblem) {
             logger.log(Level.FINE, "Applying Synology WebDAV fix to invalid utc-offset", it.value)
             "${it.groupValues[1]}00${it.groupValues[3]}"
         }

lib/src/main/kotlin/at/bitfire/synctools/icalendar/validation/ICalPreprocessor.kt

@@ -7,15 +7,18 @@
 package at.bitfire.synctools.icalendar.validation
 
 import androidx.annotation.VisibleForTesting
+import com.google.common.io.CharSource
 import net.fortuna.ical4j.model.Calendar
 import net.fortuna.ical4j.model.Property
 import net.fortuna.ical4j.transform.rfc5545.CreatedPropertyRule
 import net.fortuna.ical4j.transform.rfc5545.DateListPropertyRule
 import net.fortuna.ical4j.transform.rfc5545.DatePropertyRule
 import net.fortuna.ical4j.transform.rfc5545.Rfc5545PropertyRule
+import java.io.BufferedReader
 import java.io.Reader
 import java.util.logging.Level
 import java.util.logging.Logger
+import javax.annotation.WillNotClose
 
 /**
  * Applies some rules to increase compatibility of parsed (incoming) iCalendars:
@@ -40,18 +43,50 @@ class ICalPreprocessor {
         FixInvalidDayOffsetPreprocessor()   // fix things like DURATION:PT2D
     )
 
+    /**
+     * Applies [streamPreprocessors] to a given iCalendar [line].
+     *
+     * @param line original line (taken from an iCalendar)
+     * @return the potentially repaired iCalendar line
+     */
+    @VisibleForTesting
+    fun applyPreprocessors(line: String): String {
+        var newLine = line
+        for (preprocessor in streamPreprocessors)
+            newLine = preprocessor.repairLine(newLine)
+        return newLine
+    }
+
     /**
      * Applies [streamPreprocessors] to a given [Reader] that reads an iCalendar object
      * in order to repair some things that must be fixed before parsing.
      *
-     * @param original    original iCalendar object
-     * @return            the potentially repaired iCalendar object
+     * The original reader content is processed line by line to avoid loading
+     * the whole content into memory at once.
+     *
+     * This method works in a streaming way, so **[original] must not be closed before
+     * the result of this method is consumed** like that:
+     *
+     * ~~~
+     * someSource.reader().use { original ->
+     *   val repaired = preprocessStream(original)
+     *   // closing original here would render repaired unusable, too
+     *   parse(repaired)
+     * } // use will close original
+     * ~~~
+     *
+     * @param original  original iCalendar object (must be closed by caller _after_ consuming the result of this method)
+     * @return potentially repaired iCalendar object (doesn't need to be closed separately)
      */
-    fun preprocessStream(original: Reader): Reader {
-        var reader = original
-        for (preprocessor in streamPreprocessors)
-            reader = preprocessor.preprocess(reader)
-        return reader
+    fun preprocessStream(@WillNotClose original: Reader): Reader {
+        val repairedLines = BufferedReader(original)
+            .lineSequence()
+            .map { line ->      // BufferedReader provides line without line break
+                val fixed = applyPreprocessors(line)
+                CharSource.wrap(fixed + "\r\n")     // iCalendar uses CR+LF
+            }
+            .asIterable()
+        return CharSource.concat(repairedLines).openStream()
     }
 
 

lib/src/main/kotlin/at/bitfire/synctools/icalendar/validation/StreamPreprocessor.kt

@@ -6,51 +6,20 @@
 
 package at.bitfire.synctools.icalendar.validation
 
-import java.io.IOException
-import java.io.Reader
-import java.io.StringReader
-import java.util.Scanner
-
-abstract class StreamPreprocessor {
-
-    abstract fun regexpForProblem(): Regex?
+/**
+ * This is a pre-processor for iCalendar lines that can detect and repair errors which
+ * cannot be repaired on a higher level (because parsing alone would cause syntax
+ * or other unrecoverable errors).
+ */
+interface StreamPreprocessor {
 
     /**
-     * Fixes an iCalendar string.
+     * Validates and potentially repairs an iCalendar string.
      *
-     * @param original The complete iCalendar string
-     * @return The complete iCalendar string, but fixed
+     * @param line  full line of an iCalendar lines to validate / fix (without line break)
+     *
+     * @return the potentially fixed version of [line] (without line break)
      */
-    abstract fun fixString(original: String): String
-
-    fun preprocess(reader: Reader): Reader {
-        var result: String? = null
-
-        val resetSupported = try {
-            reader.reset()
-            true
-        } catch(_: IOException) {
-            false
-        }
-
-        if (resetSupported) {
-            val regex = regexpForProblem()
-            // reset is supported, no need to copy the whole stream to another String (unless we have to fix the TZOFFSET)
-            if (regex == null || Scanner(reader).findWithinHorizon(regex.toPattern(), 0) != null) {
-                reader.reset()
-                result = fixString(reader.readText())
-            }
-        } else
-            // reset not supported, always generate a new String that will be returned
-            result = fixString(reader.readText())
-
-        if (result != null)
-            // modified or reset not supported, return new stream
-            return StringReader(result)
-
-        // not modified, return original iCalendar
-        reader.reset()
-        return reader
-    }
+    fun repairLine(line: String): String
 
 }

lib/src/test/kotlin/at/bitfire/synctools/icalendar/validation/FixInvalidDayOffsetPreprocessorTest.kt

@@ -25,7 +25,7 @@ class FixInvalidDayOffsetPreprocessorTest {
      */
     private fun assertFixedEquals(expected: String, testValue: String, parseDuration: Boolean = true) {
         // Fix the duration string
-        val fixed = processor.fixString(testValue)
+        val fixed = processor.repairLine(testValue)
 
         // Test the duration can now be parsed
         if (parseDuration)
@@ -39,15 +39,15 @@ class FixInvalidDayOffsetPreprocessorTest {
     }
 
     @Test
-    fun test_FixString_NoOccurrence() {
+    fun test_repairLine_NoOccurrence() {
         assertEquals(
             "Some String",
-            processor.fixString("Some String"),
+            processor.repairLine("Some String"),
         )
     }
 
     @Test
-    fun test_FixString_SucceedsAsValueOnCorrectProperties() {
+    fun test_repairLine_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")
@@ -57,18 +57,18 @@ class FixInvalidDayOffsetPreprocessorTest {
     }
 
     @Test
-    fun test_FixString_FailsAsValueOnWrongProperty() {
+    fun test_repairLine_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() {
+    fun test_repairLine_FailsIfNotAtStartOfLine() {
         assertFixedEquals("xxDURATION;VALUE=DURATION:PT1D", "xxDURATION;VALUE=DURATION:PT1D", parseDuration = false)
     }
 
     @Test
-    fun test_FixString_DayOffsetFrom_Invalid() {
+    fun test_repairLine_DayOffsetFrom_Invalid() {
         assertFixedEquals("DURATION:-P1D", "DURATION:-PT1D")
         assertFixedEquals("TRIGGER:-P2D", "TRIGGER:-PT2D")
 
@@ -77,38 +77,38 @@ class FixInvalidDayOffsetPreprocessorTest {
     }
 
     @Test
-    fun test_FixString_DayOffsetFrom_Valid() {
+    fun test_repairLine_DayOffsetFrom_Valid() {
         assertFixedEquals("DURATION:-PT12H", "DURATION:-PT12H")
         assertFixedEquals("TRIGGER:-PT12H", "TRIGGER:-PT12H")
     }
 
     @Test
-    fun test_FixString_DayOffsetFromMultiple_Invalid() {
+    fun test_repairLine_DayOffsetFromMultiple_Invalid() {
         assertFixedEquals("DURATION:-P1D\nTRIGGER:-P2D", "DURATION:-PT1D\nTRIGGER:-PT2D")
         assertFixedEquals("DURATION:-P1D\nTRIGGER:-P2D", "DURATION:-P1DT\nTRIGGER:-P2DT")
     }
 
     @Test
-    fun test_FixString_DayOffsetFromMultiple_Valid() {
+    fun test_repairLine_DayOffsetFromMultiple_Valid() {
         assertFixedEquals("DURATION:-PT12H\nTRIGGER:-PT12H", "DURATION:-PT12H\nTRIGGER:-PT12H")
     }
 
     @Test
-    fun test_FixString_DayOffsetFromMultiple_Mixed() {
+    fun test_repairLine_DayOffsetFromMultiple_Mixed() {
         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
     fun test_RegexpForProblem_DayOffsetTo_Invalid() {
-        val regex = processor.regexpForProblem()
+        val regex = processor.regexpForProblem
         assertTrue(regex.matches("DURATION:PT2D"))
         assertTrue(regex.matches("TRIGGER:PT1D"))
     }
 
     @Test
     fun test_RegexpForProblem_DayOffsetTo_Valid() {
-        val regex = processor.regexpForProblem()
+        val regex = processor.regexpForProblem
         assertFalse(regex.matches("DURATION:-PT12H"))
         assertFalse(regex.matches("TRIGGER:-PT15M"))
     }

lib/src/test/kotlin/at/bitfire/synctools/icalendar/validation/FixInvalidUtcOffsetPreprocessorTest.kt

@@ -16,46 +16,46 @@ class FixInvalidUtcOffsetPreprocessorTest {
     private val processor = FixInvalidUtcOffsetPreprocessor()
 
     @Test
-    fun test_FixString_NoOccurrence() {
+    fun test_repairLine_NoOccurrence() {
         assertEquals(
             "Some String",
-            processor.fixString("Some String"))
+            processor.repairLine("Some String"))
     }
 
     @Test
-    fun test_FixString_TzOffsetFrom_Invalid() {
+    fun test_repairLine_TzOffsetFrom_Invalid() {
         assertEquals("TZOFFSETFROM:+005730",
-            processor.fixString("TZOFFSETFROM:+5730"))
+            processor.repairLine("TZOFFSETFROM:+5730"))
     }
 
     @Test
-    fun test_FixString_TzOffsetFrom_Valid() {
+    fun test_repairLine_TzOffsetFrom_Valid() {
         assertEquals("TZOFFSETFROM:+005730",
-            processor.fixString("TZOFFSETFROM:+005730"))
+            processor.repairLine("TZOFFSETFROM:+005730"))
     }
 
     @Test
-    fun test_FixString_TzOffsetTo_Invalid() {
+    fun test_repairLine_TzOffsetTo_Invalid() {
         assertEquals("TZOFFSETTO:+005730",
-            processor.fixString("TZOFFSETTO:+5730"))
+            processor.repairLine("TZOFFSETTO:+5730"))
     }
 
     @Test
-    fun test_FixString_TzOffsetTo_Valid() {
+    fun test_repairLine_TzOffsetTo_Valid() {
         assertEquals("TZOFFSETTO:+005730",
-            processor.fixString("TZOFFSETTO:+005730"))
+            processor.repairLine("TZOFFSETTO:+005730"))
     }
 
 
     @Test
     fun test_RegexpForProblem_TzOffsetTo_Invalid() {
-        val regex = processor.regexpForProblem()
+        val regex = processor.regexpForProblem
         assertTrue(regex.matches("TZOFFSETTO:+5730"))
     }
 
     @Test
     fun test_RegexpForProblem_TzOffsetTo_Valid() {
-        val regex = processor.regexpForProblem()
+        val regex = processor.regexpForProblem
         assertFalse(regex.matches("TZOFFSETTO:+005730"))
     }