Feature Request: PersisterPolicy/Builder

[DanielAsher]

As I understand it, there is no way to specify the maximum number of items, total size, expiry time, purging policy etc. of locally persisted data items. This could lead to the unbounded growth of cached data and force the system to purge locally cached data or issue exceptions.

The responsibility is for the client to implement local cache purging, perhaps in the writer method, or as a background service.

The use case for using Store as a image/video caching library would seem to require such functionality. Is this beyond the scope of the project?

Thanks.

[yigit]

it seems reasonable but not that trivial. right now, store does not track timestamps of all data in persister, doing so will require assuming we know everything about the persister (e.g. no out of the band changes into persister and transactional guaratees between such key recording and real persistence of the data into persister).
Maybe this is better to solve in the file-system persister and make it act like a persistent cache rather than a single source of truth.

[DanielAsher]

@yigit is there any sample code?

file-system persister

I could only find: https://github.com/dropbox/Store/blob/68276197441909be357fe31007b678f882a54be5/filesystem/src/main/java/com/dropbox/android/external/fs3/FileSystemPersister.kt

in package com.dropbox.android.external.fs3 which does not seem to be part of the current release.

[DanielAsher]

Another issue could be that is look difficult (impossible?) to call clear() from the delete lambda and I'm concerned this would cause issues in the sychronization of the Store and the file system.

[alexandercasal]

A problem I'm seeing is if I leave a screen and return to the screen immediately after, the fetcher still makes a network request, even though the response was cached seconds earlier. It was a wasted network call.

I almost created a different bug, but I think this might fit into a PersisterPolicy concept... What if we had a lambda such as shouldFetch that when the stream is started, shouldFetch is checked before executing the fetcher? shouldFetch could provide us a parameter with the key and either the initial memory cache or persisted value from the reader and returns a boolean, thus letting us determine if the persisted cache has expired or not (for example being able to read the cache date from an entity object in a list or run a query against the db for the earliest cache date).

It would be a control value to determine whether or not the fetcher should be called when used with a persister.

myStore.stream(StoreRequest.cached(MyKey("KEY"), refresh = false) isn't sufficient because the network call from the fetcher is never executed, whereas refresh = true is too aggressive where the fetcher will always execute regardless if I made the call seconds ago and am just returning to the screen.

[digitalbuddha]

I like the control value/shouldFetch idea. It seems fairly small of an addition which delegates the decision back to the user, lets see how others feel about it and then we can decide on an api

[digitalbuddha]

Would something like this work for your use case?

suspend inline fun <Key : Any, Output : Any> Store<Key, Output>.fetch(
    key: Key,
    forceFresh: Boolean = false,
    crossinline doFreshIf: (Output) -> Boolean = { false }
): Output {
    return if (forceFresh) {
        // If we're forcing a fresh fetch, do it now
        fresh(key)
    } else {
        // Else we'll check the current cached value
        val cached = cachedOnly(key)
        if (cached == null || doFreshIf(cached)) {
            // Our cached value isn't valid, do a fresh fetch
            fresh(key)
        } else {
            // We have a current cached value
            cached
        }
    }
}

suspend fun <Key : Any, Output : Any> Store<Key, Output>.cachedOnly(key: Key): Output? {
    return stream(StoreRequest.cached(key, refresh = false))
        .filterNot { it is StoreResponse.Loading }
        .first()
        .dataOrNull()
}

[hansenji]

I don't recommend the null check because null can be a valid value.
I also need this on the stream call.

[alexandercasal]

I agree that I would also want to use this with a stream call on a store that utilizes a persister.

I'm not sure this is the best option considering we only need to use doFreshIf when we use a persister with the Store, but what if doFreshIf was added to the stream call:
override fun stream(request: StoreRequest<Key>, doFreshIf: (Output) -> Boolean = { request.refresh }): Flow<StoreResponse<Output>>

Then the stream call could take into account doFreshIf before making any network calls. It looks like this may need to be in the diskNetworkCombined call?

[digitalbuddha]

Ok, rather than using null what if we add an additional RecordProvider override to persisters where you can return RecordState of Missing | Present | Expired . Then the store builder will have the doFreshIf function which gets called on each stream call. @alexandercasal I wanted to do this at the store builder rather than the request level as I don't see the policy being different key to key. I'd be curious what your use case is and how we can adapt to it.

[alexandercasal]

@digitalbuddha That sounds like it would work. I agree doing it at the store builder would be better rather than request level. My previous snippet was more to show I'm primarily using stream and would need the expired api to support stream.

I use stream for both the primary store flow and my pull refresh flow. This allows me to get the StoreResponse data on pull refresh events and handle toggling loading state or showing errors. I then use first with a predicate to collect the refresh flow so it doesn't collect longer than needed. This has proven to be most useful so far over using a suspending call to fresh().

My primary flow I would want to take into account the expired so I don't hit the fetcher if the local cache is still valid, but for my pull refresh stream I would want to ignore expired and always hit the fetcher.

[alexandercasal]

@digitalbuddha I think the solution to this already exists. Upon further experimentation, my team has come this the following conclusion: Use the fetcher's block to handle the logic if the request should be made, etc...

Here's a simple example:

private val myStore = StoreBuilder.fromNonFlow<Key, List<Data>> {
   if (! cacheValid()) {
      remoteDataSource.requestData()
   } else {
      emptyResult()
   }
}

We can see we already have the ability to do the requested checks if the call should be made based on cache validity as well as other logic if we wanted to.

[digitalbuddha]

perfect! I'd prefer to keep our api surface small and can roll your example above into WIP recipes