apollo-client icon indicating copy to clipboard operation
apollo-client copied to clipboard

`fetchMore` race conditions when paginating after change to `useQuery` variables

Open Athelian opened this issue 6 months ago • 5 comments

Issue Description

I have a strange issue where two successive calls to fetchMore will cause a bug only if the query was changed once before.

Reproduction outline

  • fetchpolicy = "network-only"
  • paginating with a:
    • [x] custom merge function
    • [ ] custom read function
  • query can be updated to include one extra field of the same object
  • make three requests of 10 items each as soon as:
    1. page loads (✅ no problem)
    2. query updates (🐞 bug)

Link to Reproduction

Code sandbox

Reproduction Steps

  1. Press the "change query" button Result: data returned from useQuery is stale, despite sequential calls to fetchMore (fetchMore updates not present).

Example workarounds:

  • Reduce the number of times fetchMore is called, (instead of two times just one time: change line 46 - change number 30 to 20)
  • In the merge function, if the query is changed (detect by: incoming is first ten items), then throw away the merged result and return only incoming
  • In the original query, call more than one field. In the updated query, call fewer of those fields (seems to return cached result despite using "network-only")

@apollo/client version

3.9.5

Athelian avatar Feb 29 '24 04:02 Athelian

Phew, that's a weird one, and I'm still trying to wrap my head around this.

I'll try to explain what's happening here

  • you select 10 items with their url field
  • they are not present in the cache, so they are fetched from the server
  • you repeat that two more times using fetchMore
  • your cache now contains 30 items with an url field
  • now you switch over to selecting 10 items with an url and title field (and because you have set keyArgs: false, those will be saved in the same cache entry)
  • they can not be satisfied by the cache. They are received from the server, and your merge function overwrites the first 10 elements of your cache entry
  • at this point, your query can still not be satisfied by the cache: the entry there is missing the title field for entries 11-30
  • you call fetchMore for the next 10 fields
  • your merge function writes them to the cache (11-20)
  • because there are still entries in your array in the cache that cannot satisfy your query (entries 21-30 still miss the title field), your query cannot be updated from the cache at all.
  • from your component, it looks like the query is "stuck" on the first 10 fields.

Honestly, I can't really think of a way to resolve this right now - you are maneuvering yourself into a weird corner there.

The only solutions I could think of right now would be

  • introduce a keyArg to distinguish between the two queries
  • have merge reset the array in some way
  • enable returnPartialData in your useQuery options

But none of these is really satisfying.

phryneas avatar Feb 29 '24 16:02 phryneas

Hi @phryneas thanks very much for your reply.

I added a test in https://github.com/apollographql/apollo-client/pull/11657 demonstrating similar behaviour.

I found that there a missing field error is thrown in Apollo's internals for the added field in the second query.

It seems like the query is stuck from this point on.

It's interesting that if the reverse is done: a query with more fields is replaced by a query with fewer fields, it works fine. I suppose because the query shape can be fulfilled albeit with excess properties.

I wonder if Apollo can normalize the cache result somehow as in the usecase these examples are derived from the query can change frequently and take 5-10 seconds to complete each time. Throwing results away on a regular basis is not ideal.

Athelian avatar Mar 10 '24 14:03 Athelian

Hey @Athelian 👋

I'd like to ask, whats the motivation behind using useQuery in this manner? Is there a reason you don't just use the "bigger" query first and paginate on that?

I ask not because we shouldn't be able to handle this, but because I'd like to understand what the use case is we are solving against. Ideally however this gets fixed has this use case in mind so that we aren't solving the wrong problem.

jerelmiller avatar Mar 11 '24 18:03 jerelmiller

Hey @Athelian 👋

I'd like to ask, whats the motivation behind using useQuery in this manner? Is there a reason you don't just use the "bigger" query first and paginate on that?

I ask not because we shouldn't be able to handle this, but because I'd like to understand what the use case is we are solving against. Ideally however this gets fixed has this use case in mind so that we aren't solving the wrong problem.

Hi @jerelmiller

The use case is a table where every row is a list item and every column is 1-1 with a GraphQL property. Columns must be selected on the UI to be displayed. We are querying for the selected columns only - therefore we are querying for the GraphQL properties that the user desires only.

This means we dynamically reconstruct the query every time the user changes their column selection.

As for "why not more and then less", this is because we may save column state to the user account and init the table with only a few columns at first.

With 1000 columns to choose from, we cannot mount a hook for every combination. And we must optimize where possible as some columns can significantly delay the request.

Athelian avatar Mar 12 '24 01:03 Athelian

What, you don't want to write 1000! (factorial) queries in your code 😜.

This is super helpful context. Thanks so much!

jerelmiller avatar Mar 12 '24 05:03 jerelmiller