Updating Queries on Mutation (for queries issued with different variables)

[richburdon]

Using either the Query's options.reducer or the Mutation's props.updateQueries we can "patch" the current query result to reflect the recently executed mutation.

But this seems only to apply to the currently cached query result for the current variables (i.e., set by the query's options(props) function).

For example, say I have a ListView that has a ListQuery that can be parameterized by a $filter variable which the server uses to filter a set of items. Let's say the user can toggle between different filter variables (e.g., A: { filter: { label:"red" }}, B { filter: { label:"blue" }}). If the ListView is currently displaying filter A and I mutate an item to change its label from red to blue, then how can I patch the result set for filter B, so that when I switch to that filter the mutated item is now visible.

Alternatively, how can I invalidate the query cache for the OTHER filter when the mutation occurs. Or more generally, warn all other queries across the framework that may be affected by the current mutation?

Furthermore, what is the roadmap for offline -- where simply invalidating the query sets wouldn't be appropriate?

(Possibly related to #621)

[yurigenin]

When the reducer or updateQueries are called there is one extra property passed along with mutationResults: queryVariables. So, what you are supposed to do is to check which variable was used for this query and update your query result accordingly. So, in your case, your updateQueries will be called twice - once for List A query and once for List B query. Alternatively, both reducers will be called - one for List A query and one for List B query. Hope this helps.

[richburdon]

Hi @yurigenin, Yep, I'm using queryVariables already.

Just to be clear and make sure I understand this:

I have a single graphql container, ListView. The parent react component configures the container like this, <ListView filter={ filter }/>, where the filter variable is configured via params passed by the react-router.

I have something like this to configure the ListView query:

graphql(ItemsQuery, {
    options: (props) => {
      let { filter } = props;
      return {
        variables: {
          filter
        },
    },
}(ListView)

If I toggle between "/inbox" and "/archived" router views the filter will change.

Now, if I understand this correctly, apollo-client caches the the results from both queries (for the same ListView component) with the different variables.

So, suppose I want to move an item from one view to the other: when I mutate the item in "/inbox", my updateQueries callback can filter the view (using the queryVariables for "/inbox") and remove the item from the displayed list (and modify the cache for this query). BUT, I have no control (so far) over the cached result set for "/archived". So when I switch to this view, I don't see the mutated item.

In my app updateQueries and reducers is only called once per mutation.

Does that make sense, and am I missing something?

[yurigenin]

Before we continue, the example you show would not work. You are wrapping your ListView into Apollo HOC. So you should export this HOC and use it in the parent. Using <ListView filter={filter} /> directly will bypass Apollo HOC altogether. Is it a typo on your part?

[yurigenin]

Before you respond and assuming it was a typo in your example, looking at the apollo-client code I'm pretty sure it is a bug. When you switch variables the same query id is being reused when fetching data so in the queries property of the apollo state you have the same query but its variables are different. Once the query returns, the data in the store has a new query result. So now you have two query results corresponding to two different queries. However when the mutation result is processed the queries property of the apollo state is used to enumerate all previous query results and it only contains the reference to one of them (the one that was run the last time), not both of them. So, your reducer is only called for the last query and the previous query is ignored. So now when you switch to the previous view its cached query result is still stale and you see what you see.

It is definitely a bug. The only workaround it to make sure all your object selections in queries use id and __typename so the returned results are normalized. This will make sure that both queries will reference the same updated object after mutation.

[richburdon]

Thanks for the detailed response.

First, yes I'm exporting and using the HOC; the convention I'm following is to use the same name.

class ListView extends React.Component { ... }
export default graphql({ ... })(ListView);
...
import ListView from 'list';

OK, thanks for confirming this. I don't understand your workaround though. Here's my query (already has id and __typename).

const ItemsQuery = gql`
  query ItemsQuery($filter: Filter, $offset: Int, $count: Int) { 
    items(filter: $filter, offset: $offset, count: $count) {
      id
      __typename   
      ...ItemFragment
    }
  }
`;

Can you point me at the relevant code you mention where the results are stored/processed.

Thanks again.

[yurigenin]

In your case the workaround will not work because both queries, as I understand, return different set of items based on a filter.

This is a bug in the QueryManager.collectResultBehaviorsFromUpdateQueries. When the variables change a new query should be generated under a different query id so that mutations can update all query results not just the one that was returned last.

I'm not a contributor (yet) and still 'learning the ropes'. So let's wait and see what the official response will be from the apollo guys. They may point to some other technique to solve this problem as it looks like a very basic scenario and I'm surprised nobody experienced it yet even though
I reproduced it easily in my environment.

[yurigenin]

You need to add a Redux extension from the chrome app store to your dev tools. There, you just go to the state tab and look under the apollo key to find both queries (under queries key) and results (under data key). Look under ROOT_QUERY to follow all the normalized query results. You will see that in your case you will have different results based on a different filter value. But under queries key you will only see one query that references only one set of variables. This is the only query whose results are being passed to your reducer.

[helfer]

@richburdon Thanks for pointing it out. @yurigenin is right that apollo-client currently reuses the query id, which means you can't apply updates to those results. What should really happen is a bit of a tricky question, because technically your old query is no longer active, and so its result shouldn't be updated anyway. But since in your case you are switching back and forth, you would naturally expect to be able to update both result sets. There may be something we can do with custom resolvers, but it would involve a lot of "manual" work, and I'm not convinced that that's the right way to go.
As a workaround, you could create two separate components for the different states, and switch between them in your view. That way they would always both be active, and you could update their results.

[yurigenin]

Unfortunately, creating different components defeats the purpose of variables in this case. I can have a view that allows users to filter by 4 dropdowns. Each dropdown is represented by a variable. So potentially, I can have 16 different states. Creating 16 different views to represent each state would be prohibitive and non-intuitive.

[yurigenin]

@helfer I think we could fix this by saving information about all combinations of variables that this query was running with in the past. So before the query is reused we add a current set of variables to the query store. Then, when we need to call reducers we use these to find all the query results in the store and pass them to the reducers. What do you think?

[yurigenin]

@stubailo @helfer Do you guys think this is something that you will be actively looking into in foreseeable future? This is one of the pretty common use cases and creating a separate component every time you want to use a different value for a variable is not a very credible solution, I would think. I have a presentation in a couple of weeks to evaluate different options for graphql clients and this is one of the stumbling blocks in my little demo using apollo-client. My company wants to go full monty on using apollo stack but this could be a big hurdle.

Thanks for all the great work you are doing, by the way.

[helfer]

@yurigenin I'm not sure if this is something worth building in the short term, but it would definitely warrant some design discussion. I think the problem is quite clear, and it's real, so we should have a solution, but it's not clear to me what a good solution to this would look like that doesn't either make the code a lot more complicated or the usage a lot more complicated. If we can (collectively) come up with something that avoids both of those problems, then there's a good chance that we'll build it in!

[yurigenin]

@helfer I just found another issue where I get an apollo exception when using optimistic response while using a mutation.

So, I have a wrapped apollo component that accepts a variable that can take on two values: 'Open' and 'Closed' (in real life it will be more than just two but for the sake of the discussion let's just limit it to two). The component is supposed to show items in a list and items are filtered on the gql server based on their status: open or closed. So when I first open the view it retrieves all open items by default (query variable is set 'Open'). Now, I decide to flip one item's status to Closed so I perform a mutation with optimistic result. I artificially put a delay on returning a real result from the server (timeout set to 5 secs on the server to test some real time scenarios). Now, I flip the variable on the query to 'Closed' which submits a new query but it sees that no refetch flag is set so it tries to use the current results (which is the issue that started this thread). But since the real result of mutation did not come back yet it tries to use the optimistic results stashed in the 'optimistic' reducer part of the store. And here it cannot find the query with the 'Closed' variable and throws an error:

"Can't find field quotes({"status":"CLOSED"}) on object (ROOT_QUERY) {
  "quotes({\"status\":\"OPEN\"})": [
    "Item1",
    "Item10",
    "Item3",
......
  ]
}.
Perhaps you want to use the 'returnPartialData' option?"

[yurigenin]

@helfer Actually, I just switched to two different HOCs for the above scenario but I'm still getting the same error when using optimistic response in the mutation. So, it looks like it is a separate bug.

The bug is that even if I now fetch fresh data for the second component, when its results are being processed the optimistic reducer still has the previous data and it is being used instead of the real data which just came in. Here's the culprit in the QueryManager.queryListenerForObserver:

resultFromStore = {
              data: readQueryFromStore({
                store: this.getDataWithOptimisticResults(),
                query: this.queryDocuments[queryId],
                variables: queryStoreValue.previousVariables || queryStoreValue.variables,
                returnPartialData: options.returnPartialData || options.noFetch,
              }),
              loading: queryStoreValue.loading,
              networkStatus: queryStoreValue.networkStatus,
            };

As you see, this.getDataWithOptimisticResults() is called regardless if the query already has the fresh data in the store. So, it seems as though the optimistic data is not purged from the store in time.

[yurigenin]

@helfer I guess nobody sees this because the mutation results come in pretty quickly before you switch to another view that is affected by it. But putting a little delay and using optimistic response exposes the bug.

[yurigenin]

@helfer @stubailo The view will handle this error gracefully but it will result in an empty result. Only after the real mutation result comes back , the optimistic store will be purged and the view will be updated with results from the store. These results were available for a long time but were blocked by non-empty optimistic store. The problem is that that store has no info on what query results it is related to. So it blocks any query results until the results of mutation are in. The fix would be to store some additional information in the optimistic store so that it can be tied to a particular query, not catchall.

[yurigenin]

@helfer @stubailo When we add a new optimistic branch we have access to resultBehaviors that contains info about queries and variables associated with the optimistic results they return. So we could add this info to each optimistic branch so when it is time to read query data from the store we can see if optimistic data matches the parameters of the current query asking for data. If yes, then use optimistic data, if no, use the real store.

[helfer]

@yurigenin if you've pinpointed the issue could you submit a pull request with a failing test? That would help me fix it quickly. 🙂

[richburdon]

@yurigenin I'm not sure if this is something worth building in the short term

1). Hi @helfer, i'm surprised this isn't a major use case. @yurigenin illustrates the open/closed issues filter (100s other example: is starred as favorite, prioritized task lists, ANY list that can be filtered).

If we can (collectively) come up with something that avoids both of those problems, then there's a good chance that we'll build it in!

2). Great. I'm switching over from Relay which already solve this (which is complex and opaque -- so, so far Apollo feels right). I haven't dug into the code yet, but I think instead of caching by query ID they "munge" the ID and the query args.

What should really happen is a bit of a tricky question, because technically your old query is no longer active, and so its result shouldn't be updated anyway.

So, wouldn't a trivial solution be to do the above (munge the ID) and automatically invalidate any queries that aren't visible. You wouldn't get caching, but a) you won't get stale results when you switch; b) we can later add hooks via queriesUpdated/resolve to manually augment the cache. But stage 1 would solve this blocking, can't-ship-without-it problem. (My current workaround (agh) is to have a @noncache directive on these kinds of queries and drive refetch on each component view!)

If that makes sense I'll dig in a bit and try to submit a PR. I haven't gone deep into the code yet, so if you can give me a pointer to the cache map (that is currently indexed by query) it'll help me get started.

3). Offline. While that may not be on the roadmap, offline will never work without this. I've built complex off line systems before (Google Inbox). It's not as hard as it sounds at first and would like to work on this with Apollo. Ideally I'd like to surface the caching internals and be able to update/patch any part of the cache in the resolver. Perhaps registering a global resolve that is updated on any mutation. Currently thinking of intercepting this in the network interface as a workaround. [https://github.com//issues/424]

