diff --git a/infra/network/src/main/java/com/simprints/infra/network/SimNetworkImpl.kt b/infra/network/src/main/java/com/simprints/infra/network/SimNetworkImpl.kt index f0606fb368..e65a9d8643 100644 --- a/infra/network/src/main/java/com/simprints/infra/network/SimNetworkImpl.kt +++ b/infra/network/src/main/java/com/simprints/infra/network/SimNetworkImpl.kt @@ -1,14 +1,16 @@ package com.simprints.infra.network import com.simprints.infra.network.apiclient.SimApiClientImpl -import com.simprints.infra.network.httpclient.DefaultOkHttpClientBuilder +import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase import com.simprints.infra.network.url.BaseUrlProvider import javax.inject.Inject +import javax.inject.Singleton import kotlin.reflect.KClass +@Singleton internal class SimNetworkImpl @Inject constructor( private val baseUrlProvider: BaseUrlProvider, - private val okHttpClientBuilder: DefaultOkHttpClientBuilder, + private val buildOkHttpClient: BuildOkHttpClientUseCase, ) : SimNetwork { override fun getSimApiClient( remoteInterface: KClass, @@ -17,7 +19,7 @@ internal class SimNetworkImpl @Inject constructor( authToken: String?, ): SimNetwork.SimApiClient = SimApiClientImpl( remoteInterface, - okHttpClientBuilder, + buildOkHttpClient, getApiBaseUrl(), deviceId, versionName, diff --git a/infra/network/src/main/java/com/simprints/infra/network/apiclient/SimApiClientImpl.kt b/infra/network/src/main/java/com/simprints/infra/network/apiclient/SimApiClientImpl.kt index cf20d3e6bd..080e1a1f38 100644 --- a/infra/network/src/main/java/com/simprints/infra/network/apiclient/SimApiClientImpl.kt +++ b/infra/network/src/main/java/com/simprints/infra/network/apiclient/SimApiClientImpl.kt @@ -9,11 +9,10 @@ import com.simprints.infra.network.exceptions.NetworkConnectionException import com.simprints.infra.network.exceptions.RetryableCloudException import com.simprints.infra.network.exceptions.SyncCloudIntegrationException import com.simprints.infra.network.exceptions.isCausedFromBadNetworkConnection -import com.simprints.infra.network.httpclient.DefaultOkHttpClientBuilder +import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase import com.simprints.infra.network.json.JsonHelper import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext -import okhttp3.OkHttpClient import retrofit2.HttpException import retrofit2.Retrofit import retrofit2.converter.jackson.JacksonConverterFactory @@ -27,7 +26,7 @@ import kotlin.reflect.KClass */ internal class SimApiClientImpl( private val service: KClass, - private val okHttpClientBuilder: DefaultOkHttpClientBuilder, + private val buildOkHttpClient: BuildOkHttpClientUseCase, private val url: String, private val deviceId: String, private val versionName: String, @@ -51,14 +50,10 @@ internal class SimApiClientImpl( .addConverterFactory(ScalarsConverterFactory.create()) .addConverterFactory(JacksonConverterFactory.create(JsonHelper.jackson)) .baseUrl(url) - .client(okHttpClientConfig.build()) + .client(buildOkHttpClient(authToken, deviceId, versionName)) .build() } - private val okHttpClientConfig: OkHttpClient.Builder by lazy { - okHttpClientBuilder.get(authToken, deviceId, versionName) - } - override suspend fun executeCall(networkBlock: suspend (T) -> V): V = try { retryIO( times = attempts, @@ -106,7 +101,7 @@ internal class SimApiClientImpl( private fun HttpException.parseEstimatedOutage(): Long? = try { response()?.headers()?.get(HEADER_RETRY_AFTER)?.toLong() - } catch (e: NumberFormatException) { + } catch (_: NumberFormatException) { null } } diff --git a/infra/network/src/main/java/com/simprints/infra/network/httpclient/DefaultOkHttpClientBuilder.kt b/infra/network/src/main/java/com/simprints/infra/network/httpclient/BuildOkHttpClientUseCase.kt similarity index 75% rename from infra/network/src/main/java/com/simprints/infra/network/httpclient/DefaultOkHttpClientBuilder.kt rename to infra/network/src/main/java/com/simprints/infra/network/httpclient/BuildOkHttpClientUseCase.kt index 9cc9cac2f5..ceb935d6a5 100644 --- a/infra/network/src/main/java/com/simprints/infra/network/httpclient/DefaultOkHttpClientBuilder.kt +++ b/infra/network/src/main/java/com/simprints/infra/network/httpclient/BuildOkHttpClientUseCase.kt @@ -19,8 +19,10 @@ import okio.buffer import java.io.IOException import java.util.concurrent.TimeUnit import javax.inject.Inject +import javax.inject.Singleton -internal class DefaultOkHttpClientBuilder @Inject constructor( +@Singleton +internal class BuildOkHttpClientUseCase @Inject constructor( @ApplicationContext private val ctx: Context, private val networkCache: Cache, private val persistentLogger: PersistentLogger, @@ -29,6 +31,8 @@ internal class DefaultOkHttpClientBuilder @Inject constructor( const val DEVICE_ID_HEADER = "X-Device-ID" const val AUTHORIZATION_HEADER = "Authorization" const val USER_AGENT_HEADER = "User-Agent" + const val READ_TIMEOUT = 60L + const val WRITE_TIMEOUT = 60L /** * This header can be used to force a specific version of the API endpoint to be used. @@ -40,20 +44,49 @@ internal class DefaultOkHttpClientBuilder @Inject constructor( const val FORCE_VERSION_HEADER = "X-Force-Version" } - fun get( + private var okHttpClient: OkHttpClient? = null + private var currentAuthToken: String? = null + + private val lock = Any() // synchronization lock for the okHttpClient lazy property + + /** + * Retrieves an instance of [OkHttpClient], ensuring that the same instance is reused unless the authentication token changes. + * + * This method is **thread-safe** using `synchronized(lock)`, ensuring that only one instance of `OkHttpClient` is created at a time. + * If the provided `authToken` differs from the current stored token, a **new OkHttpClient instance** is created. + * Otherwise, the existing client instance is reused to optimize memory usage and prevent OOM errors. + * + * @param authToken The authentication token used for secure API requests. If `null`, a non-authenticated client is used. + * @param deviceId A unique identifier for the device, added to the request headers. + * @param versionName The application version name, added to the request headers. + * @return An instance of [OkHttpClient] configured based on the provided parameters. + * + */ + operator fun invoke( authToken: String? = null, deviceId: String, versionName: String, - ): OkHttpClient.Builder = OkHttpClient + ): OkHttpClient = synchronized(lock) { + if (okHttpClient == null || currentAuthToken != authToken) { + currentAuthToken = authToken + okHttpClient = buildOkHttpClient(deviceId, versionName) + } + okHttpClient!! + } + + private fun buildOkHttpClient( + deviceId: String, + versionName: String, + ) = OkHttpClient .Builder() .cache(networkCache) .followRedirects(false) .followSslRedirects(false) - .readTimeout(60, TimeUnit.SECONDS) - .writeTimeout(60, TimeUnit.SECONDS) + .readTimeout(READ_TIMEOUT, TimeUnit.SECONDS) + .writeTimeout(WRITE_TIMEOUT, TimeUnit.SECONDS) .apply { - if (!authToken.isNullOrBlank()) { - addInterceptor(buildAuthenticationInterceptor(authToken)) + if (!currentAuthToken.isNullOrBlank()) { + addInterceptor(buildAuthenticationInterceptor(currentAuthToken!!)) } }.addNetworkInterceptor(ChuckerInterceptor.Builder(ctx).build()) .addInterceptor(buildDeviceIdInterceptor(deviceId)) @@ -64,6 +97,7 @@ internal class DefaultOkHttpClientBuilder @Inject constructor( addInterceptor(buildSimberLoggingInterceptor()) } }.addInterceptor(buildPersistentLoggerInterceptor()) + .build() private fun buildAuthenticationInterceptor(authToken: String) = Interceptor { chain -> val newRequest = chain diff --git a/infra/network/src/test/java/com/simprints/infra/network/SimNetworkImplTest.kt b/infra/network/src/test/java/com/simprints/infra/network/SimNetworkImplTest.kt index 778504c073..17b5da82ad 100644 --- a/infra/network/src/test/java/com/simprints/infra/network/SimNetworkImplTest.kt +++ b/infra/network/src/test/java/com/simprints/infra/network/SimNetworkImplTest.kt @@ -1,7 +1,7 @@ package com.simprints.infra.network import com.simprints.infra.network.apiclient.SimApiClientImpl -import com.simprints.infra.network.httpclient.DefaultOkHttpClientBuilder +import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase import com.simprints.infra.network.url.BaseUrlProvider import io.mockk.MockKAnnotations import io.mockk.impl.annotations.MockK @@ -15,7 +15,7 @@ class SimNetworkImplTest { private lateinit var baseUrlProvider: BaseUrlProvider @MockK - private lateinit var okHttpClientBuilder: DefaultOkHttpClientBuilder + private lateinit var okHttpClientBuilder: BuildOkHttpClientUseCase @Before fun setUp() { diff --git a/infra/network/src/test/java/com/simprints/infra/network/apiclient/SimApiClientImplTest.kt b/infra/network/src/test/java/com/simprints/infra/network/apiclient/SimApiClientImplTest.kt index 40a5c1b9c6..8d2dcc0f58 100644 --- a/infra/network/src/test/java/com/simprints/infra/network/apiclient/SimApiClientImplTest.kt +++ b/infra/network/src/test/java/com/simprints/infra/network/apiclient/SimApiClientImplTest.kt @@ -7,7 +7,7 @@ import com.simprints.infra.network.exceptions.ApiError import com.simprints.infra.network.exceptions.BackendMaintenanceException import com.simprints.infra.network.exceptions.NetworkConnectionException import com.simprints.infra.network.exceptions.SyncCloudIntegrationException -import com.simprints.infra.network.httpclient.DefaultOkHttpClientBuilder +import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase import com.simprints.logging.persistent.PersistentLogger import com.simprints.testtools.common.syntax.assertThrows import io.mockk.MockKAnnotations @@ -38,7 +38,7 @@ class SimApiClientImplTest { private lateinit var mockWebServer: MockWebServer private lateinit var simApiClientImpl: SimApiClientImpl - private lateinit var httpClientBuilder: DefaultOkHttpClientBuilder + private lateinit var httpClientBuilder: BuildOkHttpClientUseCase @Before fun setup() { @@ -46,7 +46,7 @@ class SimApiClientImplTest { mockWebServer = MockWebServer() mockWebServer.start() - httpClientBuilder = DefaultOkHttpClientBuilder( + httpClientBuilder = BuildOkHttpClientUseCase( mockk(), networkCache, persistentLogger, diff --git a/infra/network/src/test/java/com/simprints/infra/network/httpclient/DefaultOkHttpClientBuilderTest.kt b/infra/network/src/test/java/com/simprints/infra/network/httpclient/BuildOkHttpClientUseCaseTest.kt similarity index 58% rename from infra/network/src/test/java/com/simprints/infra/network/httpclient/DefaultOkHttpClientBuilderTest.kt rename to infra/network/src/test/java/com/simprints/infra/network/httpclient/BuildOkHttpClientUseCaseTest.kt index befa2fab64..2881737f02 100644 --- a/infra/network/src/test/java/com/simprints/infra/network/httpclient/DefaultOkHttpClientBuilderTest.kt +++ b/infra/network/src/test/java/com/simprints/infra/network/httpclient/BuildOkHttpClientUseCaseTest.kt @@ -1,9 +1,12 @@ package com.simprints.infra.network.httpclient import android.content.Context -import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.* +import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase.Companion.AUTHORIZATION_HEADER +import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase.Companion.DEVICE_ID_HEADER +import com.simprints.infra.network.httpclient.BuildOkHttpClientUseCase.Companion.USER_AGENT_HEADER import com.simprints.logging.persistent.PersistentLogger -import io.mockk.MockKAnnotations +import io.mockk.* import io.mockk.impl.annotations.MockK import okhttp3.MediaType.Companion.toMediaTypeOrNull import okhttp3.OkHttpClient @@ -15,7 +18,7 @@ import org.junit.After import org.junit.Before import org.junit.Test -class DefaultOkHttpClientBuilderTest { +class BuildOkHttpClientUseCaseTest { // mock server to read http request parameters private lateinit var mockWebServer: MockWebServer private lateinit var okHttpClient: OkHttpClient @@ -29,7 +32,7 @@ class DefaultOkHttpClientBuilderTest { @MockK lateinit var persistentLogger: PersistentLogger - private lateinit var okHttpBuilder: DefaultOkHttpClientBuilder + private lateinit var buildOkHttpClient: BuildOkHttpClientUseCase @Before fun setup() { @@ -37,7 +40,7 @@ class DefaultOkHttpClientBuilderTest { mockWebServer = MockWebServer() mockWebServer.start() - okHttpBuilder = DefaultOkHttpClientBuilder(ctx, networkCache, persistentLogger) + buildOkHttpClient = BuildOkHttpClientUseCase(ctx, networkCache, persistentLogger) } @After @@ -52,21 +55,16 @@ class DefaultOkHttpClientBuilderTest { val versionName = "cxxlvxzn.12.2049" // create okHttp client using default builder - okHttpClient = okHttpBuilder - .get("", "", versionName) - .build() + okHttpClient = buildOkHttpClient("", "", versionName) - val mockHttpRequest = Request - .Builder() - .url(mockWebServer.url("/")) - .build() + val mockHttpRequest = Request.Builder().url(mockWebServer.url("/")).build() // execute mock request okHttpClient.newCall(mockHttpRequest).execute() // read recorded request val recordedRequest = mockWebServer.takeRequest() - assertThat(recordedRequest.getHeader(DefaultOkHttpClientBuilder.USER_AGENT_HEADER)) + assertThat(recordedRequest.getHeader(USER_AGENT_HEADER)) .isEqualTo("SimprintsID/$versionName") } @@ -76,21 +74,16 @@ class DefaultOkHttpClientBuilderTest { val deviceId = "symeAwxomedyvexeid" - okHttpClient = okHttpBuilder - .get("", deviceId, "") - .build() + okHttpClient = buildOkHttpClient("", deviceId, "") - val mockHttpRequest = Request - .Builder() - .url(mockWebServer.url("/")) - .build() + val mockHttpRequest = Request.Builder().url(mockWebServer.url("/")).build() // execute mock request okHttpClient.newCall(mockHttpRequest).execute() // read recorded request val recordedRequest = mockWebServer.takeRequest() - assertThat(recordedRequest.getHeader(DefaultOkHttpClientBuilder.DEVICE_ID_HEADER)) + assertThat(recordedRequest.getHeader(DEVICE_ID_HEADER)) .isEqualTo(deviceId) } @@ -98,21 +91,16 @@ class DefaultOkHttpClientBuilderTest { fun `should not include auth token in request headers, when auth token is null`() { mockWebServer.enqueue(MockResponse()) - okHttpClient = okHttpBuilder - .get(null, "", "") - .build() + okHttpClient = buildOkHttpClient(null, "", "") - val mockHttpRequest = Request - .Builder() - .url(mockWebServer.url("/")) - .build() + val mockHttpRequest = Request.Builder().url(mockWebServer.url("/")).build() // execute mock request okHttpClient.newCall(mockHttpRequest).execute() // read recorded request val recordedRequest = mockWebServer.takeRequest() - assertThat(recordedRequest.getHeader(DefaultOkHttpClientBuilder.AUTHORIZATION_HEADER)).isNull() + assertThat(recordedRequest.getHeader(AUTHORIZATION_HEADER)).isNull() } @Test @@ -121,37 +109,50 @@ class DefaultOkHttpClientBuilderTest { val authToken = "eyxSomeAwesomeAuth.TokenThatIsUsed.ForUnitTesting" - okHttpClient = okHttpBuilder - .get(authToken, "", "") - .build() + okHttpClient = buildOkHttpClient(authToken, "", "") - val mockHttpRequest = Request - .Builder() - .url(mockWebServer.url("/")) - .build() + val mockHttpRequest = Request.Builder().url(mockWebServer.url("/")).build() // execute mock request okHttpClient.newCall(mockHttpRequest).execute() // read recorded request val recordedRequest = mockWebServer.takeRequest() - assertThat(recordedRequest.getHeader(DefaultOkHttpClientBuilder.AUTHORIZATION_HEADER)) + assertThat(recordedRequest.getHeader(AUTHORIZATION_HEADER)) .isEqualTo("Bearer $authToken") } + @Test + fun `should return same instance for same token`() { + val client1 = buildOkHttpClient(authToken = "token123", deviceId = "12345", versionName = "1.0") + val client2 = buildOkHttpClient(authToken = "token123", deviceId = "12345", versionName = "1.0") + + assertThat(client1).isSameInstanceAs(client2) + } + + @Test + fun `should create new client when authToken changes`() { + val client1 = buildOkHttpClient(authToken = null, deviceId = "12345", versionName = "1.0") + val client2 = buildOkHttpClient(authToken = "token2", deviceId = "12345", versionName = "1.0") + val client3 = buildOkHttpClient(authToken = "token3", deviceId = "12345", versionName = "1.0") + + assertThat(client1).isNotSameInstanceAs(client2) + assertThat(client2).isNotSameInstanceAs(client3) + assertThat(client1).isNotSameInstanceAs(client3) + } + @Test fun `should not compress request body if gzip header not provided`() { mockWebServer.enqueue(MockResponse()) - okHttpClient = okHttpBuilder - .get(null, "", "") - .build() + okHttpClient = buildOkHttpClient(null, "", "") - val mockHttpRequest = Request - .Builder() - .url(mockWebServer.url("/")) - .post("some request body".toRequestBody("text/plain".toMediaTypeOrNull())) - .build() + val mockHttpRequest = + Request + .Builder() + .url(mockWebServer.url("/")) + .post("some request body".toRequestBody("text/plain".toMediaTypeOrNull())) + .build() // execute mock request okHttpClient.newCall(mockHttpRequest).execute() @@ -165,9 +166,7 @@ class DefaultOkHttpClientBuilderTest { fun `should compress request body if gzip header provided`() { mockWebServer.enqueue(MockResponse()) - okHttpClient = okHttpBuilder - .get(null, "", "") - .build() + okHttpClient = buildOkHttpClient(null, "", "") val mockHttpRequest = Request .Builder()