diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt index 4e220e0adfa..c742e8436e9 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt +++ b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt @@ -100,7 +100,7 @@ class DefaultTopSitesStorage( ) { try { val providerTopSites = topSitesProvider - .getTopSites() + .getTopSites(allowCache = true) .take(numSitesRequired) topSites.addAll(providerTopSites) numSitesRequired -= providerTopSites.size diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesProvider.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesProvider.kt index 751f2b62883..4ac82009016 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesProvider.kt +++ b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesProvider.kt @@ -11,6 +11,10 @@ interface TopSitesProvider { /** * Provides a list of top sites. + * + * @param allowCache Whether or not the result may be provided from a previously + * cached response. + * @return a list of top sites from the provider. */ - suspend fun getTopSites(): List + suspend fun getTopSites(allowCache: Boolean = true): List } diff --git a/components/service/contile/src/main/java/mozilla/components/service/contile/ContileTopSitesProvider.kt b/components/service/contile/src/main/java/mozilla/components/service/contile/ContileTopSitesProvider.kt index bb7b006bc00..53b282f67c4 100644 --- a/components/service/contile/src/main/java/mozilla/components/service/contile/ContileTopSitesProvider.kt +++ b/components/service/contile/src/main/java/mozilla/components/service/contile/ContileTopSitesProvider.kt @@ -4,6 +4,9 @@ package mozilla.components.service.contile +import android.content.Context +import android.util.AtomicFile +import androidx.annotation.VisibleForTesting import mozilla.components.concept.fetch.Client import mozilla.components.concept.fetch.Request import mozilla.components.concept.fetch.isSuccess @@ -11,25 +14,60 @@ import mozilla.components.feature.top.sites.TopSite import mozilla.components.feature.top.sites.TopSitesProvider import mozilla.components.support.base.log.logger.Logger import mozilla.components.support.ktx.android.org.json.asSequence +import mozilla.components.support.ktx.util.readAndDeserialize +import mozilla.components.support.ktx.util.writeString import org.json.JSONException import org.json.JSONObject +import java.io.File import java.io.IOException +import java.util.Date internal const val CONTILE_ENDPOINT_URL = "https://contile.services.mozilla.com/v1/tiles" +internal const val CACHE_FILE_NAME = "mozilla_components_service_contile.json" +internal const val MINUTE_IN_MS = 60 * 1000 /** * Provide access to the Contile services API. * + * @property context A reference to the application context. * @property client [Client] used for interacting with the Contile HTTP API. + * @property endPointURL The url of the endpoint to fetch from. Defaults to [CONTILE_ENDPOINT_URL]. + * @property maxCacheAgeInMinutes Maximum time (in minutes) the cache should remain valid + * before a refresh is attempted. Defaults to -1, meaning no cache is being used by default. */ class ContileTopSitesProvider( - private val client: Client + private val context: Context, + private val client: Client, + private val endPointURL: String = CONTILE_ENDPOINT_URL, + private val maxCacheAgeInMinutes: Long = -1 ) : TopSitesProvider { private val logger = Logger("ContileTopSitesProvider") + private val diskCacheLock = Any() + + /** + * Fetches from the top sites [endPointURL] to provide a list of provided top sites. + * Returns a cached response if [allowCache] is true and the cache is not expired + * (@see [maxCacheAgeInMinutes]). + * + * @param allowCache Whether or not the result may be provided from a previously cached + * response. Note that a [maxCacheAgeInMinutes] must be provided in order for the cache to be + * active. + * @throws IOException if the request failed to fetch any top sites. + */ @Throws(IOException::class) - override suspend fun getTopSites(): List { + override suspend fun getTopSites(allowCache: Boolean): List { + val cachedTopSites = if (allowCache && !isCacheExpired()) { + readFromDiskCache() + } else { + null + } + + if (!cachedTopSites.isNullOrEmpty()) { + return cachedTopSites + } + return try { fetchTopSites() } catch (e: IOException) { @@ -40,13 +78,17 @@ class ContileTopSitesProvider( private fun fetchTopSites(): List { client.fetch( - Request(url = CONTILE_ENDPOINT_URL) + Request(url = endPointURL) ).use { response -> if (response.isSuccess) { val responseBody = response.body.string(Charsets.UTF_8) return try { - JSONObject(responseBody).getTopSites() + JSONObject(responseBody).getTopSites().also { + if (maxCacheAgeInMinutes > 0) { + writeToDiskCache(responseBody) + } + } } catch (e: JSONException) { throw IOException(e) } @@ -58,6 +100,37 @@ class ContileTopSitesProvider( } } } + + @VisibleForTesting + internal fun readFromDiskCache(): List? { + synchronized(diskCacheLock) { + return getCacheFile().readAndDeserialize { + JSONObject(it).getTopSites() + } + } + } + + @VisibleForTesting + internal fun writeToDiskCache(responseBody: String) { + synchronized(diskCacheLock) { + getCacheFile().writeString { responseBody } + } + } + + @VisibleForTesting + internal fun isCacheExpired() = + getCacheLastModified() < Date().time - maxCacheAgeInMinutes * MINUTE_IN_MS + + @VisibleForTesting + internal fun getCacheLastModified(): Long { + val file = getBaseCacheFile() + return if (file.exists()) file.lastModified() else -1 + } + + private fun getCacheFile(): AtomicFile = AtomicFile(getBaseCacheFile()) + + @VisibleForTesting + internal fun getBaseCacheFile(): File = File(context.filesDir, CACHE_FILE_NAME) } internal fun JSONObject.getTopSites(): List = diff --git a/components/service/contile/src/test/java/mozilla/components/service/contile/ContileTopSitesProviderTest.kt b/components/service/contile/src/test/java/mozilla/components/service/contile/ContileTopSitesProviderTest.kt index 1d3c0f7c668..b9a4238e9c4 100644 --- a/components/service/contile/src/test/java/mozilla/components/service/contile/ContileTopSitesProviderTest.kt +++ b/components/service/contile/src/test/java/mozilla/components/service/contile/ContileTopSitesProviderTest.kt @@ -11,24 +11,31 @@ import mozilla.components.concept.fetch.Response import mozilla.components.support.test.any import mozilla.components.support.test.file.loadResourceAsString import mozilla.components.support.test.mock +import mozilla.components.support.test.robolectric.testContext import mozilla.components.support.test.whenever import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith +import org.mockito.Mockito.never +import org.mockito.Mockito.spy +import org.mockito.Mockito.verify +import java.io.File import java.io.IOException +import java.util.Date @RunWith(AndroidJUnit4::class) class ContileTopSitesProviderTest { @Test - fun `GIVEN a successful status response WHEN getTopSites is called THEN response should contain top sites`() = runBlocking { + fun `GIVEN a successful status response WHEN top sites are fetched THEN response should contain top sites`() = runBlocking { val client = prepareClient() - val provider = ContileTopSitesProvider(client) + val provider = ContileTopSitesProvider(testContext, client) val topSites = provider.getTopSites() var topSite = topSites.first() assertEquals(2, topSites.size) - assertEquals(1L, topSite.id) assertEquals("Firefox", topSite.title) assertEquals("https://firefox.com", topSite.url) @@ -47,13 +54,98 @@ class ContileTopSitesProviderTest { } @Test(expected = IOException::class) - fun `GIVEN a 500 status response WHEN getTopSites is called THEN throw an exception`() = runBlocking { + fun `GIVEN a 500 status response WHEN top sites are fetched THEN throw an exception`() = runBlocking { val client = prepareClient(status = 500) - val provider = ContileTopSitesProvider(client) + val provider = ContileTopSitesProvider(testContext, client) provider.getTopSites() Unit } + @Test + fun `GIVEN a cache configuration is allowed and not expired WHEN top sites are fetched THEN read from the disk cache`() = runBlocking { + val client = prepareClient() + val provider = spy(ContileTopSitesProvider(testContext, client)) + + provider.getTopSites(allowCache = false) + verify(provider, never()).readFromDiskCache() + + whenever(provider.isCacheExpired()).thenReturn(true) + provider.getTopSites(allowCache = true) + verify(provider, never()).readFromDiskCache() + + whenever(provider.isCacheExpired()).thenReturn(false) + provider.getTopSites(allowCache = true) + verify(provider).readFromDiskCache() + + Unit + } + + @Test + fun `GIVEN a cache configuration is allowed WHEN top sites are fetched THEN write response to cache`() = runBlocking { + val jsonResponse = loadResourceAsString("/contile/contile.json") + val client = prepareClient(jsonResponse) + val provider = spy(ContileTopSitesProvider(testContext, client)) + val cachingProvider = spy( + ContileTopSitesProvider( + context = testContext, + client = client, + maxCacheAgeInMinutes = 1L + ) + ) + + provider.getTopSites() + verify(provider, never()).writeToDiskCache(jsonResponse) + + cachingProvider.getTopSites() + verify(cachingProvider).writeToDiskCache(jsonResponse) + } + + @Test + fun `WHEN the base cache file getter is called THEN return existing base cache file`() { + val client = prepareClient() + val provider = spy(ContileTopSitesProvider(testContext, client)) + val file = File(testContext.filesDir, CACHE_FILE_NAME) + + file.createNewFile() + + assertTrue(file.exists()) + + val cacheFile = provider.getBaseCacheFile() + + assertTrue(cacheFile.exists()) + assertEquals(file.name, cacheFile.name) + + assertTrue(file.delete()) + assertFalse(cacheFile.exists()) + } + + @Test + fun `GIVEN a max cache age WHEN the cache expiration is checked THEN return whether the cache is expired`() { + var provider = + spy(ContileTopSitesProvider(testContext, client = mock(), maxCacheAgeInMinutes = -1)) + + whenever(provider.getCacheLastModified()).thenReturn(Date().time) + assertTrue(provider.isCacheExpired()) + + whenever(provider.getCacheLastModified()).thenReturn(-1) + assertTrue(provider.isCacheExpired()) + + provider = + spy(ContileTopSitesProvider(testContext, client = mock(), maxCacheAgeInMinutes = 10)) + + whenever(provider.getCacheLastModified()).thenReturn(-1) + assertTrue(provider.isCacheExpired()) + + whenever(provider.getCacheLastModified()).thenReturn(Date().time - 60 * MINUTE_IN_MS) + assertTrue(provider.isCacheExpired()) + + whenever(provider.getCacheLastModified()).thenReturn(Date().time) + assertFalse(provider.isCacheExpired()) + + whenever(provider.getCacheLastModified()).thenReturn(Date().time + 60 * MINUTE_IN_MS) + assertFalse(provider.isCacheExpired()) + } + private fun prepareClient( jsonResponse: String = loadResourceAsString("/contile/contile.json"), status: Int = 200