I'll try to create some tests... Guidance/pointers would be welcome.

[yurigenin]

@richburdon @helfer @stubailo
I actually spent the whole day yesterday trying to come up with a PR for these issues but the more I dug into the code the more I realized that there is no easy solution based on the current design.

Once a component mounts its query is added to a map (QueryManager.addObservableQuery) and never, never gets removed from it. There is a method that is supposed to be called to remove it (QueryManager.removeObservableQuery) but it is not called from any path in the app. So every time you unmount and remount any component with a query it adds a new query structure to that map (QueryManager.observableQueries and QueryManager.queryIdsByName). I used the GitHunt-React app and was able to reproduce it easily. Just switch between the Feed view and Submit view. Every time you click on the Feed view a new query is added to the map. I clicked 100 times and got 100 queries. If you register a mutation with updateQueries for the Feed query it will be called exactly 100 times, no kidding. Imagine 20 HOCs with queries and 20 HOCs with mutations. Could get ugly fast. Resetting the store does not solve this problem as observableQueries and queryIdsByName are never cleaned up.

I think the reason the queries are not removed is because they are tied to how updateQueries and reducer logic works for mutations. If you switch to another view but still want the updateQueries and reducers to be called, then you cannot drop these queries so they hang around forever.

This must be redesigned to decouple queries and query reducers. Those should ideally live independently and be registered once as an app starts up and shared among multiple queries if need be. But based on where the code is now it would require major overhaul.

