fix: incorrect alias behavior for required fields

[Noroth]

Aliases are not correctly taken into accoutn when using required fields. This PR fixes the behavior.

@coderabbitai summary

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.

Open Source AI Manifesto

This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cabc2ee8-532c-4334-b398-7a3438f9205b

📥 Commits

Reviewing files that changed from the base of the PR and between 0cf6a47 and 6fa75ac.

📒 Files selected for processing (4)

• execution/engine/execution_engine_grpc_requires_test.go
• v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
• v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
• v2/pkg/grpctest/schema.go

---

📝 Walkthrough

Walkthrough

Federation @requires planning now records each required-field selection instance, preserves GraphQL aliases in RPC response paths, and forwards per-instance arguments. Tests cover aliased selections, duplicate plain and aliased fields, expanded schema metadata, and full HTTP-to-gRPC execution.

Changes

Federation Requires Planning and Execution

Layer / File(s)	Summary
Per-selection required-field planning v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go	Required-field instances, key messages, configured selection sets, and field arguments are tracked separately before RPC calls are generated.
Alias-aware required-field RPC construction v2/pkg/engine/datasource/grpc_datasource/execution_plan.go	Required-field RPC construction uses planned instances and targets response fields through AliasOrPath().
Planner and datasource validation v2/pkg/engine/datasource/grpc_datasource/*_test.go	Tests cover aliased fields and simultaneous plain and aliased selections in execution plans and entity responses.
Full gRPC requires integration coverage execution/engine/execution_engine_grpc_requires_test.go	End-to-end tests exercise scalar, list, nested-object, and error-propagation @requires flows across HTTP and gRPC subgraphs.
Test schema and federation metadata v2/pkg/grpctest/schema.go	Field configurations, federation metadata, root nodes, child nodes, and supporting GraphQL types are expanded.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ExecutionEngine
  participant OwningHTTPSubgraph
  participant GRPCProductsSubgraph
  Client->>ExecutionEngine: execute federated operation
  ExecutionEngine->>OwningHTTPSubgraph: fetch entity representations
  OwningHTTPSubgraph-->>ExecutionEngine: return entities and key fields
  ExecutionEngine->>GRPCProductsSubgraph: resolve `@requires` fields
  GRPCProductsSubgraph-->>ExecutionEngine: return required results
  ExecutionEngine-->>Client: return combined response or propagated error

Loading

Possibly related PRs

• wundergraph/graphql-go-tools#1452: Modifies the same federation @requires planning and required-field RPC construction path.
• wundergraph/graphql-go-tools#1606: Shares the gRPC @requires planning refactor and end-to-end execution test coverage.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: fixing alias handling for required fields.
Description check	✅ Passed	The description is related to the PR and states the alias-handling fix, despite the typo and checklist boilerplate.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch ludwig/router-581-engine-alias-on-required-fields-is-misbehaving

---

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

[claude]

Claude Code Review

Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.

_{Tip: disable this comment in your organization's Code Review settings.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (1)

440-467: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Cloned entry's fields get overwritten by stale data — requiredField local var never repointed to the clone.

In the else branch (append-clone path), requiredField.clone() is appended into config.requiredFields, but the local requiredField variable is never reassigned to that clone — it still holds the previously-bound entry. The subsequent lines (requiredField.ref = ref, .fieldDefRef = fieldDefRef, .resultField = field) mutate this stale value, and fieldArguments is only overwritten when the current occurrence has arguments (len(fieldArgs) > 0). If the current (aliased) occurrence has no arguments while the previously-bound entry did, the stale fieldArguments leaks into the newly appended entry at line 467, silently overwriting the intentionally-cleared clone.

Example: nullableFilteredTagSummary(prefix: $prefix) plus an aliased selection of the same field without the argument would send the wrong (stale) prefix argument for the aliased required-fields call.

🐛 Proposed fix

 		} else {
 			index = len(config.requiredFields)
-			config.requiredFields = append(config.requiredFields, requiredField.clone())
+			requiredField = requiredField.clone()
+			config.requiredFields = append(config.requiredFields, requiredField)
 			r.entityConfig.setEntity(r.entityInfo.typeName, config)
 		}

Consider adding a regression test (e.g. in execution_plan_requires_test.go) combining an aliased selection with a field that has optional arguments, to catch this class of bug.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go`
around lines 440 - 467, In the clone branch of the required-field handling
logic, repoint the requiredField local variable to the newly appended clone and
update index accordingly before mutating ref, fieldDefRef, resultField, and
fieldArguments. Ensure each response-key entry starts with the clone’s cleared
argument state when the current occurrence has no arguments, preventing stale
arguments from the previously bound entry from being copied.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In
`@v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go`:
- Around line 440-467: In the clone branch of the required-field handling logic,
repoint the requiredField local variable to the newly appended clone and update
index accordingly before mutating ref, fieldDefRef, resultField, and
fieldArguments. Ensure each response-key entry starts with the clone’s cleared
argument state when the current occurrence has no arguments, preventing stale
arguments from the previously bound entry from being copied.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b6cac6bd-a5ec-476f-ba15-d06bf8261d65

📥 Commits

Reviewing files that changed from the base of the PR and between dd73327 and 0cf6a47.

📒 Files selected for processing (4)

• v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
• v2/pkg/engine/datasource/grpc_datasource/execution_plan_requires_test.go
• v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
• v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go