From c8f1f477cc424bbb0b8279ec37e06deeaf74bbd8 Mon Sep 17 00:00:00 2001 From: Kirill Kamakin Date: Sat, 13 Jan 2024 12:18:12 +0100 Subject: [PATCH] Fix transition when base URL is actually incorrect (#196) --- .../mealient/data/baseurl/ServerInfoRepo.kt | 3 ++- .../data/baseurl/ServerInfoRepoImpl.kt | 18 ++++++----------- .../data/baseurl/VersionDataSource.kt | 2 +- .../data/baseurl/VersionDataSourceImpl.kt | 4 ++-- .../mealient/ui/baseurl/BaseURLViewModel.kt | 2 +- .../data/baseurl/ServerInfoRepoTest.kt | 9 ++++++++- .../mealient/test/AuthImplTestData.kt | 2 ++ .../ui/baseurl/BaseURLViewModelTest.kt | 7 ++++--- .../mealient/datasource/MealieDataSource.kt | 2 +- .../mealient/datasource/MealieService.kt | 2 +- .../datasource/impl/MealieDataSourceImpl.kt | 20 ++++++++++--------- .../datasource/impl/MealieServiceKtor.kt | 18 ++++++++++++++--- 12 files changed, 54 insertions(+), 35 deletions(-) diff --git a/app/src/main/java/gq/kirmanak/mealient/data/baseurl/ServerInfoRepo.kt b/app/src/main/java/gq/kirmanak/mealient/data/baseurl/ServerInfoRepo.kt index 624ae6e..8cbfe49 100644 --- a/app/src/main/java/gq/kirmanak/mealient/data/baseurl/ServerInfoRepo.kt +++ b/app/src/main/java/gq/kirmanak/mealient/data/baseurl/ServerInfoRepo.kt @@ -1,5 +1,6 @@ package gq.kirmanak.mealient.data.baseurl +import gq.kirmanak.mealient.datasource.models.VersionResponse import kotlinx.coroutines.flow.Flow interface ServerInfoRepo { @@ -8,7 +9,7 @@ interface ServerInfoRepo { suspend fun getUrl(): String? - suspend fun tryBaseURL(baseURL: String): Result + suspend fun tryBaseURL(baseURL: String): Result } diff --git a/app/src/main/java/gq/kirmanak/mealient/data/baseurl/ServerInfoRepoImpl.kt b/app/src/main/java/gq/kirmanak/mealient/data/baseurl/ServerInfoRepoImpl.kt index 0af8d62..bad2370 100644 --- a/app/src/main/java/gq/kirmanak/mealient/data/baseurl/ServerInfoRepoImpl.kt +++ b/app/src/main/java/gq/kirmanak/mealient/data/baseurl/ServerInfoRepoImpl.kt @@ -1,6 +1,7 @@ package gq.kirmanak.mealient.data.baseurl import gq.kirmanak.mealient.datasource.ServerUrlProvider +import gq.kirmanak.mealient.datasource.models.VersionResponse import gq.kirmanak.mealient.logging.Logger import kotlinx.coroutines.flow.Flow import javax.inject.Inject @@ -20,18 +21,11 @@ class ServerInfoRepoImpl @Inject constructor( return result } - override suspend fun tryBaseURL(baseURL: String): Result { - val oldBaseUrl = serverInfoStorage.getBaseURL() - serverInfoStorage.storeBaseURL(baseURL) - - try { - versionDataSource.requestVersion() - } catch (e: Throwable) { - serverInfoStorage.storeBaseURL(oldBaseUrl) - return Result.failure(e) + override suspend fun tryBaseURL(baseURL: String): Result { + return versionDataSource.runCatching { + requestVersion(baseURL) + }.onSuccess { + serverInfoStorage.storeBaseURL(baseURL) } - - return Result.success(Unit) } - } \ No newline at end of file diff --git a/app/src/main/java/gq/kirmanak/mealient/data/baseurl/VersionDataSource.kt b/app/src/main/java/gq/kirmanak/mealient/data/baseurl/VersionDataSource.kt index 5e2d12c..4b38692 100644 --- a/app/src/main/java/gq/kirmanak/mealient/data/baseurl/VersionDataSource.kt +++ b/app/src/main/java/gq/kirmanak/mealient/data/baseurl/VersionDataSource.kt @@ -4,5 +4,5 @@ import gq.kirmanak.mealient.datasource.models.VersionResponse interface VersionDataSource { - suspend fun requestVersion(): VersionResponse + suspend fun requestVersion(baseURL: String): VersionResponse } \ No newline at end of file diff --git a/app/src/main/java/gq/kirmanak/mealient/data/baseurl/VersionDataSourceImpl.kt b/app/src/main/java/gq/kirmanak/mealient/data/baseurl/VersionDataSourceImpl.kt index 656f546..a9f28e4 100644 --- a/app/src/main/java/gq/kirmanak/mealient/data/baseurl/VersionDataSourceImpl.kt +++ b/app/src/main/java/gq/kirmanak/mealient/data/baseurl/VersionDataSourceImpl.kt @@ -8,7 +8,7 @@ class VersionDataSourceImpl @Inject constructor( private val dataSource: MealieDataSource, ) : VersionDataSource { - override suspend fun requestVersion(): VersionResponse { - return dataSource.getVersionInfo() + override suspend fun requestVersion(baseURL: String): VersionResponse { + return dataSource.getVersionInfo(baseURL) } } \ No newline at end of file diff --git a/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt b/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt index e5fea75..4437dad 100644 --- a/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt +++ b/app/src/main/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModel.kt @@ -77,7 +77,7 @@ internal class BaseURLViewModel @Inject constructor( return } - val result: Result = serverInfoRepo.tryBaseURL(url).recoverCatching { + val result = serverInfoRepo.tryBaseURL(url).recoverCatching { logger.e(it) { "checkBaseURL: trying to recover, had prefix = $hasPrefix" } val certificateError = it.findCauseAsInstanceOf() if (certificateError != null) { diff --git a/app/src/test/java/gq/kirmanak/mealient/data/baseurl/ServerInfoRepoTest.kt b/app/src/test/java/gq/kirmanak/mealient/data/baseurl/ServerInfoRepoTest.kt index a330905..5f872dc 100644 --- a/app/src/test/java/gq/kirmanak/mealient/data/baseurl/ServerInfoRepoTest.kt +++ b/app/src/test/java/gq/kirmanak/mealient/data/baseurl/ServerInfoRepoTest.kt @@ -2,6 +2,7 @@ package gq.kirmanak.mealient.data.baseurl import com.google.common.truth.Truth.assertThat import gq.kirmanak.mealient.test.AuthImplTestData.TEST_BASE_URL +import gq.kirmanak.mealient.test.AuthImplTestData.VERSION_RESPONSE import gq.kirmanak.mealient.test.BaseUnitTest import io.mockk.coEvery import io.mockk.coVerify @@ -48,10 +49,16 @@ class ServerInfoRepoTest : BaseUnitTest() { @Test fun `when tryBaseURL succeeds expect call to storage`() = runTest { - coEvery { storage.getBaseURL() } returns null + coEvery { dataSource.requestVersion(TEST_BASE_URL) } returns VERSION_RESPONSE subject.tryBaseURL(TEST_BASE_URL) coVerify { storage.storeBaseURL(eq(TEST_BASE_URL)) } } + + @Test + fun `when tryBaseURL succeeds expect response`() = runTest { + coEvery { dataSource.requestVersion(TEST_BASE_URL) } returns VERSION_RESPONSE + assertThat(subject.tryBaseURL(TEST_BASE_URL)).isEqualTo(Result.success(VERSION_RESPONSE)) + } } \ No newline at end of file diff --git a/app/src/test/java/gq/kirmanak/mealient/test/AuthImplTestData.kt b/app/src/test/java/gq/kirmanak/mealient/test/AuthImplTestData.kt index ac25d1c..31c88f1 100644 --- a/app/src/test/java/gq/kirmanak/mealient/test/AuthImplTestData.kt +++ b/app/src/test/java/gq/kirmanak/mealient/test/AuthImplTestData.kt @@ -1,6 +1,7 @@ package gq.kirmanak.mealient.test import gq.kirmanak.mealient.datasource.models.GetUserInfoResponse +import gq.kirmanak.mealient.datasource.models.VersionResponse object AuthImplTestData { const val TEST_USERNAME = "TEST_USERNAME" @@ -11,4 +12,5 @@ object AuthImplTestData { val FAVORITE_RECIPES_LIST = listOf("cake", "porridge") val USER_INFO = GetUserInfoResponse("userId", FAVORITE_RECIPES_LIST) + val VERSION_RESPONSE = VersionResponse("1.0.0") } \ No newline at end of file diff --git a/app/src/test/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModelTest.kt b/app/src/test/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModelTest.kt index b7e2235..5c8eca7 100644 --- a/app/src/test/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModelTest.kt +++ b/app/src/test/java/gq/kirmanak/mealient/ui/baseurl/BaseURLViewModelTest.kt @@ -9,6 +9,7 @@ import gq.kirmanak.mealient.data.recipes.RecipeRepo import gq.kirmanak.mealient.datasource.NetworkError import gq.kirmanak.mealient.datasource.TrustedCertificatesStore import gq.kirmanak.mealient.test.AuthImplTestData.TEST_BASE_URL +import gq.kirmanak.mealient.test.AuthImplTestData.VERSION_RESPONSE import gq.kirmanak.mealient.test.BaseUnitTest import io.mockk.coEvery import io.mockk.coVerify @@ -91,7 +92,7 @@ internal class BaseURLViewModelTest : BaseUnitTest() { private fun TestScope.setupSaveBaseUrlWithOldUrl() { coEvery { serverInfoRepo.getUrl() } returns TEST_BASE_URL - coEvery { serverInfoRepo.tryBaseURL(any()) } returns Result.success(Unit) + coEvery { serverInfoRepo.tryBaseURL(any()) } returns Result.success(VERSION_RESPONSE) subject.saveBaseUrl(TEST_BASE_URL) advanceUntilIdle() } @@ -116,7 +117,7 @@ internal class BaseURLViewModelTest : BaseUnitTest() { private fun TestScope.setupSaveBaseUrlWithNewUrl() { coEvery { serverInfoRepo.getUrl() } returns null - coEvery { serverInfoRepo.tryBaseURL(any()) } returns Result.success(Unit) + coEvery { serverInfoRepo.tryBaseURL(any()) } returns Result.success(VERSION_RESPONSE) subject.saveBaseUrl(TEST_BASE_URL) advanceUntilIdle() } @@ -135,7 +136,7 @@ internal class BaseURLViewModelTest : BaseUnitTest() { coEvery { serverInfoRepo.getUrl() } returns null val err = NetworkError.MalformedUrl(SSLHandshakeException("test")) coEvery { serverInfoRepo.tryBaseURL("https://test") } returns Result.failure(err) - coEvery { serverInfoRepo.tryBaseURL("http://test") } returns Result.success(Unit) + coEvery { serverInfoRepo.tryBaseURL("http://test") } returns Result.success(VERSION_RESPONSE) subject.saveBaseUrl("test") coVerifyOrder { serverInfoRepo.tryBaseURL("https://test") diff --git a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/MealieDataSource.kt b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/MealieDataSource.kt index a0bc64e..807262e 100644 --- a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/MealieDataSource.kt +++ b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/MealieDataSource.kt @@ -35,7 +35,7 @@ interface MealieDataSource { password: String, ): String - suspend fun getVersionInfo(): VersionResponse + suspend fun getVersionInfo(baseURL: String): VersionResponse suspend fun requestRecipes( page: Int, diff --git a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/MealieService.kt b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/MealieService.kt index c6cb086..d4749aa 100644 --- a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/MealieService.kt +++ b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/MealieService.kt @@ -28,7 +28,7 @@ internal interface MealieService { slug: String, ): GetRecipeResponse - suspend fun getVersion(): VersionResponse + suspend fun getVersion(baseURL: String): VersionResponse suspend fun getRecipeSummary(page: Int, perPage: Int): GetRecipesResponse diff --git a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/MealieDataSourceImpl.kt b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/MealieDataSourceImpl.kt index 0c3c80b..2bd6864 100644 --- a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/MealieDataSourceImpl.kt +++ b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/MealieDataSourceImpl.kt @@ -65,16 +65,18 @@ internal class MealieDataSourceImpl @Inject constructor( throw if (errorDetail.detail == "Unauthorized") NetworkError.Unauthorized(it) else it } - override suspend fun getVersionInfo(): VersionResponse = networkRequestWrapper.makeCall( - block = { service.getVersion() }, - logMethod = { "getVersionInfo" }, - ).getOrElse { - throw when (it) { - is ResponseException, is NoTransformationFoundException -> NetworkError.NotMealie(it) - is SocketTimeoutException, is SocketException -> NetworkError.NoServerConnection(it) - else -> NetworkError.MalformedUrl(it) + override suspend fun getVersionInfo(baseURL: String): VersionResponse = + networkRequestWrapper.makeCall( + block = { service.getVersion(baseURL) }, + logMethod = { "getVersionInfo" }, + logParameters = { "baseURL = $baseURL" } + ).getOrElse { + throw when (it) { + is ResponseException, is NoTransformationFoundException -> NetworkError.NotMealie(it) + is SocketTimeoutException, is SocketException -> NetworkError.NoServerConnection(it) + else -> NetworkError.MalformedUrl(it) + } } - } override suspend fun requestRecipes( page: Int, diff --git a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/MealieServiceKtor.kt b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/MealieServiceKtor.kt index d7afd2c..3af8013 100644 --- a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/MealieServiceKtor.kt +++ b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/MealieServiceKtor.kt @@ -76,9 +76,9 @@ internal class MealieServiceKtor @Inject constructor( }.body() } - override suspend fun getVersion(): VersionResponse { + override suspend fun getVersion(baseURL: String): VersionResponse { return httpClient.get { - endpoint("/api/app/about") + endpoint(baseURL, "/api/app/about") }.body() } @@ -198,9 +198,21 @@ internal class MealieServiceKtor @Inject constructor( private suspend fun HttpRequestBuilder.endpoint( path: String, - block: URLBuilder.() -> Unit = {} + block: URLBuilder.() -> Unit = {}, ) { val baseUrl = checkNotNull(serverUrlProvider.getUrl()) { "Server URL is not set" } + endpoint( + baseUrl = baseUrl, + path = path, + block = block + ) + } + + private fun HttpRequestBuilder.endpoint( + baseUrl: String, + path: String, + block: URLBuilder.() -> Unit = {}, + ) { url { takeFrom(baseUrl) path(path)