Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fully eliminate the concept of heuristic fragment matching. #5684

Merged
merged 6 commits into from
Dec 13, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/cache/inmemory/__tests__/diffAgainstStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ describe('diffing queries against the store', () => {
returnPartialData: false,
});

expect(complete).toBe(false);
expect(complete).toBe(true);
});
});

Expand Down Expand Up @@ -306,7 +306,7 @@ describe('diffing queries against the store', () => {
query: unionQuery,
});

expect(complete).toBe(false);
expect(complete).toBe(true);
});

it('throws an error on a query with fields missing from matching named fragments', () => {
Expand Down
10 changes: 8 additions & 2 deletions src/cache/inmemory/__tests__/entityStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1199,10 +1199,16 @@ describe('EntityStore', () => {

expect(() => cache.readQuery({
query: queryWithAliases,
})).toThrow(/Can't find field a on object/);
})).toThrow(
"Attempted to match fragment Rest with type condition ABCs against " +
"object with unknown __typename"
);

expect(() => cache.readQuery({
query: queryWithoutAliases,
})).toThrow(/Can't find field a on object/);
})).toThrow(
"Attempted to match fragment Rest with type condition ABCs against " +
"object with unknown __typename"
);
});
});
27 changes: 18 additions & 9 deletions src/cache/inmemory/__tests__/roundtrip.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,12 @@ function assertDeeplyFrozen(value: any, stack: any[] = []) {
}

function storeRoundtrip(query: DocumentNode, result: any, variables = {}) {
const policies = new Policies();
const policies = new Policies({
possibleTypes: {
Character: ["Jedi", "Droid"],
},
});

const reader = new StoreReader({ policies });
const writer = new StoreWriter({ policies });

Expand Down Expand Up @@ -303,8 +308,8 @@ describe('roundtrip', () => {
);
});

