Cache corruption (fails silently) when mutation fields don't match query?

[richburdon]

Intended outcome:

After applying mutations the cache should be updated and the UX updated reactively.

Actual outcome:

1). The HOC query is re-run but returns no data. There are no errors. The DevTools extension returns the corrects data (from the server), but "load from cache" hangs.

mutate.update is called with the correct data.

When I try to manually query the client form the JS console, I get the error:

  Error: Can't find field project on object (Task/7ee933c7-30cb-e31a-2c11-cd8c16d6b20d)
  ...
  at readStoreResolver (eval at <anonymous> (http://localhost:3000/app/assets/web.bundle.js:1096:1), <anonymous>:49:19)

But Task/7ee933c7-30cb-e31a-2c11-cd8c16d6b20d does indeed exist in the Store (cache) -- I can see it in the DevTools extension, but it doesn't have the project property because the mutation query spec doesn't include that. Adding this field partially resolves the problem (see part 2).

a). Shouldn't the cache be merged intelligently? (Possibly related to #1693)

(In general, it seems to me quite rare that a mutation will return ALL of the fields that are being queried; that would be a big leakage of separation?)

b). If not, can that be done manually (e.g., in mutate.update -- it's not query-specific so I don't see how currently)?

c). At the very least, the error should not be swallowed (Side note: I've lost a lot of time in the past trying to debug errors that have been silently swallowed by Apollo; furthermore, I don't seem to be able to catch errors that are reported in a global error handler).

NOTE: I'm still trying to isolate a repro. It would be helpful if the missing field warning provided the context.

2). When I add an optimisticResponse, I get the following error after the response is returned from the server (i.e., not when processing the optimistic response):

TypeError: Cannot read property 'variables' of undefined

This happens in the store:

    else if (isMutationResultAction(constAction)) {
        if (!constAction.result.errors) {
            var queryStoreValue = mutations[constAction.mutationId]; // undefined!

since mutations is an empty object at this point (when invoked with the optimisticResponse it has the appropriate value, but the cache is still not updated).

How to reproduce the issue:
Query data and then issue Mutations with a different query spec.

    "apollo-client": "^1.2.2",
    "react-apollo": "^1.2.0",

[cesarsolorzano]

@richburdon just to be sure, did you get the missing field warning message or no error/warning at all?

[richburdon]

Yes, I get the missing field warning.

BTW, in trying to create a minimal repro... Outside of the unit tests are there minimally complete working examples that are in sync with the repo and docs? For example, http://dev.apollodata.com/core/read-and-write.html#updating-the-cache-after-a-mutation lightly documents some pretty complex behavior; it would be super helpful if the examples were part of a working test app (e.g., TodoCreateMutation). Sorry if I've missed this somewhere.

In particular related to above: since reducers are being deprecated in favor of batch.update, is it the intention that this method updates items in the cache directly or ALL outstanding queries that may be affected by the mutation? If the former, then why is a query required (and in fact, which query should be used) to retrieve the items -- why not just update the item by key directly? If the latter then doesn't this revert to centralizing cache reconciliation across all queries?

[cesarsolorzano]

I don't quietly understand the issue. When you update the store, the updated data must have the same shape of the query you pretends to update.

Although I may agree that the missing field warning could provide a better context, if the warning is thrown then how is it failing silently or the error been swallowed? How do you think it should behave?

btw, you could use react-apollo-error-template for reproductions 🙂

[richburdon]

@cesarsolorzano thanks for the quick response.

Some thoughts/questions (and thanks in advance for your patience working through this):

1). as you note, the warning provides no context (some field in some part of the selection set is missing).

2). at this point, i'm really just trying to understand what it means -- why does a missing field break cache reconciliation? why not just ignore the missing field by default?

3). it's not really a warning if this causes the framework to break. e.g., the Devtools extension just hangs if you try to query from the cache after such an error.

4). i can't relate the error to the optimistic responses (above). i.e., with optimistic off, the query just fails (with the warning -- perhaps the "data.error" field could be set in the query response?) with an optimistic response provided, the second (network) update causes an internal error (not just a warning).

5). IMO, "misconfiguration" of query specs shouldn't cause internal (uncatchable) errors? (default to returning the previously cached result -- i.e., drop the mutation response rather than break?)

6). MAIN BLOCKING ISSUE: if I have a mutation that affects multiple (many) queries (across different parts of my app), which spec should I use? Suppose my mutation updaters Item X; if I'm manually patching the cache (in batch.update), how do I update Item X (which is referenced by 3 different queries)? should i proxy.query ALL queries? how would I spec the input variables needed to resolve each of these? (I feel I'm misunderstanding this, since the consequence would be for me to build a separate QueryRegistry so that I can do centralized reconciliation in each mutation's update?)

Re react-apollo-error-template, thanks. Would be great to grow this to cover the docs/examples? I'll use it in case I need to send a PR to illustrate further problems.

[cesarsolorzano]

You're right, it should throw an error. However, for the time being, the warning was used to somehow provide backwards compatibility.

It's my understanding that ignoring a field (or updating the store with a result that has a missing field) could break existing query observers. I'm pinging @helfer who could provide a better answer to this and your other questions.

[richburdon]

could break existing query observers

Don't we already deal with this when two queries have different specs for the same item (i.e., I thought they were merged).

Now looking for examples to understand No. 6.

[richburdon]

Answering my own question #6.

In mutation.update, optimistic responses should update fragments -- and the relevant queries should update themselves (via dataIdFromObject magic).

Awesome.

[Tushant]

I am getting missing field '_id', 'meta;, 'owner' etc warning when trying to update optimistically. That means, the server has to anyhow send the meta, owner, comment field in a response in order to work the optimistic update of UI.

Is there no other way to customize the query result while updating the UI ? Here is the full code with the schema for mutation and the response from the server when mutating

https://gist.github.com/SanskarSans/907063ed9a27d3f38f7de25ca4072faf