Skip to content
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

Jump to bottom

various setContext bug fixes #3017

Merged
merged 5 commits into from
May 29, 2024
Merged

various setContext bug fixes #3017

Show file tree
Hide file tree
Changes from 1 commit
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
8 changes: 3 additions & 5 deletions internals-js/src/federation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,13 +414,11 @@
const { element, selectionSet: childSelectionSet } = selection;
assert(element.definition.type, 'Element type definition should exist');
let type = element.definition.type;
if (type.kind === 'NonNullType') {
type = type.ofType;
}

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');

Check warning on line 419 in internals-js/src/federation.ts

View check run for this annotation

Codecov / codecov/patch

internals-js/src/federation.ts#L419 

Added line #L419 was not covered by tests
const { resolvedType } = validateFieldValueType({
currentType: type,
currentType: baseType(type) as CompositeType,
selectionSet: childSelectionSet,
errorCollector,
metadata,
Expand Down
4 changes: 3 additions & 1 deletion internals-js/src/operations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,13 +376,15 @@
// 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);

Check warning on line 379 in internals-js/src/operations.ts

View check run for this annotation

Codecov / codecov/patch

internals-js/src/operations.ts#L379 

Added line #L379 was not covered by tests
if (fieldInParent && fieldInParent.arguments().some(arg => arg.appliedDirectivesOf(fromContextDirective).length > 0)) {
if (fieldInParent && fieldInParent.arguments()
.some(arg => arg.appliedDirectivesOf(fromContextDirective).length > 0 && (!this.args || this.args[arg.name] === undefined))
Copy link
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?

Copy link
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;

Check warning on line 383 in internals-js/src/operations.ts

View check run for this annotation

Codecov / codecov/patch

internals-js/src/operations.ts#L383 

Added line #L383 was not covered by tests
}
}

return returnType;

Check warning on line 387 in internals-js/src/operations.ts

View check run for this annotation

Codecov / codecov/patch

internals-js/src/operations.ts#L387 

Added line #L387 was not covered by tests
}

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 @@
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;

Check warning on line 660 in query-graphs-js/src/graphPath.ts

View check run for this annotation

Codecov / codecov/patch

query-graphs-js/src/graphPath.ts#L660 

Added line #L660 was not covered by tests
}

// 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