The problem with parameterized queries is that they are repurposed every time you change a variable value (the opposite of the problem above). Let's say you have a view that has a dropdown with 3 choices (Open, InProgress, Closed) and you want to filter data in the view based on one of these values. So you create a query with one variable (status). Every time user picks a different choice from a dropdown you send a new query. Let's say you chose all of them and now you have three different results in the cache. So far so good. Now you want to add several items to the list via a mutation. But since it is the same query only the last one is actually registered with observableQueries and queryIdsByName in the QueryManager. And it means updateQueries and reducer will only be called once with the variable value set to the last one used. For example, you last viewed items with an Open state. In mutation you added some items with a Closed state. updateQueries/reducer is only called for the query results with an Open status. Now if you go to see your list filtered by a Closed state you will not find the newly added ones there since a reducer was not called for these query results. Interesting that GitHunt-React 'solves' this problem by setting forceFetch flag on the query so it always ignores cache which defeats the whole purpose, right?

One more major issue I found (probably need to submit it separately but want to mention it here since it is related) is that it looks like the current design results in memory leaks. See, you can add a reducer function on any query HOC. It is a function that is attached to query options object. But when you switch between different HOCs they are unmounted and remounted again resulting in new objects being constructed. But unmounted HOCs that have reducers attached cannot be garbage collected since they are kept being referenced by observableQueries that never get cleaned up for the reason I described above. That is why reducers should probably be registered globally so that they do not prevent unmounted components from being garbage collected.

