Merge pull request #608 from android/tunjid-batch-sync

Batch sync news resources from remote

Author: tunjid

core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/OfflineFirstNewsRepository.kt

@@ -34,6 +34,10 @@ 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
+
 /**
  * Disk storage backed implementation of the [NewsRepository].
  * Reads are exclusively from local storage to support offline access.
@@ -65,26 +69,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(),
+                    )
+                }
             },
         )
 }

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
@@ -111,8 +112,8 @@ class OfflineFirstNewsRepositoryTest {
             )
 
             assertEquals(
-                emptyList(),
-                subject.getNewsResources(
+                expected = emptyList(),
+                actual = subject.getNewsResources(
                     query = NewsResourceQuery(
                         filterTopicIds = nonPresentInterestsIds,
                     ),
@@ -135,14 +136,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,
             )
         }
 
@@ -176,14 +177,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,
             )
         }
 
@@ -215,14 +216,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,
             )
         }
 
@@ -232,12 +233,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),
             )
         }
 
@@ -247,11 +250,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),
             )
         }
 }

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<NewsResourceEntity>(),
     )
 
     internal var topicCrossReferences: List<NewsResourceTopicCrossRef> = listOf()
@@ -78,7 +66,14 @@ class TestNewsResourceDao : NewsResourceDao {
     override suspend fun insertOrIgnoreNewsResources(
         entities: List<NewsResourceEntity>,
     ): List<Long> {
-        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<NewsResourceEntity>) {
-        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<NewsResourceTopicCrossRef>,
     ) {
-        topicCrossReferences = newsResourceTopicCrossReferences
+        // Keep old values over new ones
+        topicCrossReferences = (topicCrossReferences + newsResourceTopicCrossReferences)
+            .distinctBy { it.newsResourceId to it.topicId }
     }
 
     override suspend fun deleteNewsResources(ids: List<String>) {

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<TopicEntity>(),
     )
 
     override fun getTopicEntity(topicId: String): Flow<TopicEntity> {
@@ -53,8 +44,10 @@ class TestTopicDao : TopicDao {
             .map { topics -> topics.filter { it.id in ids } }
 
     override suspend fun insertOrIgnoreTopics(topicEntities: List<TopicEntity>): List<Long> {
-        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<TopicEntity>) {
-        entitiesStateFlow.value = entities
+        // Overwrite old values with new values
+        entitiesStateFlow.update { oldValues ->
+            (entities + oldValues).distinctBy(TopicEntity::id)
+        }
     }
 
     override suspend fun deleteTopics(ids: List<String>) {