Relay connections requested with `first: 0` read from cache for `first: 10`

[tpict]

Issue Description

When using relayStylePagination, queries that request first: 0 are read from the cache for queries of the same field that request first > 0.

Link to Reproduction

https://github.com/tpict/apollo-connection-first-0-repro

Reproduction Steps

• Configure a field to use relayStylePagination
• Perform a query that requests first: 0 and writes the results to cache
• Perform another query that requests e.g. first: 10

With fetch policies that prefer reads from cache, the latter query will use the results from the former query. This only seems to happen for the first: 0 case e.g. changing it to first: 1 results in the latter query hitting the network for the remaining edges.

@apollo/client version

3.11.4

[phryneas]

Hi @tpict 👋

This might be a misunderstanding of what relayStylePagination does - it only implements merging of incoming Connection types into the cache and returning all accumulated data from the field that you merged them into.

You'd then have to use fetchMore to trigger any network requests beyond the initial one and data would get merged into one big blog that you could then manually filter data out.

relayStylePagination itself doesn't know about first or other filters.

[tpict]

You'd then have to use fetchMore to trigger any network requests beyond the initial one

I don't think that's true, otherwise requesting first: 1 wouldn't resolve the issue described.

relayStylePagination itself doesn't know about first or other filters.

but it does use cursor, startCursor, endCursor, and is presumably confused by these being null

[phryneas]

