apollographql / apollo-kotlin

:rocket:  A strongly-typed, caching GraphQL client for the JVM, Android, and Kotlin multiplatform.
https://www.apollographql.com/docs/kotlin
MIT License
3.74k stars 653 forks source link

Add support for mutations when offline #1006

Closed wtrocki closed 6 years ago

wtrocki commented 6 years ago

Motivation

Apollo Android comes with the first class support for caching queries, however there seems to be missing support for mutation (for obvious reasons because of the complexity etc.) JavaScript client has some advanced functionalities like eventual consistency that could be migrated into the Android client.

Definition of the problem

If you perform mutation when offline or server is not available mutation will be lost. Users need to handle that and save mutation themselves.

Investigation

During my investigation I found out that mutations are still processed by cache layer but they are not cached because appropriate headers are missing. Currently if mutation is going to fail users will need to handle this and reschedule mutation themselves. If mutation is executed with failure refetchQueries method is still executed (in onComplete method).

Proposal

Couple separate actions can be done to improve offline support for mutations. They can be individually logged as separate issues, but I wanted to to provide wider context for each of those:

1. Do not include mutations in query cache mechanism.

Cache for the mutations makes no sense.

Raw proposal for change: https://github.com/wtrocki/apollo-android/compare/master...mutation-cache?quick_pull=1

It will need more testing before PR will be created

2. Do not refetch queries for failed mutations

Do not refetch queries in onComplete method

Raw proposal for change: https://github.com/wtrocki/apollo-android/compare/master...query-refetch-onsuccess?quick_pull=1 It will need more testing before PR will be created

3. Provide support for optimisticResponses

Provide support for optimisticResponse in mutations (similar to implementation in JavaScript)

This is possible at the moment on the user level by directly interacting with cache, but I'm curious if team see value and sense to have similar implementation to js client.

4. Add option to store mutations when their fail with the network problem (offline) and provide way to reattempt them when network status changes.

This will mean that if mutation will fail it will be stored into storage and all pending mutations will be rescheduled when user need them. To avoid many corner cases it's still best to give users api to reschedule and flush pending mutations.

High level interface:

interface PendingMutation {
    // Gets Mutations from storage and execute them again
    public void rescheduleAllPending();

    // Removing all cached mutations
    public void flushAll();
}

I'm happy to work on this functionality if this is something that Apollo team will find useful.

wtrocki commented 6 years ago

I found similar ideas proposed for the JS library while back: https://github.com/apollographql/apollo-cache-asyncstorage/issues/6

sav007 commented 6 years ago

Thx for bringing this up, couple notes:

  1. Do not include mutations in query cache mechanism. Cache for the mutations makes no sense.
  1. It's not quite true. The response of the execution of mutation operation should be cached, as for example you update user profile by running mutation and as a response of the mutation you request the new update user profile. This response should be reflected in the cache.

  2. Mutation has feature called optimistic updates what means that the cache will be updated with the response that you expect before mutation execution and will be rolled back if it fails. See more https://www.apollographql.com/docs/react/features/optimistic-ui.html And we are using ApolloCacheInterceptor to perform such logic.

By all these above not sure removing ApolloCacheInterceptor from mutation calls is a great idea.

  1. Do not refetch queries for failed mutations

From giving a quick thought, here is the case where refetch queries make sense. We don't know for sure if actual mutation execution failed or it's failed due to some other reasons, like network or parsing etc. So does it make sense to re-fetch queries to make sure we are in sync even when mutation failed?

  1. Provide support for optimisticResponses

We do have it. See com.apollographql.apollo.ApolloMutationCall.Factory#mutate

    /**
     * <p>Creates and prepares a new {@link ApolloMutationCall} call with optimistic updates.</p>
     *
     * Provided optimistic updates will be stored in {@link com.apollographql.apollo.cache.normalized.ApolloStore}
     * immediately before mutation execution. Any {@link ApolloQueryWatcher} dependent on the changed cache records will
     * be re-fetched.
     *
     * @param mutation              the {@link Mutation} which needs to be performed
     * @param withOptimisticUpdates optimistic updates for this mutation
     * @return prepared {@link ApolloMutationCall} call to be executed at some point in the future
     */
    <D extends Mutation.Data, T, V extends Mutation.Variables> ApolloMutationCall<T> mutate(
        @NotNull Mutation<D, T, V> mutation, @NotNull D withOptimisticUpdates);
  }
  1. Add option to store mutations when their fail with the network problem (offline) and provide way to reattempt them when network status changes.

Have a little concern here, is this is a really core Apollo feature? Or this is up to user to define how he / she would like to store / re-try mutation execution. My general opinion is that any lib should provide only minimum core features and make possible for the user to build more complicated one. I don't mind to have this feature and I think that would be awesome, but we should be careful as this is additional responsibilities to take like support / maintenance / new feature requests etc. I would love to see this kind of discussion: what we can do in order to support user to build this feature on their end? Are we missing smth that will help? Like Apollo call serialization?

wtrocki commented 6 years ago

Thank you so much for valuable information. Fully agree with all statements. withOptimisticUpdates was the option I was looking for.

wtrocki commented 6 years ago

I would love to see this kind of discussion: what we can do in order to support user to build this feature on their end? Are we missing smth that will help? Like Apollo call serialization?

I think that I was wrong here by expecting core lib to have more than just cache layer. Ideally users should also have their own offline store that is being kept up to date by grapql queries. Cache layer is good for persisting the last query state etc. If user want to perform mutation he should still update his offline store first and then perform network/GraphQL update. This is totally outside of the competence of the core lib - it is awesome that library provides cache, but it will be cool to get more documentation for mutations to cover offline behaviour patterns.

This issue provided really valuable input. I will try to collect all of that in form of blog post or even add some elements to mutation documentation.

CarsonRedeye commented 5 years ago

So is it not recommended to use the Apollo cache as the source of truth for the client state? Does anyone know of an example where graphql is used alongside an offline cache to provide offline first functionality?

wtrocki commented 5 years ago

So is it not recommended to use the Apollo cache as the source of truth for the client state?

@CarsonRedeye It is best for cache to reflect server state and have a separate place for stagging offline changes. This is simply to avoid unneeded cache invalidation and make it work with Apollo client without modifications.

Does anyone know of an example where GraphQL is used alongside an offline cache to provide offline first functionality?

If you look for example check https://github.com/graphql-heroes/offix (this is my side project that extends js client by adding separate cache, but the same rule will apply to android)