Merge pull request #342 from hotwired/config-parsing-fix

Path Configuration parsing fix
hotwired · Sep 16, 2024 · 52cdc35 · 52cdc35
2 parents e58d686 + ad43e4a
commit 52cdc35
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 37 deletions.
diff --git a/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationLoader.kt b/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationLoader.kt
@@ -1,8 +1,7 @@
 package dev.hotwire.turbo.config
 
 import android.content.Context
-import dev.hotwire.turbo.util.toObject
-import com.google.gson.reflect.TypeToken
+import dev.hotwire.turbo.config.Turbo.config
 import dev.hotwire.turbo.util.dispatcherProvider
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Job
@@ -30,29 +29,26 @@ internal class TurboPathConfigurationLoader(val context: Context) : CoroutineSco
         loadCachedConfigurationForUrl(url, onCompletion)
 
         launch {
-            repository.getRemoteConfiguration(url)?.let {
-                onCompletion(load(it))
-                cacheConfigurationForUrl(url, load(it))
+            repository.getRemoteConfiguration(url)?.let { config ->
+                onCompletion(config)
+                cacheConfigurationForUrl(url, config)
             }
         }
     }
 
     private fun loadBundledAssetConfiguration(filePath: String, onCompletion: (TurboPathConfiguration) -> Unit) {
-        val configuration = repository.getBundledConfiguration(context, filePath)
-        onCompletion(load(configuration))
+        repository.getBundledConfiguration(context, filePath)?.let { config ->
+            onCompletion(config)
+        }
     }
 
     private fun loadCachedConfigurationForUrl(url: String, onCompletion: (TurboPathConfiguration) -> Unit) {
-        repository.getCachedConfigurationForUrl(context, url)?.let {
-            onCompletion(load(it))
+        repository.getCachedConfigurationForUrl(context, url)?.let { config ->
+            onCompletion(config)
         }
     }
 
     private fun cacheConfigurationForUrl(url: String, pathConfiguration: TurboPathConfiguration) {
         repository.cacheConfigurationForUrl(context, url, pathConfiguration)
     }
-
-    private fun load(json: String): TurboPathConfiguration {
-        return json.toObject(object : TypeToken<TurboPathConfiguration>() {})
-    }
 }
diff --git a/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt b/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt
@@ -2,32 +2,36 @@ package dev.hotwire.turbo.config
 
 import android.content.Context
 import android.content.SharedPreferences
+import androidx.annotation.VisibleForTesting
 import androidx.core.content.edit
+import com.google.gson.reflect.TypeToken
 import dev.hotwire.turbo.http.TurboHttpClient
 import dev.hotwire.turbo.util.dispatcherProvider
 import dev.hotwire.turbo.util.logError
 import dev.hotwire.turbo.util.toJson
+import dev.hotwire.turbo.util.toObject
 import kotlinx.coroutines.withContext
 import okhttp3.Request
-import java.io.IOException
 
 internal class TurboPathConfigurationRepository {
     private val cacheFile = "turbo"
 
-    suspend fun getRemoteConfiguration(url: String): String? {
+    suspend fun getRemoteConfiguration(url: String): TurboPathConfiguration? {
         val request = Request.Builder().url(url).build()
 
         return withContext(dispatcherProvider.io) {
-            issueRequest(request)
+            issueRequest(request)?.let { parseFromJson(it) }
         }
     }
 
-    fun getBundledConfiguration(context: Context, filePath: String): String {
-        return contentFromAsset(context, filePath)
+    fun getBundledConfiguration(context: Context, filePath: String): TurboPathConfiguration? {
+        val bundledConfigJson = contentFromAsset(context, filePath)
+        return parseFromJson(bundledConfigJson)
     }
 
-    fun getCachedConfigurationForUrl(context: Context, url: String): String? {
-        return prefs(context).getString(url, null)
+    fun getCachedConfigurationForUrl(context: Context, url: String): TurboPathConfiguration? {
+        val cachedConfigJson = prefs(context).getString(url, null)
+        return cachedConfigJson?.let { parseFromJson(it) }
     }
 
     fun cacheConfigurationForUrl(context: Context, url: String, pathConfiguration: TurboPathConfiguration) {
@@ -66,4 +70,14 @@ internal class TurboPathConfigurationRepository {
             String(it.readBytes())
         }
     }
+
+    @VisibleForTesting
+    fun parseFromJson(json: String): TurboPathConfiguration? {
+        return try {
+            json.toObject(object : TypeToken<TurboPathConfiguration>() {})
+        } catch (e: Exception) {
+            logError("PathConfigurationLoadingException", e)
+            null
+        }
+    }
 }
diff --git a/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt b/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt
@@ -4,9 +4,8 @@ import android.content.Context
 import android.os.Build
 import androidx.test.core.app.ApplicationProvider
 import dev.hotwire.turbo.BaseRepositoryTest
+import dev.hotwire.turbo.config.Turbo.config
 import dev.hotwire.turbo.http.TurboHttpClient
-import dev.hotwire.turbo.util.toObject
-import com.google.gson.reflect.TypeToken
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.launch
@@ -36,41 +35,45 @@ class TurboPathConfigurationRepositoryTest : BaseRepositoryTest() {
 
         runBlocking {
             launch(Dispatchers.Main) {
-                val json = repository.getRemoteConfiguration(baseUrl())
-                assertThat(json).isNotNull()
-
-                val config = load(json)
+                val config = repository.getRemoteConfiguration(baseUrl())
+                assertThat(config).isNotNull()
                 assertThat(config?.rules?.size).isEqualTo(2)
             }
         }
     }
 
+    @Test
+    fun getMalformedRemoteConfiguration() {
+        enqueueResponse("test-configuration-malformed.json")
+
+        runBlocking {
+            launch(Dispatchers.Main) {
+                val config = repository.getRemoteConfiguration(baseUrl())
+                assertThat(config).isNull()
+            }
+        }
+    }
+
     @Test
     fun getBundledAssetConfiguration() {
-        val json = repository.getBundledConfiguration(context, "json/test-configuration.json")
-        assertThat(json).isNotNull()
+        val config = repository.getBundledConfiguration(context, "json/test-configuration.json")
+        assertThat(config).isNotNull()
 
-        val config = load(json)
         assertThat(config?.rules?.size).isEqualTo(10)
     }
 
     @Test
     fun getCachedConfiguration() {
         val url = "https://turbo.hotwired.dev/demo/configurations/android-v1.json"
-        val config = requireNotNull(load(json()))
+        val config = requireNotNull(repository.parseFromJson(json()))
         repository.cacheConfigurationForUrl(context, url, config)
 
-        val json = repository.getCachedConfigurationForUrl(context, url)
-        assertThat(json).isNotNull()
+        val cachedConfig = repository.getCachedConfigurationForUrl(context, url)
+        assertThat(cachedConfig).isNotNull()
 
-        val cachedConfig = load(json)
         assertThat(cachedConfig?.rules?.size).isEqualTo(1)
     }
 
-    private fun load(json: String?): TurboPathConfiguration? {
-        return json?.toObject(object : TypeToken<TurboPathConfiguration>() {})
-    }
-
     private fun json(): String {
         return """
         {

diff --git a/turbo/src/test/resources/http-responses/test-configuration-malformed.json b/turbo/src/test/resources/http-responses/test-configuration-malformed.json
@@ -0,0 +1 @@
+Not a valid path configuration