various setContext bug fixes #3017

clenfest · 2024-05-23T17:09:32Z

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

- 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

changeset-bot · 2024-05-23T17:09:35Z

🦋 Changeset detected

Latest commit: 70a97b0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
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

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codesandbox-ci · 2024-05-23T17:10:21Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

sachindshinde

A few comments below. The last comment isn't really a change request, but more just making sure I understand selectionSetAsKeyRenamers().

One other thing to note (and this can be handled in a follow-up PR), but there's a bug in GraphPath.checkDirectPathFromPreviousSubgraphTo(). Specifically, that function is used to check a hypothetical, mainly whether if part of the graph path could instead be successfully planned "directly" against a different starting vertex (prevSubgraphStartingVertex). Really what this is checking for, is that if we can easily tell that planning would work, we return back where this direct graph path would now lead instead (if we can't tell whether it would work e.g. because of a @requires condition, or if we know it wouldn't work, then we return undefined).

The gist is that there's a for loop in that function that iterates through this.props.edgeTriggers. If any of those triggers returns a non-null composite type for getFieldParentType() (which you'll have to pass through from checkDirectPathFromPreviousSubgraphTo()'s single callsite), and that composite type has @context on it in at least one subgraph (you may need to pre-compute a list of such types during query graph creation, and also it'll help to pull type-matching logic from canSatisfyConditions() into a new function to reduce duplication), then you'll want to return undefined from checkDirectPathFromPreviousSubgraphTo().

internals-js/src/federation.ts

internals-js/src/operations.ts

query-planner-js/src/buildPlan.ts

sachindshinde · 2024-05-29T00:08:11Z

As a stopgap fix for checkDirectPathFromPreviousSubgraphTo(), you could also just disable the optimization when @context is present. The way that would work, is that whenever any subgraph uses contexts at all (which you can check by looking at whether QueryGraph.subgraphToArgs has at least one entry), then checkDirectPathFromPreviousSubgraphTo() should just early return undefined.

clenfest · 2024-05-29T00:23:32Z

internals-js/src/operations.ts

@@ -377,7 +377,9 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
    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)) {
+      if (fieldInParent && fieldInParent.arguments()
+          .some(arg => arg.appliedDirectivesOf(fromContextDirective).length > 0 && (!this.args || this.args[arg.name] === undefined))


Actually did I get this right?

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.

@key

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to next, this PR will be updated. # Releases ## @apollo/composition@2.8.0 ### Minor Changes - Implement new directives to allow getting and setting context. This allows resolvers to reference and access data referenced by entities that exist in the GraphPath that was used to access the field. The following example demonstrates the ability to access the `prop` field within the Child resolver. ([#2988](#2988)) ```graphql type Query { p: Parent! } type Parent @key(fields: "id") @context(name: "context") { id: ID! child: Child! prop: String! } type Child @key(fields: "id") { id: ID! b: String! field(a: String @fromcontext(field: "$context { prop }")): Int! } ``` ### Patch Changes - Various set context bugfixes ([#3017](#3017)) - Updated dependencies \[[`c4744da360235d8bb8270ea048f0e0fa5d03be1e`](c4744da), [`8a936d741a0c05835ff2533714cf330d18209179`](8a936d7), [`f5fe3e74d36722f78004c1e2e03c77d8b95cd6bf`](f5fe3e7)]: - @apollo/query-graphs@2.8.0 - @apollo/federation-internals@2.8.0 ## @apollo/gateway@2.8.0 ### Minor Changes - Implement new directives to allow getting and setting context. This allows resolvers to reference and access data referenced by entities that exist in the GraphPath that was used to access the field. The following example demonstrates the ability to access the `prop` field within the Child resolver. ([#2988](#2988)) ```graphql type Query { p: Parent! } type Parent @key(fields: "id") @context(name: "context") { id: ID! child: Child! prop: String! } type Child @key(fields: "id") { id: ID! b: String! field(a: String @fromcontext(field: "$context { prop }")): Int! } ``` ### Patch Changes - Various set context bugfixes ([#3017](#3017)) - Updated dependencies \[[`c4744da360235d8bb8270ea048f0e0fa5d03be1e`](c4744da), [`8a936d741a0c05835ff2533714cf330d18209179`](8a936d7), [`daf36bd242ba4db0cfcf0e18c1eed235ff0dfaf2`](daf36bd)]: - @apollo/query-planner@2.8.0 - @apollo/composition@2.8.0 - @apollo/federation-internals@2.8.0 ## @apollo/federation-internals@2.8.0 ### Minor Changes - Implement new directives to allow getting and setting context. This allows resolvers to reference and access data referenced by entities that exist in the GraphPath that was used to access the field. The following example demonstrates the ability to access the `prop` field within the Child resolver. ([#2988](#2988)) ```graphql type Query { p: Parent! } type Parent @key(fields: "id") @context(name: "context") { id: ID! child: Child! prop: String! } type Child @key(fields: "id") { id: ID! b: String! field(a: String @fromcontext(field: "$context { prop }")): Int! } ``` ### Patch Changes - Various set context bugfixes ([#3017](#3017)) ## @apollo/query-graphs@2.8.0 ### Minor Changes - Implement new directives to allow getting and setting context. This allows resolvers to reference and access data referenced by entities that exist in the GraphPath that was used to access the field. The following example demonstrates the ability to access the `prop` field within the Child resolver. ([#2988](#2988)) ```graphql type Query { p: Parent! } type Parent @key(fields: "id") @context(name: "context") { id: ID! child: Child! prop: String! } type Child @key(fields: "id") { id: ID! b: String! field(a: String @fromcontext(field: "$context { prop }")): Int! } ``` ### Patch Changes - Various set context bugfixes ([#3017](#3017)) - Fix bug in context-matching logic for interfaces-implementing-interfaces (#3014) ([#3015](#3015)) A field is considered to match a context if the field's parent type (in the original query) either has `@context` on it, or implements/is a member of a type with `@context` on it. We ended up missing the case where interfaces implement interfaces; this PR introduces a fix. - Updated dependencies \[[`c4744da360235d8bb8270ea048f0e0fa5d03be1e`](c4744da), [`8a936d741a0c05835ff2533714cf330d18209179`](8a936d7)]: - @apollo/federation-internals@2.8.0 ## @apollo/query-planner@2.8.0 ### Minor Changes - Implement new directives to allow getting and setting context. This allows resolvers to reference and access data referenced by entities that exist in the GraphPath that was used to access the field. The following example demonstrates the ability to access the `prop` field within the Child resolver. ([#2988](#2988)) ```graphql type Query { p: Parent! } type Parent @key(fields: "id") @context(name: "context") { id: ID! child: Child! prop: String! } type Child @key(fields: "id") { id: ID! b: String! field(a: String @fromcontext(field: "$context { prop }")): Int! } ``` ### Patch Changes - Various set context bugfixes ([#3017](#3017)) - Fix relative path logic when eliding subgraph jumps for `@fromContext` ([#3005](#3005)) - Updated dependencies \[[`c4744da360235d8bb8270ea048f0e0fa5d03be1e`](c4744da), [`8a936d741a0c05835ff2533714cf330d18209179`](8a936d7), [`f5fe3e74d36722f78004c1e2e03c77d8b95cd6bf`](f5fe3e7)]: - @apollo/query-graphs@2.8.0 - @apollo/federation-internals@2.8.0 ## @apollo/subgraph@2.8.0 ### Patch Changes - Various set context bugfixes ([#3017](#3017)) - Updated dependencies \[[`c4744da360235d8bb8270ea048f0e0fa5d03be1e`](c4744da), [`8a936d741a0c05835ff2533714cf330d18209179`](8a936d7)]: - @apollo/federation-internals@2.8.0 ## apollo-federation-integration-testsuite@2.8.0 ### Patch Changes - Various set context bugfixes ([#3017](#3017)) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

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

@key

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to next, this PR will be updated. # Releases ## @apollo/composition@2.8.0 ### Minor Changes - Implement new directives to allow getting and setting context. This allows resolvers to reference and access data referenced by entities that exist in the GraphPath that was used to access the field. The following example demonstrates the ability to access the `prop` field within the Child resolver. ([#2988](#2988)) ```graphql type Query { p: Parent! } type Parent @key(fields: "id") @context(name: "context") { id: ID! child: Child! prop: String! } type Child @key(fields: "id") { id: ID! b: String! field(a: String @fromcontext(field: "$context { prop }")): Int! } ``` ### Patch Changes - Various set context bugfixes ([#3017](#3017)) - Updated dependencies \[[`c4744da360235d8bb8270ea048f0e0fa5d03be1e`](c4744da), [`8a936d741a0c05835ff2533714cf330d18209179`](8a936d7), [`f5fe3e74d36722f78004c1e2e03c77d8b95cd6bf`](f5fe3e7)]: - @apollo/query-graphs@2.8.0 - @apollo/federation-internals@2.8.0 ## @apollo/gateway@2.8.0 ### Minor Changes - Implement new directives to allow getting and setting context. This allows resolvers to reference and access data referenced by entities that exist in the GraphPath that was used to access the field. The following example demonstrates the ability to access the `prop` field within the Child resolver. ([#2988](#2988)) ```graphql type Query { p: Parent! } type Parent @key(fields: "id") @context(name: "context") { id: ID! child: Child! prop: String! } type Child @key(fields: "id") { id: ID! b: String! field(a: String @fromcontext(field: "$context { prop }")): Int! } ``` ### Patch Changes - Various set context bugfixes ([#3017](#3017)) - Updated dependencies \[[`c4744da360235d8bb8270ea048f0e0fa5d03be1e`](c4744da), [`8a936d741a0c05835ff2533714cf330d18209179`](8a936d7), [`daf36bd242ba4db0cfcf0e18c1eed235ff0dfaf2`](daf36bd)]: - @apollo/query-planner@2.8.0 - @apollo/composition@2.8.0 - @apollo/federation-internals@2.8.0 ## @apollo/federation-internals@2.8.0 ### Minor Changes - Implement new directives to allow getting and setting context. This allows resolvers to reference and access data referenced by entities that exist in the GraphPath that was used to access the field. The following example demonstrates the ability to access the `prop` field within the Child resolver. ([#2988](#2988)) ```graphql type Query { p: Parent! } type Parent @key(fields: "id") @context(name: "context") { id: ID! child: Child! prop: String! } type Child @key(fields: "id") { id: ID! b: String! field(a: String @fromcontext(field: "$context { prop }")): Int! } ``` ### Patch Changes - Various set context bugfixes ([#3017](#3017)) ## @apollo/query-graphs@2.8.0 ### Minor Changes - Implement new directives to allow getting and setting context. This allows resolvers to reference and access data referenced by entities that exist in the GraphPath that was used to access the field. The following example demonstrates the ability to access the `prop` field within the Child resolver. ([#2988](#2988)) ```graphql type Query { p: Parent! } type Parent @key(fields: "id") @context(name: "context") { id: ID! child: Child! prop: String! } type Child @key(fields: "id") { id: ID! b: String! field(a: String @fromcontext(field: "$context { prop }")): Int! } ``` ### Patch Changes - Various set context bugfixes ([#3017](#3017)) - Fix bug in context-matching logic for interfaces-implementing-interfaces (#3014) ([#3015](#3015)) A field is considered to match a context if the field's parent type (in the original query) either has `@context` on it, or implements/is a member of a type with `@context` on it. We ended up missing the case where interfaces implement interfaces; this PR introduces a fix. - Updated dependencies \[[`c4744da360235d8bb8270ea048f0e0fa5d03be1e`](c4744da), [`8a936d741a0c05835ff2533714cf330d18209179`](8a936d7)]: - @apollo/federation-internals@2.8.0 ## @apollo/query-planner@2.8.0 ### Minor Changes - Implement new directives to allow getting and setting context. This allows resolvers to reference and access data referenced by entities that exist in the GraphPath that was used to access the field. The following example demonstrates the ability to access the `prop` field within the Child resolver. ([#2988](#2988)) ```graphql type Query { p: Parent! } type Parent @key(fields: "id") @context(name: "context") { id: ID! child: Child! prop: String! } type Child @key(fields: "id") { id: ID! b: String! field(a: String @fromcontext(field: "$context { prop }")): Int! } ``` ### Patch Changes - Various set context bugfixes ([#3017](#3017)) - Fix relative path logic when eliding subgraph jumps for `@fromContext` ([#3005](#3005)) - Updated dependencies \[[`c4744da360235d8bb8270ea048f0e0fa5d03be1e`](c4744da), [`8a936d741a0c05835ff2533714cf330d18209179`](8a936d7), [`f5fe3e74d36722f78004c1e2e03c77d8b95cd6bf`](f5fe3e7)]: - @apollo/query-graphs@2.8.0 - @apollo/federation-internals@2.8.0 ## @apollo/subgraph@2.8.0 ### Patch Changes - Various set context bugfixes ([#3017](#3017)) - Updated dependencies \[[`c4744da360235d8bb8270ea048f0e0fa5d03be1e`](c4744da), [`8a936d741a0c05835ff2533714cf330d18209179`](8a936d7)]: - @apollo/federation-internals@2.8.0 ## apollo-federation-integration-testsuite@2.8.0 ### Patch Changes - Various set context bugfixes ([#3017](#3017)) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

clenfest requested a review from a team as a code owner May 23, 2024 17:09

clenfest added 3 commits May 23, 2024 12:10

add changeset

e492f50

Merge branch 'next' into clenfest/set_context_bugfixes

d667dba

adding a testcase for hopping back and forth between subgraphs

dcd8a88

clenfest requested a review from sachindshinde May 28, 2024 13:45

sachindshinde requested changes May 29, 2024

View reviewed changes

internals-js/src/federation.ts Outdated Show resolved Hide resolved

internals-js/src/operations.ts Outdated Show resolved Hide resolved

query-planner-js/src/buildPlan.ts Show resolved Hide resolved

Updates based on code review comments

70a97b0

clenfest commented May 29, 2024

View reviewed changes

sachindshinde approved these changes May 29, 2024

View reviewed changes

clenfest merged commit 8a936d7 into next May 29, 2024
15 checks passed

clenfest deleted the clenfest/set_context_bugfixes branch May 29, 2024 00:28

github-actions bot mentioned this pull request May 29, 2024

release: on branch next #3020

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

various setContext bug fixes #3017

various setContext bug fixes #3017

clenfest commented May 23, 2024

changeset-bot bot commented May 23, 2024 •

edited

Loading

codesandbox-ci bot commented May 23, 2024 •

edited

Loading

sachindshinde left a comment

sachindshinde commented May 29, 2024

clenfest May 29, 2024

sachindshinde May 29, 2024

various setContext bug fixes #3017

various setContext bug fixes #3017

Conversation

clenfest commented May 23, 2024

changeset-bot bot commented May 23, 2024 • edited Loading

🦋 Changeset detected

codesandbox-ci bot commented May 23, 2024 • edited Loading

sachindshinde left a comment

Choose a reason for hiding this comment

sachindshinde commented May 29, 2024

clenfest May 29, 2024

Choose a reason for hiding this comment

sachindshinde May 29, 2024

Choose a reason for hiding this comment

changeset-bot bot commented May 23, 2024 •

edited

Loading

codesandbox-ci bot commented May 23, 2024 •

edited

Loading