Skip to content

Commit

Permalink
First crack at fixing the overfetching problem. This breaks some test…
Browse files Browse the repository at this point in the history 
…s that I need to go back and review to see if any are real problems. Also, I think that the initial fetch of `oldKey` in the added test may need to include __typename

Refs #2759
  • Loading branch information
@clenfest
clenfest committed Nov 30, 2023
1 parent eba1008 commit a46ec90
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 72 deletions.
24 changes: 0 additions & 24 deletions internals-js/src/operations.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2041,30 +2041,6 @@ export class SelectionSet {
return fragmentsDefinitions + rootKind + nameAndVariables + " " + this.toString(expandFragments, true, indent);
}

typeConditionForSelections(): CompositeType | undefined {
const selections = this.selections();
const first = selections[0];
if (!first || first.kind !== 'FragmentSelection' || !first.element.typeCondition) {
return undefined;
}
const typeCondition = first.element.typeCondition
return selections.slice(1).every(s => {
if (s.kind === 'FragmentSelection') {
return typeCondition === s.element.typeCondition;
}
return false;
}) ? typeCondition : undefined;
}

allSelectionsHaveTypeCondition(): boolean {
return this.selections().every((s) => {
if (s.kind === 'FragmentSelection') {
return !!s.element.typeCondition;
}
return false;
});
}

