From 09f5c3bc619c0d3ee194a804df7dcab9fa35d306 Mon Sep 17 00:00:00 2001 From: Adetunji Dahunsi Date: Mon, 13 Feb 2023 10:33:30 -0500 Subject: [PATCH] Add NewsResourceQuery to better query encapsulation Change-Id: I93f8c8ae2d1144975f6f9ff1ba93be9a4600768a --- .../core/data/repository/NewsRepository.kt | 24 +++-- .../repository/OfflineFirstNewsRepository.kt | 11 +- .../repository/fake/FakeNewsRepository.kt | 27 ++--- .../OfflineFirstNewsRepositoryTest.kt | 13 ++- .../data/testdoubles/TestNewsResourceDao.kt | 24 +++-- .../core/database/dao/NewsResourceDaoTest.kt | 101 ++++++++++++++++++ .../core/database/dao/NewsResourceDao.kt | 33 +++--- .../domain/GetUserNewsResourcesUseCase.kt | 14 ++- .../domain/GetUserNewsResourcesUseCaseTest.kt | 7 +- .../testing/repository/TestNewsRepository.kt | 20 ++-- .../feature/foryou/ForYouViewModel.kt | 7 +- .../feature/topic/TopicViewModel.kt | 9 +- 12 files changed, 224 insertions(+), 66 deletions(-) diff --git a/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/NewsRepository.kt b/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/NewsRepository.kt index e4f184c44..0e53f1239 100644 --- a/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/NewsRepository.kt +++ b/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/NewsRepository.kt @@ -21,18 +21,30 @@ import com.google.samples.apps.nowinandroid.core.model.data.NewsResource import kotlinx.coroutines.flow.Flow /** - * Data layer implementation for [NewsResource] + * Encapsulation class for query parameters for [NewsResource] */ -interface NewsRepository : Syncable { +data class NewsResourceQuery( + /** + * Topic ids to filter for. Null means any topic id will match. + */ + val filterTopicIds: Set? = null, /** - * Returns available news resources as a stream. + * News ids to filter for. Null means any news id will match. */ - fun getNewsResources(): Flow> + val filterNewsIds: Set? = null, +) +/** + * Data layer implementation for [NewsResource] + */ +interface NewsRepository : Syncable { /** - * Returns available news resources as a stream filtered by topics. + * Returns available news resources that match the specified [query]. */ fun getNewsResources( - filterTopicIds: Set = emptySet(), + query: NewsResourceQuery = NewsResourceQuery( + filterTopicIds = null, + filterNewsIds = null, + ), ): Flow> } 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 9e041b956..9e419b485 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 @@ -44,14 +44,13 @@ class OfflineFirstNewsRepository @Inject constructor( private val network: NiaNetworkDataSource, ) : NewsRepository { - override fun getNewsResources(): Flow> = - newsResourceDao.getNewsResources() - .map { it.map(PopulatedNewsResource::asExternalModel) } - override fun getNewsResources( - filterTopicIds: Set, + query: NewsResourceQuery, ): Flow> = newsResourceDao.getNewsResources( - filterTopicIds = filterTopicIds, + useFilterTopicIds = query.filterTopicIds != null, + filterTopicIds = query.filterTopicIds ?: emptySet(), + useFilterNewsIds = query.filterNewsIds != null, + filterNewsIds = query.filterNewsIds ?: emptySet(), ) .map { it.map(PopulatedNewsResource::asExternalModel) } diff --git a/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/fake/FakeNewsRepository.kt b/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/fake/FakeNewsRepository.kt index d6a712538..39ad05d1e 100644 --- a/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/fake/FakeNewsRepository.kt +++ b/core/data/src/main/java/com/google/samples/apps/nowinandroid/core/data/repository/fake/FakeNewsRepository.kt @@ -19,6 +19,7 @@ package com.google.samples.apps.nowinandroid.core.data.repository.fake import com.google.samples.apps.nowinandroid.core.data.Synchronizer import com.google.samples.apps.nowinandroid.core.data.model.asEntity import com.google.samples.apps.nowinandroid.core.data.repository.NewsRepository +import com.google.samples.apps.nowinandroid.core.data.repository.NewsResourceQuery import com.google.samples.apps.nowinandroid.core.database.model.NewsResourceEntity import com.google.samples.apps.nowinandroid.core.database.model.asExternalModel import com.google.samples.apps.nowinandroid.core.model.data.NewsResource @@ -43,26 +44,28 @@ class FakeNewsRepository @Inject constructor( private val datasource: FakeNiaNetworkDataSource, ) : NewsRepository { - override fun getNewsResources(): Flow> = - flow { - emit( - datasource.getNewsResources() - .map(NetworkNewsResource::asEntity) - .map(NewsResourceEntity::asExternalModel), - ) - }.flowOn(ioDispatcher) - override fun getNewsResources( - filterTopicIds: Set, + query: NewsResourceQuery, ): Flow> = flow { emit( datasource .getNewsResources() - .filter { it.topics.intersect(filterTopicIds).isNotEmpty() } + .filter { networkNewsResource -> + // Filter out any news resources which don't match the current query. + // If no query parameters (filterTopicIds or filterNewsIds) are specified + // then the news resource is returned. + listOfNotNull( + true, + query.filterNewsIds?.contains(networkNewsResource.id), + query.filterTopicIds?.let { filterTopicIds -> + networkNewsResource.topics.intersect(filterTopicIds).isNotEmpty() + }, + ) + .all(true::equals) + } .map(NetworkNewsResource::asEntity) .map(NewsResourceEntity::asExternalModel), - ) }.flowOn(ioDispatcher) 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 74848d655..9f43d3441 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 @@ -92,13 +92,16 @@ class OfflineFirstNewsRepositoryTest { fun offlineFirstNewsRepository_news_resources_for_topic_is_backed_by_news_resource_dao() = runTest { assertEquals( - newsResourceDao.getNewsResources( + expected = newsResourceDao.getNewsResources( filterTopicIds = filteredInterestsIds, + useFilterTopicIds = true, ) .first() .map(PopulatedNewsResource::asExternalModel), - subject.getNewsResources( - filterTopicIds = filteredInterestsIds, + actual = subject.getNewsResources( + query = NewsResourceQuery( + filterTopicIds = filteredInterestsIds, + ), ) .first(), ) @@ -106,7 +109,9 @@ class OfflineFirstNewsRepositoryTest { assertEquals( emptyList(), subject.getNewsResources( - filterTopicIds = nonPresentInterestsIds, + query = NewsResourceQuery( + filterTopicIds = nonPresentInterestsIds, + ), ) .first(), ) 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 f63014075..baeff4a66 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 @@ -52,19 +52,27 @@ class TestNewsResourceDao : NewsResourceDao { internal var topicCrossReferences: List = listOf() - override fun getNewsResources(): Flow> = - entitiesStateFlow.map { - it.map(NewsResourceEntity::asPopulatedNewsResource) - } - override fun getNewsResources( + useFilterTopicIds: Boolean, filterTopicIds: Set, + useFilterNewsIds: Boolean, + filterNewsIds: Set, ): Flow> = - getNewsResources() + entitiesStateFlow + .map { it.map(NewsResourceEntity::asPopulatedNewsResource) } .map { resources -> - resources.filter { resource -> - resource.topics.any { it.id in filterTopicIds } + var result = resources + if (useFilterTopicIds) { + result = result.filter { resource -> + resource.topics.any { it.id in filterTopicIds } + } + } + if (useFilterNewsIds) { + result = result.filter { resource -> + resource.entity.id in filterNewsIds + } } + result } override suspend fun insertOrIgnoreNewsResources( diff --git a/core/database/src/androidTest/java/com/google/samples/apps/nowinandroid/core/database/dao/NewsResourceDaoTest.kt b/core/database/src/androidTest/java/com/google/samples/apps/nowinandroid/core/database/dao/NewsResourceDaoTest.kt index c1c1b39ba..83dc3c2e6 100644 --- a/core/database/src/androidTest/java/com/google/samples/apps/nowinandroid/core/database/dao/NewsResourceDaoTest.kt +++ b/core/database/src/androidTest/java/com/google/samples/apps/nowinandroid/core/database/dao/NewsResourceDaoTest.kt @@ -84,6 +84,44 @@ class NewsResourceDaoTest { ) } + @Test + fun newsResourceDao_filters_items_by_news_ids_by_descending_publish_date() = runTest { + val newsResourceEntities = listOf( + testNewsResource( + id = "0", + millisSinceEpoch = 0, + ), + testNewsResource( + id = "1", + millisSinceEpoch = 3, + ), + testNewsResource( + id = "2", + millisSinceEpoch = 1, + ), + testNewsResource( + id = "3", + millisSinceEpoch = 2, + ), + ) + newsResourceDao.upsertNewsResources( + newsResourceEntities, + ) + + val savedNewsResourceEntities = newsResourceDao.getNewsResources( + useFilterNewsIds = true, + filterNewsIds = setOf("3", "0"), + ) + .first() + + assertEquals( + listOf("3", "0"), + savedNewsResourceEntities.map { + it.entity.id + }, + ) + } + @Test fun newsResourceDao_filters_items_by_topic_ids_by_descending_publish_date() = runTest { val topicEntities = listOf( @@ -132,6 +170,7 @@ class NewsResourceDaoTest { ) val filteredNewsResources = newsResourceDao.getNewsResources( + useFilterTopicIds = true, filterTopicIds = topicEntities .map(TopicEntity::id) .toSet(), @@ -143,6 +182,68 @@ class NewsResourceDaoTest { ) } + @Test + fun newsResourceDao_filters_items_by_news_and_topic_ids_by_descending_publish_date() = runTest { + val topicEntities = listOf( + testTopicEntity( + id = "1", + name = "1", + ), + testTopicEntity( + id = "2", + name = "2", + ), + ) + val newsResourceEntities = listOf( + testNewsResource( + id = "0", + millisSinceEpoch = 0, + ), + testNewsResource( + id = "1", + millisSinceEpoch = 3, + ), + testNewsResource( + id = "2", + millisSinceEpoch = 1, + ), + testNewsResource( + id = "3", + millisSinceEpoch = 2, + ), + ) + val newsResourceTopicCrossRefEntities = topicEntities.mapIndexed { index, topicEntity -> + NewsResourceTopicCrossRef( + newsResourceId = index.toString(), + topicId = topicEntity.id, + ) + } + + topicDao.insertOrIgnoreTopics( + topicEntities = topicEntities, + ) + newsResourceDao.upsertNewsResources( + newsResourceEntities, + ) + newsResourceDao.insertOrIgnoreTopicCrossRefEntities( + newsResourceTopicCrossRefEntities, + ) + + val filteredNewsResources = newsResourceDao.getNewsResources( + useFilterTopicIds = true, + filterTopicIds = topicEntities + .map(TopicEntity::id) + .toSet(), + useFilterNewsIds = true, + filterNewsIds = setOf("1"), + ).first() + + assertEquals( + listOf("1"), + filteredNewsResources.map { it.entity.id }, + ) + } + @Test fun newsResourceDao_deletes_items_by_ids() = runTest { diff --git a/core/database/src/main/java/com/google/samples/apps/nowinandroid/core/database/dao/NewsResourceDao.kt b/core/database/src/main/java/com/google/samples/apps/nowinandroid/core/database/dao/NewsResourceDao.kt index af0a59bce..782e5c87a 100644 --- a/core/database/src/main/java/com/google/samples/apps/nowinandroid/core/database/dao/NewsResourceDao.kt +++ b/core/database/src/main/java/com/google/samples/apps/nowinandroid/core/database/dao/NewsResourceDao.kt @@ -34,29 +34,36 @@ import kotlinx.coroutines.flow.Flow */ @Dao interface NewsResourceDao { - @Transaction - @Query( - value = """ - SELECT * FROM news_resources - ORDER BY publish_date DESC - """, - ) - fun getNewsResources(): Flow> + /** + * Fetches news resources that match the query parameters + */ @Transaction @Query( value = """ SELECT * FROM news_resources - WHERE id in - ( - SELECT news_resource_id FROM news_resources_topics - WHERE topic_id IN (:filterTopicIds) - ) + WHERE + CASE WHEN :useFilterNewsIds + THEN id IN (:filterNewsIds) + ELSE 1 + END + AND + CASE WHEN :useFilterTopicIds + THEN id IN + ( + SELECT news_resource_id FROM news_resources_topics + WHERE topic_id IN (:filterTopicIds) + ) + ELSE 1 + END ORDER BY publish_date DESC """, ) fun getNewsResources( + useFilterTopicIds: Boolean = false, filterTopicIds: Set = emptySet(), + useFilterNewsIds: Boolean = false, + filterNewsIds: Set = emptySet(), ): Flow> /** diff --git a/core/domain/src/main/java/com/google/samples/apps/nowinandroid/core/domain/GetUserNewsResourcesUseCase.kt b/core/domain/src/main/java/com/google/samples/apps/nowinandroid/core/domain/GetUserNewsResourcesUseCase.kt index db274bbbd..393b7b08b 100644 --- a/core/domain/src/main/java/com/google/samples/apps/nowinandroid/core/domain/GetUserNewsResourcesUseCase.kt +++ b/core/domain/src/main/java/com/google/samples/apps/nowinandroid/core/domain/GetUserNewsResourcesUseCase.kt @@ -17,6 +17,7 @@ package com.google.samples.apps.nowinandroid.core.domain import com.google.samples.apps.nowinandroid.core.data.repository.NewsRepository +import com.google.samples.apps.nowinandroid.core.data.repository.NewsResourceQuery import com.google.samples.apps.nowinandroid.core.data.repository.UserDataRepository import com.google.samples.apps.nowinandroid.core.domain.model.UserNewsResource import com.google.samples.apps.nowinandroid.core.domain.model.mapToUserNewsResources @@ -38,17 +39,14 @@ class GetUserNewsResourcesUseCase @Inject constructor( /** * Returns a list of UserNewsResources which match the supplied set of topic ids. * - * @param filterTopicIds - A set of topic ids used to filter the list of news resources. If - * this is empty the list of news resources will not be filtered. + * @param query - Summary of query parameters for news resources. */ operator fun invoke( - filterTopicIds: Set = emptySet(), + query: NewsResourceQuery = NewsResourceQuery(), ): Flow> = - if (filterTopicIds.isEmpty()) { - newsRepository.getNewsResources() - } else { - newsRepository.getNewsResources(filterTopicIds = filterTopicIds) - }.mapToUserNewsResources(userDataRepository.userData) + newsRepository.getNewsResources( + query = query, + ).mapToUserNewsResources(userDataRepository.userData) } private fun Flow>.mapToUserNewsResources( diff --git a/core/domain/src/test/java/com/google/samples/apps/nowinandroid/core/domain/GetUserNewsResourcesUseCaseTest.kt b/core/domain/src/test/java/com/google/samples/apps/nowinandroid/core/domain/GetUserNewsResourcesUseCaseTest.kt index 32ee8773c..0ff863d7c 100644 --- a/core/domain/src/test/java/com/google/samples/apps/nowinandroid/core/domain/GetUserNewsResourcesUseCaseTest.kt +++ b/core/domain/src/test/java/com/google/samples/apps/nowinandroid/core/domain/GetUserNewsResourcesUseCaseTest.kt @@ -16,6 +16,7 @@ package com.google.samples.apps.nowinandroid.core.domain +import com.google.samples.apps.nowinandroid.core.data.repository.NewsResourceQuery import com.google.samples.apps.nowinandroid.core.domain.model.mapToUserNewsResources import com.google.samples.apps.nowinandroid.core.model.data.NewsResource import com.google.samples.apps.nowinandroid.core.model.data.NewsResourceType.Video @@ -67,7 +68,11 @@ class GetUserNewsResourcesUseCaseTest { @Test fun whenFilteredByTopicId_matchingNewsResourcesAreReturned() = runTest { // Obtain a stream of user news resources for the given topic id. - val userNewsResources = useCase(filterTopicIds = setOf(sampleTopic1.id)) + val userNewsResources = useCase( + NewsResourceQuery( + filterTopicIds = setOf(sampleTopic1.id), + ), + ) // Send test data into the repositories. newsRepository.sendNewsResources(sampleNewsResources) diff --git a/core/testing/src/main/java/com/google/samples/apps/nowinandroid/core/testing/repository/TestNewsRepository.kt b/core/testing/src/main/java/com/google/samples/apps/nowinandroid/core/testing/repository/TestNewsRepository.kt index 87e2fe009..d0bfd21a1 100644 --- a/core/testing/src/main/java/com/google/samples/apps/nowinandroid/core/testing/repository/TestNewsRepository.kt +++ b/core/testing/src/main/java/com/google/samples/apps/nowinandroid/core/testing/repository/TestNewsRepository.kt @@ -18,6 +18,7 @@ package com.google.samples.apps.nowinandroid.core.testing.repository import com.google.samples.apps.nowinandroid.core.data.Synchronizer import com.google.samples.apps.nowinandroid.core.data.repository.NewsRepository +import com.google.samples.apps.nowinandroid.core.data.repository.NewsResourceQuery import com.google.samples.apps.nowinandroid.core.model.data.NewsResource import com.google.samples.apps.nowinandroid.core.model.data.Topic import kotlinx.coroutines.channels.BufferOverflow @@ -33,13 +34,20 @@ class TestNewsRepository : NewsRepository { private val newsResourcesFlow: MutableSharedFlow> = MutableSharedFlow(replay = 1, onBufferOverflow = BufferOverflow.DROP_OLDEST) - override fun getNewsResources(): Flow> = newsResourcesFlow - - override fun getNewsResources(filterTopicIds: Set): Flow> = - getNewsResources().map { newsResources -> - newsResources.filter { - it.topics.map(Topic::id).intersect(filterTopicIds).isNotEmpty() + override fun getNewsResources(query: NewsResourceQuery): Flow> = + newsResourcesFlow.map { newsResources -> + var result = newsResources + query.filterTopicIds?.let { filterTopicIds -> + result = newsResources.filter { + it.topics.map(Topic::id).intersect(filterTopicIds).isNotEmpty() + } + } + query.filterNewsIds?.let { filterNewsIds -> + result = newsResources.filter { + filterNewsIds.contains(it.id) + } } + result } /** diff --git a/feature/foryou/src/main/java/com/google/samples/apps/nowinandroid/feature/foryou/ForYouViewModel.kt b/feature/foryou/src/main/java/com/google/samples/apps/nowinandroid/feature/foryou/ForYouViewModel.kt index cd029b4af..085593932 100644 --- a/feature/foryou/src/main/java/com/google/samples/apps/nowinandroid/feature/foryou/ForYouViewModel.kt +++ b/feature/foryou/src/main/java/com/google/samples/apps/nowinandroid/feature/foryou/ForYouViewModel.kt @@ -18,6 +18,7 @@ package com.google.samples.apps.nowinandroid.feature.foryou import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope +import com.google.samples.apps.nowinandroid.core.data.repository.NewsResourceQuery import com.google.samples.apps.nowinandroid.core.data.repository.UserDataRepository import com.google.samples.apps.nowinandroid.core.data.util.SyncStatusMonitor import com.google.samples.apps.nowinandroid.core.domain.GetFollowableTopicsUseCase @@ -127,7 +128,11 @@ private fun UserDataRepository.getFollowedUserNewsResources( if (followedTopics == null) { flowOf(emptyList()) } else { - getUserNewsResources(filterTopicIds = followedTopics) + getUserNewsResources( + NewsResourceQuery( + filterTopicIds = followedTopics, + ), + ) } } diff --git a/feature/topic/src/main/java/com/google/samples/apps/nowinandroid/feature/topic/TopicViewModel.kt b/feature/topic/src/main/java/com/google/samples/apps/nowinandroid/feature/topic/TopicViewModel.kt index c58d40b5e..fcabff16b 100644 --- a/feature/topic/src/main/java/com/google/samples/apps/nowinandroid/feature/topic/TopicViewModel.kt +++ b/feature/topic/src/main/java/com/google/samples/apps/nowinandroid/feature/topic/TopicViewModel.kt @@ -19,6 +19,7 @@ package com.google.samples.apps.nowinandroid.feature.topic import androidx.lifecycle.SavedStateHandle import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope +import com.google.samples.apps.nowinandroid.core.data.repository.NewsResourceQuery import com.google.samples.apps.nowinandroid.core.data.repository.TopicsRepository import com.google.samples.apps.nowinandroid.core.data.repository.UserDataRepository import com.google.samples.apps.nowinandroid.core.decoder.StringDecoder @@ -120,9 +121,11 @@ private fun topicUiState( ), ) } + is Result.Loading -> { TopicUiState.Loading } + is Result.Error -> { TopicUiState.Error } @@ -137,7 +140,9 @@ private fun newsUiState( ): Flow { // Observe news val newsStream: Flow> = getSaveableNewsResources( - filterTopicIds = setOf(element = topicId), + NewsResourceQuery( + filterTopicIds = setOf(element = topicId), + ), ) // Observe bookmarks @@ -156,9 +161,11 @@ private fun newsUiState( val news = newsToBookmarksResult.data.first NewsUiState.Success(news) } + is Result.Loading -> { NewsUiState.Loading } + is Result.Error -> { NewsUiState.Error }