Skip to content

Commit

Permalink
Fully eliminate the concept of heuristic fragment matching.
Browse files Browse the repository at this point in the history
Although we removed the FragmentMatcher abstraction in PR #5073, and the
HeuristicFragmentMatcher no longer exists, we kept trying to match
fragments whenever all their fields matched, even if the fragment type
condition was not satisfied by the __typename of the object in question.

I recently came to appreciate just how harmful this best-effort matching
can be, when I realized that it means we call executeSelectionSet (on the
reading side) and processSelectionSet (on the writing side) for every
unmatched fragment, and then just throw away the result if any fields are
missing, but not before recording those fields as missing in a weaker way
than usual, using the ExecResultMissingField.tolerable boolean.

Not only is this strategy potentially costly for queries with lots of
fragments that really should not match, it also only ever made sense on
the writing side, where the fields of the fragment can be compared to the
fields of an incoming result object, so the presence of all the fragment's
fields is a pretty good indication that the fragment matched. On the
reading side, we're comparing the fields of the fragment to all the fields
we've written into the cache so far, which means heuristic fragment
matching is sensitive to the whole history of cache writes for the entity
in question, not just the current query.

We could eliminate heuristic fragment matching just for reading, but then
we'd have to tell people to pass possibleTypes to the InMemoryCache
constructor (the correct, non-heuristic solution), which would make
heuristic fragment matching unnecessary on the writing side, too, so we
might as well completely eliminate heuristic matching altogether.
  • Loading branch information
benjamn committed Dec 13, 2019
1 parent ab9d53e commit 6cfa9af
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 29 deletions.
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
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
7 changes: 2 additions & 5 deletions src/cache/inmemory/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ export class Policies {
public fragmentMatches(
fragment: InlineFragmentNode | FragmentDefinitionNode,
typename: string,
): boolean | "heuristic" {
): boolean {
if (!fragment.typeCondition) return true;

const supertype = fragment.typeCondition.name.value;
Expand All @@ -399,12 +399,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
27 changes: 8 additions & 19 deletions src/cache/inmemory/readFromStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -298,25 +298,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

0 comments on commit 6cfa9af

Please sign in to comment.