chore: simplify Processor.Process()

[ysmolski]

Do no return anything from it since it is mutation of a plan anyway. Cosmo already does not use the returned value.

Summary by CodeRabbit

• Refactor

  • Restructured query plan processing workflow to separate caching and post-processing logic.
  • Updated plan processor architecture with new internal components for enhanced plan optimization.

• Tests

  • Updated test cases to reflect changes in plan processing flow.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The postprocessing pipeline is refactored to change the Process method from returning a modified plan to performing processing in-place without a return value. Related callers in the execution engine are updated to handle this signature change, and tests are adapted accordingly.

Changes

Cohort / File(s)	Change Summary
Postprocessing Pipeline v2/pkg/engine/postprocess/postprocess.go	Processor.Process method signature changed to remove return value; processing logic moved in-function. Processor struct enhanced with new fields: resolveInputTemplates, appendFetchID, dedupe, processResponseTree, processFetchTree. Processing now handles both SynchronousResponsePlan and SubscriptionResponsePlan flows. Added type documentation comment. Fixed typo in createFetchTree comment.
Postprocessing Tests v2/pkg/engine/postprocess/postprocess_test.go	Test cases refactored: renamed pre field to plan; updated Process invocations and assertions to reflect new void return signature.
Execution Engine execution/engine/execution_engine.go	getCachedPlan updated to call ctx.postProcessor.Process(planResult) without capturing return value and to cache the original planResult instead of a processed result.
Plan Visitor v2/pkg/engine/plan/visitor.go	Added documentation comment for configureFetch method.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Key areas requiring extra attention:
  • Processor.Process signature change from returning plan.Plan to void: verify all callers have been correctly updated and behavior is preserved
  • In-function processing logic in postprocess.go: review order and correctness of operation chains for both response and subscription paths
  • Caching strategy change in execution_engine.go: confirm that caching the unmodified plan aligns with intended postprocessing semantics and doesn't break downstream consumers
  • Test updates: ensure test assertions properly validate the new in-place processing behavior

Possibly related PRs

• fix: fix merging fetches and add dependencies update #1232: Modifies the postprocess pipeline, including changes to Processor.Process method signature and fetch-tree processing logic, indicating closely related architectural updates.

Pre-merge checks and finishing touches

✅ 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 accurately summarizes the main change: removing the return value from Processor.Process() to simplify the method signature, which is the primary objective of the PR.

✨ 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 yury/refactor_processor_process

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 33b847e and a612bf1.

📒 Files selected for processing (4)

• execution/engine/execution_engine.go (1 hunks)
• v2/pkg/engine/plan/visitor.go (1 hunks)
• v2/pkg/engine/postprocess/postprocess.go (3 hunks)
• v2/pkg/engine/postprocess/postprocess_test.go (10 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-11-19T10:53:06.342Z

Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.

Applied to files:

• v2/pkg/engine/postprocess/postprocess_test.go

🧬 Code graph analysis (2)

v2/pkg/engine/postprocess/postprocess_test.go (1)

v2/pkg/engine/plan/plan.go (3)

• Plan (14-17)
• SynchronousResponsePlan (19-22)
• SynchronousResponsePlan (28-30)

v2/pkg/engine/postprocess/postprocess.go (1)

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

• Plan (14-17)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: Build and test (go 1.25 / ubuntu-latest)
• GitHub Check: Build and test (go 1.25 / windows-latest)
• GitHub Check: Build and test (go 1.25 / ubuntu-latest)
• GitHub Check: Build and test (go 1.25 / windows-latest)

🔇 Additional comments (8)

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

1316-1317: LGTM!

The added documentation comment accurately describes the function's purpose and improves code readability.

v2/pkg/engine/postprocess/postprocess.go (3)

19-20: LGTM!

Clear and concise documentation for the Processor type that explains its role in the execution pipeline.

---

136-140: LGTM!

The signature change from func (p *Processor) Process(pre plan.Plan) plan.Plan to func (p *Processor) Process(pre plan.Plan) is correct. Since Process mutates the plan in-place via pointer receivers on the concrete plan types (*plan.SynchronousResponsePlan, *plan.SubscriptionResponsePlan), returning the value was indeed unnecessary.

---

173-175: LGTM!

Good fix for the typo ("inital" → "initial") in the documentation comment.

execution/engine/execution_engine.go (1)

263-265: LGTM!

The caller correctly adapts to the new Process() signature. Since Process() mutates planResult in-place, caching and returning planResult directly is the correct approach.

v2/pkg/engine/postprocess/postprocess_test.go (3)

19-20: LGTM!

The field rename from pre to plan improves clarity and the struct correctly reflects that the same object serves as both input and the expected-after-mutation value.

---

371-384: LGTM!

The test correctly validates the in-place mutation behavior by calling Process(c.plan) and then comparing c.plan (now mutated) against c.expected.

---

682-695: LGTM!

Same correct pattern applied consistently to the second test function.

---

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