feat(router): add support for composite types in requires fields#2689
Merged
feat(router): add support for composite types in requires fields#2689
Conversation
Noroth
requested review from
a team,
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 selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR extends a GraphQL federation demo by introducing work-related type definitions and fields for employees. It adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 #2689 +/- ##
===========================================
+ Coverage 45.48% 63.10% +17.62%
===========================================
Files 1028 245 -783
Lines 138352 26273 -112079
Branches 8594 0 -8594
===========================================
- Hits 62925 16580 -46345
+ Misses 73710 8352 -65358
+ Partials 1717 1341 -376 🚀 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: 6
🧹 Nitpick comments (1)
router-tests/protocol/grpc_subgraph_test.go (1)
398-401: Verify ifPrintQueryPlansshould remain enabled.
cfg.Debug.PrintQueryPlans = truewas added along with the test cases. This is useful for debugging but may add noise to test output in CI. Consider whether this should remain enabled or be removed before merge.If debug output is not needed in CI:
testenv.Run(t, &testenv.Config{ RouterConfigJSONTemplate: testenv.ConfigWithGRPCJSONTemplate, - ModifyEngineExecutionConfiguration: func(cfg *config.EngineExecutionConfiguration) { - cfg.Debug.PrintQueryPlans = true - }, EnableGRPC: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/protocol/grpc_subgraph_test.go` around lines 398 - 401, The test enables verbose plan printing by setting cfg.Debug.PrintQueryPlans = true inside ModifyEngineExecutionConfiguration which may clutter CI logs; change this to false (or remove the assignment) so tests run with PrintQueryPlans disabled, or gate it behind a CI-debug flag (e.g., check an env var in ModifyEngineExecutionConfiguration and only set cfg.Debug.PrintQueryPlans = true when that flag is present) to preserve the option for local debugging.
🤖 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/schema.graphql`:
- Line 270: The WorkReviewResult union definition is missing the `@shareable`
directive; update the union declaration for WorkReviewResult (which composes
WorkApproval and WorkRejection) to include `@shareable` so it matches the
employees subgraph and allows proper federation composition. Locate the union
named WorkReviewResult and append the `@shareable` directive to its declaration.
In `@demo/pkg/subgraphs/projects/src/service/service.go`:
- Around line 624-648: In RequireEmployeeWorkItemHandlerInfoById, guard against
a nil primary work item and nil instance before doing the type switch: after
retrieving item := ctx.GetFields().GetPrimaryWorkItem(), check if item == nil ||
item.GetInstance() == nil and set info = "Unknown handler" (or equivalent) and
skip the type switch; otherwise perform the existing switch on
item.GetInstance() to populate info and append the
service.RequireEmployeeWorkItemHandlerInfoByIdResult. Ensure you reference the
same variables (ctx, item, info) and return the response as before.
- Around line 571-595: In RequireEmployeeReviewReportById, guard against nil
lastWorkReview before calling review.GetValue(): check
ctx.GetFields().GetLastWorkReview() for nil (and that review.GetValue() is
non-nil) and handle that case by producing a default "Unknown review" (or
similar) report instead of dereferencing; update the switch on review.GetValue()
to only run when non-nil and fall back to the default report when review is nil
or value is nil, keeping the existing result append logic.
- Around line 597-622: In RequireEmployeeWorkSetupSummaryById, guard against a
nil work setup returned by ctx.GetFields().GetWorkSetup(): check that setup !=
nil before calling setup.GetPrimaryItem() and setup.GetPriority(); if setup is
nil, use a safe default (e.g., itemSummary "Unknown work item" and priority from
a default or empty string) or append a result that indicates missing work setup,
then continue building results to avoid a panic when accessing
setup.GetPriority().
- Around line 545-569: The handler RequireEmployeeWorkItemInfoById may
dereference a nil primary work item; before calling item.GetInstance() inside
the loop over req.GetContext(), check whether ctx.GetFields()==nil or item :=
ctx.GetFields().GetPrimaryWorkItem(); item==nil and handle that case (e.g. set
summary = "No work item" or skip) so you avoid a nil pointer panic; update the
switch to only run when item is non-nil and construct the
service.RequireEmployeeWorkItemInfoByIdResult accordingly.
- Around line 680-713: The handler RequireEmployeeDeepWorkItemInfoById risks nil
derefs when accessing nested fields; defend by checking for nil before using
item := ctx.GetFields().GetPrimaryWorkItem() (skip or set "Unknown deep item" if
nil), then when handling the technical branch validate handler :=
v.TechnicalWorkItem.GetHandler() is non-nil before calling
handler.GetAssignedItem(), and validate assignedItem is non-nil before switching
on assignedItem.GetInstance(); on any nil, set a clear fallback info string
(e.g., "Unknown deep item" or "TechnicalHandler->Unknown") and continue,
updating the branches that reference av (ManagementWorkItem / TechnicalWorkItem)
only after confirming av is non-nil as well.
---
Nitpick comments:
In `@router-tests/protocol/grpc_subgraph_test.go`:
- Around line 398-401: The test enables verbose plan printing by setting
cfg.Debug.PrintQueryPlans = true inside ModifyEngineExecutionConfiguration which
may clutter CI logs; change this to false (or remove the assignment) so tests
run with PrintQueryPlans disabled, or gate it behind a CI-debug flag (e.g.,
check an env var in ModifyEngineExecutionConfiguration and only set
cfg.Debug.PrintQueryPlans = true when that flag is present) to preserve the
option for local debugging.
🪄 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: d57d7520-1287-4d89-9f3d-93f37d3e5c63
⛔ Files ignored due to path filters (8)
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/**demo/pkg/subgraphs/projects/generated/service.proto.lock.jsonis 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 (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/go.modrouter-tests/protocol/grpc_subgraph_test.gorouter-tests/protocol/router_plugin_test.gorouter-tests/testenv/testdata/configWithGRPC.jsonrouter-tests/testenv/testdata/configWithPlugins.jsonrouter/go.mod
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
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 `@router-tests/operations/testdata/introspection/base-graph-schema.json`:
- Around line 2704-2719: The introspection JSON incorrectly records several
SDL-backed scalar types as String; update the introspection entries for the
fields approvedAt (WorkApproval), rejectionCode (WorkRejection), and priority
(WorkSetup) so their "type" objects reflect the correct NON_NULL SCALAR names
(DateTime for approvedAt, Int for rejectionCode and priority) instead of
"String", and ensure the DateTime scalar appears in the schema "types" list so
it is included in snapshots; apply the same correction to the other occurrences
noted (around the other ranges referenced).
- Around line 2425-2440: The introspection fixture incorrectly types
WorkSetup.priority as Int! whereas the demo SDL declares it as String!;
regenerate the introspection fixture from the composed schema so the
WorkSetup.priority entry (and any other mismatched scalar fields) reflect the
composed schema types (String!), replacing the incorrect "Int" kind/ofType
entries with the correct scalar "String" non-null structure in the fixture.
🪄 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: 178c1b7b-0ea8-4f97-a019-ad50845f69d6
⛔ Files ignored due to path filters (1)
demo/pkg/subgraphs/employees/subgraph/generated/generated.gois excluded by!**/generated/**
📒 Files selected for processing (4)
router-tests/operations/testdata/introspection/base-graph-schema.jsonrouter-tests/operations/testdata/introspection/feature-graph-schema.jsonrouter-tests/testenv/testdata/config.jsonrouter-tests/testenv/testdata/configWithEdfs.json
…226-add-composite-type-support-in-cosmo
endigma
approved these changes
Mar 27, 2026
This was referenced Mar 27, 2026
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
2 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.
This PR adds support for using composite types in
@requiresin the routerSummary by CodeRabbit
Release Notes
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.