Skip to content

Conversation

@benjamn
Copy link
Member

@benjamn benjamn commented Sep 2, 2025

Note

This PR targets the branch for #8073, which is a preview release branch for connect/v0.3-associated features. This PR's branch is targeted in turn by #8143.

While a single JSONSelection string (e.g. the selection argument to a @connect directive) represents everything a particular HTTP endpoint might return, in most cases individual GraphQL operations (especially those generated by the query planner) will ask for fewer fields, and may alias/rename those fields.

Historically (since PR #5635), we've used apply_selection_set in json_selection/selection_set.rs to transform the full JSONSelection into whatever a given operation asks for, including implementing field aliasing within the transformed JSONSelection string. In theory, this selection filtering/transforming should make selection execution less expensive at runtime, but we never implemented transform caching, so the transform was happening for every incoming operation at runtime, probably not improving performance overall.

More importantly, with the advent of abstract types, conditional selections, and more complicated use of -> methods (where changing something about the output would require rewriting input arguments), it is no longer generally possible to accomplish everything we want by transforming the JSONSelection. This limitation became apparent recently when a field that came from within a ->match method was renamed with an alias in the GraphQL query. The renamed field ended up null because the field was not actually renamed in the JSONSelection, because apply_selection_set does not descend into rewriting -> method arguments.

Instead, this PR implements a foolproof backup plan: instead of relying on apply_selection_set alone, we execute incoming GraphQL operations against the JSONSelection mapping results, using knowledge of fragments, aliases, and supertype-subtype relationships from the schema. This means apply_selection_set is no longer responsible for any alias renaming, since all of that is handled later. However, apply_selection_set is still responsible for pruning as much of the JSONSelection as possible, when there are fields we can statically detect are definitely unused.

Another approach here would be to make the JSONSelection::apply_to_path method take an ExecutableDocument and only execute fields that are needed by the operation, so we wouldn't have to transform the selection, but that's an optimization that adds complexity, compared to this PR.

@benjamn benjamn self-assigned this Sep 2, 2025
@apollo-librarian
Copy link

apollo-librarian bot commented Sep 2, 2025

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: f194957f134fc66b3a668576
Build Logs: View logs

@benjamn benjamn force-pushed the benjamn/dynamic-connectors-response-mapping branch 2 times, most recently from a7d8dc6 to 542122e Compare September 2, 2025 17:14
@benjamn benjamn changed the title Stop relying entirely on apply_selection_set for connectors GraphQL response mapping Stop relying on apply_selection_set for connectors GraphQL response mapping Sep 2, 2025
@benjamn benjamn marked this pull request as ready for review September 2, 2025 18:06
@benjamn benjamn requested review from a team as code owners September 2, 2025 18:06
@benjamn benjamn force-pushed the benjamn/dynamic-connectors-response-mapping branch 2 times, most recently from eef1acc to 43f056a Compare September 7, 2025 15:10
@dylan-apollo
Copy link
Member

Does this also impact the response caching stuff that @lennyburdette is working on? Or is that keyed on operation? Would be cool if user(id: 1) { name email} being cached meant that user(id: 1) { name } didn't trigger a fresh request.

@benjamn benjamn changed the base branch from am/connectorspreview to dev September 8, 2025 19:21
@benjamn benjamn requested review from a team as code owners September 8, 2025 19:21
@benjamn benjamn force-pushed the benjamn/dynamic-connectors-response-mapping branch from 2a35586 to 756e131 Compare September 8, 2025 19:31
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2025

@benjamn, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

@benjamn
Copy link
Member Author

benjamn commented Sep 8, 2025

@dylan-apollo Not sure about the exact cache key inputs (curious to hear from @lennyburdette), but this PR should in principle reduce the total number of distinct JSONSelections produced by apply_selection_set, since aliases are now ignored at this level, which I suppose could be good for caching.

@benjamn benjamn force-pushed the benjamn/dynamic-connectors-response-mapping branch 2 times, most recently from 0c91f46 to d355501 Compare September 8, 2025 20:01
@benjamn benjamn force-pushed the benjamn/dynamic-connectors-response-mapping branch 6 times, most recently from a001d9d to eada1f5 Compare September 25, 2025 19:30
@benjamn benjamn force-pushed the benjamn/dynamic-connectors-response-mapping branch 2 times, most recently from fe2b18b to edbc5cc Compare November 10, 2025 14:59
@hwillson hwillson requested review from a team, dylan-apollo and naomijub November 10, 2025 15:26
@benjamn benjamn force-pushed the benjamn/dynamic-connectors-response-mapping branch from edbc5cc to a22b42f Compare November 12, 2025 15:00
@benjamn benjamn enabled auto-merge (squash) November 12, 2025 15:06
@benjamn benjamn merged commit 6a83b98 into dev Nov 12, 2025
15 checks passed
@benjamn benjamn deleted the benjamn/dynamic-connectors-response-mapping branch November 12, 2025 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants