feat: add module to add operation name also as operationName field#3048
feat: add module to add operation name also as operationName field#3048alepane21 wants to merge 3 commits into
Conversation
WalkthroughThis PR adds a new router module, ChangesAdd Operation Name Module
Estimated code review effort: 2 (Simple) | ~12 minutes Related PRs: None identified. Suggested labels: router, tests, enhancement Suggested reviewers: None identified. Poem 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
❌ Internal Query Planner CI checks failedThe Internal Query Planner CI checks failed in the celestial repository, and this is going to stop the merge of this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3048 +/- ##
==========================================
- Coverage 61.69% 61.24% -0.46%
==========================================
Files 261 262 +1
Lines 30602 30643 +41
==========================================
- Hits 18881 18766 -115
- Misses 10201 10328 +127
- Partials 1520 1549 +29
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router-tests/modules/add_operation_name_test.go (1)
64-81: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting no GraphQL errors / expected data.
The test only checks HTTP status 200 and the presence of
operationNamein the captured request; it doesn't verify the client actually received validemployeesdata with noerrors. Combined with the body-draining issue above, a downstream failure could go undetected. Adding an assertion on the response payload would make this an effective end-to-end check.🤖 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 `@router-tests/modules/add_operation_name_test.go` around lines 64 - 81, The add_operation_name_test end-to-end check only validates the request shape and HTTP 200, but it should also assert the GraphQL response is successful. Update the test around the request/response handling in the add_operation_name_test case to verify the returned payload contains the expected employees data and does not include GraphQL errors, using the existing res/response assertion flow and the capturedBodies checks to locate the test logic.
🤖 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.
Inline comments:
In `@router-tests/modules/add_operation_name_test.go`:
- Around line 39-50: The GlobalMiddleware wrapper in add_operation_name_test is
draining r.Body with io.ReadAll before passing the request to handler.ServeHTTP,
so restore the body afterward by resetting r.Body to a new io.NopCloser over the
captured bytes before calling the wrapped handler. Keep the existing capture
logic for capturedBodies, and use the middleware/test helper identifiers
GlobalMiddleware and handler.ServeHTTP to locate the fix; if useful, also add an
assertion on the response payload so the test still validates GraphQL execution.
---
Nitpick comments:
In `@router-tests/modules/add_operation_name_test.go`:
- Around line 64-81: The add_operation_name_test end-to-end check only validates
the request shape and HTTP 200, but it should also assert the GraphQL response
is successful. Update the test around the request/response handling in the
add_operation_name_test case to verify the returned payload contains the
expected employees data and does not include GraphQL errors, using the existing
res/response assertion flow and the capturedBodies checks to locate the test
logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 023003f1-3734-41b8-82a2-609962a3a6e3
📒 Files selected for processing (2)
router-tests/modules/add_operation_name_test.gorouter-tests/modules/custom-add-operation-name/module.go
| GlobalMiddleware: func(handler http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| body, err := io.ReadAll(r.Body) | ||
| require.NoError(t, err) | ||
|
|
||
| mu.Lock() | ||
| capturedBodies = append(capturedBodies, body) | ||
| mu.Unlock() | ||
|
|
||
| handler.ServeHTTP(w, r) | ||
| }) | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='router-tests/modules/add_operation_name_test.go'
echo '--- file outline ---'
ast-grep outline "$file" --view expanded || true
echo '--- relevant lines ---'
nl -ba "$file" | sed -n '1,140p'
echo '--- search for body restoration or similar patterns in this test file ---'
rg -n "io\.NopCloser|bytes\.NewReader|ReadAll\(r\.Body\)|GlobalMiddleware|ServeHTTP\(w, r\)" "$file"Repository: wundergraph/cosmo
Length of output: 340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='router-tests/modules/add_operation_name_test.go'
echo '--- relevant lines (1-160) ---'
sed -n '1,160p' "$file"
echo '--- search for response assertions in this test file ---'
rg -n "errors|data|status|require\.Equal|assert\." "$file"Repository: wundergraph/cosmo
Length of output: 2748
Restore r.Body before calling the wrapped handler.
io.ReadAll(r.Body) drains the request, so the downstream subgraph handler receives an empty body and the test no longer exercises real GraphQL execution. Rewind it with io.NopCloser(bytes.NewReader(body)), and consider asserting the response payload too.
🤖 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 `@router-tests/modules/add_operation_name_test.go` around lines 39 - 50, The
GlobalMiddleware wrapper in add_operation_name_test is draining r.Body with
io.ReadAll before passing the request to handler.ServeHTTP, so restore the body
afterward by resetting r.Body to a new io.NopCloser over the captured bytes
before calling the wrapped handler. Keep the existing capture logic for
capturedBodies, and use the middleware/test helper identifiers GlobalMiddleware
and handler.ServeHTTP to locate the fix; if useful, also add an assertion on the
response payload so the test still validates GraphQL execution.
…dd-option-to-include-operationname-in-subgraph
1 participant
Summary by CodeRabbit
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.