Fixed legacy config (EXPOSUREAPP-4195) (#1858)

* legacy config is never valid and should always trigger download of remote config

* If the validity duration is 0, the config is never valid.
Add unit tests for validity edge cases.

* Add negative time cases.

Co-authored-by: Matthias Urhahn <matthias.urhahn@sap.com>

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/appconfig/internal/ConfigDataContainer.kt

@@ -15,8 +15,10 @@ data class ConfigDataContainer(
 ) : ConfigData, ConfigMapping by mappedConfig {
     override val updatedAt: Instant = serverTime.plus(localOffset)
 
-    override fun isValid(nowUTC: Instant): Boolean {
+    override fun isValid(nowUTC: Instant): Boolean = if (cacheValidity == Duration.ZERO) {
+        false
+    } else {
         val expiresAt = updatedAt.plus(cacheValidity)
-        return nowUTC.isBefore(expiresAt)
+        nowUTC.isBefore(expiresAt)
     }
 }

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/appconfig/sources/local/AppConfigStorage.kt

@@ -49,10 +49,10 @@ class AppConfigStorage @Inject constructor(
             return@withLock try {
                 InternalConfigData(
                     rawData = legacyConfigFile.readBytes(),
-                    serverTime = timeStamper.nowUTC,
+                    serverTime = Instant.ofEpochMilli(legacyConfigFile.lastModified()),
                     localOffset = Duration.ZERO,
                     etag = "legacy.migration",
-                    cacheValidity = Duration.standardMinutes(5)
+                    cacheValidity = Duration.standardSeconds(0)
                 )
             } catch (e: Exception) {
                 Timber.e(e, "Legacy config exits but couldn't be read.")
@@ -80,6 +80,12 @@ class AppConfigStorage @Inject constructor(
             Timber.v("Overwriting %d from %s", configFile.length(), configFile.lastModified())
         }
 
+        if (legacyConfigFile.exists()) {
+            if (legacyConfigFile.delete()) {
+                Timber.i("Legacy config file deleted, superseeded.")
+            }
+        }
+
         if (value == null) {
             if (configFile.delete()) Timber.d("Config file was deleted (value=null).")
             return
@@ -87,12 +93,6 @@ class AppConfigStorage @Inject constructor(
 
         try {
             gson.toJson(value, configFile)
-
-            if (legacyConfigFile.exists()) {
-                if (legacyConfigFile.delete()) {
-                    Timber.i("Legacy config file deleted, superseeded.")
-                }
-            }
         } catch (e: Exception) {
             // We'll not rethrow as we could still keep working just with the remote config,
             // but we will notify the user.

Corona-Warn-App/src/test/java/de/rki/coronawarnapp/appconfig/internal/ConfigDataContainerTest.kt

+package de.rki.coronawarnapp.appconfig.internal
+
+import de.rki.coronawarnapp.appconfig.ConfigData
+import io.kotest.matchers.shouldBe
+import io.mockk.mockk
+import org.joda.time.Duration
+import org.joda.time.Instant
+import org.junit.jupiter.api.Test
+import testhelpers.BaseTest
+
+class ConfigDataContainerTest : BaseTest() {
+
+    @Test
+    fun `cache validity is evaluated`() {
+        val now = Instant.EPOCH
+        val config = ConfigDataContainer(
+            serverTime = now,
+            localOffset = Duration.standardHours(1),
+            mappedConfig = mockk(),
+            configType = ConfigData.Type.LAST_RETRIEVED,
+            identifier = "localetag",
+            cacheValidity = Duration.standardSeconds(300)
+        )
+        config.isValid(now) shouldBe true
+        config.isValid(now.plus(Duration.standardSeconds(300))) shouldBe true
+        config.isValid(now.minus(Duration.standardSeconds(300))) shouldBe true
+
+        val nowWithOffset = now.plus(config.localOffset)
+        config.isValid(nowWithOffset.plus(Duration.standardSeconds(299))) shouldBe true
+        config.isValid(nowWithOffset.minus(Duration.standardSeconds(299))) shouldBe true
+
+        config.isValid(nowWithOffset) shouldBe true
+        config.isValid(nowWithOffset.minus(Duration.standardSeconds(300))) shouldBe true
+        config.isValid(nowWithOffset.plus(Duration.standardSeconds(300))) shouldBe false
+    }
+
+    @Test
+    fun `cache validity can be set to 0`() {
+        val config = ConfigDataContainer(
+            serverTime = Instant.EPOCH,
+            localOffset = Duration.standardHours(1),
+            mappedConfig = mockk(),
+            configType = ConfigData.Type.LAST_RETRIEVED,
+            identifier = "localetag",
+            cacheValidity = Duration.standardSeconds(0)
+        )
+        config.isValid(Instant.EPOCH) shouldBe false
+        config.isValid(Instant.EPOCH.plus(Duration.standardHours(1))) shouldBe false
+        config.isValid(Instant.EPOCH.plus(Duration.standardHours(24))) shouldBe false
+        config.isValid(Instant.EPOCH.plus(Duration.standardDays(14))) shouldBe false
+
+        config.isValid(Instant.EPOCH.minus(Duration.standardHours(1))) shouldBe false
+        config.isValid(Instant.EPOCH.minus(Duration.standardHours(24))) shouldBe false
+        config.isValid(Instant.EPOCH.minus(Duration.standardDays(14))) shouldBe false
+    }
+
+    @Test
+    fun `updated at is based on servertime and offset`() {
+        val config = ConfigDataContainer(
+            serverTime = Instant.EPOCH,
+            localOffset = Duration.standardHours(1),
+            mappedConfig = mockk(),
+            configType = ConfigData.Type.LAST_RETRIEVED,
+            identifier = "localetag",
+            cacheValidity = Duration.standardSeconds(0)
+        )
+        config.updatedAt shouldBe Instant.EPOCH.plus(Duration.standardHours(1))
+    }
+}

Corona-Warn-App/src/test/java/de/rki/coronawarnapp/appconfig/sources/local/AppConfigStorageTest.kt

@@ -130,6 +130,26 @@ class AppConfigStorageTest : BaseIOTest() {
         configPath.exists() shouldBe false
     }
 
+    @Test
+    fun `nulling deletes legacy config`() = runBlockingTest {
+        val storage = createStorage()
+        configPath.exists() shouldBe false
+
+        storage.getStoredConfig() shouldBe null
+        storage.setStoredConfig(null)
+        configPath.exists() shouldBe false
+
+        legacyConfigPath.exists() shouldBe false
+        legacyConfigPath.parentFile!!.mkdirs()
+        legacyConfigPath.writeBytes(APPCONFIG_RAW)
+        legacyConfigPath.exists() shouldBe true
+
+        storage.setStoredConfig(null)
+        storage.getStoredConfig() shouldBe null
+        configPath.exists() shouldBe false
+        legacyConfigPath.exists() shouldBe false
+    }
+
     @Test
     fun `if no fallback exists, but we have a legacy config, use that`() = runBlockingTest {
         configPath.exists() shouldBe false
@@ -142,10 +162,10 @@ class AppConfigStorageTest : BaseIOTest() {
 
         storage.getStoredConfig() shouldBe InternalConfigData(
             rawData = APPCONFIG_RAW,
-            serverTime = Instant.ofEpochMilli(1234),
+            serverTime = Instant.ofEpochMilli(legacyConfigPath.lastModified()),
             localOffset = Duration.ZERO,
             etag = "I am an ETag :)!",
-            cacheValidity = Duration.standardMinutes(5)
+            cacheValidity = Duration.standardSeconds(0)
         )
     }