Hmm. Looking at your example startCursor and endCursor seem to be the empty string instead of null (but I think that's valid), and hasPreviousPage and hasNextPage are "true" and "false" as strings instead of boolean.
I'm not sure if that might cause problems.

[tpict]

I wrote this as a repro of an honest-to-goodness Relay-conformant API where I see the same issue... but had ChatGPT write the dummy schema implementation for me. My bad, let me address that and make sure it still fails.

[tpict]

Pushed a fix for the booleans serialized as strings. The cursor fields appear to be either null or a non-empty string on my end, though.

[phryneas]

So, from the merging logic with the data you had previously, everything is as I'd expect:

    it("test", () => {
      const result1 = merge(
        undefined,
        {
          pageInfo: {
            hasNextPage: "true",
            hasPreviousPage: "false",
            startCursor: "",
            endCursor: "",
          },
          edges: [],
        },
        options
      );
      expect(result1).toEqual({
        edges: [],
        pageInfo: {
          hasPreviousPage: false,
          hasNextPage: true,
          startCursor: "",
          endCursor: "",
        },
      });
      const result2 = merge(
        result1,
        {
          pageInfo: {
            __typename: "PageInfo",
            hasNextPage: "false",
            hasPreviousPage: "false",
            startCursor: "MA==",
            endCursor: "Mg==",
          },
          edges: [
            {
              __typename: "PersonEdge",
              cursor: "MA==",
              node: {
                __typename: "Person",
                id: "1",
                name: "John Smith",
              },
            },
            {
              __typename: "PersonEdge",
              cursor: "MQ==",
              node: {
                __typename: "Person",
                id: "2",
                name: "Sara Smith",
              },
            },
            {
              __typename: "PersonEdge",
              cursor: "Mg==",
              node: {
                __typename: "Person",
                id: "3",
                name: "Budd Deey",
              },
            },
          ],
        },
        options
      );
      console.dir(result2, { depth: 5 });
      expect(result2).toEqual({
        edges: [
          {
            __typename: "PersonEdge",
            cursor: "MA==",
            node: { __typename: "Person", id: "1", name: "John Smith" },
          },
          {
            __typename: "PersonEdge",
            cursor: "MQ==",
            node: { __typename: "Person", id: "2", name: "Sara Smith" },
          },
          {
            __typename: "PersonEdge",
            cursor: "Mg==",
            node: { __typename: "Person", id: "3", name: "Budd Deey" },
          },
        ],
        pageInfo: {
          __typename: "PageInfo",
          hasNextPage: "false",
          hasPreviousPage: "false",
          startCursor: "MA==",
          endCursor: "Mg==",
        },
      });
    });

So the merging works as expected based on the input data. (Bit wonky around the booleans but that shouldn't matter here)

Which brings us back to the initial point I was trying to make:

All relayStylePagination does is merge everything into one big blob, and no matter if you pass first: 1, first: 300, or first: 500 into the hook, it will always return the full accumulated cache entry.

In your example, if I put in first: 1 in parallel with first: 3, one will make a request for 1 and one will make a request for 3, but in the end, both render 3. That is to be expected.

[phryneas]

The only unexpected behavior is that it doesn't do a request for 3 if you request 0 and 3 - but then again, this might be a timing thing here - generally, it wouldn't make a request if something (including an empty array) is already in the cache. This merge strategy is meant to be used mainly with fetchMore to kick off requests beyond the first request, not normal fetchPolicies.

[phryneas]

Ah, I now see why that happens, though:

scenario 1:

• "zero people" kicks off a network request (because the cache entry is undefined) with edges { node { id } } and writes an empty array to the cache
• "three people" mounts and sees that there's not undefined in the cache, but an emtpy array. That is enough to satisfy the query for edges { node { cursor id } }. It doesn't make another network request
• as no more network request happens, both display 0 entries

scenario 1:

• "one person" kicks off a network request (because the cache entry is undefined) with edges { node { id } } and writes an array with one element to the cache
• "three people" mounts and sees that there's not undefined in the cache, but an array with one element. Those cache contents cannot satisfy the query though, because there is no cursor property, so it has to make a network request to also retrieve the cursor. Then it writes three people into the array
• both components now display three entries, just with a different subselection of fields

[tpict]

Yep, I think we're on the same page. The use case in our scenario is that our "zero people" request gathers some aggregate metadata over the connection, but we don't actually need any edges resolved. The Relay spec explicitly disallows negative values for first but not zero.

because there is no cursor property

This is something I overlooked. Matching the selection sets of the two queries, I see

• same behavior for first: 0
• with first: 1, the first: 3 query is satisfied by the cache result for first: 1

The latter is surprising to me! I would have expected it to kick off another query.

I know the docs state wrt. relayStylePagination

even if you end up copy/pasting its code and making changes to suit your specific needs.

but I'd like to clarify intended behavior a little.

My mental model was that discontinuous requests (i.e. after/endCursor, before/startCursor don't line up) over the same connection would basically wipe out whatever's in the cache with the last result from the API. Then continuous requests–dispatched by fetchMore or otherwise–would be stitched together in cache.

[phryneas]

The latter is surprising to me! I would have expected it to kick off another query.

As I said, this merge policy is only about merging, it doesn't check if your current available data satisfies what you need.

This is the behavior of the read function: if it returns undefined, we do a network request, if it returns anything, we assume that we have a "cache hit". We'd do a network request if the cache hit doesn't include all fields of the selection set, but nothing beyond that.

My mental model was that discontinuous requests (i.e. after/endCursor, before/startCursor don't line up) over the same connection would basically wipe out whatever's in the cache with the last result from the API. Then continuous requests–dispatched by fetchMore or otherwise–would be stitched together in cache.

There is a lot of things we don't know about your api that are not part of the relay spec but that realistically happen in real-life apis - but slightly differently everywhere. So the implementation is more of a blueprint of what is probably common code for all apis, but you'll likely have to copy-paste it over to your app and adjust accordingly.

apollo-client/src/utilities/policies/pagination.ts

Lines 101 to 287 in 21810ba

	// As proof of the flexibility of field policies, this function generates
	// one that handles Relay-style pagination, without Apollo Client knowing
	// anything about connections, edges, cursors, or pageInfo objects.
	/**
	* A field policy that attempts to handle pagination for fields that adhere to
	* the [Relay Connections Spec](https://relay.dev/graphql/connections.htm).
	*
	* @param keyArgs - `keyArgs` that should be applied to the field policy
	* @returns The field policy that handles Relay pagination
	*/
	export function relayStylePagination<TNode extends Reference = Reference>(
	keyArgs: KeyArgs = false
	): RelayFieldPolicy<TNode> {
	return {
	keyArgs,
	read(existing, { canRead, readField }) {
	if (!existing) return existing;
	const edges: TRelayEdge<TNode>[] = [];
	let firstEdgeCursor = "";
	let lastEdgeCursor = "";
	existing.edges.forEach((edge) => {
	// Edges themselves could be Reference objects, so it's important
	// to use readField to access the edge.edge.node property.
	if (canRead(readField("node", edge))) {
	edges.push(edge);
	if (edge.cursor) {
	firstEdgeCursor = firstEdgeCursor || edge.cursor || "";
	lastEdgeCursor = edge.cursor || lastEdgeCursor;
	}
	}
	});
	if (edges.length > 1 && firstEdgeCursor === lastEdgeCursor) {
	firstEdgeCursor = "";
	}
	const { startCursor, endCursor } = existing.pageInfo || {};
	return {
	// Some implementations return additional Connection fields, such
	// as existing.totalCount. These fields are saved by the merge
	// function, so the read function should also preserve them.
	...getExtras(existing),
	edges,
	pageInfo: {
	...existing.pageInfo,
	// If existing.pageInfo.{start,end}Cursor are undefined or "", default
	// to firstEdgeCursor and/or lastEdgeCursor.
	startCursor: startCursor || firstEdgeCursor,
	endCursor: endCursor || lastEdgeCursor,
	},
	};
	},
	merge(existing, incoming, { args, isReference, readField }) {
	if (!existing) {
	existing = makeEmptyData();
	}
	if (!incoming) {
	return existing;
	}
	const incomingEdges =
	incoming.edges ?
	incoming.edges.map((edge) => {
	if (isReference((edge = { ...edge }))) {
	// In case edge is a Reference, we read out its cursor field and
	// store it as an extra property of the Reference object.
	edge.cursor = readField<string>("cursor", edge);
	}
	return edge;
	})
	: [];
	if (incoming.pageInfo) {
	const { pageInfo } = incoming;
	const { startCursor, endCursor } = pageInfo;
	const firstEdge = incomingEdges[0];
	const lastEdge = incomingEdges[incomingEdges.length - 1];
	// In case we did not request the cursor field for edges in this
	// query, we can still infer cursors from pageInfo.
	if (firstEdge && startCursor) {
	firstEdge.cursor = startCursor;
	}
	if (lastEdge && endCursor) {
	lastEdge.cursor = endCursor;
	}
	// Cursors can also come from edges, so we default
	// pageInfo.{start,end}Cursor to {first,last}Edge.cursor.
	const firstCursor = firstEdge && firstEdge.cursor;
	if (firstCursor && !startCursor) {
	incoming = mergeDeep(incoming, {
	pageInfo: {
	startCursor: firstCursor,
	},
	});
	}
	const lastCursor = lastEdge && lastEdge.cursor;
	if (lastCursor && !endCursor) {
	incoming = mergeDeep(incoming, {
	pageInfo: {
	endCursor: lastCursor,
	},
	});
	}
	}
	let prefix = existing.edges;
	let suffix: typeof prefix = [];
	if (args && args.after) {
	// This comparison does not need to use readField("cursor", edge),
	// because we stored the cursor field of any Reference edges as an
	// extra property of the Reference object.
	const index = prefix.findIndex((edge) => edge.cursor === args.after);
	if (index >= 0) {
	prefix = prefix.slice(0, index + 1);
	// suffix = []; // already true
	}
	} else if (args && args.before) {
	const index = prefix.findIndex((edge) => edge.cursor === args.before);
	suffix = index < 0 ? prefix : prefix.slice(index);
	prefix = [];
	} else if (incoming.edges) {
	// If we have neither args.after nor args.before, the incoming
	// edges cannot be spliced into the existing edges, so they must
	// replace the existing edges. See #6592 for a motivating example.
	prefix = [];
	}
	const edges = [...prefix, ...incomingEdges, ...suffix];
	const pageInfo: TRelayPageInfo = {
	// The ordering of these two ...spreads may be surprising, but it
	// makes sense because we want to combine PageInfo properties with a
	// preference for existing values, *unless* the existing values are
	// overridden by the logic below, which is permitted only when the
	// incoming page falls at the beginning or end of the data.
	...incoming.pageInfo,
	...existing.pageInfo,
	};
	if (incoming.pageInfo) {
	const {
	hasPreviousPage,
	hasNextPage,
	startCursor,
	endCursor,
	...extras
	} = incoming.pageInfo;
	// If incoming.pageInfo had any extra non-standard properties,
	// assume they should take precedence over any existing properties
	// of the same name, regardless of where this page falls with
	// respect to the existing data.
	Object.assign(pageInfo, extras);
	// Keep existing.pageInfo.has{Previous,Next}Page unless the
	// placement of the incoming edges means incoming.hasPreviousPage
	// or incoming.hasNextPage should become the new values for those
	// properties in existing.pageInfo. Note that these updates are
	// only permitted when the beginning or end of the incoming page
	// coincides with the beginning or end of the existing data, as
	// determined using prefix.length and suffix.length.
	if (!prefix.length) {
	if (void 0 !== hasPreviousPage)
	pageInfo.hasPreviousPage = hasPreviousPage;
	if (void 0 !== startCursor) pageInfo.startCursor = startCursor;
	}
	if (!suffix.length) {
	if (void 0 !== hasNextPage) pageInfo.hasNextPage = hasNextPage;
	if (void 0 !== endCursor) pageInfo.endCursor = endCursor;
	}
	}
	return {
	...getExtras(existing),
	...getExtras(incoming),
	edges,
	pageInfo,
	};
	},
	};
	}

Two thoughts here:

• make sure to add your filters to keyFields
• if you're not using this in the most basic way, you'll probably have to adjust the read function
• for your "wipe out" use case, you probably will also have to adjust the merge function

[tpict]

We're making use of keyArgs but it feels to me that the @connection directive would solve a lot of these problems e.g. the "fetch metadata over connection without loading edges" vs "actually load the edges" documents could be segregated in cache. Does that seem like a reasonable use case for that directive?

[phryneas]

Give it a try and if it works nicely, sure, go for it :)

[tpict]

Unfortunately it seems that using relayStylePagination clobbers the @connection directive. I've made a repro here where I modified the mock schema to resolve no results if the name field isn't requested, and added the @connection directive to both operations with a unique key. We see the "zero length edges satisfies query with a different selection set" behavior again.

I understand now is the expected behavior of my original repro, but I would expect the @connection directive to prevent the latter query from reading results for the former. I'm not aware of the mechanism by which @connection works so I could be in the wrong here.

edit: ...and then I found this! #8659 which looks promising. I'll give it a go.