fix: detecting requires on interface members

[devsergiy]

Summary by CodeRabbit

• Bug Fixes

  • Corrects query planning for interface fields whose implementing types use requires directives, ensuring proper per-type rewrites even when queries don’t include fragments.
  • Improves reliability and data completeness for federated schemas when resolving interface fields with dependent data.

• Tests

  • Added federation-aware tests covering interface selections with requires, validating behavior for queries with and without concrete type fragments.

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]

Walkthrough

Extends interface selection rewrite logic to consider requires-configuration on implementing types when no fragments are present, potentially triggering a rewrite. Adds federation-aware tests covering interface fields with @requires on implementations, both with and without explicit fragments.

Changes

Cohort / File(s)	Summary
Planner logic: interface selection rewrite v2/pkg/engine/plan/abstract_selection_rewriter.go	Adds a check in the no-fragments path to rewrite interface field selections if any implementing type requires configuration for any requested field; retains existing behavior for fragments, objects, and unions.
Tests: federation and interface @requires coverage v2/pkg/engine/plan/abstract_selection_rewriter_test.go	Introduces federation metadata builders and schemas to model implementations with @requires on name; adds two tests verifying rewrite behavior for interface fields without fragments and with a concrete type fragment.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• fix: fix rewriting an interface object implementing interface #1265 — Also updates interface-selection rewrite logic in the same area, expanding interface objects to concrete types; likely touches complementary helper paths.

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ 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: detecting requires on interface members" succinctly and accurately summarizes the primary change—adding detection of @requires on interface implementors so the selection rewriter triggers per-type rewrites. It is concise, specific to the change, and uses the conventional commit "fix" prefix without extraneous detail.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

✨ Finishing touches

• 📝 Generate Docstrings

---

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

v2/pkg/engine/plan/abstract_selection_rewriter.go (2)

475-482: Scope the requires-check to entities lacking fragments to avoid unnecessary rewrites

Using entityNames here can trigger rewrites even when only types that already have concrete fragments require extra selections. Check only entitiesWithoutFragment.

-        // check if any implementing type has requiresConfiguration for one of the requested fields
-        if slices.ContainsFunc(entityNames, func(entityName string) bool {
+        // check if any implementing type without its own fragment has requiresConfiguration for one of the requested fields
+        if slices.ContainsFunc(entitiesWithoutFragment, func(entityName string) bool {
             return r.hasRequiresConfigurationForField(entityName, selectionSetInfo.fields)
         }) {
             return true
         }

---

434-446: Micro-opt: guard the no-fragment requires scan behind hasFields

If there are no shared fields (e.g., only __typename), there’s nothing to attach requires to; skip the scan.

-    // when we do not have fragments
-    if !selectionSetInfo.hasInlineFragmentsOnInterfaces &&
+    // when we do not have fragments
+    if !selectionSetInfo.hasInlineFragmentsOnInterfaces &&
         !selectionSetInfo.hasInlineFragmentsOnUnions &&
         !selectionSetInfo.hasInlineFragmentsOnObjects {
-        // check that all types implementing the interface have a root node with the requested fields
+        if !selectionSetInfo.hasFields {
+            return false
+        }
+        // check that all types implementing the interface have a root node with the requested fields
         if !r.allEntitiesHaveFieldsAsRootNode(entityNames, selectionSetInfo.fields) {
             return true
         }
 
         return slices.ContainsFunc(entityNames, func(entityName string) bool {
             return r.hasRequiresConfigurationForField(entityName, selectionSetInfo.fields)
         })
     }

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

4023-4076: Add a coverage hole: when only types with @requires already have concrete fragments, no rewrite should be needed

To prevent regressions from over-rewriting, add a case where only Admin.name has @requires and the query already includes an Admin fragment.

@@
 	{
+		name:               "requires only on Admin; Admin has fragment — no rewrite",
+		definition:         definitionC,
+		upstreamDefinition: `
+			interface Named { name: String! }
+			type User implements Named @key(fields: "id") {
+				id: ID!, name: String!, fullName: String! @external, surname: String!
+			}
+			type Admin implements Named @key(fields: "id") {
+				id: ID!, name: String! @requires(fields: "fullName"), fullName: String! @external, title: String!
+			}
+			type Query { user: Named! }
+		`,
+		dsBuilder: func() *dsBuilder {
+			return dsb().
+				RootNode("Query", "user").
+				RootNode("User", "id", "name", "surname").
+				AddRootNodeExternalFieldNames("User", "fullName").
+				RootNode("Admin", "id", "name", "title").
+				AddRootNodeExternalFieldNames("Admin", "fullName").
+				ChildNode("Named", "name").
+				WithMetadata(func(m *FederationMetaData) {
+					m.Requires = []FederationFieldConfiguration{
+						{ TypeName: "Admin", FieldName: "name", SelectionSet: "fullName" },
+					}
+					m.Keys = []FederationFieldConfiguration{
+						{ TypeName: "User", SelectionSet: "id" },
+						{ TypeName: "Admin", SelectionSet: "id" },
+					}
+				})
+		}(),
+		fieldName: "user",
+		operation: `
+			query {
+				user {
+					name
+					... on Admin { title }
+				}
+			}`,
+		expectedOperation: `
+			query {
+				user {
+					name
+					... on Admin { title }
+				}
+			}`,
+		shouldRewrite: false,
+	},

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 345b051 and 1e9fc4c.

📒 Files selected for processing (2)

• v2/pkg/engine/plan/abstract_selection_rewriter.go (1 hunks)
• v2/pkg/engine/plan/abstract_selection_rewriter_test.go (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

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

v2/pkg/engine/plan/federation_metadata.go (2)

• FederationMetaData (10-16)
• FederationFieldConfiguration (73-82)

🔇 Additional comments (2)

v2/pkg/engine/plan/abstract_selection_rewriter_test.go (2)

222-297: Nice, focused federation fixture for @requires on interface implementations

The Named/User/Admin setup plus dsBuilderC cleanly models the requires/external scenario and keeps tests readable. LGTM.

---

4049-4076: Tests correctly assert rewriting for @requires on interface members

Both no-fragment and mixed-fragment paths validate the new behavior. Nicely done.