diff --git a/.api-reports/api-report-core.api.md b/.api-reports/api-report-core.api.md index 7cf888e8264..3e13bdd965f 100644 --- a/.api-reports/api-report-core.api.md +++ b/.api-reports/api-report-core.api.md @@ -1377,8 +1377,8 @@ export const windowFocusSource: RefetchEventManager.EventSource; // Warnings were encountered during analysis: // // src/core/ApolloClient.ts:635:5 - (ae-forgotten-export) The symbol "NextFetchPolicyContext" needs to be exported by the entry point index.d.ts -// src/core/ObservableQuery.ts:375:5 - (ae-forgotten-export) The symbol "QueryManager" needs to be exported by the entry point index.d.ts -// src/core/QueryManager.ts:195:5 - (ae-forgotten-export) The symbol "MutationStoreValue" needs to be exported by the entry point index.d.ts +// src/core/ObservableQuery.ts:376:5 - (ae-forgotten-export) The symbol "QueryManager" needs to be exported by the entry point index.d.ts +// src/core/QueryManager.ts:196:5 - (ae-forgotten-export) The symbol "MutationStoreValue" needs to be exported by the entry point index.d.ts // (No @packageDocumentation comment for this package) diff --git a/.api-reports/api-report-incremental.api.md b/.api-reports/api-report-incremental.api.md index e61cd6dc389..79b7ea5b2eb 100644 --- a/.api-reports/api-report-incremental.api.md +++ b/.api-reports/api-report-incremental.api.md @@ -10,6 +10,7 @@ import type { DocumentNode } from 'graphql'; import type { FormattedExecutionResult } from 'graphql'; import type { GraphQLFormattedError } from 'graphql'; import type { HKT } from '@apollo/client/utilities'; +import { StreamInfoTrie } from '@apollo/client/utilities/internal'; // @public (undocumented) namespace Defer20220824Handler { @@ -196,17 +197,28 @@ export namespace Incremental { } // (undocumented) export interface IncrementalRequest, TData> { - // (undocumented) - getPendingType?: (id: string) => "defer" | "stream"; + // @internal @deprecated (undocumented) + getPendingWithInfo?: () => Array; // (undocumented) handle: (cacheData: TData | DeepPartial | undefined | null, chunk: Chunk) => FormattedExecutionResult; // (undocumented) hasNext: boolean; - // (undocumented) - pending?: Array; + // @internal @deprecated (undocumented) + readonly streamInfo?: StreamInfoTrie; } // (undocumented) export type Path = ReadonlyArray; + // @internal @deprecated (undocumented) + export interface PendingDeferResultWithInfo { + // (undocumented) + delivered: boolean; + // (undocumented) + path: Incremental.Path; + // (undocumented) + type: "defer"; + } + // @internal @deprecated (undocumented) + export type PendingItemWithInfo = PendingDeferResultWithInfo | PendingStreamResultWithInfo; // (undocumented) export interface PendingResult { // (undocumented) @@ -217,6 +229,13 @@ export namespace Incremental { path: Incremental.Path; } // @internal @deprecated (undocumented) + export interface PendingStreamResultWithInfo { + // (undocumented) + path: Incremental.Path; + // (undocumented) + type: "stream"; + } + // @internal @deprecated (undocumented) export interface StartRequestOptions { // (undocumented) query: DocumentNode; @@ -232,14 +251,22 @@ export namespace Incremental { // @public (undocumented) class IncrementalRequest implements Incremental.IncrementalRequest, TData> { - // (undocumented) - getPendingType(id: string): "defer" | "stream"; + // @internal @deprecated (undocumented) + getPendingWithInfo(): ({ + type: "stream"; + path: Incremental.Path; + delivered?: undefined; + } | { + type: "defer"; + delivered: boolean; + path: Incremental.Path; + })[]; // (undocumented) handle(cacheData: TData | DeepPartial | null | undefined, chunk: GraphQL17Alpha9Handler.Chunk): FormattedExecutionResult; // (undocumented) hasNext: boolean; - // (undocumented) - get pending(): GraphQL17Alpha9Handler.PendingResult[]; + // @internal @deprecated (undocumented) + get streamInfo(): StreamInfoTrie | undefined; } // @public (undocumented) diff --git a/.api-reports/api-report.api.md b/.api-reports/api-report.api.md index fb92d59789d..00d06d6a0ab 100644 --- a/.api-reports/api-report.api.md +++ b/.api-reports/api-report.api.md @@ -1598,17 +1598,33 @@ namespace Incremental { } // (undocumented) interface IncrementalRequest, TData> { - // (undocumented) - getPendingType?: (id: string) => "defer" | "stream"; + // Warning: (ae-forgotten-export) The symbol "Incremental" needs to be exported by the entry point index.d.ts + // + // @internal @deprecated (undocumented) + getPendingWithInfo?: () => Array; // (undocumented) handle: (cacheData: TData | DeepPartial | undefined | null, chunk: Chunk) => FormattedExecutionResult; // (undocumented) hasNext: boolean; - // (undocumented) - pending?: Array; + // @internal @deprecated (undocumented) + readonly streamInfo?: StreamInfoTrie; } // (undocumented) type Path = ReadonlyArray; + // @internal @deprecated (undocumented) + interface PendingDeferResultWithInfo { + // (undocumented) + delivered: boolean; + // (undocumented) + path: Incremental.Path; + // (undocumented) + type: "defer"; + } + // Warning: (ae-forgotten-export) The symbol "Incremental" needs to be exported by the entry point index.d.ts + // Warning: (ae-forgotten-export) The symbol "Incremental" needs to be exported by the entry point index.d.ts + // + // @internal @deprecated (undocumented) + type PendingItemWithInfo = PendingDeferResultWithInfo | PendingStreamResultWithInfo; // (undocumented) interface PendingResult { // (undocumented) @@ -1619,6 +1635,13 @@ namespace Incremental { path: Incremental.Path; } // @internal @deprecated (undocumented) + interface PendingStreamResultWithInfo { + // (undocumented) + path: Incremental.Path; + // (undocumented) + type: "stream"; + } + // @internal @deprecated (undocumented) interface StartRequestOptions { // (undocumented) query: DocumentNode; @@ -3263,8 +3286,8 @@ interface WriteContext extends ReadMergeModifyContext { // src/cache/inmemory/types.ts:162:3 - (ae-forgotten-export) The symbol "FragmentRegistryAPI" needs to be exported by the entry point index.d.ts // src/core/ApolloClient.ts:201:5 - (ae-forgotten-export) The symbol "IgnoreModifier" needs to be exported by the entry point index.d.ts // src/core/ApolloClient.ts:635:5 - (ae-forgotten-export) The symbol "NextFetchPolicyContext" needs to be exported by the entry point index.d.ts -// src/core/ObservableQuery.ts:375:5 - (ae-forgotten-export) The symbol "QueryManager" needs to be exported by the entry point index.d.ts -// src/core/QueryManager.ts:195:5 - (ae-forgotten-export) The symbol "MutationStoreValue" needs to be exported by the entry point index.d.ts +// src/core/ObservableQuery.ts:376:5 - (ae-forgotten-export) The symbol "QueryManager" needs to be exported by the entry point index.d.ts +// src/core/QueryManager.ts:196:5 - (ae-forgotten-export) The symbol "MutationStoreValue" needs to be exported by the entry point index.d.ts // src/local-state/LocalState.ts:149:5 - (ae-forgotten-export) The symbol "LocalState" needs to be exported by the entry point index.d.ts // src/local-state/LocalState.ts:202:7 - (ae-forgotten-export) The symbol "LocalState" needs to be exported by the entry point index.d.ts // src/local-state/LocalState.ts:245:7 - (ae-forgotten-export) The symbol "LocalState" needs to be exported by the entry point index.d.ts diff --git a/.changeset/chilly-actors-complain.md b/.changeset/chilly-actors-complain.md new file mode 100644 index 00000000000..fc681e000f3 --- /dev/null +++ b/.changeset/chilly-actors-complain.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix an issue where a `network-only` query leaked partial cache data for `@defer` fragments that were not delivered by the network due to an error that bubbled to the `@defer` fragment boundary. diff --git a/.changeset/loud-bulldogs-pay.md b/.changeset/loud-bulldogs-pay.md new file mode 100644 index 00000000000..cd16bc95d8e --- /dev/null +++ b/.changeset/loud-bulldogs-pay.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix an issue where a `@defer` query reported the `dataState` as `complete` instead of `streaming` when an error occurs on a deferred field that bubbled to the defer boundary. diff --git a/.changeset/swift-starfishes-know.md b/.changeset/swift-starfishes-know.md new file mode 100644 index 00000000000..c9464e21dec --- /dev/null +++ b/.changeset/swift-starfishes-know.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fix an invariant error thrown when a `@defer` boundary received a payload after it had already been marked complete. diff --git a/src/cache/inmemory/__tests__/cache.diff/incremental.test.ts b/src/cache/inmemory/__tests__/cache.diff/incremental.test.ts index f006c37dda6..443f13b2a1b 100644 --- a/src/cache/inmemory/__tests__/cache.diff/incremental.test.ts +++ b/src/cache/inmemory/__tests__/cache.diff/incremental.test.ts @@ -9645,6 +9645,57 @@ test("strips both a partial pending @defer boundary and a complete pending sibli }); }); +test("keeps fields shared with a delivered @defer boundary while pruning a pending boundary at a nested path", () => { + const cache = new InMemoryCache(); + const query = gql` + query { + ... @defer { + hero { + name + } + } + hero { + id + ... @defer { + name + homePlanet + } + } + } + `; + + cache.writeQuery({ + query, + data: { + hero: { + __typename: "Hero", + id: "1", + name: "Luke", + homePlanet: "Tatooine", + }, + }, + }); + + const deferInfo: DeferInfoTrie = new Trie(); + deferInfo.lookup("hero"); + + expect( + cache.diff({ + query, + optimistic: true, + returnPartialData: false, + [handleIncrementalSymbol]: { deferInfo }, + }) + ).toStrictEqualTyped({ + result: markAsStreaming({ + hero: { __typename: "Hero", id: "1", name: "Luke" }, + }), + dataState: "streaming", + complete: false, + missing: undefined, + }); +}); + function getMissingMessage(fieldName: string, obj: Record) { return `Can't find field '${fieldName}' on ${ isReference(obj) ? diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index ebccddc1619..01367d238ff 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -34,6 +34,7 @@ import { import { invariant } from "@apollo/client/utilities/invariant"; import type { ApolloClient } from "./ApolloClient.js"; +import { dataStateErrorCache } from "./dataStateErrorCache.js"; import { NetworkStatus } from "./networkStatus.js"; import type { QueryManager } from "./QueryManager.js"; import type { @@ -2018,7 +2019,8 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`, : notification.value; if (notification.kind === "E" && result.dataState === "streaming") { - result.dataState = "complete" as any; + result.dataState = + dataStateErrorCache.get(notification.error) ?? ("complete" as any); } if (result.error) { diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 29a73e97f86..99844350580 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -18,7 +18,6 @@ import { graphQLResultHasError, handleIncrementalSymbol, hasDirectives, - streamInfoSymbol, } from "@apollo/client/utilities/internal"; import { invariant } from "@apollo/client/utilities/invariant"; @@ -60,6 +59,7 @@ interface LastWrite { result: FormattedExecutionResult; variables: ApolloClient.WatchQueryOptions["variables"]; dmCount: number | undefined; + hasNext: boolean; } const destructiveMethodCounts = new WeakMap(); @@ -194,8 +194,7 @@ export class QueryInfo< // We have to compare these values because its possible the final chunk // emitted in the incremental result is just `hasNext: false`. This // ensures we trigger a cache write when we get `isLastChunk: true`. - result.extensions?.[streamInfoSymbol] !== - lastWrite.result.extensions?.[streamInfoSymbol] + lastWrite.hasNext !== this.hasNext ); } @@ -203,6 +202,10 @@ export class QueryInfo< return this.incremental ? this.incremental.hasNext : false; } + get incrementalHandler() { + return this.queryManager.incrementalHandler; + } + private maybeHandleIncrementalResult( cacheData: TData | DeepPartial | undefined | null, incoming: ApolloLink.Result, @@ -211,10 +214,8 @@ export class QueryInfo< DataValue.Complete | DataValue.Streaming, ExtensionsWithStreamInfo > { - const { incrementalHandler } = this.queryManager; - - if (incrementalHandler.isIncrementalResult(incoming)) { - this.incremental ||= incrementalHandler.startRequest< + if (this.incrementalHandler.isIncrementalResult(incoming)) { + this.incremental ||= this.incrementalHandler.startRequest< TData & Record >({ query, @@ -251,6 +252,8 @@ export class QueryInfo< variables, optimistic: true, }; + const isNetworkOnly = + fetchPolicy === "network-only" && networkStatus !== NetworkStatus.refetch; // Cancel the pending notify timeout (if it exists) to prevent extraneous network // requests. To allow future notify timeouts, diff and dirty are reset as well. @@ -259,13 +262,23 @@ export class QueryInfo< const skipCache = cacheWriteBehavior === CacheWriteBehavior.FORBID; const diff = skipCache ? undefined : ( - this.getDiff({ - ...diffOptions, - // Always request partial data to ensure the network incremental - // result is merged with all existing data (especially true to - // maintain @stream arrays with partial list items in the right order) - returnPartialData: true, - }) + this.getDiff( + { + ...diffOptions, + // We usually request partial data to ensure the network incremental + // result is merged with all existing data (especially true to + // maintain @stream arrays with partial list items in the right order + // or when chunk might otherwise replace a partial non-normalized + // object), but if we are about to throw away the result anyways due + // to the error policy (which early returns below), prune any + // pending boundaries so that CombinedGraphQLErrors contains the + // right `data` value. + returnPartialData: + errorPolicy !== "none" || + !this.incrementalHandler.extractErrors(incoming)?.length, + }, + this.getIncrementalInfo({ isNetworkOnly }) + ) ); const incrementalResult = this.maybeHandleIncrementalResult( @@ -279,33 +292,33 @@ export class QueryInfo< dataState: incrementalResult.data == null ? "empty" : "complete", }; - if (skipCache) { - const hasPendingDefer = this.incremental?.pending?.some( - (pending) => this.incremental?.getPendingType?.(pending.id) === "defer" - ); + const hasPendingDefer = this.incremental + ?.getPendingWithInfo?.() + .some((pending) => pending.type === "defer" && !pending.delivered); - if ( - hasPendingDefer || - // The Defer20220824Handler cannot track pending/completed incremental - // chunks due to its data format so we naively set dataState to - // streaming if we are still processing chunks. The only case where - // streaming is incorrect and should actually be complete is when - // both a @defer and @stream boundary is present and the @defer chunk - // has completed before the `@stream` array. - // - // Assigning the naive "streaming" value avoids a much more expensive - // pass over `result.data` that would otherwise need to traverse the - // selection sets and evaluate the data object at each defer boundary - // to see if it fulfills the selection set. For such a narrow case where - // its incorrect on a format that is now outdated is not worth the - // fix so we are ok with reporting a `streaming` here. - (!this.incremental?.pending && - this.hasNext && - hasDirectives(["defer"], query)) - ) { - result.dataState = "streaming"; - } + if ( + hasPendingDefer || + // The Defer20220824Handler cannot track pending/completed incremental + // chunks due to its data format so we naively set dataState to + // streaming if we are still processing chunks. The only case where + // streaming is incorrect and should actually be complete is when + // both a @defer and @stream boundary is present and the @defer chunk + // has completed before the `@stream` array. + // + // Assigning the naive "streaming" value avoids a much more expensive + // pass over `result.data` that would otherwise need to traverse the + // selection sets and evaluate the data object at each defer boundary + // to see if it fulfills the selection set. For such a narrow case where + // its incorrect on a format that is now outdated is not worth the + // fix so we are ok with reporting a `streaming` here. + (!this.incremental?.getPendingWithInfo && + this.hasNext && + hasDirectives(["defer"], query)) + ) { + result.dataState = "streaming"; + } + if (skipCache) { return result; } @@ -378,26 +391,23 @@ export class QueryInfo< result, variables, dmCount: destructiveMethodCounts.get(this.cache), + hasNext: this.hasNext, }; } - const isNetworkOnly = - fetchPolicy === "network-only" && - networkStatus !== NetworkStatus.refetch; - const { dataState, result: diffResult } = this.getDiff( { ...diffOptions, // Never deliver partial data for network-only requests returnPartialData: returnPartialData && !isNetworkOnly, }, - this.getIncrementalInfo(result, { isNetworkOnly }) + this.getIncrementalInfo({ isNetworkOnly }) ); if ( dataState === "complete" || - (returnPartialData && dataState === "partial" && shouldWrite) || - (this.hasNext && dataState === "streaming") + dataState === "streaming" || + (returnPartialData && dataState === "partial" && shouldWrite) ) { result = { ...result, data: diffResult, dataState }; } @@ -407,12 +417,9 @@ export class QueryInfo< return result; } - private getIncrementalInfo( - result: MarkQueryResult, - { isNetworkOnly }: { isNetworkOnly: boolean } - ) { - const pending = this.incremental?.pending ?? []; - const streamInfo = result.extensions?.[streamInfoSymbol]?.deref(); + private getIncrementalInfo({ isNetworkOnly }: { isNetworkOnly: boolean }) { + const pending = this.incremental?.getPendingWithInfo?.() ?? []; + const streamInfo = this.incremental?.streamInfo; const incrementalInfo: DiffIncrementalInfo = { streamInfo }; // We don't want to deliver stream items or complete defer boundaries @@ -421,12 +428,10 @@ export class QueryInfo< // can prune complete defer/stream boundaries at those paths. if (isNetworkOnly) { for (const item of pending) { - const type = this.incremental?.getPendingType?.(item.id); - - if (type === "defer") { + if (item.type === "defer" && !item.delivered) { incrementalInfo.deferInfo ||= new Trie(true, () => true); incrementalInfo.deferInfo.lookupArray(item.path as any[]); - } else if (streamInfo && type === "stream") { + } else if (streamInfo && item.type === "stream") { streamInfo.lookupArray(item.path as any[]).state.truncate = true; } } diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index 1cfd4691dea..47dbf1f4614 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -75,6 +75,7 @@ import { import { defaultCacheSizes } from "../utilities/caching/sizes.js"; import type { ApolloClient } from "./ApolloClient.js"; +import { dataStateErrorCache } from "./dataStateErrorCache.js"; import { NetworkStatus } from "./networkStatus.js"; import { logMissingFieldErrors, ObservableQuery } from "./ObservableQuery.js"; import { CacheWriteBehavior, QueryInfo } from "./QueryInfo.js"; @@ -1079,9 +1080,12 @@ export class QueryManager { if (hasErrors && errorPolicy === "none") { queryInfo.resetLastWrite(); observableQuery?.["resetNotifications"](); - throw new CombinedGraphQLErrors( + const error = new CombinedGraphQLErrors( removeStreamDetailsFromExtensions(result) ); + + dataStateErrorCache.set(error, dataState); + throw error; } const partial = dataState !== "complete"; diff --git a/src/core/__tests__/client.watchQuery/deferGraphQL17Alpha9.test.ts b/src/core/__tests__/client.watchQuery/deferGraphQL17Alpha9.test.ts index a4544d353e2..67482dcde38 100644 --- a/src/core/__tests__/client.watchQuery/deferGraphQL17Alpha9.test.ts +++ b/src/core/__tests__/client.watchQuery/deferGraphQL17Alpha9.test.ts @@ -1698,6 +1698,291 @@ test('returns partial deferred cached data as "partial" while streaming with a " await expect(stream).not.toEmitAnything(); }); +test('reports partial cached data inside a defer boundary as "partial" when the boundary completes with errors with a "cache-first" fetch policy and returnPartialData', async () => { + // Suppress expected missing field warning when writing partial value after + // first chunk + using _ = spyOnConsole("error"); + const query = gql` + query { + greeting { + message + ... on Greeting @defer { + recipient { + name + email + } + } + } + } + `; + + const { httpLink, enqueueInitialChunk, enqueueSubsequentChunk } = + mockDeferStreamGraphQL17Alpha9(); + const cache = new InMemoryCache(); + + cache.writeQuery({ + query, + data: { + greeting: { + __typename: "Greeting", + recipient: { __typename: "Person", name: "Cached Alice" }, + }, + }, + }); + + const client = new ApolloClient({ + cache, + link: httpLink, + incrementalHandler: new GraphQL17Alpha9Handler(), + }); + + const stream = new ObservableStream( + client.watchQuery({ + query, + returnPartialData: true, + errorPolicy: "all", + }) + ); + + await expect(stream).toEmitTypedValue({ + data: { + greeting: { + __typename: "Greeting", + recipient: { __typename: "Person", name: "Cached Alice" }, + }, + }, + dataState: "partial", + loading: true, + networkStatus: NetworkStatus.loading, + partial: true, + }); + + enqueueInitialChunk({ + data: { greeting: { message: "Hello world", __typename: "Greeting" } }, + pending: [{ id: "0", path: ["greeting"] }], + hasNext: true, + }); + + await expect(stream).toEmitTypedValue({ + data: { + greeting: { + __typename: "Greeting", + message: "Hello world", + recipient: { __typename: "Person", name: "Cached Alice" }, + }, + }, + dataState: "partial", + loading: true, + networkStatus: NetworkStatus.streaming, + partial: true, + }); + + enqueueSubsequentChunk({ + completed: [ + { + id: "0", + errors: [ + { + message: "Could not fetch recipient", + path: ["greeting", "recipient"], + }, + ], + }, + ], + hasNext: false, + }); + + await expect(stream).toEmitTypedValue({ + data: { + greeting: { + __typename: "Greeting", + message: "Hello world", + recipient: { __typename: "Person", name: "Cached Alice" }, + }, + }, + dataState: "partial", + error: new CombinedGraphQLErrors({ + data: { + greeting: { + __typename: "Greeting", + message: "Hello world", + recipient: { __typename: "Person", name: "Cached Alice" }, + }, + }, + errors: [ + { + message: "Could not fetch recipient", + path: ["greeting", "recipient"], + }, + ], + }), + loading: false, + networkStatus: NetworkStatus.error, + partial: true, + }); + + await expect(stream).not.toEmitAnything(); +}); + +test("reports partial data correctly when a mid-stream request is abandoned and the query is subscribed to again", async () => { + // Suppress expected missing field warning when writing partial value after + // first chunk + using _ = spyOnConsole("error"); + const query = gql` + query { + greeting { + message + ... on Greeting @defer { + recipient { + name + email + } + } + } + } + `; + + const defer1 = mockDeferStreamGraphQL17Alpha9(); + const defer2 = mockDeferStreamGraphQL17Alpha9(); + let requests = 0; + const link = ApolloLink.from([ + new ApolloLink((operation, forward) => { + requests++; + return forward(operation); + }), + ApolloLink.split(() => requests === 1, defer1.httpLink, defer2.httpLink), + ]); + + const cache = new InMemoryCache(); + cache.writeQuery({ + query, + data: { + greeting: { + __typename: "Greeting", + recipient: { __typename: "Person", name: "Cached Alice" }, + }, + }, + }); + + const client = new ApolloClient({ + cache, + link, + incrementalHandler: new GraphQL17Alpha9Handler(), + }); + + const observable = client.watchQuery({ query, returnPartialData: true }); + + const initialChunk = { + data: { greeting: { __typename: "Greeting", message: "Hello world" } }, + pending: [{ id: "0", path: ["greeting"] }], + hasNext: true, + }; + + const streamA = new ObservableStream(observable); + + await expect(streamA).toEmitTypedValue({ + data: { + greeting: { + __typename: "Greeting", + recipient: { __typename: "Person", name: "Cached Alice" }, + }, + }, + dataState: "partial", + loading: true, + networkStatus: NetworkStatus.loading, + partial: true, + }); + + defer1.enqueueInitialChunk({ ...initialChunk }); + + await expect(streamA).toEmitTypedValue({ + data: { + greeting: { + __typename: "Greeting", + message: "Hello world", + recipient: { __typename: "Person", name: "Cached Alice" }, + }, + }, + dataState: "partial", + loading: true, + networkStatus: NetworkStatus.streaming, + partial: true, + }); + + // Abandon the request before the boundary is delivered. + streamA.unsubscribe(); + + const streamB = new ObservableStream(observable); + + await expect(streamB).toEmitTypedValue({ + data: { + greeting: { + __typename: "Greeting", + message: "Hello world", + recipient: { __typename: "Person", name: "Cached Alice" }, + }, + }, + dataState: "partial", + loading: true, + networkStatus: NetworkStatus.loading, + partial: true, + }); + + defer2.enqueueInitialChunk({ ...initialChunk }); + + await expect(streamB).toEmitTypedValue({ + data: { + greeting: { + __typename: "Greeting", + message: "Hello world", + recipient: { __typename: "Person", name: "Cached Alice" }, + }, + }, + dataState: "partial", + loading: true, + networkStatus: NetworkStatus.streaming, + partial: true, + }); + + defer2.enqueueSubsequentChunk({ + incremental: [ + { + data: { + __typename: "Greeting", + recipient: { + name: "Alice", + email: "alice@example.com", + __typename: "Person", + }, + }, + id: "0", + }, + ], + completed: [{ id: "0" }], + hasNext: false, + }); + + await expect(streamB).toEmitTypedValue({ + data: { + greeting: { + __typename: "Greeting", + message: "Hello world", + recipient: { + __typename: "Person", + name: "Alice", + email: "alice@example.com", + }, + }, + }, + dataState: "complete", + loading: false, + networkStatus: NetworkStatus.ready, + partial: false, + }); + + await expect(streamB).not.toEmitAnything(); +}); + test("emits empty then streaming results for deferred queries with no data in the cache and returnPartialData", async () => { const query = gql` query { @@ -5984,6 +6269,210 @@ test('does not return complete cached deferred data while streaming with a "netw await expect(stream).not.toEmitAnything(); }); +test('does not return complete cached deferred data when a defer boundary completes with errors with a "network-only" fetch policy', async () => { + const query = gql` + query { + greeting { + message + ... on Greeting @defer { + recipient { + name + } + } + } + } + `; + + const { httpLink, enqueueInitialChunk, enqueueSubsequentChunk } = + mockDeferStreamGraphQL17Alpha9(); + const cache = new InMemoryCache(); + + cache.writeQuery({ + query, + data: { + greeting: { + __typename: "Greeting", + message: "Cached hello", + recipient: { __typename: "Person", name: "Cached Alice" }, + }, + }, + }); + + const client = new ApolloClient({ + cache, + link: httpLink, + incrementalHandler: new GraphQL17Alpha9Handler(), + }); + + const stream = new ObservableStream( + client.watchQuery({ + query, + fetchPolicy: "network-only", + errorPolicy: "all", + }) + ); + + await expect(stream).toEmitTypedValue({ + data: undefined, + dataState: "empty", + loading: true, + networkStatus: NetworkStatus.loading, + partial: true, + }); + + enqueueInitialChunk({ + data: { greeting: { __typename: "Greeting", message: "Hello world" } }, + pending: [{ id: "0", path: ["greeting"] }], + hasNext: true, + }); + + await expect(stream).toEmitTypedValue({ + data: markAsStreaming({ + greeting: { __typename: "Greeting", message: "Hello world" }, + }), + dataState: "streaming", + loading: true, + networkStatus: NetworkStatus.streaming, + partial: true, + }); + + enqueueSubsequentChunk({ + completed: [ + { + id: "0", + errors: [ + { + message: "Could not fetch recipient", + path: ["greeting", "recipient"], + }, + ], + }, + ], + hasNext: false, + }); + + await expect(stream).toEmitTypedValue({ + data: markAsStreaming({ + greeting: { __typename: "Greeting", message: "Hello world" }, + }), + dataState: "streaming", + error: new CombinedGraphQLErrors({ + data: { greeting: { __typename: "Greeting", message: "Hello world" } }, + errors: [ + { + message: "Could not fetch recipient", + path: ["greeting", "recipient"], + }, + ], + }), + loading: false, + networkStatus: NetworkStatus.error, + partial: true, + }); + + await expect(stream).not.toEmitAnything(); +}); + +test('does not return complete cached deferred data when a defer boundary completes with errors with a "network-only" fetch policy and errorPolicy "none"', async () => { + const query = gql` + query { + greeting { + message + ... on Greeting @defer { + recipient { + name + } + } + } + } + `; + + const { httpLink, enqueueInitialChunk, enqueueSubsequentChunk } = + mockDeferStreamGraphQL17Alpha9(); + const cache = new InMemoryCache(); + + cache.writeQuery({ + query, + data: { + greeting: { + __typename: "Greeting", + message: "Cached hello", + recipient: { __typename: "Person", name: "Cached Alice" }, + }, + }, + }); + + const client = new ApolloClient({ + cache, + link: httpLink, + incrementalHandler: new GraphQL17Alpha9Handler(), + }); + + const stream = new ObservableStream( + client.watchQuery({ query, fetchPolicy: "network-only" }) + ); + + await expect(stream).toEmitTypedValue({ + data: undefined, + dataState: "empty", + loading: true, + networkStatus: NetworkStatus.loading, + partial: true, + }); + + enqueueInitialChunk({ + data: { greeting: { __typename: "Greeting", message: "Hello world" } }, + pending: [{ id: "0", path: ["greeting"] }], + hasNext: true, + }); + + await expect(stream).toEmitTypedValue({ + data: markAsStreaming({ + greeting: { __typename: "Greeting", message: "Hello world" }, + }), + dataState: "streaming", + loading: true, + networkStatus: NetworkStatus.streaming, + partial: true, + }); + + enqueueSubsequentChunk({ + completed: [ + { + id: "0", + errors: [ + { + message: "Could not fetch recipient", + path: ["greeting", "recipient"], + }, + ], + }, + ], + hasNext: false, + }); + + await expect(stream).toEmitTypedValue({ + data: markAsStreaming({ + greeting: { __typename: "Greeting", message: "Hello world" }, + }), + dataState: "streaming", + error: new CombinedGraphQLErrors({ + data: { greeting: { __typename: "Greeting", message: "Hello world" } }, + errors: [ + { + message: "Could not fetch recipient", + path: ["greeting", "recipient"], + }, + ], + }), + loading: false, + networkStatus: NetworkStatus.error, + partial: true, + }); + + await expect(stream).not.toEmitAnything(); +}); + test('does not return a partial cached defer boundary while streaming with a "network-only" fetch policy', async () => { const query = gql` query { diff --git a/src/core/__tests__/client.watchQuery/streamGraphQL17Alpha9.test.ts b/src/core/__tests__/client.watchQuery/streamGraphQL17Alpha9.test.ts index f142eff2b8c..cf493638e82 100644 --- a/src/core/__tests__/client.watchQuery/streamGraphQL17Alpha9.test.ts +++ b/src/core/__tests__/client.watchQuery/streamGraphQL17Alpha9.test.ts @@ -4611,3 +4611,128 @@ function createMockStreamMergeFn() { return result; }); } + +test("does not emit when no data added when a `@stream` completes while a `@defer` boundary is still pending", async () => { + const query = gql` + query { + greeting { + message + ... on Greeting @defer { + recipient { + name + } + } + } + friendList @stream(initialCount: 1) { + name + } + } + `; + + const { httpLink, enqueueInitialChunk, enqueueSubsequentChunk } = + mockDeferStreamGraphQL17Alpha9(); + const cache = new InMemoryCache(); + + const client = new ApolloClient({ + cache, + link: httpLink, + incrementalHandler: new GraphQL17Alpha9Handler(), + }); + + const stream = new ObservableStream( + client.watchQuery({ query, fetchPolicy: "network-only" }) + ); + + await expect(stream).toEmitTypedValue({ + data: undefined, + dataState: "empty", + loading: true, + networkStatus: NetworkStatus.loading, + partial: true, + }); + + enqueueInitialChunk({ + data: { + greeting: { __typename: "Greeting", message: "Hello world" }, + friendList: [{ __typename: "Friend", name: "Luke" }], + }, + pending: [ + { id: "0", path: ["greeting"] }, + { id: "1", path: ["friendList"] }, + ], + hasNext: true, + }); + + await expect(stream).toEmitTypedValue({ + data: markAsStreaming({ + greeting: { __typename: "Greeting", message: "Hello world" }, + friendList: [{ __typename: "Friend", name: "Luke" }], + }), + dataState: "streaming", + loading: true, + networkStatus: NetworkStatus.streaming, + partial: true, + }); + + enqueueSubsequentChunk({ + incremental: [ + { id: "1", items: [{ __typename: "Friend", name: "Han" }] as any }, + ], + hasNext: true, + }); + + await expect(stream).toEmitTypedValue({ + data: markAsStreaming({ + greeting: { __typename: "Greeting", message: "Hello world" }, + friendList: [ + { __typename: "Friend", name: "Luke" }, + { __typename: "Friend", name: "Han" }, + ], + }), + dataState: "streaming", + loading: true, + networkStatus: NetworkStatus.streaming, + partial: true, + }); + + enqueueSubsequentChunk({ + completed: [{ id: "1" }], + hasNext: true, + }); + + await expect(stream).not.toEmitAnything(); + + enqueueSubsequentChunk({ + incremental: [ + { + id: "0", + data: { + __typename: "Greeting", + recipient: { __typename: "Person", name: "Alice" }, + }, + }, + ], + completed: [{ id: "0" }], + hasNext: false, + }); + + await expect(stream).toEmitTypedValue({ + data: { + greeting: { + __typename: "Greeting", + message: "Hello world", + recipient: { __typename: "Person", name: "Alice" }, + }, + friendList: [ + { __typename: "Friend", name: "Luke" }, + { __typename: "Friend", name: "Han" }, + ], + }, + dataState: "complete", + loading: false, + networkStatus: NetworkStatus.ready, + partial: false, + }); + + await expect(stream).not.toEmitAnything(); +}); diff --git a/src/core/dataStateErrorCache.ts b/src/core/dataStateErrorCache.ts new file mode 100644 index 00000000000..66303935e2c --- /dev/null +++ b/src/core/dataStateErrorCache.ts @@ -0,0 +1,15 @@ +import type { CombinedGraphQLErrors } from "@apollo/client/errors"; + +import type { DataState } from "./types.js"; + +// When working with an incremental result, errors in `@defer` fragments might +// bubble to the fragment boundary which leaves a hole in the data. When the +// `errorPolicy` is `"none"` we `throw` the constructed error so that it moves +// through the observable error flow. Because of this, we need a way to +// communicate the known dataState returned by QueryInfo to ObservableQuery. +// This ensures a "streaming" dataState can still be reported for errorPolicy: +// "none" queries. +export const dataStateErrorCache = new WeakMap< + CombinedGraphQLErrors, + DataState["dataState"] +>(); diff --git a/src/incremental/handlers/__tests__/graphql17Alpha9/defer.test.ts b/src/incremental/handlers/__tests__/graphql17Alpha9/defer.test.ts index 3ac60d37aed..9aaf62ec432 100644 --- a/src/incremental/handlers/__tests__/graphql17Alpha9/defer.test.ts +++ b/src/incremental/handlers/__tests__/graphql17Alpha9/defer.test.ts @@ -2506,13 +2506,13 @@ test("stream that returns an error but continues to stream", async () => { await expect(observableStream).toEmitTypedValue({ loading: false, - data: { + data: markAsStreaming({ hero: { __typename: "Hero", id: "1", name: "slow", }, - }, + }), error: new CombinedGraphQLErrors({ data: { hero: { @@ -2529,9 +2529,9 @@ test("stream that returns an error but continues to stream", async () => { }, ], }), - dataState: "complete", + dataState: "streaming", networkStatus: NetworkStatus.error, - partial: false, + partial: true, }); }); @@ -2705,3 +2705,87 @@ test("ignores `data` property added to subsequent chunks by misbehaving servers" partial: false, }); }); + +test("handles an incremental payload for a defer id that already completed with errors", async () => { + // `hero.name` is selected by both deferred fragments, so the server executes + // it as a single unit shared between them and reports it with the id of the + // fragment with the deepest path (`inner`). Since `nonNullName` errors, + // `inner` completes with errors before that shared unit resolves, which means + // an `incremental` payload arrives for an id that has already been completed. + const query = gql` + query { + ... @defer(label: "outer") { + hero { + name + } + } + hero { + ... on Hero @defer(label: "inner") { + name + nonNullName + } + } + } + `; + + const handler = new GraphQL17Alpha9Handler(); + const request = handler.startRequest({ query }); + + const incoming = run(query, { + hero: { + ...hero, + name: async () => { + await wait(10); + return "Luke"; + }, + nonNullName: () => null, + }, + }); + + { + const { value: chunk, done } = await incoming.next(); + + assert(!done); + assert(handler.isIncrementalResult(chunk)); + expect(request.handle(undefined, chunk)).toStrictEqualTyped({ + data: { hero: {} }, + }); + expect(request.hasNext).toBe(true); + } + + { + const { value: chunk, done } = await incoming.next(); + + assert(!done); + assert(handler.isIncrementalResult(chunk)); + expect(request.handle(undefined, chunk)).toStrictEqualTyped({ + data: { hero: {} }, + errors: [ + { + message: + "Cannot return null for non-nullable field Hero.nonNullName.", + path: ["hero", "nonNullName"], + }, + ], + }); + expect(request.hasNext).toBe(true); + } + + { + const { value: chunk, done } = await incoming.next(); + + assert(!done); + assert(handler.isIncrementalResult(chunk)); + expect(request.handle(undefined, chunk)).toStrictEqualTyped({ + data: { hero: { name: "Luke" } }, + errors: [ + { + message: + "Cannot return null for non-nullable field Hero.nonNullName.", + path: ["hero", "nonNullName"], + }, + ], + }); + expect(request.hasNext).toBe(false); + } +}); diff --git a/src/incremental/handlers/__tests__/graphql17Alpha9/stream.test.ts b/src/incremental/handlers/__tests__/graphql17Alpha9/stream.test.ts index 77d0a1d6ad4..4f8ae1d05f4 100644 --- a/src/incremental/handlers/__tests__/graphql17Alpha9/stream.test.ts +++ b/src/incremental/handlers/__tests__/graphql17Alpha9/stream.test.ts @@ -2829,6 +2829,81 @@ test("properly merges streamed data into list with more items", async () => { } }); +test("truncates cache data to the streamed items when the stream completes with errors", async () => { + const query = gql` + query { + nonNullFriendList @stream(initialCount: 1) { + name + id + } + } + `; + + const handler = new GraphQL17Alpha9Handler(); + const request = handler.startRequest({ query }); + + const incoming = run(query, { + nonNullFriendList: () => [friends[0], null, friends[1]], + }); + + { + const { value: chunk, done } = await incoming.next(); + + assert(!done); + assert(handler.isIncrementalResult(chunk)); + expect( + request.handle( + { + nonNullFriendList: [ + { name: "Luke Cached", id: "1" }, + { name: "Han Cached", id: "2" }, + { name: "Leia Cached", id: "3" }, + ], + }, + chunk + ) + ).toStrictEqualTyped({ + data: { + nonNullFriendList: [{ name: "Luke", id: "1" }], + }, + extensions: extensionsWithStreamDetails, + }); + expect(request.hasNext).toBe(true); + } + + { + const { value: chunk, done } = await incoming.next(); + + assert(!done); + assert(handler.isIncrementalResult(chunk)); + expect( + request.handle( + { + nonNullFriendList: [ + { name: "Luke", id: "1" }, + { name: "Han Cached", id: "2" }, + { name: "Leia Cached", id: "3" }, + ], + }, + chunk + ) + ).toStrictEqualTyped({ + data: { + nonNullFriendList: [{ name: "Luke", id: "1" }], + }, + errors: [ + { + message: + "Cannot return null for non-nullable field Query.nonNullFriendList.", + path: ["nonNullFriendList", 1], + }, + ], + extensions: extensionsWithStreamDetails, + }); + expect(request.hasNext).toBe(false); + } +}); + test("properly merges cache data when list is included in deferred chunk", async () => { const { promise: slowFieldPromise, resolve: resolveSlowField } = promiseWithResolvers(); diff --git a/src/incremental/handlers/graphql17Alpha9.ts b/src/incremental/handlers/graphql17Alpha9.ts index 078d77322e4..fe843a9ef64 100644 --- a/src/incremental/handlers/graphql17Alpha9.ts +++ b/src/incremental/handlers/graphql17Alpha9.ts @@ -87,7 +87,11 @@ class IncrementalRequest private errors: GraphQLFormattedError[] = []; private extensions: Record = {}; private pendingMap = new Map(); - private streamInfo = makeStreamInfoTrie(); + private completedMap = new Map< + /* pendingId */ string, + /* delivered */ boolean + >(); + private _streamInfo = makeStreamInfoTrie(); // `streamPositions` maps `pending.id` to the index that should be set by the // next `incremental` stream chunk to ensure the streamed array item is placed // at the correct point in the data array. `this.data` contains cached @@ -97,12 +101,24 @@ class IncrementalRequest // of future stream items from already merged stream items. private streamPositions: Record = {}; - get pending() { - return Array.from(this.pendingMap.values()); + /** @internal */ + get streamInfo() { + return this._streamInfo["strong"] ? this._streamInfo : undefined; } - getPendingType(id: string): "defer" | "stream" { - return id in this.streamPositions ? "stream" : "defer"; + /** @internal */ + getPendingWithInfo() { + return Array.from(this.pendingMap.values()).map((pending) => { + if (pending.id in this.streamPositions) { + return { type: "stream" as const, path: pending.path }; + } + + return { + type: "defer" as const, + delivered: !!this.completedMap.get(pending.id), + path: pending.path, + }; + }); } handle( @@ -124,7 +140,7 @@ class IncrementalRequest if (Array.isArray(dataAtPath)) { this.streamPositions[pending.id] = dataAtPath.length; - const entry = this.streamInfo.lookupArray(pending.path as any[]); + const entry = this._streamInfo.lookupArray(pending.path as any[]); entry.current = { isFirstChunk: true, isLastChunk: false, @@ -159,7 +175,7 @@ class IncrementalRequest } this.streamPositions[pending.id] += items.length; - const entry = this.streamInfo.lookupArray(path); + const entry = this._streamInfo.lookupArray(path); entry.current = { isFirstChunk: false, isLastChunk: false, @@ -211,6 +227,7 @@ class IncrementalRequest for (const completed of chunk.completed) { const { path } = this.pendingMap.get(completed.id)!; const streamPosition = this.streamPositions[completed.id]; + this.completedMap.set(completed.id, !completed.errors); // Truncate any stream arrays in case the chunk only contains `hasNext` // and `completed`. @@ -224,7 +241,7 @@ class IncrementalRequest } // peek instead of lookup to avoid creating an entry for non-array values - const details = this.streamInfo.peekArray(path as any[]); + const details = this._streamInfo.peekArray(path as any[]); if (details) { details.current = { isFirstChunk: false, @@ -232,7 +249,6 @@ class IncrementalRequest }; details.state.streamPosition = streamPosition; } - this.pendingMap.delete(completed.id); if (completed.errors) { this.errors.push(...completed.errors); @@ -250,7 +266,7 @@ class IncrementalRequest result.extensions = this.extensions; } - if (this.streamInfo["strong"]) { + if (this._streamInfo["strong"]) { result.extensions = { ...result.extensions, // Create a new object so we can check for === in QueryInfo to trigger a @@ -258,7 +274,7 @@ class IncrementalRequest // We create a `WeakRef`, not a plain object to avoid retaining memory // in case the `result` or `extensions` stays around longer than the handler // itself. - [streamInfoSymbol]: new WeakRef(this.streamInfo), + [streamInfoSymbol]: new WeakRef(this._streamInfo), } satisfies ExtensionsWithStreamInfo; } diff --git a/src/incremental/types.ts b/src/incremental/types.ts index e787ee70529..2475aec83e5 100644 --- a/src/incremental/types.ts +++ b/src/incremental/types.ts @@ -6,6 +6,7 @@ import type { import type { ApolloLink } from "@apollo/client/link"; import type { DeepPartial } from "@apollo/client/utilities"; +import type { StreamInfoTrie } from "@apollo/client/utilities/internal"; export declare namespace Incremental { export type Path = ReadonlyArray; @@ -40,14 +41,36 @@ export declare namespace Incremental { TData, > { hasNext: boolean; - pending?: Array; - getPendingType?: (id: string) => "defer" | "stream"; + + /** @internal */ + readonly streamInfo?: StreamInfoTrie; + /** @internal */ + getPendingWithInfo?: () => Array; + handle: ( cacheData: TData | DeepPartial | undefined | null, chunk: Chunk ) => FormattedExecutionResult; } + /** @internal */ + export interface PendingDeferResultWithInfo { + type: "defer"; + delivered: boolean; + path: Incremental.Path; + } + + /** @internal */ + export interface PendingStreamResultWithInfo { + type: "stream"; + path: Incremental.Path; + } + + /** @internal */ + export type PendingItemWithInfo = + | PendingDeferResultWithInfo + | PendingStreamResultWithInfo; + /** @internal */ export interface StreamFieldInfo { isFirstChunk: boolean;