Skip to content

Commit a9ed9ad

Browse files
clenfestMeschreiber
authored andcommitted
various setContext bug fixes (#3017)
Some setContext bug fixes - needsJoinDirective() logic is incorrect. We need to make sure that we add a join field when @fromDirective exists on the arguments, not the definition - Query plans were incorrect if type was entirely in one subgraph. selectionIsFullyLocalFromAllVertices was calling SelectionSet.canRebaseOn so we fixed to return false if the selection contained a field with a contextual argument - For top level queries, we don't want to have "... on Query" in the rewrite path. - Fixed up selectionSetAsKeyRenamers() logic
1 parent d5fc6af commit a9ed9ad

7 files changed

Lines changed: 445 additions & 21 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
"apollo-federation-integration-testsuite": patch
3+
"@apollo/query-planner": patch
4+
"@apollo/query-graphs": patch
5+
"@apollo/composition": patch
6+
"@apollo/federation-internals": patch
7+
"@apollo/subgraph": patch
8+
"@apollo/gateway": patch
9+
---
10+
11+
Various set context bugfixes

composition-js/src/merging/merge.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1661,11 +1661,11 @@ class Merger {
16611661
return true;
16621662
}
16631663

1664-
// if there is a @fromContext directive on one of the sources, we need a join__field
1664+
// if there is a @fromContext directive on one of the source's arguments, we need a join__field
16651665
if (sources.some((s, idx) => {
16661666
const fromContextDirective = this.subgraphs.values()[idx].metadata().fromContextDirective();
1667-
if (isFederationDirectiveDefinedInSchema(fromContextDirective)) {
1668-
return (s?.appliedDirectivesOf(fromContextDirective).length ?? 0) > 0;
1667+
if (s && isFederationDirectiveDefinedInSchema(fromContextDirective)) {
1668+
return s.kind === 'FieldDefinition' && s.arguments().some(arg => arg.appliedDirectivesOf(fromContextDirective).length > 0);
16691669
}
16701670
return false;
16711671
})) {

internals-js/src/federation.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -413,11 +413,12 @@ const validateFieldValueType = ({
413413
}
414414
const { element, selectionSet: childSelectionSet } = selection;
415415
assert(element.definition.type, 'Element type definition should exist');
416-
const type = element.definition.type;
416+
let type = element.definition.type;
417+
417418
if (childSelectionSet) {
418-
assert(isCompositeType(type), 'Child selection sets should only exist on composite types');
419+
assert(isCompositeType(baseType(type)), 'Child selection sets should only exist on composite types');
419420
const { resolvedType } = validateFieldValueType({
420-
currentType: type,
421+
currentType: baseType(type) as CompositeType,
421422
selectionSet: childSelectionSet,
422423
errorCollector,
423424
metadata,

internals-js/src/operations.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -368,10 +368,23 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
368368
if (this.name === typenameFieldName) {
369369
return parentType.typenameField()?.type;
370370
}
371-
372-
return this.canRebaseOn(parentType)
371+
372+
const returnType = this.canRebaseOn(parentType)
373373
? parentType.field(this.name)?.type
374374
: undefined;
375+
376+
// If the field has an argument with fromContextDirective on it. We should not rebase it.
377+
const fromContextDirective = federationMetadata(parentType.schema())?.fromContextDirective();
378+
if (fromContextDirective && isFederationDirectiveDefinedInSchema(fromContextDirective)) {
379+
const fieldInParent = parentType.field(this.name);
380+
if (fieldInParent && fieldInParent.arguments()
381+
.some(arg => arg.appliedDirectivesOf(fromContextDirective).length > 0 && (!this.args || this.args[arg.name] === undefined))
382+
) {
383+
return undefined;
384+
}
385+
}
386+
387+
return returnType;
375388
}
376389

377390
hasDefer(): boolean {

query-graphs-js/src/graphPath.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -653,6 +653,12 @@ export class GraphPath<TTrigger, RV extends Vertex = Vertex, TNullEdge extends n
653653
if (!enteringEdge) {
654654
return undefined;
655655
}
656+
657+
// TODO: Temporary fix to avoid optimization if context exists.
658+
// permanent fix is described here: https://github.com/apollographql/federation/pull/3017#pullrequestreview-2083949094
659+
if (this.graph.subgraphToArgs.size > 0) {
660+
return undefined;
661+
}
656662

657663
// Usually, the starting subgraph in which we want to look for a direct path is the head of
658664
// `subgraphEnteringEdge`, that is, where we were just before coming to the current subgraph.

0 commit comments

Comments
 (0)