Another minor (?) issue is that the queries in the queries sub store in the apollo store never get deleted but just marked as stopped: true. I could not find a good reason fro them to be there once they are stopped but I might have overlooked something. So, as components get remounted this sub store grows and grows and you can only trim it by resetting the entire store.

So dilemma now is that the the ideas behind the apollo-client are great (normalized cache, optimistic responses, paging, etc.) but the implementation is still not up to enterprise level where you can have lots of HOCs with queries and mutations and you need to make sure that the integrity of data in cache is not compromised and there is no unbound growth of internal objects and memory leaks are not an issue.

Thank you for hearing me out.

[calebmer]

We now have a mitigation for the updateQueries problems discussed in this issue in react-apollo. We now recycle queries to make sure they don’t get removed (apollographql/react-apollo#462). We’ve also introduced a new read/write API to give you maximum control over your data! We hope that these changes will mitigate some of the issues experienced in this thread.

Please let us know what you think. Thanks for the great discussion!

[SachaG]

I'm not sure if this fits the use case discussed here, but I've also been running into issues with data update after mutations (I'm using reducers, not updateQueries btw). I made a quick video to show the problem: https://youtu.be/jNgQfmH0nKY

I just updated both apollo-client and react-apollo to 1.0.0 but it doesn't seem to fix the issue?

EDIT: false alert, my issue was unrelated.

[emilbruckner]

@SachaG What was your issue?