fix: requires is not ignored when all fields provided

[devsergiy]

Summary by CodeRabbit

• Tests

  • Added comprehensive federation tests covering provided fields, nested @requires interactions, and implicit parent-provided scenarios.

• Improvements

  • Improved federation planning so provided fields are propagated correctly across nested queries, reducing extra fetches and improving response completeness.
  • Enhanced validation and error context when required fields are already supplied (including implicit/parent-provided cases).

• Refactor

  • Optimized internal tracking of provided fields for faster lookups and clearer diagnostics.

Checklist

• I have discussed my proposed changes in an issue and have received approval to proceed.
• I have followed the coding standards of the project.
• Tests or benchmarks have been added or updated.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Refactors provided-field tracking from slice structs to keyed maps, adds a visitor to check that @requires fields are provided (including implicit provision via accessible nested nodes), wires these checks into planning and error flows, and expands federation tests to cover nested @provides/@requires scenarios.

Changes

Cohort / File(s)	Summary
Federation tests v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_provides_test.go, v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go	Adds new federation test file and updates existing federation tests to exercise nested @provides/@requires flows and a newly used provided field; updates SDLs, data source metadata, and expected plans.
Provides collection & visitor v2/pkg/engine/plan/provides_fields_visitor.go, v2/pkg/engine/plan/provides_fields_visitor_test.go	Converts provides output from []*NodeSuggestion to map[string]struct{}; providesSuggestions now returns (map, report); visitor wiring and tests updated to assert keyed entries.
Node suggestion tracking & collectors v2/pkg/engine/plan/datasource_filter_node_suggestions.go, v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go	Adds providedFields map to NodeSuggestions; per-visitor providesEntries switches to map and collectors record provided keys then apply them to NodeSuggestions via addProvidedField.
Key/collect visitors & tests v2/pkg/engine/plan/key_fields_visitor.go, v2/pkg/engine/plan/key_fields_visitor_test.go	Replaces slice-based providesEntries with map lookups and updates tests to initialize/assert on map-based provided-field keys (composite string keys).
Required-fields validation v2/pkg/engine/plan/required_fields_provided_visitor.go, v2/pkg/engine/plan/required_fields_provided_visitor_test.go	Adds areRequiredFieldsProvided visitor to verify @requires fields are provided (supports fragments, unions, nested paths); treats accessible nested nodes as implicit provision; new unit tests cover varied cases.
Planner integration & error handling v2/pkg/engine/plan/node_selection_visitor.go, v2/pkg/engine/plan/path_builder_visitor.go	Integrates required-field existence checks into planning (early-return when provided), simplifies planner-dependency checks for @requires, and improves error messages via fmt.Errorf.
Misc plan tests & refactors v2/pkg/engine/plan/... (multiple test files)	Multiple tests updated to reflect map-based provides API, removal of fragment/report coupling where applicable, and adapted expected outcomes for provided/required logic.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• fix: improve query planning time #1326 — Overlaps on refactors to provides handling and NodeSuggestions; likely touches same collectors and visitor logic.
• fix: fix conflict of rewriter and required fields #1338 — Related changes around required-fields fragments and selection-rewriter plumbing used by the new required-fields visitor.
• feat: validate presence of optional @requires dependencies #1297 — Related to required-field checking and planning integration for @requires, touching planning/visitor behavior.

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: requires is not ignored when all fields provided' accurately summarizes the main change: fixing a bug where @requires directives should be ignored when all required fields are already provided by federation.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch sergiy/eng-8899-requires-is-not-ignored-when-all-fields-provided

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (1)

286-290: Consider clearing run‑local indexes in reset().

localSuggestionLookup and localSeenKeys are described as run‑local but aren’t cleared. If a collector is reused after global state changes, stale entries could suppress work. If persistence is intentional, a brief comment would help; otherwise, clear them here.

♻️ Suggested adjustment

func (f *collectNodesDSVisitor) reset() {
    f.localSuggestions = f.localSuggestions[:0]
    f.keys = f.keys[:0]
+   for k := range f.localSuggestionLookup {
+       delete(f.localSuggestionLookup, k)
+   }
+   for k := range f.localSeenKeys {
+       delete(f.localSeenKeys, k)
+   }
}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@v2/pkg/engine/plan/required_fields_provided_visitor_test.go`:
- Around line 160-161: areRequiredFieldsProvided returns two values (bool,
*operationreport.Report) but the test only captures one; update the test to
receive both returns from areRequiredFieldsProvided (e.g., result, report :=
areRequiredFieldsProvided(input)), then use result in the existing assert and,
if relevant, assert on report (or explicitly ignore it) so the compilation error
is resolved; refer to areRequiredFieldsProvided and the local variables result
and report in your change.

🧹 Nitpick comments (3)

v2/pkg/engine/plan/provides_fields_visitor_test.go (1)

89-94: Note: meta variable is unused in the test.

The DataSourceMetadata is initialized at lines 63-88 but never used in the test. This appears to be leftover from the previous implementation.

♻️ Proposed cleanup

 	for _, c := range cases {
 		t.Run(keySDL, func(t *testing.T) {
-			report := &operationreport.Report{}
-
-			meta := &DataSourceMetadata{
-				RootNodes: []TypeField{
-					{
-						TypeName:   "Query",
-						FieldNames: []string{"me"},
-					},
-					{
-						TypeName:           "User",
-						FieldNames:         []string{"address"},
-						ExternalFieldNames: []string{"name", "info"},
-					},
-
-					{
-						TypeName:           "Address",
-						ExternalFieldNames: []string{"street", "zip"},
-					},
-				},
-				ChildNodes: []TypeField{
-					{
-						TypeName:           "Info",
-						ExternalFieldNames: []string{"age"},
-					},
-				},
-			}
-			meta.InitNodesIndex()
-
 			input := &providesInput{

v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_provides_test.go (2)

230-299: Consider removing or documenting the commented-out code more explicitly.

The commented-out block showing the "nested fetch" variant is valuable documentation of what the plan would look like without the fix. However, for long-term maintainability, consider either:

1. Converting this to a proper doc comment explaining the expected behavior difference
2. Creating a separate test case that explicitly demonstrates when a nested fetch should occur (e.g., when fields are NOT provided)

This would make the test suite more self-documenting and prevent confusion about whether this code was accidentally commented out.

---

191-201: Consider disabling debug printing for committed tests.

The debug configuration enables PrintQueryPlans and PrintPlanningPaths, which can produce verbose output during test runs. Unless this is intentionally kept for debugging federation issues in CI, consider disabling these flags for cleaner test output.

🔧 Suggested change

 		planConfiguration := plan.Configuration{
 			DisableResolveFieldPositions: true,
-			Debug: plan.DebugConfiguration{
-				PrintQueryPlans:    true,
-				PrintPlanningPaths: true,
-			},
+			Debug: plan.DebugConfiguration{},
 			DataSources: []plan.DataSource{
 				service1DataSourceConfig,
 				service2DataSourceConfig,
 			},
 		}