it('should resolve on union types with inline fragments without typenames with warning', () => {
return withWarning(() => {
it('should error on union types with inline fragments without typenames', () => {
return expect(() => {
storeRoundtrip(
gql`
query {
Expand Down Expand Up @@ -332,13 +337,16 @@ describe('roundtrip', () => {
],
},
);
});
}).toThrowError(
'Attempted to match fragment with type condition Jedi against ' +
'object with unknown __typename'
);
});

// 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 expect(() => {
return withWarning(() => expect(() => {
storeRoundtrip(
gql`
query {
Expand All @@ -364,7 +372,7 @@ describe('roundtrip', () => {
],
},
);
}).toThrowError(/Can\'t find field rank on object/);
}).toThrowError(/Can\'t find field rank on object/));
});

it('should resolve fields it can on interface with non matching inline fragments', () => {
Expand Down Expand Up @@ -469,6 +477,7 @@ describe('roundtrip', () => {
__typename: 'Droid',
name: 'R2D2',
model: 'astromech',
side: 'bright',
},
],
},
Expand All @@ -477,7 +486,7 @@ describe('roundtrip', () => {
});

it('should throw on error on two of the same spread fragment types', () => {
expect(() =>
withWarning(() => expect(() => {
storeRoundtrip(
gql`
fragment jediSide on Jedi {
Expand Down Expand Up @@ -506,8 +515,8 @@ describe('roundtrip', () => {
},
],
},
),
).toThrowError(/Can\'t find field rank on object/);
);
}).toThrowError(/Can\'t find field rank on object/));
});

it('should resolve on @include and @skip with inline fragments', () => {
Expand Down
6 changes: 3 additions & 3 deletions src/cache/inmemory/__tests__/writeToStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1546,7 +1546,7 @@ describe('writing to the store', () => {
expect(newStore.get('1')).toEqual(result.todos[0]);
});

it('should warn when it receives the wrong data with non-union fragments (using an heuristic matcher)', () => {
it('should warn when it receives the wrong data with non-union fragments', () => {
const result = {
todos: [
{
Expand All @@ -1573,7 +1573,7 @@ describe('writing to the store', () => {
}, /Missing field description/);
});

it('should warn when it receives the wrong data inside a fragment (using an introspection matcher)', () => {
it('should warn when it receives the wrong data inside a fragment', () => {
const queryWithInterface = gql`
query {
todos {
Expand Down Expand Up @@ -1627,7 +1627,7 @@ describe('writing to the store', () => {
}, /Missing field price/);
});

it('should warn if a result is missing __typename when required (using an heuristic matcher)', () => {
it('should warn if a result is missing __typename when required', () => {
const result: any = {
todos: [
{
Expand Down
15 changes: 10 additions & 5 deletions src/cache/inmemory/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -377,10 +377,18 @@ export class Policies {
public fragmentMatches(
fragment: InlineFragmentNode | FragmentDefinitionNode,
typename: string,
): boolean | "heuristic" {
): boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super happy to see this just return a boolean.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Words like "truthy" and "heuristic" belong in the same circle of word-hell.

if (!fragment.typeCondition) return true;

const supertype = fragment.typeCondition.name.value;

invariant(
typename,
`Attempted to match fragment ${
fragment.kind === "InlineFragment" ? "" : fragment.name.value + " "
}with type condition ${supertype} against object with unknown __typename`,
);

if (typename === supertype) return true;

if (this.usingPossibleTypes) {
Expand All @@ -399,12 +407,9 @@ export class Policies {
});
}
}
// When possibleTypes is defined, we always either return true from the
// loop above or return false here (never 'heuristic' below).
return false;
}

return "heuristic";
return false;
}

public getStoreFieldName(
Expand Down
44 changes: 14 additions & 30 deletions src/cache/inmemory/readFromStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
SelectionSetNode,
} from 'graphql';
import { wrap } from 'optimism';
import { invariant } from 'ts-invariant';
import { invariant, InvariantError } from 'ts-invariant';

import {
isField,
Expand Down Expand Up @@ -51,7 +51,6 @@ interface ExecContext {
export type ExecResultMissingField = {
object: StoreObject;
fieldName: string;
tolerable: boolean;
};

export type ExecResult<R = any> = {
Expand Down Expand Up @@ -178,14 +177,11 @@ export class StoreReader {

if (hasMissingFields && ! returnPartialData) {
execResult.missing!.forEach(info => {
invariant(
info.tolerable,
`Can't find field ${info.fieldName} on object ${JSON.stringify(
info.object,
null,
2,
)}.`,
);
throw new InvariantError(`Can't find field ${
info.fieldName
} on object ${
JSON.stringify(info.object, null, 2)
}.`);
});
}

Expand Down Expand Up @@ -244,7 +240,6 @@ export class StoreReader {
getMissing().push({
object: objectOrReference as StoreObject,
fieldName: selection.name.value,
tolerable: false,
});

} else if (Array.isArray(fieldValue)) {
Expand Down Expand Up @@ -298,25 +293,14 @@ export class StoreReader {
);
}

const match = policies.fragmentMatches(fragment, typename);

if (match) {
let fragmentExecResult = this.executeSelectionSet({
selectionSet: fragment.selectionSet,
objectOrReference,
context,
});

if (match === 'heuristic' && fragmentExecResult.missing) {
fragmentExecResult = {
...fragmentExecResult,
missing: fragmentExecResult.missing.map(info => {
return { ...info, tolerable: true };
}),
};
}

objectsToMerge.push(handleMissing(fragmentExecResult));
if (policies.fragmentMatches(fragment, typename)) {
objectsToMerge.push(handleMissing(
this.executeSelectionSet({
selectionSet: fragment.selectionSet,
objectOrReference,
context,
})
));
}
}
});
Expand Down
18 changes: 14 additions & 4 deletions src/cache/inmemory/writeToStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,20 @@ export class StoreWriter {
if (sets.indexOf(selectionSet) >= 0) return store;
sets.push(selectionSet);

const typename =
// If the result has a __typename, trust that.
getTypenameFromResult(result, selectionSet, context.fragmentMap) ||
// If the entity identified by dataId has a __typename in the store,
// fall back to that.
store.getFieldValue(dataId, "__typename") as string ||
// Special dataIds like ROOT_QUERY have a known default __typename.
this.policies.rootTypenamesById[dataId];

const processed = this.processSelectionSet({
result,
selectionSet,
context,
typename: this.policies.rootTypenamesById[dataId],
typename,
});

if (processed.mergeOverrides) {
Expand All @@ -165,14 +174,13 @@ export class StoreWriter {
selectionSet,
context,
mergeOverrides,
typename = getTypenameFromResult(
result, selectionSet, context.fragmentMap),
typename,
}: {
result: Record<string, any>;
selectionSet: SelectionSetNode;
context: WriteContext;
mergeOverrides?: MergeOverrides;
typename?: string;
typename: string;
}): {
result: StoreObject;
mergeOverrides?: MergeOverrides;
Expand Down Expand Up @@ -328,6 +336,8 @@ export class StoreWriter {
result: value,
selectionSet: field.selectionSet,
context,
typename: getTypenameFromResult(
value, field.selectionSet, context.fragmentMap),
});
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/utilities/graphql/storeUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,10 @@ export function getTypenameFromResult(
selectionSet: SelectionSetNode,
fragmentMap: FragmentMap,
): string | undefined {
if (typeof result.__typename === 'string') {
return result.__typename;
}

for (const selection of selectionSet.selections) {
if (isField(selection)) {
if (selection.name.value === '__typename') {
Expand All @@ -281,10 +285,6 @@ export function getTypenameFromResult(
}
}
}
// TODO Move this first?
if (typeof result.__typename === 'string') {
return result.__typename;
}
}

export function isField(selection: SelectionNode): selection is FieldNode {
Expand Down