Skip to content

various setContext bug fixes #3017

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.

Sign up for GitHub

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

Merged
clenfest merged 5 commits into from
May 29, 2024
Merged

various setContext bug fixes #3017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fe49571
Some bugfixes for setContext
clenfest May 23, 2024
e492f50
add changeset
clenfest May 23, 2024
d667dba
Merge branch 'next' into clenfest/set_context_bugfixes
clenfest May 23, 2024
dcd8a88
adding a testcase for hopping back and forth between subgraphs
clenfest May 24, 2024
70a97b0
Updates based on code review comments
clenfest May 29, 2024
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
11 changes: 11 additions & 0 deletions .changeset/popular-chairs-happen.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
"apollo-federation-integration-testsuite": patch
"@apollo/query-planner": patch
"@apollo/query-graphs": patch
"@apollo/composition": patch
"@apollo/federation-internals": patch
"@apollo/subgraph": patch
"@apollo/gateway": patch
---

Various set context bugfixes
6 changes: 3 additions & 3 deletions composition-js/src/merging/merge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1661,11 +1661,11 @@ class Merger {
return true;
}

// if there is a @fromContext directive on one of the sources, we need a join__field
// if there is a @fromContext directive on one of the source's arguments, we need a join__field
if (sources.some((s, idx) => {
const fromContextDirective = this.subgraphs.values()[idx].metadata().fromContextDirective();
if (isFederationDirectiveDefinedInSchema(fromContextDirective)) {
return (s?.appliedDirectivesOf(fromContextDirective).length ?? 0) > 0;
if (s && isFederationDirectiveDefinedInSchema(fromContextDirective)) {
return s.kind === 'FieldDefinition' && s.arguments().some(arg => arg.appliedDirectivesOf(fromContextDirective).length > 0);
}
return false;
})) {
Expand Down
7 changes: 4 additions & 3 deletions internals-js/src/federation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,11 +413,12 @@ const validateFieldValueType = ({
}
const { element, selectionSet: childSelectionSet } = selection;
assert(element.definition.type, 'Element type definition should exist');
const type = element.definition.type;
let type = element.definition.type;

if (childSelectionSet) {
assert(isCompositeType(type), 'Child selection sets should only exist on composite types');
assert(isCompositeType(baseType(type)), 'Child selection sets should only exist on composite types');
const { resolvedType } = validateFieldValueType({
currentType: type,
currentType: baseType(type) as CompositeType,
selectionSet: childSelectionSet,
errorCollector,
metadata,
Expand Down
17 changes: 15 additions & 2 deletions internals-js/src/operations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,10 +368,23 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
if (this.name === typenameFieldName) {
return parentType.typenameField()?.type;
}

return this.canRebaseOn(parentType)
const returnType = this.canRebaseOn(parentType)
? parentType.field(this.name)?.type
: undefined;

// If the field has an argument with fromContextDirective on it. We should not rebase it.
const fromContextDirective = federationMetadata(parentType.schema())?.fromContextDirective();
if (fromContextDirective && isFederationDirectiveDefinedInSchema(fromContextDirective)) {
const fieldInParent = parentType.field(this.name);
if (fieldInParent && fieldInParent.arguments()
.some(arg => arg.appliedDirectivesOf(fromContextDirective).length > 0 && (!this.args || this.args[arg.name] === undefined))

@clenfest clenfest May 29, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually did I get this right?

@sachindshinde sachindshinde May 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks right to me, it's saying that if there's a contextual argument, but there are no args (or there are args, but there's no entry for the contextual argument's name), then you can't rebase. Effectively it's saying that a required argument would be missing if the rebase were performed.

) {
return undefined;
}
}

return returnType;
}

hasDefer(): boolean {
Expand Down
6 changes: 6 additions & 0 deletions query-graphs-js/src/graphPath.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,12 @@ export class GraphPath<TTrigger, RV extends Vertex = Vertex, TNullEdge extends n
if (!enteringEdge) {
return undefined;
}

// TODO: Temporary fix to avoid optimization if context exists.
// permanent fix is described here: https://github.com/apollographql/federation/pull/3017#pullrequestreview-2083949094
if (this.graph.subgraphToArgs.size > 0) {
return undefined;
}

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