-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Prevent incorrect result merging when aliasing __typename
or id
.
#10789
Changes from 8 commits
a3facf1
4377c08
5151eca
04f6be4
4fa9b16
46954d7
b3b1f1b
bf61045
6cf2128
f27932d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@apollo/client": patch | ||
--- | ||
|
||
Fix a bug where other fields could be aliased to `__typename` or `id`, in which case an incoming result would be merged into the wrong cache entry. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -289,16 +289,18 @@ 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') { | ||
return result[resultKeyNameFromField(selection)]; | ||
} | ||
} else { | ||
} | ||
} | ||
if (typeof result.__typename === 'string') { | ||
return result.__typename; | ||
} | ||
for (const selection of selectionSet.selections) { | ||
if (!isField(selection)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a feeling Instead of iterating over the selections again here, here's one way to avoid the whole second loop when there are no fragments in a given selection set: diff --git a/src/utilities/graphql/storeUtils.ts b/src/utilities/graphql/storeUtils.ts
index f995b41d9..f2ddec155 100644
--- a/src/utilities/graphql/storeUtils.ts
+++ b/src/utilities/graphql/storeUtils.ts
@@ -18,6 +18,7 @@ import {
NameNode,
SelectionSetNode,
DocumentNode,
+ FragmentSpreadNode,
} from 'graphql';
import { isNonNullObject } from '../common/objects';
@@ -289,18 +290,26 @@ export function getTypenameFromResult(
selectionSet: SelectionSetNode,
fragmentMap?: FragmentMap,
): string | undefined {
+ let fragments: undefined | Array<InlineFragmentNode | FragmentSpreadNode>;
+
for (const selection of selectionSet.selections) {
if (isField(selection)) {
if (selection.name.value === '__typename') {
return result[resultKeyNameFromField(selection)];
}
+ } else if (fragments) {
+ fragments.push(selection);
+ } else {
+ fragments = [selection];
}
}
+
if (typeof result.__typename === 'string') {
return result.__typename;
}
- for (const selection of selectionSet.selections) {
- if (!isField(selection)) {
+
+ if (fragments) {
+ for (const selection of fragments) {
const typename = getTypenameFromResult(
result,
getFragmentFromSelection(selection, fragmentMap)!.selectionSet, |
||
const typename = getTypenameFromResult( | ||
result, | ||
getFragmentFromSelection(selection, fragmentMap)!.selectionSet, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch @phryneas!
For future reference, I think this should have been
keyFn(storeObject, context)
from the beginning, but our test suite has a few test cases where properties of a result object were important (likeid
) even if not mentioned as fields in the query document, sokeyFn({...object, ...storeObject}, context)
ensures backwards compatibility for that edge case, while always preferringstoreObject
properties overobject
properties.