From 8a90b61b97bd4ed663d1d3b64543d26b0da8446a Mon Sep 17 00:00:00 2001 From: Michael <5672750+mibac138@users.noreply.github.com> Date: Wed, 13 Mar 2024 13:01:00 +0100 Subject: [PATCH] Refactor networkBoundResource (#2482) --------- Co-authored-by: Faierbel --- .../java/io/github/wulkanowy/data/Resource.kt | 127 ++++++++---------- .../repositories/AdminMessageRepository.kt | 3 +- .../data/repositories/MessageRepository.kt | 2 +- .../io/github/wulkanowy/data/ResourceTest.kt | 10 +- 4 files changed, 62 insertions(+), 80 deletions(-) diff --git a/app/src/main/java/io/github/wulkanowy/data/Resource.kt b/app/src/main/java/io/github/wulkanowy/data/Resource.kt index b4982b9a0..7c6c2a9ff 100644 --- a/app/src/main/java/io/github/wulkanowy/data/Resource.kt +++ b/app/src/main/java/io/github/wulkanowy/data/Resource.kt @@ -9,6 +9,7 @@ import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.emitAll import kotlinx.coroutines.flow.filter +import kotlinx.coroutines.flow.filterNot import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flow @@ -22,15 +23,15 @@ import timber.log.Timber import kotlin.time.Duration import kotlin.time.Duration.Companion.seconds -sealed class Resource { +sealed interface Resource { - open class Loading : Resource() + open class Loading : Resource data class Intermediate(val data: T) : Loading() - data class Success(val data: T) : Resource() + data class Success(val data: T) : Resource - data class Error(val error: Throwable) : Resource() + data class Error(val error: Throwable) : Resource } val Resource.dataOrNull: T? @@ -97,7 +98,7 @@ fun Flow>.logResourceStatus(name: String, showData: Boolean = fa Timber.i("$name: $description") } -fun Flow>.mapResourceData(block: suspend (T) -> U) = map { +inline fun Flow>.mapResourceData(crossinline block: suspend (T) -> U) = map { when (it) { is Resource.Success -> Resource.Success(block(it.data)) is Resource.Intermediate -> Resource.Intermediate(block(it.data)) @@ -167,33 +168,32 @@ suspend fun Flow>.waitForResult() = takeWhile { it is Resource.L // Can cause excessive amounts of `Resource.Intermediate` to be emitted. Unless that is desired, // use `debounceIntermediates` to alleviate this behavior. -inline fun combineResourceFlows( - flows: Iterable>>, -): Flow>> = combine(flows) { items -> - var isIntermediate = false - val data = mutableListOf() - for (item in items) { - when (item) { - is Resource.Success -> data.add(item.data) - is Resource.Intermediate -> { - isIntermediate = true - data.add(item.data) - } +inline fun combineResourceFlows(flows: Iterable>>): Flow>> = + combine(flows) { items -> + var isIntermediate = false + val data = mutableListOf() + for (item in items) { + when (item) { + is Resource.Success -> data.add(item.data) + is Resource.Intermediate -> { + isIntermediate = true + data.add(item.data) + } - is Resource.Loading -> return@combine Resource.Loading() - is Resource.Error -> continue + is Resource.Loading -> return@combine Resource.Loading() + is Resource.Error -> continue + } + } + if (data.isEmpty()) { + // All items have to be errors for this to happen, so just return the first one. + // mapData is functionally useless and exists only to satisfy the type checker + items.first().mapData { listOf(it) } + } else if (isIntermediate) { + Resource.Intermediate(data) + } else { + Resource.Success(data) } } - if (data.isEmpty()) { - // All items have to be errors for this to happen, so just return the first one. - // mapData is functionally useless and exists only to satisfy the type checker - items.first().mapData { listOf(it) } - } else if (isIntermediate) { - Resource.Intermediate(data) - } else { - Resource.Success(data) - } -} @OptIn(FlowPreview::class) fun Flow>.debounceIntermediates(timeout: Duration = 5.seconds) = flow { @@ -214,70 +214,51 @@ fun Flow>.debounceIntermediates(timeout: Duration = 5.seconds) = }) } + inline fun networkBoundResource( mutex: Mutex = Mutex(), - showSavedOnLoading: Boolean = true, crossinline isResultEmpty: (ResultType) -> Boolean, crossinline query: () -> Flow, - crossinline fetch: suspend (ResultType) -> RequestType, + crossinline fetch: suspend () -> RequestType, crossinline saveFetchResult: suspend (old: ResultType, new: RequestType) -> Unit, - crossinline onFetchFailed: (Throwable) -> Unit = { }, crossinline shouldFetch: (ResultType) -> Boolean = { true }, crossinline filterResult: (ResultType) -> ResultType = { it } -) = flow { - emit(Resource.Loading()) - - val data = query().first() - emitAll(if (shouldFetch(data)) { - val filteredResult = filterResult(data) - - if (showSavedOnLoading && !isResultEmpty(filteredResult)) { - emit(Resource.Intermediate(filteredResult)) - } - - try { - val newData = fetch(data) - mutex.withLock { saveFetchResult(query().first(), newData) } - query().map { Resource.Success(filterResult(it)) } - } catch (throwable: Throwable) { - onFetchFailed(throwable) - flowOf(Resource.Error(throwable)) - } - } else { - query().map { Resource.Success(filterResult(it)) } - }) -} +) = networkBoundResource( + mutex = mutex, + isResultEmpty = isResultEmpty, + query = query, + fetch = fetch, + saveFetchResult = saveFetchResult, + shouldFetch = shouldFetch, + mapResult = filterResult +) @JvmName("networkBoundResourceWithMap") -inline fun networkBoundResource( +inline fun networkBoundResource( mutex: Mutex = Mutex(), - showSavedOnLoading: Boolean = true, - crossinline isResultEmpty: (T) -> Boolean, + crossinline isResultEmpty: (MappedResultType) -> Boolean, crossinline query: () -> Flow, - crossinline fetch: suspend (ResultType) -> RequestType, + crossinline fetch: suspend () -> RequestType, crossinline saveFetchResult: suspend (old: ResultType, new: RequestType) -> Unit, - crossinline onFetchFailed: (Throwable) -> Unit = { }, crossinline shouldFetch: (ResultType) -> Boolean = { true }, - crossinline mapResult: (ResultType) -> T, + crossinline mapResult: (ResultType) -> MappedResultType, ) = flow { emit(Resource.Loading()) val data = query().first() - emitAll(if (shouldFetch(data)) { - val mappedResult = mapResult(data) + if (shouldFetch(data)) { + emit(Resource.Intermediate(data)) - if (showSavedOnLoading && !isResultEmpty(mappedResult)) { - emit(Resource.Intermediate(mappedResult)) - } try { - val newData = fetch(data) + val newData = fetch() mutex.withLock { saveFetchResult(query().first(), newData) } - query().map { Resource.Success(mapResult(it)) } } catch (throwable: Throwable) { - onFetchFailed(throwable) - flowOf(Resource.Error(throwable)) + emit(Resource.Error(throwable)) + return@flow } - } else { - query().map { Resource.Success(mapResult(it)) } - }) + } + + emitAll(query().map { Resource.Success(it) }) } + .mapResourceData { mapResult(it) } + .filterNot { it is Resource.Intermediate && isResultEmpty(it.data) } diff --git a/app/src/main/java/io/github/wulkanowy/data/repositories/AdminMessageRepository.kt b/app/src/main/java/io/github/wulkanowy/data/repositories/AdminMessageRepository.kt index b831ee755..aa0022b08 100644 --- a/app/src/main/java/io/github/wulkanowy/data/repositories/AdminMessageRepository.kt +++ b/app/src/main/java/io/github/wulkanowy/data/repositories/AdminMessageRepository.kt @@ -6,6 +6,7 @@ import io.github.wulkanowy.data.db.dao.AdminMessageDao import io.github.wulkanowy.data.db.entities.AdminMessage import io.github.wulkanowy.data.networkBoundResource import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.filterNot import kotlinx.coroutines.sync.Mutex import javax.inject.Inject import javax.inject.Singleton @@ -28,6 +29,6 @@ class AdminMessageRepository @Inject constructor( saveFetchResult = { oldItems, newItems -> adminMessageDao.removeOldAndSaveNew(oldItems, newItems) }, - showSavedOnLoading = false, ) + .filterNot { it is Resource.Intermediate } } diff --git a/app/src/main/java/io/github/wulkanowy/data/repositories/MessageRepository.kt b/app/src/main/java/io/github/wulkanowy/data/repositories/MessageRepository.kt index ede2a0fde..f91dc63e3 100644 --- a/app/src/main/java/io/github/wulkanowy/data/repositories/MessageRepository.kt +++ b/app/src/main/java/io/github/wulkanowy/data/repositories/MessageRepository.kt @@ -122,7 +122,7 @@ class MessageRepository @Inject constructor( fetch = { wulkanowySdkFactory.create(student) .getMessageDetails( - messageKey = it!!.message.messageGlobalKey, + messageKey = message.messageGlobalKey, markAsRead = message.unread && markAsRead, ) }, diff --git a/app/src/test/java/io/github/wulkanowy/data/ResourceTest.kt b/app/src/test/java/io/github/wulkanowy/data/ResourceTest.kt index ea846a57b..aa79a637b 100644 --- a/app/src/test/java/io/github/wulkanowy/data/ResourceTest.kt +++ b/app/src/test/java/io/github/wulkanowy/data/ResourceTest.kt @@ -1,6 +1,10 @@ package io.github.wulkanowy.data -import io.mockk.* +import io.mockk.Runs +import io.mockk.coEvery +import io.mockk.coVerifyOrder +import io.mockk.just +import io.mockk.mockk import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.delay import kotlinx.coroutines.flow.flowOf @@ -42,7 +46,6 @@ class ResourceTest { // first networkBoundResource( isResultEmpty = { false }, - showSavedOnLoading = false, query = { repo.query() }, fetch = { val data = repo.fetch() @@ -57,7 +60,6 @@ class ResourceTest { // second networkBoundResource( isResultEmpty = { false }, - showSavedOnLoading = false, query = { repo.query() }, fetch = { val data = repo.fetch() @@ -124,7 +126,6 @@ class ResourceTest { networkBoundResource( isResultEmpty = { false }, mutex = saveResultMutex, - showSavedOnLoading = false, query = { repo.query() }, fetch = { val data = repo.fetch() @@ -143,7 +144,6 @@ class ResourceTest { networkBoundResource( isResultEmpty = { false }, mutex = saveResultMutex, - showSavedOnLoading = false, query = { repo.query() }, fetch = { val data = repo.fetch()