From 281d1dbcf09f4b537bc6202d0543f25cf7187b82 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 17 Mar 2020 16:24:30 -0400 Subject: [PATCH] Throw when writing inconsistent query/data to InMemoryCache. (#6055) The changes in src/cache/inmemory/writeToStore.ts make it an error rather than just a warning to write a result into the InMemoryCache with missing data fields, addressing an old XXX comment that was grandfathered into the StoreWriter class. This will be a **breaking change** for any code that was previously ignoring the warning, which means this change is appropriate for Apollo Client 3.0, but likely will not require any migration steps for most applications. While updating the relevant tests, I realized that the withWarning pattern could (and should) be eliminated. --- CHANGELOG.md | 3 + src/__tests__/ApolloClient.ts | 9 +-- src/__tests__/client.ts | 46 ++--------- src/__tests__/mutationResults.ts | 78 ++++++++++--------- src/cache/inmemory/__tests__/entityStore.ts | 4 +- src/cache/inmemory/__tests__/policies.ts | 4 +- src/cache/inmemory/__tests__/readFromStore.ts | 2 +- src/cache/inmemory/__tests__/roundtrip.ts | 9 +-- src/cache/inmemory/__tests__/writeToStore.ts | 39 +++------- src/cache/inmemory/policies.ts | 2 +- src/cache/inmemory/readFromStore.ts | 4 +- src/cache/inmemory/writeToStore.ts | 19 ++--- src/core/QueryManager.ts | 23 ++++-- src/utilities/graphql/directives.ts | 9 ++- src/utilities/testing/wrap.ts | 13 ---- 15 files changed, 101 insertions(+), 163 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 243e699939f..c92dd729f97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,9 @@ - **[BREAKING]** Apollo Client 2.x allowed `@client` fields to be passed into the `link` chain if `resolvers` were not set in the constructor. This allowed `@client` fields to be passed into Links like `apollo-link-state`. Apollo Client 3 enforces that `@client` fields are local only, meaning they are no longer passed into the `link` chain, under any circumstances.
[@hwillson](https://github.com/hwillson) in [#5982](https://github.com/apollographql/apollo-client/pull/5982) +- **[BREAKING]** `InMemoryCache` now _throws_ when data with missing or undefined query fields is written into the cache, rather than just warning in development.
+ [@benjamn](https://github.com/benjamn) in [#6055](https://github.com/apollographql/apollo-client/pull/6055) + - **[BREAKING]** `client|cache.writeData` have been fully removed. `writeData` usage is one of the easiest ways to turn faulty assumptions about how the cache represents data internally, into cache inconsistency and corruption. `client|cache.writeQuery`, `client|cache.writeFragment`, and/or `cache.modify` can be used to update the cache.
[@benjamn](https://github.com/benjamn) in [#5923](https://github.com/apollographql/apollo-client/pull/5923) diff --git a/src/__tests__/ApolloClient.ts b/src/__tests__/ApolloClient.ts index b854ee46c8b..f14a7e15d65 100644 --- a/src/__tests__/ApolloClient.ts +++ b/src/__tests__/ApolloClient.ts @@ -6,7 +6,6 @@ import { ApolloLink } from '../link/core/ApolloLink'; import { HttpLink } from '../link/http/HttpLink'; import { InMemoryCache } from '../cache/inmemory/inMemoryCache'; import { stripSymbols } from '../utilities/testing/stripSymbols'; -import { withWarning } from '../utilities/testing/wrap'; import { ApolloClient } from '../'; import { DefaultOptions } from '../ApolloClient'; import { FetchPolicy, QueryOptions } from '../core/watchQueryOptions'; @@ -827,7 +826,7 @@ describe('ApolloClient', () => { }), }); - return withWarning(() => { + expect(() => { client.writeQuery({ data: { todos: [ @@ -848,7 +847,7 @@ describe('ApolloClient', () => { } `, }); - }, /Missing field description/); + }).toThrowError(/Missing field 'description' /); }); }); @@ -1109,7 +1108,7 @@ describe('ApolloClient', () => { }), }); - return withWarning(() => { + expect(() => { client.writeFragment({ data: { __typename: 'Bar', i: 10 }, id: 'bar', @@ -1120,7 +1119,7 @@ describe('ApolloClient', () => { } `, }); - }, /Missing field e/); + }).toThrowError(/Missing field 'e' /); }); describe('change will call observable next', () => { diff --git a/src/__tests__/client.ts b/src/__tests__/client.ts index 1517e0cbf7d..30d32be5ccb 100644 --- a/src/__tests__/client.ts +++ b/src/__tests__/client.ts @@ -11,7 +11,6 @@ import { WatchQueryOptions, FetchPolicy, WatchQueryFetchPolicy } from '../core/w import { ApolloError } from '../errors/ApolloError'; import { ApolloClient } from '..'; import subscribeAndCount from '../utilities/testing/subscribeAndCount'; -import { withWarning } from '../utilities/testing/wrap'; import { itAsync } from '../utilities/testing/itAsync'; import { mockSingleLink } from '../utilities/testing/mocking/mockLink'; import { NetworkStatus, ObservableQuery } from '../core'; @@ -625,41 +624,6 @@ describe('client', () => { }); }); - itAsync.skip('should pass a network error correctly on a query using an observable network interface with a warning', (resolve, reject) => { - withWarning(() => { - const query = gql` - query people { - allPeople(first: 1) { - people { - name - } - } - } - `; - - const networkError = new Error('Some kind of network error.'); - - const link = ApolloLink.from([ - () => { - return new Observable(_ => { - throw networkError; - }); - }, - ]); - - const client = new ApolloClient({ - link, - cache: new InMemoryCache({ addTypename: false }), - }); - - client.query({ query }).catch((error: ApolloError) => { - expect(error.networkError).toBeDefined(); - expect(error.networkError!.message).toEqual(networkError.message); - resolve(); - }); - }, /deprecated/); - }); - itAsync('should pass a network error correctly on a query with apollo-link network interface', (resolve, reject) => { const query = gql` query people { @@ -2620,9 +2584,13 @@ describe('client', () => { }), }); - return withWarning( - () => client.query({ query }), - /Missing field description/, + return client.query({ query }).then( + result => { + fail("should have errored"); + }, + error => { + expect(error.message).toMatch(/Missing field 'description' /); + }, ).then(resolve, reject); }); diff --git a/src/__tests__/mutationResults.ts b/src/__tests__/mutationResults.ts index 56ef7ad3488..21fa13ebac3 100644 --- a/src/__tests__/mutationResults.ts +++ b/src/__tests__/mutationResults.ts @@ -6,7 +6,6 @@ import { ApolloLink } from '../link/core/ApolloLink'; import { mockSingleLink } from '../utilities/testing/mocking/mockLink'; import { ApolloClient } from '..'; import { InMemoryCache } from '../cache/inmemory/inMemoryCache'; -import { withWarning } from '../utilities/testing/wrap'; import { itAsync } from '../utilities/testing/itAsync'; describe('mutation results', () => { @@ -358,45 +357,50 @@ describe('mutation results', () => { }, }; - return withWarning(() => { - const { client, obsQuery } = setupObsQuery( - reject, - { - request: { query: queryTodos }, - result: queryTodosResult, - }, - { - request: { query: mutationTodo }, - result: mutationTodoResult, - }, - ); + const { client, obsQuery } = setupObsQuery( + reject, + { + request: { query: queryTodos }, + result: queryTodosResult, + }, + { + request: { query: mutationTodo }, + result: mutationTodoResult, + }, + ); - return obsQuery.result().then(() => { - // we have to actually subscribe to the query to be able to update it - return new Promise(resolve => { - handle = client.watchQuery({ query: queryTodos }); - subscriptionHandle = handle.subscribe({ - next(res: any) { - counter++; - resolve(res); - }, - }); - }); - }).then(() => client.mutate({ - mutation: mutationTodo, - updateQueries: { - todos: (prev, { mutationResult }) => { - const newTodo = (mutationResult as any).data.createTodo; - const newResults = { - todos: [...(prev as any).todos, newTodo], - }; - return newResults; + return obsQuery.result().then(() => { + // we have to actually subscribe to the query to be able to update it + return new Promise(resolve => { + handle = client.watchQuery({ query: queryTodos }); + subscriptionHandle = handle.subscribe({ + next(res: any) { + counter++; + resolve(res); }, + }); + }); + }).then(() => client.mutate({ + mutation: mutationTodo, + updateQueries: { + todos: (prev, { mutationResult }) => { + const newTodo = (mutationResult as any).data.createTodo; + const newResults = { + todos: [...(prev as any).todos, newTodo], + }; + return newResults; }, - })).then( - () => subscriptionHandle.unsubscribe() - ).then(resolve, reject); - }, /Missing field description/); + }, + })).then( + () => { + subscriptionHandle.unsubscribe(); + fail("should have errored"); + }, + error => { + subscriptionHandle.unsubscribe(); + expect(error.message).toMatch(/Missing field 'description' /); + }, + ).then(resolve, reject); }); describe('updateQueries', () => { diff --git a/src/cache/inmemory/__tests__/entityStore.ts b/src/cache/inmemory/__tests__/entityStore.ts index 20eeade67b3..34210c4af2a 100644 --- a/src/cache/inmemory/__tests__/entityStore.ts +++ b/src/cache/inmemory/__tests__/entityStore.ts @@ -1170,8 +1170,8 @@ describe('EntityStore', () => { c: 3, })).toBe('ABCs:{"b":2,"a":1,"c":3}'); - expect(() => cache.identify(ABCs)).toThrow( - "Missing field b while computing key fields", + expect(() => cache.identify(ABCs)).toThrowError( + "Missing field 'b' while computing key fields", ); expect(cache.readFragment({ diff --git a/src/cache/inmemory/__tests__/policies.ts b/src/cache/inmemory/__tests__/policies.ts index 5eefe0707f4..a90216eb2d1 100644 --- a/src/cache/inmemory/__tests__/policies.ts +++ b/src/cache/inmemory/__tests__/policies.ts @@ -281,7 +281,7 @@ describe("type policies", function () { book: theInformationBookData, }, }); - }).toThrow("Missing field year while computing key fields"); + }).toThrowError("Missing field 'year' while computing key fields"); }); describe("field policies", function () { @@ -1859,7 +1859,7 @@ describe("type policies", function () { } ` }); - }).toThrow("Can't find field secret"); + }).toThrowError("Can't find field 'secret' "); expect(secretReadAttempted).toBe(true); }); diff --git a/src/cache/inmemory/__tests__/readFromStore.ts b/src/cache/inmemory/__tests__/readFromStore.ts index 6037e8aaf7a..91c09d38fc9 100644 --- a/src/cache/inmemory/__tests__/readFromStore.ts +++ b/src/cache/inmemory/__tests__/readFromStore.ts @@ -553,7 +553,7 @@ describe('reading from the store', () => { } `, }); - }).toThrowError(/Can't find field missingField on ROOT_QUERY object/); + }).toThrowError(/Can't find field 'missingField' on ROOT_QUERY object/); }); it('runs a nested query where the reference is null', () => { diff --git a/src/cache/inmemory/__tests__/roundtrip.ts b/src/cache/inmemory/__tests__/roundtrip.ts index ba6a75796d0..4d1c256f73c 100644 --- a/src/cache/inmemory/__tests__/roundtrip.ts +++ b/src/cache/inmemory/__tests__/roundtrip.ts @@ -2,7 +2,6 @@ import { DocumentNode } from 'graphql'; import gql from 'graphql-tag'; import { withError } from './diffAgainstStore'; -import { withWarning } from './writeToStore'; import { EntityStore } from '../entityStore'; import { StoreReader } from '../readFromStore'; import { StoreWriter } from '../writeToStore'; @@ -311,7 +310,7 @@ describe('roundtrip', () => { // XXX this test is weird because it assumes the server returned an incorrect result // However, the user may have written this result with client.writeQuery. it('should throw an error on two of the same inline fragment types', () => { - return withWarning(() => expect(() => { + expect(() => { storeRoundtrip( gql` query { @@ -337,7 +336,7 @@ describe('roundtrip', () => { ], }, ); - }).toThrowError(/Can\'t find field rank on object/)); + }).toThrowError(/Missing field 'rank' /); }); it('should resolve fields it can on interface with non matching inline fragments', () => { @@ -451,7 +450,7 @@ describe('roundtrip', () => { }); it('should throw on error on two of the same spread fragment types', () => { - withWarning(() => expect(() => { + expect(() => { storeRoundtrip( gql` fragment jediSide on Jedi { @@ -481,7 +480,7 @@ describe('roundtrip', () => { ], }, ); - }).toThrowError(/Can\'t find field rank on object/)); + }).toThrowError(/Missing field 'rank' /); }); it('should resolve on @include and @skip with inline fragments', () => { diff --git a/src/cache/inmemory/__tests__/writeToStore.ts b/src/cache/inmemory/__tests__/writeToStore.ts index c481cc9703b..a361161f0ac 100644 --- a/src/cache/inmemory/__tests__/writeToStore.ts +++ b/src/cache/inmemory/__tests__/writeToStore.ts @@ -20,21 +20,6 @@ import { defaultNormalizedCacheFactory } from '../entityStore'; import { InMemoryCache } from '../inMemoryCache'; import { Policies } from '../policies'; -export function withWarning(func: Function, regex?: RegExp) { - let message: string = null as never; - const oldWarn = console.warn; - - console.warn = (m: string) => (message = m); - - return Promise.resolve(func()).then(val => { - if (regex) { - expect(message).toMatch(regex); - } - console.warn = oldWarn; - return val; - }); -} - const getIdField = ({ id }: { id: string }) => id; describe('writing to the store', () => { @@ -1563,14 +1548,12 @@ describe('writing to the store', () => { }), }); - return withWarning(() => { - const newStore = writer.writeQueryToStore({ + expect(() => { + writer.writeQueryToStore({ query, result, }); - - expect((newStore as any).lookup('1')).toEqual(result.todos[0]); - }, /Missing field description/); + }).toThrowError(/Missing field 'description' /); }); it('should warn when it receives the wrong data inside a fragment', () => { @@ -1617,14 +1600,12 @@ describe('writing to the store', () => { }), }); - return withWarning(() => { - const newStore = writer.writeQueryToStore({ + expect(() => { + writer.writeQueryToStore({ query: queryWithInterface, result, }); - - expect((newStore as any).lookup('1')).toEqual(result.todos[0]); - }, /Missing field price/); + }).toThrowError(/Missing field 'price' /); }); it('should warn if a result is missing __typename when required', () => { @@ -1645,14 +1626,12 @@ describe('writing to the store', () => { }), }); - return withWarning(() => { - const newStore = writer.writeQueryToStore({ + expect(() => { + writer.writeQueryToStore({ query: addTypenameToDocument(query), result, }); - - expect((newStore as any).lookup('1')).toEqual(result.todos[0]); - }, /Missing field __typename/); + }).toThrowError(/Missing field '__typename' /); }); it('should not warn if a field is null', () => { diff --git a/src/cache/inmemory/policies.ts b/src/cache/inmemory/policies.ts index aaf7748f9eb..a2e0429f348 100644 --- a/src/cache/inmemory/policies.ts +++ b/src/cache/inmemory/policies.ts @@ -785,7 +785,7 @@ function computeKeyObject( invariant( hasOwn.call(response, responseName), // TODO Make this appropriate for keyArgs as well - `Missing field ${responseName} while computing key fields`, + `Missing field '${responseName}' while computing key fields`, ); keyObj[prevKey = s] = response[responseName]; } diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts index 9ee9f20070d..b14692c8c7f 100644 --- a/src/cache/inmemory/readFromStore.ts +++ b/src/cache/inmemory/readFromStore.ts @@ -242,9 +242,9 @@ export class StoreReader { if (fieldValue === void 0) { if (!addTypenameToDocument.added(selection)) { - getMissing().push(new InvariantError(`Can't find field ${ + getMissing().push(new InvariantError(`Can't find field '${ selection.name.value - } on ${ + }' on ${ isReference(objectOrReference) ? objectOrReference.__ref + " object" : "object " + JSON.stringify(objectOrReference, null, 2) diff --git a/src/cache/inmemory/writeToStore.ts b/src/cache/inmemory/writeToStore.ts index de617f5dddf..ef5b7506148 100644 --- a/src/cache/inmemory/writeToStore.ts +++ b/src/cache/inmemory/writeToStore.ts @@ -1,5 +1,5 @@ import { SelectionSetNode, FieldNode, DocumentNode } from 'graphql'; -import { invariant } from 'ts-invariant'; +import { InvariantError } from 'ts-invariant'; import { createFragmentMap, @@ -22,7 +22,7 @@ import { StoreObject, } from '../../utilities/graphql/storeUtils'; -import { shouldInclude } from '../../utilities/graphql/directives'; +import { shouldInclude, hasDirectives } from '../../utilities/graphql/directives'; import { cloneDeep } from '../../utilities/common/cloneDeep'; import { Policies, ReadMergeContext } from './policies'; @@ -229,19 +229,10 @@ export class StoreWriter { } else if ( policies.usingPossibleTypes && - !( - selection.directives && - selection.directives.some( - ({ name }) => - name && (name.value === 'defer' || name.value === 'client'), - ) - ) + !hasDirectives(["defer", "client"], selection) ) { - // XXX We'd like to throw an error, but for backwards compatibility's sake - // we just print a warning for the time being. - //throw new WriteError(`Missing field ${resultFieldKey} in ${JSON.stringify(result, null, 2).substring(0, 100)}`); - invariant.warn( - `Missing field ${resultFieldKey} in ${JSON.stringify( + throw new InvariantError( + `Missing field '${resultFieldKey}' in ${JSON.stringify( result, null, 2, diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index 240021df4d4..22c4b90c929 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -239,14 +239,21 @@ export class QueryManager { self.mutationStore.markMutationResult(mutationId); if (fetchPolicy !== 'no-cache') { - markMutationResult({ - mutationId, - result, - document: mutation, - variables, - queryUpdatersById: generateUpdateQueriesInfo(), - update: updateWithProxyFn, - }, self.cache); + try { + markMutationResult({ + mutationId, + result, + document: mutation, + variables, + queryUpdatersById: generateUpdateQueriesInfo(), + update: updateWithProxyFn, + }, self.cache); + } catch (e) { + error = new ApolloError({ + networkError: e, + }); + return; + } } storeResult = result as FetchResult; diff --git a/src/utilities/graphql/directives.ts b/src/utilities/graphql/directives.ts index 31658f1be16..9f5d10748f9 100644 --- a/src/utilities/graphql/directives.ts +++ b/src/utilities/graphql/directives.ts @@ -8,6 +8,7 @@ import { DocumentNode, ArgumentNode, ValueNode, + ASTNode, } from 'graphql'; import { visit } from 'graphql/language/visitor'; @@ -42,10 +43,10 @@ export function shouldInclude( }); } -export function getDirectiveNames(doc: DocumentNode) { +export function getDirectiveNames(root: ASTNode) { const names: string[] = []; - visit(doc, { + visit(root, { Directive(node) { names.push(node.name.value); }, @@ -54,8 +55,8 @@ export function getDirectiveNames(doc: DocumentNode) { return names; } -export function hasDirectives(names: string[], doc: DocumentNode) { - return getDirectiveNames(doc).some( +export function hasDirectives(names: string[], root: ASTNode) { + return getDirectiveNames(root).some( (name: string) => names.indexOf(name) > -1, ); } diff --git a/src/utilities/testing/wrap.ts b/src/utilities/testing/wrap.ts index 6e635a29903..6c020a3ef86 100644 --- a/src/utilities/testing/wrap.ts +++ b/src/utilities/testing/wrap.ts @@ -11,19 +11,6 @@ export default ( } }; -export function withWarning(func: Function, regex: RegExp) { - let message: string = null as never; - const oldWarn = console.warn; - - console.warn = (m: string) => (message = m); - - return Promise.resolve(func()).then(val => { - expect(message).toMatch(regex); - console.warn = oldWarn; - return val; - }); -} - export function withError(func: Function, regex: RegExp) { let message: string = null as never; const oldError = console.error;