Skip to content

fix(federation): merge fragments whose keys collide after directive stripping - #10376

Open
inanna-apollo wants to merge 1 commit into
devfrom
inanna/fix-5-fragment-key-collision
Open

inanna-apollo wants to merge 1 commit into
devfrom
inanna/fix-5-fragment-key-collision

Conversation

@inanna-apollo

@inanna-apollo inanna-apollo commented Oct 2, 2026 •

Copy link
Copy Markdown

remove_unneeded_top_level_fragment_directives removes @include/@skip applications from top-level inline fragments when they are already implied by the path a fetch node is merged at. Removing a directive changes the fragment's selection key, so two fragments that were distinct before (... on A @include(if: $x) { id } and ... on A { value }) can end up with the same key. The result was built with SelectionMap::insert, which appends without checking for an existing key, so the returned selection set held two entries for one key, breaking the invariant every key-based lookup and merge on SelectionMap relies on.

Build the result with add_local_selection, which merges selections whose keys collide.

Reachability

Not reached by the planner today: instrumenting every strip across the query plan suite and hand-built shapes found no collision, and the only caller re-merges top-level duplicates. Defensive fix for the SelectionMap key invariant.

Testing

The regression test(s) in this PR fail on dev and pass with this change; neighboring test suites pass with no snapshot changes. Found during property-based testing of apollo-federation.


Checklist

  • PR description explains the motivation for the change and relevant context for reviewing
  • PR description links appropriate GitHub/Jira tickets (creating when necessary)
  • Changeset is included for user-facing changes
  • Changes are compatible
  • Documentation completed
  • Performance impact assessed and acceptable
  • Metrics and logs are added and documented
  • Tests added and passing
    • Unit tests
    • Integration tests
    • Manual tests, as necessary

@apollo-librarian

apollo-librarian Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

✅ Docs preview ready

The preview is ready to be viewed. View the preview

File Changes

0 new, 1 changed, 0 removed
* graphos/routing/(latest)/_sidebar.yaml

Build ID: 02cf6155759e9914b4639c6e
Build Logs: View logs

URL: https://www.apollographql.com/docs/deploy-preview/02cf6155759e9914b4639c6e


✅ AI Style Review — No Changes Detected

No MDX files were changed in this pull request.

Review Log: View detailed log

This review is AI-generated. Please use common sense when accepting these suggestions, as they may not always be accurate or appropriate for your specific context.

@github-actions

This comment has been minimized.

@inanna-apollo
inanna-apollo force-pushed the inanna/fix-5-fragment-key-collision branch from e32b1b6 to de333c5 Compare October 5, 2026 18:04
@inanna-apollo
inanna-apollo marked this pull request as ready for review October 5, 2026 18:04
@inanna-apollo
inanna-apollo requested review from a team as code owners October 5, 2026 18:04
…tripping

`remove_unneeded_top_level_fragment_directives` removes `@include`/`@skip`
applications from top-level inline fragments when they are already implied
by the path a fetch node is merged at. Removing a directive changes the
fragment's selection key, so two fragments that were distinct before
(`... on A @include(if: $x) { id }` and `... on A { value }`) can end up
with the same key. The result was built with `SelectionMap::insert`, which
appends without checking for an existing key, so the returned selection set
held two entries for one key, breaking the invariant every key-based lookup
and merge on `SelectionMap` relies on.

Build the result with `add_local_selection`, which merges selections whose
keys collide.
@inanna-apollo
inanna-apollo force-pushed the inanna/fix-5-fragment-key-collision branch 3 times, most recently from de333c5 to 3e433bd Compare October 5, 2026 18:24
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.

1 participant