Conversation
…-field-args-on-required-fields-on
dkorittki
requested review from
a team,
Noroth,
SkArchon,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
WalkthroughAdded a federated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2731 +/- ##
==========================================
- Coverage 63.46% 63.24% -0.22%
==========================================
Files 251 251
Lines 26767 26767
==========================================
- Hits 16987 16929 -58
- Misses 8416 8459 +43
- Partials 1364 1379 +15 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demo/pkg/subgraphs/projects/src/service/service.go`:
- Around line 523-529: The deduplication uses the raw tag as the map key (seen)
while the match uses case-insensitive comparison with tagFilter, which allows
duplicates that differ only by case; normalize the tag before using it as a
dedup key (e.g., use strings.ToLower or strings.EqualFold normalization) so that
seen uses the same case-insensitive form, update references to seen, tag and
matchingTags in the loop where tagFilter is checked to use the normalizedKey for
deduping and still append the original tag to matchingTags when appropriate.
In `@router-tests/go.mod`:
- Line 31: The go.mod currently requires go.opentelemetry.io/otel/sdk at an
older version and has a replace that pins it to v1.28.0; update the direct
require for go.opentelemetry.io/otel/sdk to v1.40.0 (or later) and remove or
update the replace directive so it no longer downgrades the SDK to v1.28.0
(ensure any replace entries referencing go.opentelemetry.io/otel/sdk are deleted
or set to >= v1.40.0); after changes run `go mod tidy` to refresh transitive
deps and verify compatibility with google.golang.org/grpc.
🪄 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: 13d40700-5983-4e83-b949-79509e3b23e3
⛔ Files ignored due to path filters (6)
demo/pkg/subgraphs/projects/generated/mapping.jsonis excluded by!**/generated/**demo/pkg/subgraphs/projects/generated/service.pb.gois excluded by!**/*.pb.go,!**/generated/**demo/pkg/subgraphs/projects/generated/service.protois excluded by!**/generated/**demo/pkg/subgraphs/projects/generated/service_grpc.pb.gois excluded by!**/*.pb.go,!**/generated/**router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
demo/pkg/subgraphs/projects/src/schema.graphqldemo/pkg/subgraphs/projects/src/service/service.gorouter-tests/go.modrouter-tests/protocol/grpc_subgraph_test.gorouter-tests/testenv/testdata/configWithGRPC.jsonrouter/go.mod
dkorittki
changed the title
Noroth
requested changes
Apr 2, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router-tests/protocol/router_plugin_test.go (1)
513-522: Consider adding a mixed-case tag test to lock behavior.Since resolver matching is case-insensitive, a case like
tag: "CLOUD"would protect this contract from regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/protocol/router_plugin_test.go` around lines 513 - 522, Add a new test case next to the existing employee filteredProjectSummary tests that uses a mixed-case tag to lock case-insensitive matching: create an entry named like "query employee `@requires` field with argument — mixed-case tag" with query `{ employee(id: 1) { id filteredProjectSummary(tag: "CLOUD") } }` and expected JSON `{"data":{"employee":{"id":1,"filteredProjectSummary":"expertise: Backend Architecture, filtered tags (tag=CLOUD): [cloud]"}}}` so the resolver's case-insensitive match is asserted (update the test slice where the existing two cases are defined).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router-tests/protocol/router_plugin_test.go`:
- Around line 513-522: Add a new test case next to the existing employee
filteredProjectSummary tests that uses a mixed-case tag to lock case-insensitive
matching: create an entry named like "query employee `@requires` field with
argument — mixed-case tag" with query `{ employee(id: 1) { id
filteredProjectSummary(tag: "CLOUD") } }` and expected JSON
`{"data":{"employee":{"id":1,"filteredProjectSummary":"expertise: Backend
Architecture, filtered tags (tag=CLOUD): [cloud]"}}}` so the resolver's
case-insensitive match is asserted (update the test slice where the existing two
cases are defined).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4dcd88bb-2943-45fc-9261-4ae38f9a90a4
⛔ Files ignored due to path filters (1)
router/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
router-tests/protocol/router_plugin_test.gorouter-tests/testenv/testdata/configWithPlugins.json
…-field-args-on-required-fields-on
…-field-args-on-required-fields-on
devsergiy
approved these changes
Apr 7, 2026
…-field-args-on-required-fields-on
Noroth
approved these changes
Apr 8, 2026
dkorittki
deleted the
dominik/eng-9263-utilize-engines-support-for-field-args-on-required-fields-on
branch
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR:
This PR on the engine adds grpc support for fields with arguments and the @requires directive. Here we reference this engine version, so it's supported by the router as well. Once the engine PR gets merged this PR gets updated to use the latest engine version including that feature. I also added some router-tests. Cosmo docs will be updated seperately.
It's a lot of LOC change but only because it involves code generation. The actual changes are small.
Summary by CodeRabbit
New Features
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.