Skip to content

fix(federation): keep condition exclusions duplicate-free so equality and cache lookups are set-based - #10367

Open
inanna-apollo wants to merge 1 commit into
devfrom
inanna/fix-19-excluded-conditions
Open

inanna-apollo wants to merge 1 commit into
devfrom
inanna/fix-19-excluded-conditions

Conversation

@inanna-apollo

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

Copy link
Copy Markdown

ExcludedConditions is compared as a set (equal length plus one-way membership), but add_item appended unconditionally. Once a condition was added twice, equality stopped being symmetric ([A, A] == [A, B] but not the reverse), and ConditionResolverCache::contains could return a resolution cached under [A, A] for a request excluding [A, B].

Make add_item a no-op when the condition is already excluded, matching ExcludedDestinations::add_excluded. The length+containment equality is then a correct set equality, so the cache key is independent of how a set of exclusions was built. Current planner and composition call sites already skip edges whose condition is excluded before adding it, so plans do not change.

Reachability

Not reachable from current planner paths: every call site skips edges whose condition is already excluded before adding it (a temporary duplicate-add panic never fired across the full lib and --test main suites). This makes the data structure's equality and the condition cache correct by construction; plans do not change.

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: 688cdc0ac113668888bdddd6
Build Logs: View logs

URL: https://www.apollographql.com/docs/deploy-preview/688cdc0ac113668888bdddd6


✅ 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-19-excluded-conditions branch from f2febb6 to c99ee54 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
… and cache lookups are set-based

`ExcludedConditions` is compared as a set (equal length plus one-way
membership), but `add_item` appended unconditionally. Once a condition was
added twice, equality stopped being symmetric ([A, A] == [A, B] but not the
reverse), and `ConditionResolverCache::contains` could return a resolution
cached under [A, A] for a request excluding [A, B].

Make `add_item` a no-op when the condition is already excluded, matching
`ExcludedDestinations::add_excluded`. The length+containment equality is then
a correct set equality, so the cache key is independent of how a set of
exclusions was built. Current planner and composition call sites already skip
edges whose condition is excluded before adding it, so plans do not change.
@inanna-apollo
inanna-apollo force-pushed the inanna/fix-19-excluded-conditions branch 3 times, most recently from c99ee54 to 1807844 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