From e152d6e32d703e4effd2f4fa31ae6394ce42bd73 Mon Sep 17 00:00:00 2001 From: Milan Barta Date: Mon, 16 Sep 2024 09:46:45 +0200 Subject: [PATCH 1/4] Handle errors when parsing remote config JSON --- .../config/TurboPathConfigurationLoader.kt | 26 +++++++++---------- .../TurboPathConfigurationRepository.kt | 12 ++++++++- 2 files changed, 24 insertions(+), 14 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 index 15953fbc..8ea9ece6 100644 --- 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,6 @@ 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.util.dispatcherProvider import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job @@ -30,29 +28,31 @@ 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 { remoteConfigJson -> + repository.parseFromJson(remoteConfigJson)?.let { config -> + onCompletion(config) + cacheConfigurationForUrl(url, config) + } } } } private fun loadBundledAssetConfiguration(filePath: String, onCompletion: (TurboPathConfiguration) -> Unit) { - val configuration = repository.getBundledConfiguration(context, filePath) - onCompletion(load(configuration)) + val bundledConfigJson = repository.getBundledConfiguration(context, filePath) + repository.parseFromJson(bundledConfigJson)?.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 { cachedConfigJson -> + repository.parseFromJson(cachedConfigJson)?.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() {}) - } } diff --git a/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt b/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt index 6c09c9c5..3fcd9635 100644 --- a/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt +++ b/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt @@ -3,13 +3,14 @@ package dev.hotwire.turbo.config import android.content.Context import android.content.SharedPreferences 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" @@ -66,4 +67,13 @@ internal class TurboPathConfigurationRepository { String(it.readBytes()) } } + + fun parseFromJson(json: String): TurboPathConfiguration? { + return try { + json.toObject(object : TypeToken() {}) + } catch (e: Exception) { + logError("PathConfigurationLoadingException", e) + null + } + } } From c4d9b1985457ef0996004f1cda06011b38acaa31 Mon Sep 17 00:00:00 2001 From: Milan Barta Date: Mon, 16 Sep 2024 09:47:19 +0200 Subject: [PATCH 2/4] Add test for malformed remote config JSON parsing --- .../TurboPathConfigurationRepositoryTest.kt | 29 ++++++++++++------- .../test-configuration-malformed.json | 1 + 2 files changed, 20 insertions(+), 10 deletions(-) create mode 100644 turbo/src/test/resources/http-responses/test-configuration-malformed.json diff --git a/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt b/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt index 553eb063..e05777df 100644 --- a/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt +++ b/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt @@ -5,8 +5,6 @@ import android.os.Build import androidx.test.core.app.ApplicationProvider import dev.hotwire.turbo.BaseRepositoryTest 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 @@ -39,38 +37,49 @@ class TurboPathConfigurationRepositoryTest : BaseRepositoryTest() { val json = repository.getRemoteConfiguration(baseUrl()) assertThat(json).isNotNull() - val config = load(json) + val config = repository.parseFromJson(json!!) assertThat(config?.rules?.size).isEqualTo(2) } } } + @Test + fun getMalformedRemoteConfiguration() { + enqueueResponse("test-configuration-malformed.json") + + runBlocking { + launch(Dispatchers.Main) { + val json = repository.getRemoteConfiguration(baseUrl()) + assertThat(json).isNotNull() + + val config = repository.parseFromJson(json!!) + assertThat(config).isNull() + } + } + } + @Test fun getBundledAssetConfiguration() { val json = repository.getBundledConfiguration(context, "json/test-configuration.json") assertThat(json).isNotNull() - val config = load(json) + val config = repository.parseFromJson(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 = load(json) + val cachedConfig = repository.parseFromJson(json!!) assertThat(cachedConfig?.rules?.size).isEqualTo(1) } - private fun load(json: String?): TurboPathConfiguration? { - return json?.toObject(object : TypeToken() {}) - } - 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 new file mode 100644 index 00000000..7a0581c3 --- /dev/null +++ b/turbo/src/test/resources/http-responses/test-configuration-malformed.json @@ -0,0 +1 @@ +Not a valid path configuration \ No newline at end of file From 23fa80797141040fe3210429fc0e0517f774e1c1 Mon Sep 17 00:00:00 2001 From: Milan Barta Date: Mon, 16 Sep 2024 15:41:11 +0200 Subject: [PATCH 3/4] Refactor code --- .../turbo/config/TurboPathConfigurationLoader.kt | 10 ++++------ .../turbo/config/TurboPathConfigurationRepository.kt | 12 ++++++++---- .../config/TurboPathConfigurationRepositoryTest.kt | 11 +++++------ 3 files changed, 17 insertions(+), 16 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 index 8ea9ece6..3ce876c3 100644 --- a/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationLoader.kt +++ b/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationLoader.kt @@ -1,6 +1,7 @@ package dev.hotwire.turbo.config import android.content.Context +import dev.hotwire.turbo.config.Turbo.config import dev.hotwire.turbo.util.dispatcherProvider import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job @@ -38,17 +39,14 @@ internal class TurboPathConfigurationLoader(val context: Context) : CoroutineSco } private fun loadBundledAssetConfiguration(filePath: String, onCompletion: (TurboPathConfiguration) -> Unit) { - val bundledConfigJson = repository.getBundledConfiguration(context, filePath) - repository.parseFromJson(bundledConfigJson)?.let { config -> + repository.getBundledConfiguration(context, filePath)?.let { config -> onCompletion(config) } } private fun loadCachedConfigurationForUrl(url: String, onCompletion: (TurboPathConfiguration) -> Unit) { - repository.getCachedConfigurationForUrl(context, url)?.let { cachedConfigJson -> - repository.parseFromJson(cachedConfigJson)?.let { config -> - onCompletion(config) - } + repository.getCachedConfigurationForUrl(context, url)?.let { config -> + onCompletion(config) } } diff --git a/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt b/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt index 3fcd9635..bdc3672a 100644 --- a/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt +++ b/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt @@ -2,6 +2,7 @@ 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 @@ -23,12 +24,14 @@ internal class TurboPathConfigurationRepository { } } - 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) { @@ -68,6 +71,7 @@ internal class TurboPathConfigurationRepository { } } + @VisibleForTesting fun parseFromJson(json: String): TurboPathConfiguration? { return try { json.toObject(object : TypeToken() {}) diff --git a/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt b/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt index e05777df..a2f6e7fe 100644 --- a/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt +++ b/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt @@ -4,6 +4,7 @@ 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 kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -60,10 +61,9 @@ class TurboPathConfigurationRepositoryTest : BaseRepositoryTest() { @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 = repository.parseFromJson(json) assertThat(config?.rules?.size).isEqualTo(10) } @@ -73,10 +73,9 @@ class TurboPathConfigurationRepositoryTest : BaseRepositoryTest() { 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 = repository.parseFromJson(json!!) assertThat(cachedConfig?.rules?.size).isEqualTo(1) } From ad43e4a86440f16619ebc2e66ff14993634216ca Mon Sep 17 00:00:00 2001 From: Milan Barta Date: Mon, 16 Sep 2024 15:50:56 +0200 Subject: [PATCH 4/4] Refactor remote config code --- .../turbo/config/TurboPathConfigurationLoader.kt | 8 +++----- .../turbo/config/TurboPathConfigurationRepository.kt | 4 ++-- .../config/TurboPathConfigurationRepositoryTest.kt | 11 +++-------- 3 files changed, 8 insertions(+), 15 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 index 3ce876c3..a05375d7 100644 --- a/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationLoader.kt +++ b/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationLoader.kt @@ -29,11 +29,9 @@ internal class TurboPathConfigurationLoader(val context: Context) : CoroutineSco loadCachedConfigurationForUrl(url, onCompletion) launch { - repository.getRemoteConfiguration(url)?.let { remoteConfigJson -> - repository.parseFromJson(remoteConfigJson)?.let { config -> - onCompletion(config) - cacheConfigurationForUrl(url, config) - } + repository.getRemoteConfiguration(url)?.let { config -> + onCompletion(config) + cacheConfigurationForUrl(url, config) } } } diff --git a/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt b/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt index bdc3672a..90895ac9 100644 --- a/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt +++ b/turbo/src/main/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepository.kt @@ -16,11 +16,11 @@ import okhttp3.Request 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) } } } diff --git a/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt b/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt index a2f6e7fe..71d92353 100644 --- a/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt +++ b/turbo/src/test/kotlin/dev/hotwire/turbo/config/TurboPathConfigurationRepositoryTest.kt @@ -35,10 +35,8 @@ class TurboPathConfigurationRepositoryTest : BaseRepositoryTest() { runBlocking { launch(Dispatchers.Main) { - val json = repository.getRemoteConfiguration(baseUrl()) - assertThat(json).isNotNull() - - val config = repository.parseFromJson(json!!) + val config = repository.getRemoteConfiguration(baseUrl()) + assertThat(config).isNotNull() assertThat(config?.rules?.size).isEqualTo(2) } } @@ -50,10 +48,7 @@ class TurboPathConfigurationRepositoryTest : BaseRepositoryTest() { runBlocking { launch(Dispatchers.Main) { - val json = repository.getRemoteConfiguration(baseUrl()) - assertThat(json).isNotNull() - - val config = repository.parseFromJson(json!!) + val config = repository.getRemoteConfiguration(baseUrl()) assertThat(config).isNull() } }