From 383b292b167357043045d6e4c11ff1053d1314a1 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 10 Sep 2020 19:07:59 -0400 Subject: [PATCH 1/3] Respect errorPolicy for mutation and subscription results. Using the default ErrorPolicy of "none" for queries means no data will be written to the cache when a GraphQL result has errors. Queries can use errorPolicy:"ignore" and errorPolicy:"all" to ensure data is written to the cache in spite of GraphQL errors, but mutations and subscriptions were previously limited to the default policy, "none". This commit makes mutations and subscriptions respect the non-default "ignore" and "all" ErrorPolicy values, just as queries do, hopefully addressing #6965. Since these changes did not cause any tests to fail, clearly we need more tests of non-default ErrorPolicy usage by mutations and subscriptions. --- src/core/QueryInfo.ts | 26 ++++++++++++++++---------- src/core/QueryManager.ts | 12 ++++++++---- src/core/watchQueryOptions.ts | 5 +++++ 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index dff299889df..e3c82e511c7 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -2,7 +2,7 @@ import { DocumentNode, GraphQLError } from 'graphql'; import { equal } from "@wry/equality"; import { Cache, ApolloCache } from '../cache'; -import { WatchQueryOptions } from './watchQueryOptions'; +import { WatchQueryOptions, ErrorPolicy } from './watchQueryOptions'; import { ObservableQuery } from './ObservableQuery'; import { QueryListener } from './types'; import { FetchResult } from '../link/core'; @@ -288,15 +288,7 @@ export class QueryInfo { this.diff = { result: result.data, complete: true }; } else if (allowCacheWrite) { - const ignoreErrors = - options.errorPolicy === 'ignore' || - options.errorPolicy === 'all'; - let writeWithErrors = !graphQLResultHasError(result); - if (!writeWithErrors && ignoreErrors && result.data) { - writeWithErrors = true; - } - - if (writeWithErrors) { + if (shouldWriteResult(result, options.errorPolicy)) { // Using a transaction here so we have a chance to read the result // back from the cache before the watch callback fires as a result // of writeQuery, so we can store the new diff quietly and ignore @@ -405,3 +397,17 @@ export class QueryInfo { return error; } } + +export function shouldWriteResult( + result: FetchResult, + errorPolicy: ErrorPolicy = "none", +) { + const ignoreErrors = + errorPolicy === "ignore" || + errorPolicy === "all"; + let writeWithErrors = !graphQLResultHasError(result); + if (!writeWithErrors && ignoreErrors && result.data) { + writeWithErrors = true; + } + return writeWithErrors; +} diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index af0fb4437fc..42fde4aefcb 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -39,7 +39,7 @@ import { } from './types'; import { LocalState } from './LocalState'; -import { QueryInfo, QueryStoreValue } from './QueryInfo'; +import { QueryInfo, QueryStoreValue, shouldWriteResult } from './QueryInfo'; const { hasOwnProperty } = Object.prototype; @@ -192,6 +192,7 @@ export class QueryManager { result: { data: optimistic }, document: mutation, variables: variables, + errorPolicy, queryUpdatersById: generateUpdateQueriesInfo(), update: updateWithProxyFn, }, cache); @@ -235,6 +236,7 @@ export class QueryManager { result, document: mutation, variables, + errorPolicy, queryUpdatersById: generateUpdateQueriesInfo(), update: updateWithProxyFn, }, self.cache); @@ -588,6 +590,7 @@ export class QueryManager { public startGraphQLSubscription({ query, fetchPolicy, + errorPolicy, variables, context = {}, }: SubscriptionOptions): Observable> { @@ -601,10 +604,10 @@ export class QueryManager { variables, false, ).map(result => { - if (!fetchPolicy || fetchPolicy !== 'no-cache') { + if (fetchPolicy !== 'no-cache') { // the subscription interface should handle not sending us results we no longer subscribe to. // XXX I don't think we ever send in an object with errors, but we might in the future... - if (!graphQLResultHasError(result)) { + if (shouldWriteResult(result, errorPolicy)) { this.cache.write({ query, result: result.data, @@ -1078,6 +1081,7 @@ function markMutationResult( result: FetchResult; document: DocumentNode; variables: any; + errorPolicy: ErrorPolicy; queryUpdatersById: Record; update: ((cache: ApolloCache, mutationResult: Object) => void) | @@ -1086,7 +1090,7 @@ function markMutationResult( cache: ApolloCache, ) { // Incorporate the result from this mutation into the store - if (!graphQLResultHasError(mutation.result)) { + if (shouldWriteResult(mutation.result, mutation.errorPolicy)) { const cacheWrites: Cache.WriteOptions[] = [{ result: mutation.result.data, dataId: 'ROOT_MUTATION', diff --git a/src/core/watchQueryOptions.ts b/src/core/watchQueryOptions.ts index 4382a9888e3..e3b2ebe6602 100644 --- a/src/core/watchQueryOptions.ts +++ b/src/core/watchQueryOptions.ts @@ -169,6 +169,11 @@ export interface SubscriptionOptions Date: Fri, 11 Sep 2020 11:03:00 -0400 Subject: [PATCH 2/3] Mention PR #7003 in CHANGELOG.md. --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6396331ce7d..1359a1669e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,9 @@ - Move `apollo-link-persisted-queries` implementation to `@apollo/client/link/persisted-queries`. Try running our [automated imports transform](https://github.com/apollographql/apollo-client/tree/main/codemods/ac2-to-ac3) to handle this conversion, if you're using `apollo-link-persisted-queries`.
[@hwillson](https://github.com/hwillson) in [#6837](https://github.com/apollographql/apollo-client/pull/6837) +- Support non-default `ErrorPolicy` values (that is, `"ignore"` and `"all"`, in addition to the default value `"none"`) for mutations and subscriptions, like we do for queries.
+ [@benjamn](https://github.com/benjamn) in [#7003](https://github.com/apollographql/apollo-client/pull/7003) + - Remove invariant forbidding a `FetchPolicy` of `cache-only` in `ObservableQuery#refetch`.
[@benjamn](https://github.com/benjamn) in [ccb0a79a](https://github.com/apollographql/apollo-client/pull/6774/commits/ccb0a79a588721f08bf87a131c31bf37fa3238e5), fixing [#6702](https://github.com/apollographql/apollo-client/issues/6702) From f17f92a209b40c07a1443725cd48cd33474f52e6 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 11 Sep 2020 14:58:01 -0400 Subject: [PATCH 3/3] Tests of mutation error policies. --- .../__snapshots__/mutationResults.ts.snap | 40 ++++++++ src/__tests__/mutationResults.ts | 94 +++++++++++++++++++ 2 files changed, 134 insertions(+) create mode 100644 src/__tests__/__snapshots__/mutationResults.ts.snap diff --git a/src/__tests__/__snapshots__/mutationResults.ts.snap b/src/__tests__/__snapshots__/mutationResults.ts.snap new file mode 100644 index 00000000000..ebcfff7bb25 --- /dev/null +++ b/src/__tests__/__snapshots__/mutationResults.ts.snap @@ -0,0 +1,40 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`mutation results should write results to cache according to errorPolicy 1`] = `Object {}`; + +exports[`mutation results should write results to cache according to errorPolicy 2`] = ` +Object { + "Person:{\\"name\\":\\"Jenn Creighton\\"}": Object { + "__typename": "Person", + "name": "Jenn Creighton", + }, + "ROOT_MUTATION": Object { + "__typename": "Mutation", + "newPerson({\\"name\\":\\"Jenn Creighton\\"})": Object { + "__ref": "Person:{\\"name\\":\\"Jenn Creighton\\"}", + }, + }, +} +`; + +exports[`mutation results should write results to cache according to errorPolicy 3`] = ` +Object { + "Person:{\\"name\\":\\"Ellen Shapiro\\"}": Object { + "__typename": "Person", + "name": "Ellen Shapiro", + }, + "Person:{\\"name\\":\\"Jenn Creighton\\"}": Object { + "__typename": "Person", + "name": "Jenn Creighton", + }, + "ROOT_MUTATION": Object { + "__typename": "Mutation", + "newPerson({\\"name\\":\\"Ellen Shapiro\\"})": Object { + "__ref": "Person:{\\"name\\":\\"Ellen Shapiro\\"}", + }, + "newPerson({\\"name\\":\\"Jenn Creighton\\"})": Object { + "__ref": "Person:{\\"name\\":\\"Jenn Creighton\\"}", + }, + }, +} +`; diff --git a/src/__tests__/mutationResults.ts b/src/__tests__/mutationResults.ts index 9d87841f9fe..ed36c587cba 100644 --- a/src/__tests__/mutationResults.ts +++ b/src/__tests__/mutationResults.ts @@ -1,5 +1,6 @@ import { cloneDeep } from 'lodash'; import gql from 'graphql-tag'; +import { GraphQLError } from 'graphql'; import { ApolloClient } from '../core'; import { InMemoryCache } from '../cache'; @@ -306,6 +307,99 @@ describe('mutation results', () => { }); }); + itAsync("should write results to cache according to errorPolicy", async (resolve, reject) => { + const expectedFakeError = new GraphQLError("expected/fake error"); + + const client = new ApolloClient({ + cache: new InMemoryCache({ + typePolicies: { + Person: { + keyFields: ["name"], + }, + }, + }), + + link: new ApolloLink(operation => new Observable(observer => { + observer.next({ + errors: [ + expectedFakeError, + ], + data: { + newPerson: { + __typename: "Person", + name: operation.variables.newName, + }, + }, + }); + observer.complete(); + })).setOnError(reject), + }); + + const mutation = gql` + mutation AddNewPerson($newName: String!) { + newPerson(name: $newName) { + name + } + } + `; + + await client.mutate({ + mutation, + variables: { + newName: "Hugh Willson", + }, + }).then(() => { + reject("should have thrown for default errorPolicy"); + }, error => { + expect(error.message).toBe(expectedFakeError.message); + }); + + expect(client.cache.extract()).toMatchSnapshot(); + + const ignoreErrorsResult = await client.mutate({ + mutation, + errorPolicy: "ignore", + variables: { + newName: "Jenn Creighton", + }, + }); + + expect(ignoreErrorsResult).toEqual({ + data: { + newPerson: { + __typename: "Person", + name: "Jenn Creighton", + }, + }, + }); + + expect(client.cache.extract()).toMatchSnapshot(); + + const allErrorsResult = await client.mutate({ + mutation, + errorPolicy: "all", + variables: { + newName: "Ellen Shapiro", + }, + }); + + expect(allErrorsResult).toEqual({ + data: { + newPerson: { + __typename: "Person", + name: "Ellen Shapiro", + }, + }, + errors: [ + expectedFakeError, + ], + }); + + expect(client.cache.extract()).toMatchSnapshot(); + + resolve(); + }); + itAsync("should warn when the result fields don't match the query fields", (resolve, reject) => { let handle: any; let subscriptionHandle: Subscription;