Conversation
ysmolski
requested review from
Noroth,
StarpTech,
alepane21,
devsergiy and
jensneuse
as code owners
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (2)
⛔ Files ignored due to path filters (1)
You can disable this status message by setting the WalkthroughBump graphql-go-tools to v2.0.0-rc.220; add PlanningBehavior() on PlannerFactory types and remove Planner.DataSourcePlanningBehavior(); adjust fake test signatures and add PlanningBehavior; extend demo projects schema and add ProjectsService.QueryNodesById; format generated JSON in demo Makefile; add gRPC test for nodesByID inline fragments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
ysmolski
force-pushed
the
yury/eng-7487-enforce-rewriting-abstract-fragments-for-grpc-datasource
branch
from
44a45dc to
1fe945a
Compare
ysmolski
force-pushed
the
yury/eng-7487-enforce-rewriting-abstract-fragments-for-grpc-datasource
branch
from
1fe945a to
36b63b4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
demo/pkg/subgraphs/projects/src/service/service.go (1)
606-624: New gRPC handler for Named interface looks correct; consider preallocating the sliceThe read lock usage and Named wrapper construction are consistent with the rest of the service and the generated types. Minor perf/readability nit: preallocate the slice to avoid reallocations.
Apply this diff to preallocate:
- // Populate relationships for all projects - var populatedProjects []*service.Named + // Populate relationships for all projects + populatedProjects := make([]*service.Named, 0, len(data.ServiceProjects)) for _, project := range data.ServiceProjects { n := &service.Named{ Instance: &service.Named_Project{ Project: p.populateProjectRelationships(project), }, } populatedProjects = append(populatedProjects, n) }demo/pkg/subgraphs/projects/src/schema.graphql (1)
110-111: Comment doesn’t match the field’s nullabilityThe current type
priorityMatrix: [[[Task!]!]!]has a nullable outermost list, while the comment states “non-nullable list of non-nullable lists of non-nullable lists.”Consider fixing the comment to reflect the actual type:
- priorityMatrix: [[[Task!]!]!] # triple nested: non-nullable list of non-nullable lists of non-nullable lists + priorityMatrix: [[[Task!]!]!] # triple nested: nullable outer list; inner lists and items are non-nullIf you intended the outer list to be non-null, change the type instead:
- priorityMatrix: [[[Task!]!]!] + priorityMatrix: [[[Task!]!]!]!router-tests/grpc_subgraph_test.go (1)
136-152: Good test to exercise nested inline fragments on interfacesThis covers interface fragment rewriting for gRPC by nesting a fragment on
Projectinside a fragment onTimestamped. It validates both id and name selection paths.Optional: add a companion test that also selects
__typenamewithininterfaceNamedto assert type resolution after rewriting. This would further harden the coverage for fragment rewriting across interface boundaries without altering this test’s expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
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 (9)
demo/Makefile(1 hunks)demo/pkg/subgraphs/projects/src/schema.graphql(7 hunks)demo/pkg/subgraphs/projects/src/service/service.go(1 hunks)router-tests/go.mod(1 hunks)router-tests/grpc_subgraph_test.go(1 hunks)router/go.mod(1 hunks)router/pkg/graphqlschemausage/schemausage_test.go(3 hunks)router/pkg/pubsub/datasource/factory.go(1 hunks)router/pkg/pubsub/datasource/planner.go(0 hunks)
💤 Files with no reviewable changes (1)
- router/pkg/pubsub/datasource/planner.go
✅ Files skipped from review due to trivial changes (2)
- demo/Makefile
- router/go.mod
🚧 Files skipped from review as they are similar to previous changes (3)
- router/pkg/pubsub/datasource/factory.go
- router-tests/go.mod
- router/pkg/graphqlschemausage/schemausage_test.go
⏰ 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). (1)
- GitHub Check: integration_test (./events)
🔇 Additional comments (3)
demo/pkg/subgraphs/projects/src/schema.graphql (3)
30-31: Expose Named interface via Query: aligned with service implementationThe new field
interfaceNamed: [Named!]!matches the new RPC handler and enables exercising inline fragments on interfaces. No concerns.
71-74: New Named interface is straightforward and sufficientDefining a minimal
Namedinterface withname: String!is appropriate for the test coverage you’re adding.
85-85: Project implements Named: consistent with schema and serviceAdding
NamedtoProject’s implements list keeps the contract coherent with theinterfaceNamedquery and the service’s Named_Project wrapper.
Integrates a feature of the engine, that always rewrites inline fragments for gRPC datasource. Integration test required adding the "Named" interface and new query field "interfaceNamed".
ysmolski
force-pushed
the
yury/eng-7487-enforce-rewriting-abstract-fragments-for-grpc-datasource
branch
from
36b63b4 to
135f5ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
demo/pkg/subgraphs/projects/src/schema.graphql (1)
71-74: Consider whetherNodecould cover this use case (acknowledging prior discussion).A prior review suggested using
Nodeand exposing a generic entry point (e.g.,nodesById) to retrieve a list of interface-typed values. Given the PR’s goal to test interface inline fragment rewriting specifically onNamed, keepingNamedis reasonable. If broader reuse is a goal,Nodecould reduce surface area.
🧹 Nitpick comments (5)
demo/Makefile (1)
20-20: Pin Prettier for reproducible formatting across environmentsUsing an unpinned CLI version can yield inconsistent diffs across machines/CI over time. Pin the Prettier version to stabilize output.
Apply this diff:
- pnpx prettier ./configWithPlugins.json -w + pnpx prettier@3.3.3 ./configWithPlugins.json -wOptionally, add Prettier as a devDependency in the workspace and call it via pnpm scripts to avoid on-the-fly downloads.
router/pkg/graphqlschemausage/schemausage_test.go (3)
539-546: Nit: avoid spawning a no-op goroutineStart() is a no-op; launching a goroutine is unnecessary overhead in tests. Call it directly or document why concurrency is required.
Apply this diff to remove the unnecessary goroutine:
- go source.Start() + // Start is a no-op in tests; call directly to avoid spawning an unnecessary goroutine. + source.Start()
598-600: Nit: make the zero return explicit for readabilityReturning named zero values is fine, but being explicit improves clarity.
Apply this diff:
- return + return "", false
591-596: Optional: remove planner-level DataSourcePlanningBehavior to mirror API shiftWith PlanningBehavior() moved to the factory, keeping this method on the planner can cause drift/confusion. If no interface requires it anymore, consider removing it.
Apply this diff to remove the redundant method:
-func (f *FakePlanner[T]) DataSourcePlanningBehavior() plan.DataSourcePlanningBehavior { - return plan.DataSourcePlanningBehavior{ - MergeAliasedRootNodes: false, - OverrideFieldPathFromAlias: false, - } -}Before removing, please confirm no other tests or build tags still depend on the planner-level method in this repo. If you want, I can provide a script to grep usages across the codebase.
demo/pkg/subgraphs/projects/src/service/service.go (1)
686-704: Preallocate slice capacity and add consistent logging.Minor perf/readability tweaks and consistency with existing query handlers.
Apply this diff:
func (p *ProjectsService) QueryInterfaceNamed(ctx context.Context, req *service.QueryInterfaceNamedRequest) (*service.QueryInterfaceNamedResponse, error) { - p.lock.RLock() - defer p.lock.RUnlock() + logger := hclog.FromContext(ctx) + logger.Info("QueryInterfaceNamed") + + p.lock.RLock() + defer p.lock.RUnlock() // Populate relationships for all projects - var populatedProjects []*service.Named + populatedProjects := make([]*service.Named, 0, len(data.ServiceProjects)) for _, project := range data.ServiceProjects { n := &service.Named{ - Instance: &service.Named_Project{ + Instance: &service.Named_Project{ Project: p.populateProjectRelationships(project), }, } populatedProjects = append(populatedProjects, n) } return &service.QueryInterfaceNamedResponse{InterfaceNamed: populatedProjects}, nil }Note: The field
Instanceis left as-is; see the separate comment to verify whether it should beValue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
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 (9)
demo/Makefile(2 hunks)demo/pkg/subgraphs/projects/src/schema.graphql(7 hunks)demo/pkg/subgraphs/projects/src/service/service.go(1 hunks)router-tests/go.mod(1 hunks)router-tests/grpc_subgraph_test.go(1 hunks)router/go.mod(1 hunks)router/pkg/graphqlschemausage/schemausage_test.go(3 hunks)router/pkg/pubsub/datasource/factory.go(1 hunks)router/pkg/pubsub/datasource/planner.go(0 hunks)
💤 Files with no reviewable changes (1)
- router/pkg/pubsub/datasource/planner.go
🚧 Files skipped from review as they are similar to previous changes (3)
- router/pkg/pubsub/datasource/factory.go
- router-tests/go.mod
- router-tests/grpc_subgraph_test.go
⏰ 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_test
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
router/pkg/graphqlschemausage/schemausage_test.go (4)
531-533: Good: underscore for unused parameter clarifies intentRenaming the parameter to the blank identifier communicates it’s unused and silences linters. Implementation unchanged and correct.
535-538: PlanningBehavior on FakeFactory added correctly with sane defaultsAdding PlanningBehavior() on the factory aligns with the updated planning API. Returning the zero-value DataSourcePlanningBehavior matches the explicit false/false in the planner below and preserves behavior.
566-568: Good: underscore for unused parameter clarifies intentConsistent with the factory change; keeps the signature tidy and silences linters.
570-572: Good: underscore placeholders for unused EnterDocument paramsMatches the visitor registration use and avoids unused warnings. No functional change.
router/go.mod (1)
34-34: Dependency bump to graphql-go-tools rc.220 looks goodThe version update aligns with the PR intent to leverage new planning behavior and fragment rewrite support.
demo/pkg/subgraphs/projects/src/service/service.go (1)
686-704: ConfirmInstanceIs the Correct Oneof Field onservice.NamedVerified that in
generated/service.pb.gotheNamedmessage defines its oneof with the field nameInstance(notValue). The existing usage ofInstance: &service.Named_Project{…}inQueryInterfaceNamedaligns with the generated API. No changes are required.demo/pkg/subgraphs/projects/src/schema.graphql (4)
30-31: New root fieldinterfaceNamedis aligned with the new RPC. LGTM.The schema addition matches the new
QueryInterfaceNamedhandler in the service and enables the interface inline fragment test.
85-111: Project now implementsNamedand adds nested list fields — service coverage looks correct.The service’s
populateProjectRelationshipspopulates all of these fields (dependencies, resourceGroups, tasksByPhase, milestoneGroups, priorityMatrix) and preservestags/alternativeProjects. This aligns with the schema changes.
124-128: PopulateMilestoneRelationships now covers the new schema fields
I’ve confirmed thatPopulateMilestoneRelationshipsassigns all three updated fields:
- Dependencies via
GetMilestoneDependencies(milestone.Id)- Subtasks via
GetMilestoneSubtasks(milestone.Id)- Reviewers via
GetMilestoneReviewers(milestone.Id)
144-150: PopulateTaskRelationships correctly populates all new Task fieldsPopulateTaskRelationships already handles every added field:
- Labels: assigned from task.Labels
- AttachmentUrls: assigned from task.AttachmentUrls
- ReviewerIds: assigned from task.ReviewerIds
- Subtasks: loaded via GetTaskSubtasks(task.Id)
- Dependencies: loaded via GetTaskDependencies(task.Id)
No further changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
demo/pkg/subgraphs/projects/src/service/service.go (1)
686-734: QueryNodesById: Correct, thread-safe implementation; consider tiny nits for efficiency and clarity
- Good use of RLock and deterministic iteration order (projects → milestones → tasks → updates), which matches the test’s expected order.
- For a micro-optimization and clearer intent, preallocate the slice (max 4 entries) and early-return on empty IDs.
Apply this diff:
func (p *ProjectsService) QueryNodesById(ctx context.Context, req *service.QueryNodesByIdRequest) (*service.QueryNodesByIdResponse, error) { logger := hclog.FromContext(ctx) logger.Info("QueryNodesById", "id", req.Id) p.lock.RLock() defer p.lock.RUnlock() - var nodes []*service.Node + nodes := make([]*service.Node, 0, 4) + if req.Id == "" { + return &service.QueryNodesByIdResponse{NodesById: nodes}, nil + } for _, project := range data.ServiceProjects { if project.Id == req.Id { nodes = append(nodes, &service.Node{ Instance: &service.Node_Project{ Project: p.populateProjectRelationships(project), }, }) } } for _, milestone := range data.ServiceMilestones { if milestone.Id == req.Id { nodes = append(nodes, &service.Node{ Instance: &service.Node_Milestone{ Milestone: data.PopulateMilestoneRelationships(milestone), }, }) } } for _, task := range data.ServiceTasks { if task.Id == req.Id { nodes = append(nodes, &service.Node{ Instance: &service.Node_Task{ Task: data.PopulateTaskRelationships(task), }, }) } } for _, update := range data.ServiceProjectUpdates { if update.Id == req.Id { - nodes = append(nodes, &service.Node { + nodes = append(nodes, &service.Node{ Instance: &service.Node_ProjectUpdate{ ProjectUpdate: p.populateProjectUpdateRelationships(update), }, }) } } return &service.QueryNodesByIdResponse{NodesById: nodes}, nil }demo/pkg/subgraphs/projects/src/schema.graphql (2)
30-30: nodesByID: Good move to return the Node interface; consider naming consistencyReturning [Node!]! aligns well with the interface-based test and the engine’s fragment rewriting behavior. For consistency with the rest of the schema (e.g., projectId, projectsByStatus), consider renaming nodesByID → nodesById.
If you agree, update the schema and tests:
- nodesByID(id: ID!): [Node!]! + nodesById(id: ID!): [Node!]!
99-107: Comment vs. type mismatch for priorityMatrix nullabilityThe comment states “non-nullable list of non-nullable lists of non-nullable lists,” but the type is
[[[Task!]!]!](outer list appears nullable). Either update the type to match the comment or adjust the comment to reflect actual nullability.Apply one of the following:
- Option A (fix the comment to match the current type):
- priorityMatrix: [[[Task!]!]!] # triple nested: non-nullable list of non-nullable lists of non-nullable lists + priorityMatrix: [[[Task!]!]!] # triple nested: nullable outer list, with non-null lists of non-null lists of non-null Tasks
- Option B (change the type to match the existing comment):
- priorityMatrix: [[[Task!]!]!] + priorityMatrix: [[[Task!]!]!]!router-tests/grpc_subgraph_test.go (1)
136-165: Inline fragments on interface: solid coverage; consider minor robustness and naming consistency
- Test correctly validates fragment rewriting on the Node interface and nested interface (Timestamped).
- To align with schema naming conventions used elsewhere (Id vs ID), consider renaming nodesByID to nodesById in both the query and expected JSON, if you decide to adopt the schema rename.
If you choose to rename the field, apply:
- query { - nodesByID(id: 1) { + query { + nodesById(id: 1) { __typename ... on Project { id } ... on Milestone { id } ... on Task { id } ... on ProjectUpdate { id } ... on Timestamped { ... on Project { name } ... on Milestone { name } } } }`, - expected: `{"data":{"nodesByID":[{"__typename":"Project","id":"1","name":"Cloud Migration Overhaul"},{"__typename":"Milestone","id":"1","name":"Infrastructure Assessment"},{"__typename":"Task","id":"1"},{"__typename":"ProjectUpdate","id":"1"}]}}`, + expected: `{"data":{"nodesById":[{"__typename":"Project","id":"1","name":"Cloud Migration Overhaul"},{"__typename":"Milestone","id":"1","name":"Infrastructure Assessment"},{"__typename":"Task","id":"1"},{"__typename":"ProjectUpdate","id":"1"}]}}`,Optionally, to reduce brittleness against dataset changes, you could assert on membership rather than strict array order in a follow-up refactor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
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/**
📒 Files selected for processing (4)
demo/Makefile(2 hunks)demo/pkg/subgraphs/projects/src/schema.graphql(6 hunks)demo/pkg/subgraphs/projects/src/service/service.go(1 hunks)router-tests/grpc_subgraph_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- demo/Makefile
🧰 Additional context used
🧬 Code Graph Analysis (1)
demo/pkg/subgraphs/projects/src/service/service.go (4)
demo/pkg/subgraphs/projects/generated/service.pb.go (29)
QueryNodesByIdRequest(3116-3122)QueryNodesByIdRequest(3137-3137)QueryNodesByIdRequest(3152-3154)QueryNodesByIdResponse(3164-3170)QueryNodesByIdResponse(3185-3185)QueryNodesByIdResponse(3200-3202)Node(4553-4565)Node(4580-4580)Node(4595-4597)Node_Project(4638-4640)Node_Project(4654-4654)Project(3603-3627)Project(3642-3642)Project(3657-3659)Node_Milestone(4642-4644)Node_Milestone(4656-4656)Milestone(3794-3810)Milestone(3825-3825)Milestone(3840-3842)Node_Task(4646-4648)Node_Task(4658-4658)Task(3921-3943)Task(3958-3958)Task(3973-3975)Node_ProjectUpdate(4650-4652)Node_ProjectUpdate(4660-4660)ProjectUpdate(4915-4927)ProjectUpdate(4942-4942)ProjectUpdate(4957-4959)demo/pkg/subgraphs/projects/src/data/projects.go (1)
ServiceProjects(8-156)demo/pkg/subgraphs/projects/src/data/milestones.go (1)
PopulateMilestoneRelationships(210-226)demo/pkg/subgraphs/projects/src/data/tasks.go (1)
PopulateTaskRelationships(326-349)
⏰ 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). (10)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
demo/pkg/subgraphs/projects/src/schema.graphql (1)
89-89: Tightened nullability on teamMembers requires resolver guarantees.[Employee!]! means:
- the list is never null, and
- no element can be null.
Ensure the resolver always returns an array (possibly empty) and never includes null entries; otherwise consider relaxing to [Employee!].
If you decide to relax it:
- teamMembers: [Employee!]! + teamMembers: [Employee!]router/pkg/graphqlschemausage/schemausage_test.go (3)
531-533: Return false when upstreamSchema is nil to respect the existence contractThe second return value typically signals presence. Returning true when upstreamSchema is nil could hide issues in future tests.
-func (f *FakeFactory[T]) UpstreamSchema(_ plan.DataSourceConfiguration[T]) (*ast.Document, bool) { - return f.upstreamSchema, true -} +func (f *FakeFactory[T]) UpstreamSchema(_ plan.DataSourceConfiguration[T]) (*ast.Document, bool) { + if f.upstreamSchema == nil { + return nil, false + } + return f.upstreamSchema, true +}
539-546: Avoid spawning a no-op goroutine in testsStart() is empty; spawning a goroutine adds noise without benefit and can complicate test teardown in the future.
func (f *FakeFactory[T]) Planner(_ abstractlogger.Logger) plan.DataSourcePlanner[T] { source := &StatefulSource{} - go source.Start() + source.Start() return &FakePlanner[T]{ source: source, upstreamSchema: f.upstreamSchema, } }
566-568: Mirror the existence check in FakePlanner.UpstreamSchemaSame rationale as the factory: signal absence if upstreamSchema is nil.
-func (f *FakePlanner[T]) UpstreamSchema(_ plan.DataSourceConfiguration[T]) (*ast.Document, bool) { - return f.upstreamSchema, true -} +func (f *FakePlanner[T]) UpstreamSchema(_ plan.DataSourceConfiguration[T]) (*ast.Document, bool) { + if f.upstreamSchema == nil { + return nil, false + } + return f.upstreamSchema, true +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
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/**
📒 Files selected for processing (4)
demo/pkg/subgraphs/projects/src/schema.graphql(6 hunks)demo/pkg/subgraphs/projects/src/service/service.go(1 hunks)router-tests/grpc_subgraph_test.go(1 hunks)router/pkg/graphqlschemausage/schemausage_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- demo/pkg/subgraphs/projects/src/service/service.go
- router-tests/grpc_subgraph_test.go
⏰ 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). (11)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
demo/pkg/subgraphs/projects/src/schema.graphql (2)
30-30: Ignore the nodesByID vs nodesById casing mismatch
Thegrpc_subgraph_test.gotests invokenodesById(…), matching the schema’s camelCase definition. No instances ofnodesByIDwere found.Likely an incorrect or invalid review comment.
205-205: ✅ Federation key type consistency verified: Employee.id remains Int! across all subgraphsAll subgraphs (
test1,employees,family,products_fg,mood,products,hobbies,availability,employeeupdated, andprojects) defineEmployee.idasInt!. No mismatches were found—no updates are required.router/pkg/graphqlschemausage/schemausage_test.go (3)
570-572: Idiomatic unused parameter silencing; LGTMBlank identifiers on EnterDocument parameters are appropriate for a no-op visitor in tests.
591-593: Zero-value alias return is fine for test doublesReturning "", false is appropriate when aliasing isn’t exercised by these tests.
535-538: No lingering Planner.DataSourcePlanningBehavior usage
- rg search confirms zero calls to
Planner.DataSourcePlanningBehavior()in the codebase.- Only two
PlanningBehavior()methods exist—onFakeFactoryandPlannerFactory—and both intentionally return the default stub.- All gRPC‐based factories are created via
graphql_datasource.NewFactoryGRPCClientProvider(in the externalgrpc_datasourcepackage), which provides its own non‐default behavior (including inline‐fragment rewrites).Looks good to merge.
ysmolski
changed the title
ysmolski
deleted the
yury/eng-7487-enforce-rewriting-abstract-fragments-for-grpc-datasource
branch
3 participants
Integrates a feature of the engine, that always rewrites inline fragments for gRPC datasource.
Integration test required adding new query field: "nodesById(id: ID!) [Node!]!".
Summary by CodeRabbit
New Features
Chores
Tests