Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Fallback to source of truth if fetcher fails on a fresh request #394

Closed
sky-ricardocarrapico opened this issue Jan 27, 2022 · 12 comments
Labels
enhancement New feature or request

Comments

@sky-ricardocarrapico
Copy link

sky-ricardocarrapico commented Jan 27, 2022

We currently do not have a good way of requesting fresh data (ie. it's the first data we receive on the stream) and then keep observing source of truth. Creating a StoreRequest.fresh(key) works for the happy path but if the fetcher fails, we'll no longer be observing data.

To implement this behavior we're currently using StoreRequest.cached(key, refresh = true) together with this extension:

private fun <Output> Flow<StoreResponse<Output>>.waitForFetcher(): Flow<StoreResponse<Output>> {
    return flow {
        var hasFetcherEmitted = false
        var cacheResponse: StoreResponse<Output>? = null
        collect { response ->
            if (response.origin == ResponseOrigin.Fetcher) {
                hasFetcherEmitted = true
            } else {
                cacheResponse = response
            }

            if (hasFetcherEmitted) {
                emit(response)
            }

            if (response.origin == ResponseOrigin.Fetcher && response is StoreResponse.Error) {
                cacheResponse?.let { emit(it) }
            }
        }
    }
}

But it would be nice if this was supported on Store.

@sky-ricardocarrapico sky-ricardocarrapico added the enhancement New feature or request label Jan 27, 2022
@digitalbuddha
Copy link
Contributor

Could you write a failing test case please. Happy to play with it and see of making some generic solution. Thank you

@sky-ricardocarrapico
Copy link
Author

Added a failing test here: https://github.com/sky-ricardocarrapico/Store/blob/53fb10b4fc308333cc625304a2f5f70a8e17e598/store/src/test/java/com/dropbox/android/external/store4/impl/FlowStoreTest.kt#L775-L804

Not sure if this will need a new API or not. I've seen another issue talking about adding a boolean to fresh to indicate if we should fallback to cache. That could work while keeping current behavior if the boolean had a false default.

And let me ask a different question: what is the recommended pattern to read the current cache/SoT value without triggering the fetcher? There is StoreRequest.cached(key, refresh = false) but if we have no cache, it'll trigger the fetcher. Should we skip Store and go directly to SoT?

Thanks!

@digitalbuddha
Copy link
Contributor

Hi Ricardo it feels reasonable to add the functionality you want. Maybe something like StoreRequest.tryFresh where it will try to get fresh but fallback to SOT if error returned. I can take a look at implementing next week unless you or someone want to try taking it on.

As far as your second question. Correct if you want to have cache data or error (without fallback to fetcher) you would need to access your SOT directly.

Could you link the other issue you found here too. Thanks!

@sky-ricardocarrapico
Copy link
Author

Could you link the other issue you found here too. Thanks!

#185 (comment)

As far as your second question. Correct if you want to have cache data or error (without fallback to fetcher) you would need to access your SOT directly.

I see, makes sense. Thanks!

@digitalbuddha
Copy link
Contributor

Hi friends, I'm looking at our tests particularly https://github.com/dropbox/Store/blob/862da9b4f9847939c9cdb1d14695ca0b8e1ea986/store/src/test/java/com/dropbox/android/external/store4/impl/FetcherResponseTest.kt#L46 which validates that on an error emission from fetcher we will still emit new data from fetcher. Maybe you want to use the ofResult style of fetcher which will allow you to never throw the exception and "stop your stream" but instead return a StoreResult.Data that your SOT processes as an error and never saves but still emits?

Alternatively could using the catch operator work in your case?
Something like:
store.stream(StoreRequest.fresh(key = 3))
.catch { emitAll(StoreRequest.cached(key = 3))

This would effectively let you switch to SOT anytime the error happens (which I would imagine can only happen from fetcher)

I worry about making it a generic case as I am not sure if this behavior can be generalized without too many variants. For example sometimes you want to switch directly to SOT (like the catch operator above) while other times user might want to keep consuming the starting flow (By using fetcher.OfResultFlow. Similar you might want to restart the upstream flow when there is an error (RetryWhen). Another example is folks might want to only catch the first error while others want to catch all others.

Since you are able to solve your use case with an extension function on Store, I hesitate making any changes to store internals. Hope that makes sense

@sky-ricardocarrapico
Copy link
Author

which validates that on an error emission from fetcher we will still emit new data from fetcher. Maybe you want to use the ofResult style of fetcher which will allow you to never throw the exception and "stop your stream" but instead return a StoreResult.Data that your SOT processes as an error and never saves but still emits?

I'm already using an OfResult fetcher. But this is only for flow fetchers. I would say that the most common use case of this lib is actually a non flow fetcher with a flow SoT.

And changing my fetcher to cover this use case means that I'm affecting the other use cases that are served by the same store.

Alternatively could using the catch operator work in your case?

I guess this could work by looking into a FetcherResult.Error, but then it doesn't set the memory cache value with the value from SoT.

But I understand your point in which the user might want different behaviors when they receive an error and it's hard to cover them all.

To be honest, to me this feels like such a common use, that I would argue for it to be on the lib. But if it was not needed until now, it probably isn't very common?

@digitalbuddha
Copy link
Contributor

I don’t think I’ll be able to implement myself but if you’d like to do a pr to add the functionality I would accept it

@sky-ricardocarrapico
Copy link
Author

I can give it a try. How would you like the see the API for this?

  • StoreRequest.fresh(fallbackToSourceOfTruth: Boolean = false)
  • StoreRequest.tryFresh()

The first one makes more sense to me because it adds some parallelism with the StoreRequest.cache(refresh). But it means assuming false by default. But I'm willing to create the second one, if you prefer it.

@digitalbuddha
Copy link
Contributor

Since you’ll be the first consumer I’ll leave the choice up to you 😀

thank you

@matt-ramotar
Copy link
Collaborator

Hey @sky-ricardocarrapico - I think we can handle this with Superstore. For example take a look at this test case. Thoughts?

@sky-ricardocarrapico
Copy link
Author

That seems to cover it, yes. Thank you!

This was one of the use cases that I felt was missing from Store. The other was getting cached data only, without triggering a fetch if data doesn't exist, but that seems to be tracked in #457.

@matt-ramotar
Copy link
Collaborator

Closed by #540

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants