inefficient query plans when using @context

[clenfest]

Fixes edge case where contextual arguments can lead to inefficient query plans. Also fixes naming of query plan arguments which can be a problem when using contextual variables in multiple subraphs.

[changeset-bot]

🦋 Changeset detected

Latest commit: a04979f

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

This PR includes changesets to release 7 packages

Name	Type
@apollo/query-planner	Patch
@apollo/query-graphs	Patch
@apollo/gateway	Patch
@apollo/composition	Patch
@apollo/federation-internals	Patch
@apollo/subgraph	Patch
apollo-federation-integration-testsuite	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

[netlify]

✅ Deploy Preview for apollo-federation-docs canceled.

Name	Link
🔨 Latest commit	a04979f
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-federation-docs/deploys/670ee0cfcc43e40008ab2eae

[codesandbox-ci]

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.

[dariuszkuc]

previously this was calculated in a single loop above -> any reason to move it down here? I think we only need the builder.setContextMaps(...) outside of the loop?

[clenfest]

Yeah, it's like that because we only need to do the calculations once at the end. The fact that it was inside the loop was causing naming colisions.

[sachindshinde]

Reviewed this synchronously with Chris, will summarize the feedback:

1. The changes to query graph creation are fine, and can be in their own PR (it's been split off into Fixes issue where there can be naming collisions in contextual parameter names #3155 ).
2. handleRequires() needs to be split up, so that we may reuse the first half of it. Specifically, the part that applies the condition's OpPathTree and hoists (and maybe merges) the created nodes up the ancestors of the fetch dependency graph where possible. This function would return the main group that the start of the conditions merged into, along with any unmerged created groups.
   • The second half of handleRequires() is effectively the part specific to @requires, and takes in the outputs of the first function (along with e.g. current group/path). It's responsible for adding the inputs to the node querying the @requires field, and setting up parent/child relationships.
   • The idea here is that we can make an analogous second-half function for @fromContext-containing nodes, which will add the context arguments/input rewrites to the node, along with setting up parent/child relationships.
   • IIRC we've been choosing to put the subgraph jump (if needed) at the @context type and not near the @fromContext field, so I believe we'd need to insert the subgraph jump in the first-half function.

[sachindshinde]

The splitting of handleRequires() generally looks fine to me (would change a bit in the results passed between functions), but some changes are needed in how it's called in computeGroupsForTree(). Going to pair program on that with @clenfest.

[sachindshinde]

This is the @key-handling logic, which still uses newGroup (it's not actually copying the current group, but genuinely creating a new group based off the @key edge's tail).

[sachindshinde]

Note that conditions being truthy from the OpPathTree just tells us something wants to fetch data here, for use either immediately (@requires edge, InterfaceObjectFakeDownCast edge) or later (@fromContext edge), potentially multiple of these. conditions meant truthy means we'd need to execute the first-half function handleRequiresTree(). edge.conditions being additionally truthy means this edge requires at least some of those conditions to be provided as input (via _entities), so we'd also want to execute the @requires second-half function, createPostRequiresGroup().

[sachindshinde]

It's a bit misleading here to indicate unmergedGroups as [], as no merging of created groups takes place at all in this branch (so it's more like unmergedGroups should be createdGroups).

I think it'd be more clear here to:

1. Swap createdGroups with a noCreatedGroups boolean.
2. Have unmergedGroups be createdGroups when no merging took place.
3. Use unmergedGroups in the other half of handleRequires(), in the place of createdGroups.

[sachindshinde]

Was curious about why we had this if block, synced with @clenfest but we didn't see a reason here.

[sachindshinde]

Not sure why we'd only set the condition groups when this boolean expression is true, in general we need these condition groups so the downstream @fromContext will be able to depend on these.

[svc-apollo-docs]

✅ Docs Preview Ready

No new or changed pages found.

[apollo-bot2]

Detected Secrets

🔴 Secrets Found

If you are seeing this message, it means that the secret scanning tool that Apollo uses to prevent secrets from entering our repositories has identified a secret in your branch. Information about what was detected and steps to move forward are below.

If the secret scanner detected a valid secret, please take immediate action to remove it from your branch. Apollo also recommends that you invalidate the secret in the system it grants access to. While there are additional steps to prepare the branch for merging, protecting the secret is the top priority.

If the scanner detected a value that is not actually secret or it detected a value that was a secret but has been invalidated, follow the steps in the "Resolution Process" section below to mark it as a false/benign positive. One common reason for benign positive detections is test values. For example, if you are committing a value (like a private key, JWT, etc.) that would usually be secret but is being used in a test case to validate functionality, a detection will be generated. This is safe to allowlist.

Values Detected

Signature	File	Commit	Start Line	Start Column	Link To Secret
generic-api-key	query-planner-js/src/tests/buildPlan.test.ts	98882688d195e195234d8af24fd64fdd6d105a2b	9178	15	Link
generic-api-key	query-planner-js/src/tests/buildPlan.test.ts	ff5e2fac1ed4731c3392546a361291b3905b494b	10334	16	Link
generic-api-key	query-planner-js/src/tests/buildPlan.test.ts	ff5e2fac1ed4731c3392546a361291b3905b494b	10339	16	Link

---

Resolution Process

[!NOTE]
Apollo does not use all of the native options provided by Gitleaks to perform allowlisting. We use Gitleaks' standard line comments but rely on a custom convention written in a .gitleaks.toml file at the root of each repository for allowlisting detections in Git history.

To resolve the issue, you will need to:

1. Prevent new detections of the "secret":
   • Add a comment containing gitleaks:allow to the line containing the detected value.
   • Skip this step if it's not feasible. For example, the detection is in file types like JSON or Markdown, you've completely removed the value, etc.
2. Configure the scanner to ignore the "secret" in git history.
   • Follow the instructions below for either "Allowlist by Commit Hash" or "Allowlist by File Path"
   • You will need to ensure that a .gitleaks.toml file exists at the root of this repository in your branch. If not, create one!

Allowlist by Commit Hash

This is the preferred option. Update the .gitleaks.toml file with the following contents:

[[ rules ]]
    id = "signature-from-table-above"
    [ rules.allowlist ]
        commits = [
            "full-commit-hash-1",
            "full-commit-hash-2"
        ]

If a [[rules]] block already exists for the signature involved in your detection, simply append your list of commits.

If a matching [[rules]] block is already present but has a paths attribute rather than commits, proceed to the next option.

This configuration tells the scanner to ignore the specified signature in the list of commits provided.

Allowlist by File Path

This is a backup option to allowlist specific detections. Add/update the .gitleaks.toml file with the following contents:

[[ rules ]]
    id = "signature-from-table-above"
    [ rules.allowlist ]
        paths = [
            '''path/to/detected/file1.json$''',
            '''path/to/other/detected/file.md$'''
        ]

This configuration tells the scanner to ignore any detections with the specified signature generated for the list of files.

---

Getting Help

The Apollo Security team has been notified and is available to assist in resolving this issue. Please tag us on this PR using @apollographql/security if you need assistance!

---

How do I know I fixed this correctly?

Once you have resolved the issue completely, this message will disappear! If you're still seeing this message, there is more to do prior to merging.

[sachindshinde]

Finished pairing with @clenfest , code should be simpler now around __typename additions and we should be reusing the @requires optimizations for @fromContext selection sets now.