From 5771dcc4d55a957e73c829b13db908a026b9987e Mon Sep 17 00:00:00 2001 From: matt-ramotar Date: Sun, 14 Jul 2024 11:07:53 -0400 Subject: [PATCH] Cover DefaultLoadingHandler --- .../internal/logger/impl/RealPagingLogger.kt | 16 -- .../pager/impl/DefaultLoadingHandler.kt | 74 +++++----- .../runtime/DefaultLoadingHandlerTest.kt | 137 +++++++++++++++++- .../testUtils/TestExponentialBackoff.kt | 9 ++ .../paging/testUtils/TestPagingLogger.kt | 19 +++ 5 files changed, 202 insertions(+), 53 deletions(-) create mode 100644 paging-runtime/src/commonTest/kotlin/org/mobilenativefoundation/storex/paging/testUtils/TestExponentialBackoff.kt create mode 100644 paging-runtime/src/commonTest/kotlin/org/mobilenativefoundation/storex/paging/testUtils/TestPagingLogger.kt diff --git a/paging-runtime/src/commonMain/kotlin/org/mobilenativefoundation/storex/paging/runtime/internal/logger/impl/RealPagingLogger.kt b/paging-runtime/src/commonMain/kotlin/org/mobilenativefoundation/storex/paging/runtime/internal/logger/impl/RealPagingLogger.kt index 2ea4f7e..de3b23d 100644 --- a/paging-runtime/src/commonMain/kotlin/org/mobilenativefoundation/storex/paging/runtime/internal/logger/impl/RealPagingLogger.kt +++ b/paging-runtime/src/commonMain/kotlin/org/mobilenativefoundation/storex/paging/runtime/internal/logger/impl/RealPagingLogger.kt @@ -24,20 +24,4 @@ class RealPagingLogger( Logger.v("storex/paging") { message } } } -} - -class TestPagingLogger : PagingLogger { - override fun verbose(message: String) { - println(message) - } - - override fun debug(message: String) { - println(message) - } - - override fun error(message: String, error: Throwable) { - println(message) - println(error) - } - } \ No newline at end of file diff --git a/paging-runtime/src/commonMain/kotlin/org/mobilenativefoundation/storex/paging/runtime/internal/pager/impl/DefaultLoadingHandler.kt b/paging-runtime/src/commonMain/kotlin/org/mobilenativefoundation/storex/paging/runtime/internal/pager/impl/DefaultLoadingHandler.kt index 9a04075..e358c6a 100644 --- a/paging-runtime/src/commonMain/kotlin/org/mobilenativefoundation/storex/paging/runtime/internal/pager/impl/DefaultLoadingHandler.kt +++ b/paging-runtime/src/commonMain/kotlin/org/mobilenativefoundation/storex/paging/runtime/internal/pager/impl/DefaultLoadingHandler.kt @@ -75,38 +75,44 @@ internal class DefaultLoadingHandler, K : Comparable, V : updateLoadingState(loadParams.direction) - try { - - // Update max and min request so far for both append and prepend - // This ensures we're tracking the full range of requests regardless of sort order - // These methods don't necessarily update the fetching state, but simply recompute the max and min - fetchingStateHolder.updateMaxRequestSoFar(modifiedParams.key) - fetchingStateHolder.updateMinRequestSoFar(modifiedParams.key) - - loadPage(modifiedParams, loadParams.direction, addNextToQueue) - } catch (pagingError: Throwable) { - logger.error( - """ + // Update max and min request so far for both append and prepend + // This ensures we're tracking the full range of requests regardless of sort order + // These methods don't necessarily update the fetching state, but simply recompute the max and min + fetchingStateHolder.updateMaxRequestSoFar(loadParams.key) + fetchingStateHolder.updateMinRequestSoFar(loadParams.key) + + tryLoad(loadParams, addNextToQueue) + } + } + } + + // This is extracted from handleLoading because we might need to retry the load + // We can't simply re invoke handleLoading is because it will cause a deadlock + private suspend fun tryLoad(loadParams: PagingSource.LoadParams, addNextToQueue: Boolean) { + try { + + loadPage(loadParams, addNextToQueue) + } catch (pagingError: Throwable) { + logger.error( + """ Caught error Load params: $loadParams Direction: ${loadParams.direction} """.trimIndent(), - pagingError - ) - handleError(pagingError, modifiedParams, loadParams.direction) - } - } + pagingError + ) + handleError(pagingError, loadParams, addNextToQueue) } } - private suspend fun loadPage(loadParams: PagingSource.LoadParams, direction: LoadDirection, addNextToQueue: Boolean) { + private suspend fun loadPage(loadParams: PagingSource.LoadParams, addNextToQueue: Boolean) { val pageLoadFlow = store.loadPage(loadParams) // Design decision to use only the latest data for any given params // Not OK to skip params, but OK to skip stale data pageLoadFlow.collectLatest { pageLoadState -> when (pageLoadState) { - is PageLoadState.Success -> handleSuccess(pageLoadState, loadParams.key, direction, addNextToQueue) + is PageLoadState.Success -> handleSuccess(pageLoadState, loadParams.key, loadParams.direction, addNextToQueue) is PageLoadState.Error -> throw pageLoadState.toThrowable() else -> { /* Do nothing for other states */ } @@ -171,16 +177,16 @@ internal class DefaultLoadingHandler, K : Comparable, V : queueManager.updateExistingPendingJob(key, inFlight = false, completed = true) } - private suspend fun passThroughError(error: Throwable, loadParams: PagingSource.LoadParams, direction: LoadDirection) { + private suspend fun passThroughError(error: Throwable, loadParams: PagingSource.LoadParams) { // Passing the error through - updateErrorState(error, direction) + updateErrorState(error, loadParams.direction) // Complete the job - completeLoadJobAfterError(loadParams, direction) + completeLoadJobAfterError(loadParams) } - private suspend fun completeLoadJobAfterError(loadParams: PagingSource.LoadParams, direction: LoadDirection) { + private suspend fun completeLoadJobAfterError(loadParams: PagingSource.LoadParams) { // Design decision to remove placeholders when not retrying after an error // Page refreshes are not currently supported // So the page must currently contain placeholders @@ -189,7 +195,7 @@ internal class DefaultLoadingHandler, K : Comparable, V : // Marking the pending job as completed queueManager.updateExistingPendingJob(loadParams.key, inFlight = false, completed = true) - when (direction) { + when (loadParams.direction) { LoadDirection.Prepend -> { // Removing the params from the prepend queue queueManager.prependQueue.removeFirst { it.params == loadParams } @@ -205,20 +211,20 @@ internal class DefaultLoadingHandler, K : Comparable, V : retryBookkeeper.resetCount(loadParams) } - private suspend fun handleError(error: Throwable, loadParams: PagingSource.LoadParams, direction: LoadDirection) { + private suspend fun handleError(error: Throwable, loadParams: PagingSource.LoadParams, addNextToQueue: Boolean) { when (errorHandlingStrategy) { ErrorHandlingStrategy.Ignore -> { logger.error( """ Ignoring the error Load params: $loadParams - Direction: $direction + Direction: ${loadParams.direction} """.trimIndent(), error ) // Complete the job, do nothing else - completeLoadJobAfterError(loadParams, direction) + completeLoadJobAfterError(loadParams) } ErrorHandlingStrategy.PassThrough -> { @@ -226,11 +232,11 @@ internal class DefaultLoadingHandler, K : Comparable, V : """ Passing the error through Load params: $loadParams - Direction: $direction + Direction: $loadParams.direction """.trimIndent(), error ) - passThroughError(error, loadParams, direction) + passThroughError(error, loadParams) } is ErrorHandlingStrategy.RetryLast -> { @@ -241,7 +247,7 @@ internal class DefaultLoadingHandler, K : Comparable, V : """ Determining whether to retry Load params: $loadParams - Direction: $direction + Direction: ${loadParams.direction} Retry count: $retryCount Max retries: ${errorHandlingStrategy.maxRetries} """.trimIndent() @@ -254,22 +260,22 @@ internal class DefaultLoadingHandler, K : Comparable, V : // Retrying with backoff exponentialBackoff.execute(retryCount) { - when (direction) { + when (loadParams.direction) { LoadDirection.Prepend -> { logger.verbose("Retrying prepend load for: $loadParams") - handlePrependLoading(loadParams) + tryLoad(loadParams, addNextToQueue) } LoadDirection.Append -> { logger.verbose("Retrying append load for: $loadParams") - handleAppendLoading(loadParams) + tryLoad(loadParams, addNextToQueue) } } } } else { logger.verbose("At maximum retries, passing the error through") - passThroughError(error, loadParams, direction) + passThroughError(error, loadParams) } } } diff --git a/paging-runtime/src/commonTest/kotlin/org/mobilenativefoundation/storex/paging/runtime/DefaultLoadingHandlerTest.kt b/paging-runtime/src/commonTest/kotlin/org/mobilenativefoundation/storex/paging/runtime/DefaultLoadingHandlerTest.kt index 627da69..75b9886 100644 --- a/paging-runtime/src/commonTest/kotlin/org/mobilenativefoundation/storex/paging/runtime/DefaultLoadingHandlerTest.kt +++ b/paging-runtime/src/commonTest/kotlin/org/mobilenativefoundation/storex/paging/runtime/DefaultLoadingHandlerTest.kt @@ -1,11 +1,14 @@ package org.mobilenativefoundation.storex.paging.runtime +import dev.mokkery.annotations.DelicateMokkeryApi +import dev.mokkery.answering.Answer import dev.mokkery.answering.returns import dev.mokkery.every import dev.mokkery.everySuspend import dev.mokkery.matcher.any import dev.mokkery.matcher.eq import dev.mokkery.mock +import dev.mokkery.spy import dev.mokkery.verify.VerifyMode.Companion.exactly import dev.mokkery.verifySuspend import kotlinx.coroutines.flow.MutableStateFlow @@ -13,7 +16,6 @@ import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.test.runTest import kotlinx.datetime.Clock import org.mobilenativefoundation.storex.paging.custom.Middleware -import org.mobilenativefoundation.storex.paging.runtime.internal.logger.impl.TestPagingLogger import org.mobilenativefoundation.storex.paging.runtime.internal.pager.api.ExponentialBackoff import org.mobilenativefoundation.storex.paging.runtime.internal.pager.api.FetchingStateHolder import org.mobilenativefoundation.storex.paging.runtime.internal.pager.api.LoadParamsQueue @@ -26,10 +28,13 @@ import org.mobilenativefoundation.storex.paging.runtime.internal.store.api.Norma import org.mobilenativefoundation.storex.paging.runtime.internal.store.api.PageLoadState import org.mobilenativefoundation.storex.paging.testUtils.CursorIdentifier import org.mobilenativefoundation.storex.paging.testUtils.Post +import org.mobilenativefoundation.storex.paging.testUtils.TestExponentialBackoff +import org.mobilenativefoundation.storex.paging.testUtils.TestPagingLogger import org.mobilenativefoundation.storex.paging.testUtils.TimelineRequest import kotlin.test.BeforeTest import kotlin.test.Test +@OptIn(DelicateMokkeryApi::class) class DefaultLoadingHandlerTest { private val appendQueue = mock>() @@ -52,9 +57,11 @@ class DefaultLoadingHandlerTest { private val errorHandlingStrategy = ErrorHandlingStrategy.Ignore private val middleware = emptyList>() private val operationApplier = mock>() - private val retryBookkeeper = mock>() + private val retryBookkeeper = mock> { + everySuspend { getCount(any()) } returns 0 + } private val logger = TestPagingLogger() - private val exponentialBackoff = mock() + private val exponentialBackoff = spy(TestExponentialBackoff()) private lateinit var loadingHandler: DefaultLoadingHandler @@ -359,4 +366,128 @@ class DefaultLoadingHandlerTest { verifySuspend(exactly(0)) { pagingStateManager.updateWithPrependError(any()) } verifySuspend(exactly(1)) { queueManager.updateExistingPendingJob(eq(loadParams.key), eq(false), eq(true)) } } + + @Test + fun handleAppendLoading_givenRetryLastStrategy_whenPersistentError_thenShouldPassThroughAfterHittingLimit() = runTest { + // Given + val cursor = "1" + val size = 20 + val key = TimelineRequest(cursor, size) + val strategy = LoadStrategy.SkipCache + val direction = LoadDirection.Append + val loadParams = PagingSource.LoadParams(key, strategy, direction) + val expectedPosts = List(size) { createFakePost(it + 1) } + val errorHandlingStrategy = ErrorHandlingStrategy.RetryLast(1) + var retryCount = 0 + + fun incrementRetryCount() { + retryCount++ + } + + fun getRetryCount() = retryCount + val retryBookkeeper = mock> { + everySuspend { getCount(any()) }.answers(Answer.Block { getRetryCount() }) + everySuspend { incrementCount(any()) }.answers(Answer.Block { incrementRetryCount() }) + } + + loadingHandler = DefaultLoadingHandler( + store, + pagingStateManager, + queueManager, + fetchingStateHolder, + errorHandlingStrategy, + middleware, + operationApplier, + retryBookkeeper, + logger, + exponentialBackoff + ) + + val expectedStoreFlow = flowOf( + PageLoadState.Error.Message("", true) + ) + + everySuspend { store.loadPage(eq(loadParams)) }.returns(expectedStoreFlow) + everySuspend { operationApplier.applyOperations(any(), any(), any(), any()) } returns ItemSnapshotList(expectedPosts) + + // When + loadingHandler.handleAppendLoading(loadParams) + + // Then + verifySuspend(exactly(2)) { store.loadPage(eq(loadParams)) } + verifySuspend(exactly(1)) { fetchingStateHolder.updateMaxRequestSoFar(eq(key)) } + verifySuspend(exactly(1)) { fetchingStateHolder.updateMinRequestSoFar(eq(key)) } + verifySuspend(exactly(2)) { retryBookkeeper.getCount(eq(loadParams)) } + verifySuspend(exactly(1)) { retryBookkeeper.incrementCount(eq(loadParams)) } + verifySuspend(exactly(1)) { exponentialBackoff.execute(eq(0), any()) } + + verifySuspend(exactly(1)) { pagingStateManager.updateWithAppendError(any()) } + + verifySuspend(exactly(1)) { store.clearPage(eq(loadParams.key)) } + verifySuspend(exactly(1)) { queueManager.updateExistingPendingJob(eq(loadParams.key), eq(false), eq(true)) } + verifySuspend(exactly(1)) { appendQueue.removeFirst(any()) } + verifySuspend(exactly(1)) { retryBookkeeper.resetCount(eq(loadParams)) } + } + + @Test + fun handlePrependLoading_givenRetryLastStrategy_whenPersistentError_thenShouldPassThroughAfterHittingLimit() = runTest { + // Given + val cursor = "1" + val size = 20 + val key = TimelineRequest(cursor, size) + val strategy = LoadStrategy.SkipCache + val direction = LoadDirection.Prepend + val loadParams = PagingSource.LoadParams(key, strategy, direction) + val expectedPosts = List(size) { createFakePost(it + 1) } + val errorHandlingStrategy = ErrorHandlingStrategy.RetryLast(1) + var retryCount = 0 + + fun incrementRetryCount() { + retryCount++ + } + + fun getRetryCount() = retryCount + val retryBookkeeper = mock> { + everySuspend { getCount(any()) }.answers(Answer.Block { getRetryCount() }) + everySuspend { incrementCount(any()) }.answers(Answer.Block { incrementRetryCount() }) + } + + loadingHandler = DefaultLoadingHandler( + store, + pagingStateManager, + queueManager, + fetchingStateHolder, + errorHandlingStrategy, + middleware, + operationApplier, + retryBookkeeper, + logger, + exponentialBackoff + ) + + val expectedStoreFlow = flowOf( + PageLoadState.Error.Message("", true) + ) + + everySuspend { store.loadPage(eq(loadParams)) }.returns(expectedStoreFlow) + everySuspend { operationApplier.applyOperations(any(), any(), any(), any()) } returns ItemSnapshotList(expectedPosts) + + // When + loadingHandler.handlePrependLoading(loadParams) + + // Then + verifySuspend(exactly(2)) { store.loadPage(eq(loadParams)) } + verifySuspend(exactly(1)) { fetchingStateHolder.updateMaxRequestSoFar(eq(key)) } + verifySuspend(exactly(1)) { fetchingStateHolder.updateMinRequestSoFar(eq(key)) } + verifySuspend(exactly(2)) { retryBookkeeper.getCount(eq(loadParams)) } + verifySuspend(exactly(1)) { retryBookkeeper.incrementCount(eq(loadParams)) } + verifySuspend(exactly(1)) { exponentialBackoff.execute(eq(0), any()) } + + verifySuspend(exactly(1)) { pagingStateManager.updateWithPrependError(any()) } + + verifySuspend(exactly(1)) { store.clearPage(eq(loadParams.key)) } + verifySuspend(exactly(1)) { queueManager.updateExistingPendingJob(eq(loadParams.key), eq(false), eq(true)) } + verifySuspend(exactly(1)) { prependQueue.removeFirst(any()) } + verifySuspend(exactly(1)) { retryBookkeeper.resetCount(eq(loadParams)) } + } } \ No newline at end of file diff --git a/paging-runtime/src/commonTest/kotlin/org/mobilenativefoundation/storex/paging/testUtils/TestExponentialBackoff.kt b/paging-runtime/src/commonTest/kotlin/org/mobilenativefoundation/storex/paging/testUtils/TestExponentialBackoff.kt new file mode 100644 index 0000000..532e155 --- /dev/null +++ b/paging-runtime/src/commonTest/kotlin/org/mobilenativefoundation/storex/paging/testUtils/TestExponentialBackoff.kt @@ -0,0 +1,9 @@ +package org.mobilenativefoundation.storex.paging.testUtils + +import org.mobilenativefoundation.storex.paging.runtime.internal.pager.api.ExponentialBackoff + +internal class TestExponentialBackoff : ExponentialBackoff { + override suspend fun execute(retryCount: Int, block: suspend () -> Unit) { + block() + } +} \ No newline at end of file diff --git a/paging-runtime/src/commonTest/kotlin/org/mobilenativefoundation/storex/paging/testUtils/TestPagingLogger.kt b/paging-runtime/src/commonTest/kotlin/org/mobilenativefoundation/storex/paging/testUtils/TestPagingLogger.kt new file mode 100644 index 0000000..3341565 --- /dev/null +++ b/paging-runtime/src/commonTest/kotlin/org/mobilenativefoundation/storex/paging/testUtils/TestPagingLogger.kt @@ -0,0 +1,19 @@ +package org.mobilenativefoundation.storex.paging.testUtils + +import org.mobilenativefoundation.storex.paging.runtime.internal.logger.api.PagingLogger + +class TestPagingLogger : PagingLogger { + override fun verbose(message: String) { + println(message) + } + + override fun debug(message: String) { + println(message) + } + + override fun error(message: String, error: Throwable) { + println(message) + println(error) + } + +} \ No newline at end of file