feat(router): add support for @requires in grpc#2610
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an external Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
…-with-required-fields-in-cosmo
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 #2610 +/- ##
==========================================
+ Coverage 1.59% 62.41% +60.81%
==========================================
Files 296 244 -52
Lines 41472 25826 -15646
Branches 432 0 -432
==========================================
+ Hits 662 16119 +15457
+ Misses 40524 8326 -32198
- Partials 286 1381 +1095
🚀 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: 1
🤖 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 493-537: The handler RequireEmployeeTaggedProjectSummaryById
currently ignores the required external field ctx.Fields.Tag in the successful
path and silently falls back to it only on ID parse errors; change the logic so
the RPC actually depends on ctx.Fields.Tag (or rejects requests missing/invalid
tag), e.g. validate ctx.Fields.Tag early and return an error if it's empty or
malformed, or incorporate ctx.Fields.Tag into the built summary
(RequireEmployeeTaggedProjectSummaryByIdResult.TaggedProjectSummary) instead of
only using internal project tags; ensure you reference ctx.Key.Id for parsing,
ctx.Fields.Tag for the required field, and return/propagate failures via the
RequireEmployeeTaggedProjectSummaryByIdResponse error path so `@requires` is
enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4afa476-87a3-4320-bd4c-80eea6983a3c
⛔ 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 (10)
demo/pkg/subgraphs/projects/src/schema.graphqldemo/pkg/subgraphs/projects/src/service/service.gorouter-tests/go.modrouter-tests/grpc_subgraph_test.gorouter-tests/router_plugin_test.gorouter-tests/testenv/testdata/config.jsonrouter-tests/testenv/testdata/configWithGRPC.jsonrouter-tests/testenv/testdata/configWithPlugins.jsonrouter/core/factoryresolver.gorouter/go.mod
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 499-526: This loop dereferences ctx.Key.Id and ctx.Fields.Tag
without nil checks which can panic; before calling strconv.ParseInt on
ctx.Key.Id validate ctx.Key != nil and ctx.Key.Id != "" and return
status.Errorf(codes.InvalidArgument, ...) on invalid input (replace the
fmt.Errorf) and before using ctx.Fields.Tag verify ctx.Fields != nil (and
ctx.Fields.Tag presence as needed), then proceed to compute projectTags and
build summary; update error returns to use status.Errorf(codes.InvalidArgument,
"invalid employee id %q: %v", ctx.Key.Id, err) to match other handlers.
In `@router-tests/grpc_subgraph_test.go`:
- Around line 288-296: The test is asserting an empty "employee tag" because the
employee fixture (id 1) leaves the proto3 Tag string uninitialized; set the
employee fixture's Tag field to a non-empty value (e.g., "teamA" or similar) so
the forwarding/hydration path is exercised, then update the test case named
"query employee `@requires` field resolved with tag from employees subgraph" to
expect that tag value in the taggedProjectSummary JSON (replace the empty
"employee tag: ," substring with the chosen non-empty value).
In `@router-tests/router_plugin_test.go`:
- Around line 478-486: The test uses an empty sentinel for the employee's tag
which hides whether the router forwards the `@requires` field; update the test
fixture so employee with id 1 has a non-empty tag (so ctx.Fields.Tag is
populated) and change the expected string in the test case (the entry with name
"query employee `@requires` field resolved with tag from employees subgraph") to
include that tag value instead of the empty string; locate the test case in
router_plugin_test.go and the resolver reference to ctx.Fields.Tag to confirm
the exact formatted output to assert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d16e5b95-9c34-42b1-b921-29c204f492a0
📒 Files selected for processing (3)
demo/pkg/subgraphs/projects/src/service/service.gorouter-tests/grpc_subgraph_test.gorouter-tests/router_plugin_test.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
demo/pkg/subgraphs/projects/src/service/service.go (1)
499-503:⚠️ Potential issue | 🔴 CriticalGuard nullable protobuf submessages and return a gRPC status error.
Line 500 and Line 522 dereference protobuf submessages before validating them, so a malformed context item can panic this handler. Line 502 also returns
fmt.Errorf, which turns invalid input into a generic gRPCUnknowninstead ofInvalidArgument.🛠️ Suggested fix
- for _, ctx := range req.Context { + for i, ctx := range req.Context { + if ctx == nil || ctx.Key == nil || ctx.Fields == nil { + return nil, status.Errorf(codes.InvalidArgument, "context[%d] is missing key or required fields", i) + } + id, err := strconv.ParseInt(ctx.Key.Id, 10, 32) if err != nil { - return nil, fmt.Errorf("failed to parse employee id %q: %w", ctx.Key.Id, err) + return nil, status.Errorf(codes.InvalidArgument, "invalid employee id %q: %v", ctx.Key.Id, err) }#!/bin/bash set -euo pipefail rg -n -C2 'type RequireEmployeeTaggedProjectSummaryByIdContext struct|Key .*|Fields .*' demo/pkg/subgraphs/projects/generated/service.pb.go rg -n -C3 'RequireEmployeeTaggedProjectSummaryById|fmt\.Errorf|status\.Errorf' demo/pkg/subgraphs/projects/src/service/service.goAlso applies to: 522-522
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/pkg/subgraphs/projects/src/service/service.go` around lines 499 - 503, The handler is dereferencing protobuf submessages without null checks and returns fmt.Errorf which yields gRPC Unknown; update the loop over req.Context to guard nullable fields (check ctx != nil and ctx.Key != nil before using ctx.Key.Id) and similarly protect any other dereferences in the same function (e.g., the other deref near line 522), and replace fmt.Errorf returns for invalid input with status.Errorf(codes.InvalidArgument, ...) so invalid/malformed context items produce a proper InvalidArgument gRPC error; look for RequireEmployeeTaggedProjectSummaryByIdContext and the fmt.Errorf usages in this handler to locate the spots to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@demo/pkg/subgraphs/projects/src/service/service.go`:
- Around line 499-503: The handler is dereferencing protobuf submessages without
null checks and returns fmt.Errorf which yields gRPC Unknown; update the loop
over req.Context to guard nullable fields (check ctx != nil and ctx.Key != nil
before using ctx.Key.Id) and similarly protect any other dereferences in the
same function (e.g., the other deref near line 522), and replace fmt.Errorf
returns for invalid input with status.Errorf(codes.InvalidArgument, ...) so
invalid/malformed context items produce a proper InvalidArgument gRPC error;
look for RequireEmployeeTaggedProjectSummaryByIdContext and the fmt.Errorf
usages in this handler to locate the spots to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d7815b5-bcd2-47a1-b7fd-bb99baf56632
📒 Files selected for processing (4)
demo/pkg/subgraphs/employees/subgraph/employees.godemo/pkg/subgraphs/projects/src/service/service.gorouter-tests/grpc_subgraph_test.gorouter-tests/router_plugin_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- router-tests/grpc_subgraph_test.go
- router-tests/router_plugin_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 499-529: The code currently defaults a missing required field to
"none" by setting employeeTag := ctx.GetFields().GetTag(); if employeeTag == ""
{ employeeTag = "none" }, which masks missing `@requires` data; instead, detect an
empty tag after calling ctx.GetFields().GetTag() and return an error using
status.Errorf(codes.InvalidArgument, "missing tag in requires field") (or
similar) so the RPC fails fast; update the handler that iterates req.Context and
uses ctx/GetFields/GetTag (the employeeTag variable) to return InvalidArgument
when empty rather than assigning a default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c646e234-5ffa-47e3-801f-b92b09db30bc
📒 Files selected for processing (1)
demo/pkg/subgraphs/projects/src/service/service.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
demo/pkg/subgraphs/employees/subgraph/schema.resolvers.go (2)
186-194:⚠️ Potential issue | 🟡 MinorMissing
Expertisefield inFirstEmployeereturn value.Similar to
UpdateEmployeeTag, theFirstEmployeeresolver doesn't includeExpertisein the constructedEmployeeobject, creating an inconsistency with other resolvers.Suggested fix
employee := &model.Employee{ ID: Employees[0].ID, Details: Employees[0].Details, Tag: Employees[0].Tag, + Expertise: Employees[0].Expertise, Role: Employees[0].Role, Notes: Employees[0].Notes, UpdatedAt: time.Now().String(), StartDate: Employees[0].StartDate, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/pkg/subgraphs/employees/subgraph/schema.resolvers.go` around lines 186 - 194, The constructed Employee returned by the FirstEmployee resolver is missing the Expertise field; update the Employee initialization in FirstEmployee (the model.Employee literal created from Employees[0]) to include Expertise: Employees[0].Expertise so it matches other resolvers like UpdateEmployeeTag and maintains consistent returned shape.
70-78:⚠️ Potential issue | 🟡 MinorMissing
Expertisefield inUpdateEmployeeTagreturn value.The
UpdateEmployeeTagmutation constructs a newEmployeeobject but doesn't include theExpertisefield, while other resolvers (e.g.,Employee,Employees) now do. This inconsistency could cause queries that fetchexpertiseafter a tag update to return an empty string.Suggested fix
return &model.Employee{ ID: employee.ID, Details: details, Tag: tag, + Expertise: employee.Expertise, Role: employee.Role, Notes: employee.Notes, UpdatedAt: time.Now().String(), StartDate: employee.StartDate, }, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/pkg/subgraphs/employees/subgraph/schema.resolvers.go` around lines 70 - 78, The UpdateEmployeeTag resolver builds and returns a model.Employee but omits the Expertise field; modify the returned struct in UpdateEmployeeTag to include Expertise: employee.Expertise (matching other resolvers like Employee/Employees) so the updated payload includes the employee's expertise after a tag update.
🧹 Nitpick comments (1)
demo/pkg/subgraphs/projects/src/service/service.go (1)
526-529: Consider rejecting missingexpertiseinstead of defaulting.The fallback to
"none"whenexpertiseis empty could mask a broken@requiresforwarding path. Since this RPC exists specifically to resolve a field that depends on the externalexpertisefield, silently using a default defeats the purpose of validating that the router correctly forwards required data.The tests currently use employees with non-empty
Expertisevalues (e.g., "Backend Architecture"), so this won't hide bugs today, but it reduces confidence that the@requirescontract is enforced.Suggested fix
employeeExpertise := ctx.GetFields().GetExpertise() if employeeExpertise == "" { - employeeExpertise = "none" + return nil, status.Errorf(codes.InvalidArgument, "missing required expertise field for employee id %q", keyID) }Based on learnings: "fields with the requires directive are intentionally resolved by separate dedicated RPCs so the request must carry the external dependencies they depend on."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/pkg/subgraphs/projects/src/service/service.go` around lines 526 - 529, The code currently defaults an empty expertise to "none" (ctx.GetFields().GetExpertise()) which can mask missing forwarded `@requires` data; remove the fallback and instead return an error when expertise is empty by validating ctx.GetFields().GetExpertise() at the start of the resolver (the function handling this RPC in service.go) and returning an appropriate failure (e.g., InvalidArgument or Unauthenticated) with a clear message if it's empty so callers/router must forward the required field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@demo/pkg/subgraphs/employees/subgraph/schema.resolvers.go`:
- Around line 186-194: The constructed Employee returned by the FirstEmployee
resolver is missing the Expertise field; update the Employee initialization in
FirstEmployee (the model.Employee literal created from Employees[0]) to include
Expertise: Employees[0].Expertise so it matches other resolvers like
UpdateEmployeeTag and maintains consistent returned shape.
- Around line 70-78: The UpdateEmployeeTag resolver builds and returns a
model.Employee but omits the Expertise field; modify the returned struct in
UpdateEmployeeTag to include Expertise: employee.Expertise (matching other
resolvers like Employee/Employees) so the updated payload includes the
employee's expertise after a tag update.
---
Nitpick comments:
In `@demo/pkg/subgraphs/projects/src/service/service.go`:
- Around line 526-529: The code currently defaults an empty expertise to "none"
(ctx.GetFields().GetExpertise()) which can mask missing forwarded `@requires`
data; remove the fallback and instead return an error when expertise is empty by
validating ctx.GetFields().GetExpertise() at the start of the resolver (the
function handling this RPC in service.go) and returning an appropriate failure
(e.g., InvalidArgument or Unauthenticated) with a clear message if it's empty so
callers/router must forward the required field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e3be140-73d2-4618-81e4-2677b53f17a4
⛔ Files ignored due to path filters (4)
demo/pkg/subgraphs/employees/subgraph/generated/generated.gois excluded by!**/generated/**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/**
📒 Files selected for processing (13)
demo/pkg/subgraphs/employees/subgraph/employees.godemo/pkg/subgraphs/employees/subgraph/entity.resolvers.godemo/pkg/subgraphs/employees/subgraph/model/models_gen.godemo/pkg/subgraphs/employees/subgraph/schema.graphqlsdemo/pkg/subgraphs/employees/subgraph/schema.resolvers.godemo/pkg/subgraphs/projects/src/schema.graphqldemo/pkg/subgraphs/projects/src/service/service.gorouter-tests/grpc_subgraph_test.gorouter-tests/router_plugin_test.gorouter-tests/testenv/testdata/config.jsonrouter-tests/testenv/testdata/configWithEdfs.jsonrouter-tests/testenv/testdata/configWithGRPC.jsonrouter-tests/testenv/testdata/configWithPlugins.json
🚧 Files skipped from review as they are similar to previous changes (1)
- demo/pkg/subgraphs/projects/src/schema.graphql
dkorittki
reviewed
Mar 10, 2026
…-with-required-fields-in-cosmo
Noroth
requested review from
a team,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
…-with-required-fields-in-cosmo
…-with-required-fields-in-cosmo
endigma
approved these changes
Mar 11, 2026
…-with-required-fields-in-cosmo
Noroth
deleted the
ludwig/eng-9128-update-demo-schema-and-tests-with-required-fields-in-cosmo
branch
This was referenced Mar 25, 2026
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.
Summary by CodeRabbit
New Features
Tests
Chores
Checklist