/**
* The string representation of this selection set.
*
Expand Down
28 changes: 14 additions & 14 deletions query-planner-js/src/__tests__/buildPlan.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3907,7 +3907,7 @@ describe('Named fragments preservation', () => {
}
}
}
fragment FooChildSelect on Foo {
__typename
foo
Expand All @@ -3922,7 +3922,7 @@ describe('Named fragments preservation', () => {
}
}
}
fragment FooSelect on Foo {
__typename
foo
Expand Down Expand Up @@ -4096,7 +4096,7 @@ describe('Named fragments preservation', () => {
}
}
}
fragment OnV on V {
a
b
Expand Down Expand Up @@ -4170,7 +4170,7 @@ describe('Named fragments preservation', () => {
}
}
}
fragment Selection on A {
x
y
Expand Down Expand Up @@ -4264,7 +4264,7 @@ describe('Named fragments preservation', () => {
}
}
}
fragment OnV on V {
v1
v2
Expand Down Expand Up @@ -4372,7 +4372,7 @@ describe('Named fragments preservation', () => {
...OnT @include(if: $test2)
}
}
fragment OnT on T {
a
b
Expand Down Expand Up @@ -4572,7 +4572,7 @@ describe('Named fragments preservation', () => {
id
}
}
fragment OuterFrag on Outer {
inner {
v {
Expand Down Expand Up @@ -4711,7 +4711,7 @@ describe('Named fragments preservation', () => {
id
}
}
fragment OuterFrag on Outer {
w
inner {
Expand Down Expand Up @@ -4852,7 +4852,7 @@ describe('Named fragments preservation', () => {
id
}
}
fragment OuterFrag on Outer {
inner {
v
Expand Down Expand Up @@ -4991,7 +4991,7 @@ describe('Named fragments preservation', () => {
id
}
}
fragment OuterFrag on Outer {
w
inner {
Expand Down Expand Up @@ -6815,7 +6815,7 @@ describe('named fragments', () => {
}
}
}
fragment Fragment4 on I {
__typename
id1
Expand Down Expand Up @@ -6888,7 +6888,7 @@ describe('named fragments', () => {
}
}
}
fragment Fragment4 on I {
id1
id2
Expand Down Expand Up @@ -7030,7 +7030,7 @@ describe('named fragments', () => {
id
}
}
fragment allTFields on T {
v0
v1
Expand Down Expand Up @@ -7178,7 +7178,7 @@ describe('named fragments', () => {
}
}
}
fragment allUFields on U {
v0
v1
Expand Down
44 changes: 10 additions & 34 deletions query-planner-js/src/buildPlan.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -931,7 +931,6 @@ class FetchGroup {
cachedCost?: number;
isKnownUseful?: boolean;
unMergeable?: boolean;
readonly isKeyFetchGroup?: boolean;
isConditionsGroup: boolean;

private constructor({
Expand All @@ -952,7 +951,6 @@ class FetchGroup {
// Cache used to save unecessary recomputation of the `isUseless` method.
isKnownUseful = false,
unMergeable = false,
isKeyFetchGroup = false,
}: {
dependencyGraph: FetchDependencyGraph;
index: number;
Expand All @@ -968,7 +966,6 @@ class FetchGroup {
cachedCost?: number;
isKnownUseful?: boolean;
unMergeable?: boolean;
isKeyFetchGroup?: boolean;
}) {
this.dependencyGraph = dependencyGraph;
this.index = index;
Expand All @@ -984,7 +981,6 @@ class FetchGroup {
this.cachedCost = cachedCost;
this.isKnownUseful = isKnownUseful;
this.unMergeable = unMergeable;
this.isKeyFetchGroup = isKeyFetchGroup;
this.isConditionsGroup = false;

if (this._inputs) {
Expand All @@ -1010,7 +1006,6 @@ class FetchGroup {
hasInputs,
mergeAt,
deferRef,
isKeyFetchGroup,
}: {
dependencyGraph: FetchDependencyGraph,
index: number,
Expand All @@ -1020,7 +1015,6 @@ class FetchGroup {
hasInputs: boolean,
mergeAt?: ResponsePath,
deferRef?: string,
isKeyFetchGroup: boolean,
}): FetchGroup {
// Sanity checks that the selection parent type belongs to the schema of the subgraph we're querying.
assert(parentType.schema() === dependencyGraph.subgraphSchemas.get(subgraphName), `Expected parent type ${parentType} to belong to ${subgraphName}`);
Expand All @@ -1036,7 +1030,6 @@ class FetchGroup {
mergeAt,
deferRef,
subgraphAndMergeAtKey: hasInputs ? `${toValidGraphQLName(subgraphName)}-${mergeAt?.join('::') ?? ''}` : undefined,
isKeyFetchGroup,
});
}

Expand All @@ -1056,7 +1049,7 @@ class FetchGroup {
subgraphAndMergeAtKey: this.subgraphAndMergeAtKey,
cachedCost: this.cachedCost,
isKnownUseful: this.isKnownUseful,
isKeyFetchGroup: this.isKeyFetchGroup, //TODO: a couple of fields were missing. Was that intentional?
//TODO: a couple of fields were missing. Was that intentional?
});
}

Expand Down Expand Up @@ -2247,15 +2240,13 @@ class FetchDependencyGraph {
rootKind, // always "query" for entity fetches
mergeAt,
deferRef,
isKeyFetchGroup = false,
}: {
subgraphName: string,
parentType: CompositeType,
hasInputs: boolean,
rootKind: SchemaRootKind,
mergeAt?: ResponsePath,
deferRef?: string,
isKeyFetchGroup?: boolean,
}): FetchGroup {
this.onModification();
const newGroup = FetchGroup.create({
Expand All @@ -2267,7 +2258,6 @@ class FetchDependencyGraph {
hasInputs,
mergeAt,
deferRef,
isKeyFetchGroup,
});
this.groups.push(newGroup);
return newGroup;
Expand Down Expand Up @@ -2376,16 +2366,14 @@ class FetchDependencyGraph {
const parentType = this.federationMetadata(subgraphName).entityType();
assert(parentType, () => `Subgraph ${subgraphName} has no entities defined`);

const fg = this.newFetchGroup({
return this.newFetchGroup({
subgraphName,
parentType,
hasInputs: true,
rootKind: 'query',
mergeAt,
deferRef,
isKeyFetchGroup: true,
});
return fg;
}

remove(toRemove: FetchGroup) {
Expand Down Expand Up @@ -3962,15 +3950,13 @@ function computeGroupsForTree({
initialGroupPath,
initialDeferContext,
initialContext = emptyContext,
level = 0,
}: {
dependencyGraph: FetchDependencyGraph,
pathTree: OpPathTree<any>,
startGroup: FetchGroup,
initialGroupPath: GroupPath,
initialDeferContext: DeferContext,
initialContext?: PathContext,
level?: number,
}): FetchGroup[] {
const stack: {
tree: OpPathTree,
Expand All @@ -3985,16 +3971,15 @@ function computeGroupsForTree({
context: initialContext,
deferContext: initialDeferContext,
}];
let iter = 0;
const createdGroups: FetchGroup[] = [ ];
let seenUnion = false;
while (stack.length > 0) {
const { tree, group, path, context, deferContext } = stack.pop()!;
seenUnion ||= tree.vertex.type.kind === 'UnionType';
console.log('seenUnion', seenUnion);
console.log(" ".repeat(level*4), 'tree', level, ++iter, tree.toString().replace(/\n/g, ", "));
console.log('');
console.log(" ".repeat(level*4), 'groups', iter, createdGroups.map(g => g.toString()));
// console.log('seenUnion', seenUnion);
// console.log(" ".repeat(level*4), 'tree', level, ++iter, tree.toString().replace(/\n/g, ", "));
// console.log('');
// console.log(" ".repeat(level*4), 'groups', iter, createdGroups.map(g => g.toString()));
if (tree.localSelections) {
for (const selection of tree.localSelections) {
group.addAtPath(path.inGroup(), selection);
Expand All @@ -4007,10 +3992,7 @@ function computeGroupsForTree({
} else {
// We want to preserve the order of the elements in the child, but the stack will reverse everything, so we iterate
// in reverse order to counter-balance it.
const childElements = Array.from(tree.childElements(true));
// const allChildrenHaveTypeCondition = childElements.every(([_edge, _operation, conditions]) => conditions !== null) && childElements.length > 1;
// console.log('allChildrenHaveTypeCondition', allChildrenHaveTypeCondition);
for (const [edge, operation, conditions, child] of childElements) {
for (const [edge, operation, conditions, child] of tree.childElements(true)) {
if (isPathContext(operation)) {
const newContext = operation;
// The only 3 cases where we can take edge not "driven" by an operation is either when we resolve a key, resolve
Expand All @@ -4026,7 +4008,6 @@ function computeGroupsForTree({
startGroup: group,
initialGroupPath: path,
initialDeferContext: deferContextForConditions(deferContext),
level: level + 1,
});
conditionsGroups.forEach(g => {
g.isConditionsGroup = true;
Expand Down Expand Up @@ -4057,11 +4038,10 @@ function computeGroupsForTree({
conditionsGroups,
deferRef: updatedDeferContext.activeDeferRef,
});

updateCreatedGroups(createdGroups, newGroup);

// create a reusable inner function to add parents to the new group
const doAddParents = (g: FetchGroup) => {
const addParentsOnGroup = (g: FetchGroup) => {
g.addParents(conditionsGroups.map((conditionGroup) => {
// If `conditionGroup` parent is `group`, that is the same as `newGroup` current parent, then we can infer the path of `newGroup` into
// that condition `group` by looking at the paths of each to their common parent. But otherwise, we cannot have a proper
Expand All @@ -4075,12 +4055,12 @@ function computeGroupsForTree({
}));
};

doAddParents(newGroup);
addParentsOnGroup(newGroup);
if (seenUnion) {
newGroup.unMergeable = true;

createdGroups.filter(g => g.unMergeable).forEach(g => {
doAddParents(g);
addParentsOnGroup(g);
});
}

Expand Down Expand Up @@ -4215,7 +4195,6 @@ function computeGroupsForTree({
path,
context,
updatedDeferContext,
level,
);
updated.group = requireResult.group;
updated.path = requireResult.path;
Expand Down Expand Up @@ -4414,7 +4393,6 @@ function handleRequires(
path: GroupPath,
context: PathContext,
deferContext: DeferContext,
level: number = 0,
): {
group: FetchGroup,
path: GroupPath,
Expand Down Expand Up @@ -4463,7 +4441,6 @@ function handleRequires(
startGroup: newGroup,
initialGroupPath: path,
initialDeferContext: deferContextForConditions(deferContext),
level: level + 1,
});
if (createdGroups.length === 0) {
// All conditions were local. Just merge the newly created group back in the current group (we didn't need it)
Expand Down Expand Up @@ -4622,7 +4599,6 @@ function handleRequires(
startGroup: group,
initialGroupPath: path,
initialDeferContext: deferContextForConditions(deferContext),
level: level + 1,
});
// If we didn't created any group, that means the whole condition was fetched from the current group
// and we're good.
Expand Down

0 comments on commit a46ec90

Please sign in to comment.