From 267adfd27fdc59ce10443b8c0481e0e6d2524e2f Mon Sep 17 00:00:00 2001 From: Adetunji Dahunsi Date: Tue, 21 Feb 2023 17:00:40 -0500 Subject: [PATCH 1/3] Batch sync news resources from remote Batch sync news resources from remote to reduce json payload size. --- .../repository/OfflineFirstNewsRepository.kt | 41 +++++++++++-------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt b/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt index 9e419b485..ae3ad3b9c 100644 --- a/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt +++ b/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt @@ -34,6 +34,8 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.map import javax.inject.Inject +private const val SYNC_BATCH_SIZE = 40 + /** * Disk storage backed implementation of the [NewsRepository]. * Reads are exclusively from local storage to support offline access. @@ -65,26 +67,29 @@ class OfflineFirstNewsRepository @Inject constructor( }, modelDeleter = newsResourceDao::deleteNewsResources, modelUpdater = { changedIds -> - val networkNewsResources = network.getNewsResources(ids = changedIds) + changedIds.chunked(SYNC_BATCH_SIZE).forEach { chunkedIds + val networkNewsResources = network.getNewsResources(ids = chunkedIds) - // Order of invocation matters to satisfy id and foreign key constraints! + // Order of invocation matters to satisfy id and foreign key constraints! - topicDao.insertOrIgnoreTopics( - topicEntities = networkNewsResources - .map(NetworkNewsResource::topicEntityShells) - .flatten() - .distinctBy(TopicEntity::id), - ) - newsResourceDao.upsertNewsResources( - newsResourceEntities = networkNewsResources - .map(NetworkNewsResource::asEntity), - ) - newsResourceDao.insertOrIgnoreTopicCrossRefEntities( - newsResourceTopicCrossReferences = networkNewsResources - .map(NetworkNewsResource::topicCrossReferences) - .distinct() - .flatten(), - ) + topicDao.insertOrIgnoreTopics( + topicEntities = networkNewsResources + .map(NetworkNewsResource::topicEntityShells) + .flatten() + .distinctBy(TopicEntity::id), + ) + newsResourceDao.upsertNewsResources( + newsResourceEntities = networkNewsResources.map( + NetworkNewsResource::asEntity + ), + ) + newsResourceDao.insertOrIgnoreTopicCrossRefEntities( + newsResourceTopicCrossReferences = networkNewsResources + .map(NetworkNewsResource::topicCrossReferences) + .distinct() + .flatten(), + ) + } }, ) } From 1e8d1d98106cff3297efab767da5e378f0c0ea83 Mon Sep 17 00:00:00 2001 From: Adetunji Dahunsi Date: Tue, 21 Feb 2023 17:11:47 -0500 Subject: [PATCH 2/3] Spotless fix Thought I was cool enough to edit code in the browser. Spotless said no. --- .../core/data/repository/OfflineFirstNewsRepository.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt b/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt index ae3ad3b9c..e98090cba 100644 --- a/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt +++ b/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt @@ -67,7 +67,7 @@ class OfflineFirstNewsRepository @Inject constructor( }, modelDeleter = newsResourceDao::deleteNewsResources, modelUpdater = { changedIds -> - changedIds.chunked(SYNC_BATCH_SIZE).forEach { chunkedIds + changedIds.chunked(SYNC_BATCH_SIZE).forEach { chunkedIds -> val networkNewsResources = network.getNewsResources(ids = chunkedIds) // Order of invocation matters to satisfy id and foreign key constraints! @@ -80,7 +80,7 @@ class OfflineFirstNewsRepository @Inject constructor( ) newsResourceDao.upsertNewsResources( newsResourceEntities = networkNewsResources.map( - NetworkNewsResource::asEntity + NetworkNewsResource::asEntity, ), ) newsResourceDao.insertOrIgnoreTopicCrossRefEntities( From 8765d3ac57a9e365446e6ec9cb71cc256be3672d Mon Sep 17 00:00:00 2001 From: Adetunji Dahunsi Date: Wed, 22 Feb 2023 03:19:16 -0500 Subject: [PATCH 3/3] Fix tests for batched news resource sync Change-Id: I37545eba8618edc936eb12fa38628d98f85f52ed --- .../repository/OfflineFirstNewsRepository.kt | 2 + .../OfflineFirstNewsRepositoryTest.kt | 47 ++++++++++--------- .../data/testdoubles/TestNewsResourceDao.kt | 36 +++++++------- .../core/data/testdoubles/TestTopicDao.kt | 22 ++++----- 4 files changed, 57 insertions(+), 50 deletions(-) diff --git a/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt b/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt index e98090cba..c16355d69 100644 --- a/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt +++ b/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt @@ -34,6 +34,8 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.map import javax.inject.Inject +// Heuristic value to optimize for serialization and deserialization cost on client and server +// for each news resource batch. private const val SYNC_BATCH_SIZE = 40 /** diff --git a/core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepositoryTest.kt b/core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepositoryTest.kt index 9f43d3441..b2c35056f 100644 --- a/core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepositoryTest.kt +++ b/core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepositoryTest.kt @@ -27,6 +27,7 @@ import com.google.samples.apps.nowinandroid.core.data.testdoubles.TestTopicDao import com.google.samples.apps.nowinandroid.core.data.testdoubles.filteredInterestsIds import com.google.samples.apps.nowinandroid.core.data.testdoubles.nonPresentInterestsIds import com.google.samples.apps.nowinandroid.core.database.model.NewsResourceEntity +import com.google.samples.apps.nowinandroid.core.database.model.NewsResourceTopicCrossRef import com.google.samples.apps.nowinandroid.core.database.model.PopulatedNewsResource import com.google.samples.apps.nowinandroid.core.database.model.TopicEntity import com.google.samples.apps.nowinandroid.core.database.model.asExternalModel @@ -107,8 +108,8 @@ class OfflineFirstNewsRepositoryTest { ) assertEquals( - emptyList(), - subject.getNewsResources( + expected = emptyList(), + actual = subject.getNewsResources( query = NewsResourceQuery( filterTopicIds = nonPresentInterestsIds, ), @@ -131,14 +132,14 @@ class OfflineFirstNewsRepositoryTest { .map(PopulatedNewsResource::asExternalModel) assertEquals( - newsResourcesFromNetwork.map(NewsResource::id), - newsResourcesFromDb.map(NewsResource::id), + newsResourcesFromNetwork.map(NewsResource::id).sorted(), + newsResourcesFromDb.map(NewsResource::id).sorted(), ) // After sync version should be updated assertEquals( - network.latestChangeListVersion(CollectionType.NewsResources), - synchronizer.getChangeListVersions().newsResourceVersion, + expected = network.latestChangeListVersion(CollectionType.NewsResources), + actual = synchronizer.getChangeListVersions().newsResourceVersion, ) } @@ -172,14 +173,14 @@ class OfflineFirstNewsRepositoryTest { // Assert that items marked deleted on the network have been deleted locally assertEquals( - newsResourcesFromNetwork.map(NewsResource::id) - deletedItems, - newsResourcesFromDb.map(NewsResource::id), + expected = (newsResourcesFromNetwork.map(NewsResource::id) - deletedItems).sorted(), + actual = newsResourcesFromDb.map(NewsResource::id).sorted(), ) // After sync version should be updated assertEquals( - network.latestChangeListVersion(CollectionType.NewsResources), - synchronizer.getChangeListVersions().newsResourceVersion, + expected = network.latestChangeListVersion(CollectionType.NewsResources), + actual = synchronizer.getChangeListVersions().newsResourceVersion, ) } @@ -211,14 +212,14 @@ class OfflineFirstNewsRepositoryTest { .map(PopulatedNewsResource::asExternalModel) assertEquals( - newsResourcesFromNetwork.map(NewsResource::id), - newsResourcesFromDb.map(NewsResource::id), + expected = newsResourcesFromNetwork.map(NewsResource::id).sorted(), + actual = newsResourcesFromDb.map(NewsResource::id).sorted(), ) // After sync version should be updated assertEquals( - changeList.last().changeListVersion, - synchronizer.getChangeListVersions().newsResourceVersion, + expected = changeList.last().changeListVersion, + actual = synchronizer.getChangeListVersions().newsResourceVersion, ) } @@ -228,12 +229,14 @@ class OfflineFirstNewsRepositoryTest { subject.syncWith(synchronizer) assertEquals( - network.getNewsResources() + expected = network.getNewsResources() .map(NetworkNewsResource::topicEntityShells) .flatten() - .distinctBy(TopicEntity::id), - topicDao.getTopicEntities() - .first(), + .distinctBy(TopicEntity::id) + .sortedBy(TopicEntity::toString), + actual = topicDao.getTopicEntities() + .first() + .sortedBy(TopicEntity::toString), ) } @@ -243,11 +246,13 @@ class OfflineFirstNewsRepositoryTest { subject.syncWith(synchronizer) assertEquals( - network.getNewsResources() + expected = network.getNewsResources() .map(NetworkNewsResource::topicCrossReferences) + .flatten() .distinct() - .flatten(), - newsResourceDao.topicCrossReferences, + .sortedBy(NewsResourceTopicCrossRef::toString), + actual = newsResourceDao.topicCrossReferences + .sortedBy(NewsResourceTopicCrossRef::toString), ) } } diff --git a/core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/testdoubles/TestNewsResourceDao.kt b/core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/testdoubles/TestNewsResourceDao.kt index baeff4a66..bb1ac20ab 100644 --- a/core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/testdoubles/TestNewsResourceDao.kt +++ b/core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/testdoubles/TestNewsResourceDao.kt @@ -21,12 +21,10 @@ import com.google.samples.apps.nowinandroid.core.database.model.NewsResourceEnti import com.google.samples.apps.nowinandroid.core.database.model.NewsResourceTopicCrossRef import com.google.samples.apps.nowinandroid.core.database.model.PopulatedNewsResource import com.google.samples.apps.nowinandroid.core.database.model.TopicEntity -import com.google.samples.apps.nowinandroid.core.model.data.NewsResourceType.Video import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.update -import kotlinx.datetime.Instant val filteredInterestsIds = setOf("1") val nonPresentInterestsIds = setOf("2") @@ -37,17 +35,7 @@ val nonPresentInterestsIds = setOf("2") class TestNewsResourceDao : NewsResourceDao { private var entitiesStateFlow = MutableStateFlow( - listOf( - NewsResourceEntity( - id = "1", - title = "news", - content = "Hilt", - url = "url", - headerImageUrl = "headerImageUrl", - type = Video, - publishDate = Instant.fromEpochMilliseconds(1), - ), - ), + emptyList(), ) internal var topicCrossReferences: List = listOf() @@ -78,7 +66,14 @@ class TestNewsResourceDao : NewsResourceDao { override suspend fun insertOrIgnoreNewsResources( entities: List, ): List { - entitiesStateFlow.value = entities + entitiesStateFlow.update { oldValues -> + // Old values come first so new values don't overwrite them + (oldValues + entities) + .distinctBy(NewsResourceEntity::id) + .sortedWith( + compareBy(NewsResourceEntity::publishDate).reversed(), + ) + } // Assume no conflicts on insert return entities.map { it.id.toLong() } } @@ -88,13 +83,22 @@ class TestNewsResourceDao : NewsResourceDao { } override suspend fun upsertNewsResources(newsResourceEntities: List) { - entitiesStateFlow.value = newsResourceEntities + entitiesStateFlow.update { oldValues -> + // New values come first so they overwrite old values + (newsResourceEntities + oldValues) + .distinctBy(NewsResourceEntity::id) + .sortedWith( + compareBy(NewsResourceEntity::publishDate).reversed(), + ) + } } override suspend fun insertOrIgnoreTopicCrossRefEntities( newsResourceTopicCrossReferences: List, ) { - topicCrossReferences = newsResourceTopicCrossReferences + // Keep old values over new ones + topicCrossReferences = (topicCrossReferences + newsResourceTopicCrossReferences) + .distinctBy { it.newsResourceId to it.topicId } } override suspend fun deleteNewsResources(ids: List) { diff --git a/core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/testdoubles/TestTopicDao.kt b/core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/testdoubles/TestTopicDao.kt index 8ac0dc0b8..c0cef479f 100644 --- a/core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/testdoubles/TestTopicDao.kt +++ b/core/data/src/test/java/com/google/samples/apps/nowinandroid/core/data/testdoubles/TestTopicDao.kt @@ -29,16 +29,7 @@ import kotlinx.coroutines.flow.update class TestTopicDao : TopicDao { private var entitiesStateFlow = MutableStateFlow( - listOf( - TopicEntity( - id = "1", - name = "Topic", - shortDescription = "short description", - longDescription = "long description", - url = "URL", - imageUrl = "image URL", - ), - ), + emptyList(), ) override fun getTopicEntity(topicId: String): Flow { @@ -53,8 +44,10 @@ class TestTopicDao : TopicDao { .map { topics -> topics.filter { it.id in ids } } override suspend fun insertOrIgnoreTopics(topicEntities: List): List { - entitiesStateFlow.value = topicEntities - // Assume no conflicts on insert + // Keep old values over new values + entitiesStateFlow.update { oldValues -> + (oldValues + topicEntities).distinctBy(TopicEntity::id) + } return topicEntities.map { it.id.toLong() } } @@ -63,7 +56,10 @@ class TestTopicDao : TopicDao { } override suspend fun upsertTopics(entities: List) { - entitiesStateFlow.value = entities + // Overwrite old values with new values + entitiesStateFlow.update { oldValues -> + (entities + oldValues).distinctBy(TopicEntity::id) + } } override suspend fun deleteTopics(ids: List) {