New test case: provides-only-requested-fields#335
Open
ardatan wants to merge 2 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new federation audit suite (provides-only-requested-fields) to validate two @provides-related invariants that weren’t previously covered: (1) gateways must not over-forward unrequested fields from the @provides set, and (2) gateways must not re-fetch already-provided fields from the owner subgraph.
Changes:
- Register the new
provides-only-requested-fieldssuite in the main test-suite loader. - Add a new two-subgraph topology (
aowner,bprovider) with punish-mode resolvers that surface over-fetching / poor planning as subgraph errors. - Add 11 queries covering subset-selection shapes (aliases, fragments, list variants,
@skip/@include, and sibling inline fragments).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Registers the new suite so it’s served and included in the audit run. |
| src/test-suites/provides-only-requested-fields/index.ts | Wires the suite’s subgraphs + tests into serve(...). |
| src/test-suites/provides-only-requested-fields/data.ts | Adds deterministic entity fixtures used by both subgraphs. |
| src/test-suites/provides-only-requested-fields/a.subgraph.ts | Owner subgraph (a) with punish-mode field resolvers to detect re-delegation. |
| src/test-suites/provides-only-requested-fields/b.subgraph.ts | Provider subgraph (b) exposing @provides root fields and punish-mode traps for over-forwarding / wrong entry path. |
| src/test-suites/provides-only-requested-fields/test.ts | Adds 11 black-box queries that exercise @provides request trimming and planning behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New test case:
provides-only-requested-fieldsSummary
Adds a new audit suite that locks down two closely-related
@providesinvariants that today are not covered anywhere in this repository:@provides(fields: "X Y"), the gateway must forward only the subset of{X, Y, …}the client actually selected — not the full@providesset.@providesalready covered a field on the providing subgraph, the gateway must not make an extra hop to the owner subgraph (via__resolveReference) to re-fetch the same field.This is the missing audit-side counterpart to the Hive Gateway fix from the PR 2351 (request-side trimming + plan-side
flattenSelectionsForTypechange). The bug was caught against a real user query but the existingnested-provides/provides-on-interface/provides-on-unionsuites do not exercise the "client asked for a strict subset of@provides" shape, nor the "owner has the field but@providesalready resolved it" shape.Topology
Two subgraphs, one entity:
a(owner) — definesEntity { id, name, description, extra }.nameanddescriptionare@shareableso they can also be resolved viab's@provides.extrais owner-only and forces a fallback hop when selected.b(provider) — exposesQuery.entity/Query.entitieswith@provides(fields: "name description").nameanddescriptionare@external. None of the test queries selectdescription.How the bug is detected (black-box)
The two failure modes manifest as subgraph-level errors, so the suite plugs into the existing
shouldPunishForPoorPlans()mechanism (already used bynested-provides,provides-on-union,provides-on-interface) and gates the strict resolvers behindPUNISH_FOR_POOR_PLANS=1:b.Entity.descriptionthrows → catches the over-forward bug (gateway injected an unrequested@providesfield).b.Entity.__resolveReferencethrows → catches the wrong entry path intob(provided fields must be served by the@providesplan, not by an entity lookup).a.Entity.name/a.Entity.descriptionthrow → catches the owner re-delegation bug (gateway askedafor fieldsbalready provided).In default mode (no env flag) the suite runs as a pure data-correctness check, same convention as the existing
@providessuites, so it doesn't change the baseline pass-rate of well-behaved gateways and doesn't false-positive on gateways that are still being onboarded. SettingPUNISH_FOR_POOR_PLANS=1lights up the strict assertions.Test cases (11)
Designed to exercise every code path the Hive Gateway fix touched:
{ entity { id } }b, no over-forward.{ entity { id name } }displayName: name... on Entity { name }...EntityNamenamed fragment{ id name extra }namefromb,extraforces owner hop, no re-delegation ofname.{ entities { id name } }{ entities { id name extra } }... on Entity @skip(if: true) { description }@skipon the wrapping fragment must reach the providing subgraph intact.... on Entity @include(if: false) { description }@include.Cases 9–11 mirror the recent additions in
packages/federation/tests/provides-scope.test.tson the gateway side and would have caught both the directive-stripping and the shallow-fragment-walk regressions.Files
No changes outside the new suite directory other than registering it in
src/index.ts.Verification
[a, b]succeeds with@apollo/composition(the@shareablemarkers onaare required so the duplicatename/descriptionownership is composition-legal).PUNISH_FOR_POOR_PLANS=1activates the strict assertions and reproduces the original bug against a gateway that over-forwards@providesfields or re-delegates to the owner.Related
flattenSelectionsForTypechange on theprovides-fixbranch. fix(federation): handle@providesmore optimal gateway#2351