feat: support nullable types and composite types#2047
Conversation
Noroth
requested review from
a team,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughThis update introduces extensive enhancements to the projects subgraph, including new data models for milestones, tasks, products, and project updates, along with corresponding helper functions for lookups and relationship resolution. The GraphQL schema is expanded to support new queries, mutations, types, enums, and unions for richer project management features. The service logic is refactored to dynamically populate relationships, add new mutations, and improve query results. Tests are updated and extended to reflect these changes, and dependencies are incremented to newer versions. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (9)
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
demo/pkg/subgraphs/projects/src/data/products.go (1)
7-29: Consider using more robust project referencing.The hard-coded array indexing (
ServiceProjects[0],ServiceProjects[1], etc.) creates a brittle dependency on the order ofServiceProjects. Consider using a more robust approach such as referencing projects by ID or name.+// Helper function to get project by ID for safer referencing +func getProjectById(id string) *projects.Project { + for _, project := range ServiceProjects { + if project.Id == id { + return project + } + } + return nil +} + var ServiceProducts = []*projects.Product{ { Upc: "cosmo", Projects: []*projects.Project{ - ServiceProjects[0], // Cloud Migration Overhaul - ServiceProjects[1], // Microservices Revolution - ServiceProjects[3], // DevOps Transformation + getProjectById("1"), // Cloud Migration Overhaul + getProjectById("2"), // Microservices Revolution + getProjectById("4"), // DevOps Transformation }, }, // ... similar changes for other products }demo/pkg/subgraphs/projects/src/data/employee.go (1)
71-78: Consider performance implications for production use.All helper functions use linear search (O(n)) which is acceptable for demo data but may need optimization for production environments with larger datasets. Consider using maps or indexes for better performance if this scales beyond demo usage.
Also applies to: 81-90, 93-102
demo/pkg/subgraphs/projects/src/data/projects.go (1)
17-17: Clarify or remove the misleading comment.The comment states "Will be resolved by GraphQL resolvers" but this is just an empty slice initialization. The actual resolution happens in the service layer through
populateProjectRelationships.- RelatedProducts: []*projects.Product{}, // Will be resolved by GraphQL resolvers + RelatedProducts: []*projects.Product{}, // Populated dynamically in service layerdemo/pkg/subgraphs/projects/src/schema.graphql (1)
64-66: Consider using a DateTime scalar for date fields.The schema uses
Stringtype for all date fields with ISO date comments. This reduces type safety and requires manual date parsing/validation.Consider adding a DateTime scalar to improve type safety:
scalar DateTime interface Timestamped { startDate: DateTime endDate: DateTime }This would provide better type validation and automatic date parsing/formatting.
Also applies to: 77-78, 98-99, 115-116
📜 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 (14)
demo/pkg/subgraphs/projects/cmd/service/main.go(1 hunks)demo/pkg/subgraphs/projects/src/data/employee.go(1 hunks)demo/pkg/subgraphs/projects/src/data/milestones.go(1 hunks)demo/pkg/subgraphs/projects/src/data/products.go(1 hunks)demo/pkg/subgraphs/projects/src/data/project_updates.go(1 hunks)demo/pkg/subgraphs/projects/src/data/projects.go(1 hunks)demo/pkg/subgraphs/projects/src/data/tasks.go(1 hunks)demo/pkg/subgraphs/projects/src/main_test.go(2 hunks)demo/pkg/subgraphs/projects/src/schema.graphql(4 hunks)demo/pkg/subgraphs/projects/src/service/service.go(7 hunks)router-tests/go.mod(1 hunks)router-tests/grpc_subgraph_test.go(2 hunks)router-tests/router_plugin_test.go(2 hunks)router/go.mod(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
demo/pkg/subgraphs/projects/cmd/service/main.go (2)
demo/pkg/subgraphs/projects/generated/service_grpc.pb.go (1)
RegisterProjectsServiceServer(396-405)demo/pkg/subgraphs/projects/src/service/service.go (1)
ProjectsService(19-23)
router-tests/grpc_subgraph_test.go (1)
router-tests/testenv/testenv.go (5)
Run(104-121)Config(281-336)ConfigWithGRPCJSONTemplate(92-92)Environment(1720-1756)GraphQLRequest(1888-1896)
demo/pkg/subgraphs/projects/src/data/employee.go (3)
demo/pkg/subgraphs/projects/generated/service.pb.go (10)
Employee(2890-2899)Employee(2914-2914)Employee(2929-2931)Project(2517-2534)Project(2549-2549)Project(2564-2566)Task(2755-2772)Task(2787-2787)Task(2802-2804)TaskStatus_TASK_STATUS_COMPLETED(141-141)demo/pkg/subgraphs/projects/src/data/projects.go (1)
ServiceProjects(8-107)demo/pkg/subgraphs/projects/src/data/tasks.go (1)
ServiceTasks(8-203)
demo/pkg/subgraphs/projects/src/data/projects.go (5)
demo/pkg/subgraphs/projects/generated/service.pb.go (18)
Project(2517-2534)Project(2549-2549)Project(2564-2566)Employee(2890-2899)Employee(2914-2914)Employee(2929-2931)Product(2961-2968)Product(2983-2983)Product(2998-3000)Milestone(2652-2665)Milestone(2680-2680)Milestone(2695-2697)Task(2755-2772)Task(2787-2787)Task(2802-2804)ProjectUpdate(3568-3580)ProjectUpdate(3595-3595)ProjectUpdate(3610-3612)demo/pkg/subgraphs/projects/src/data/milestones.go (1)
ServiceMilestones(8-109)demo/pkg/subgraphs/projects/src/data/tasks.go (1)
ServiceTasks(8-203)demo/pkg/subgraphs/projects/src/data/project_updates.go (1)
ServiceProjectUpdates(8-108)demo/pkg/subgraphs/projects/src/data/employee.go (1)
Employees(7-68)
⏰ 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_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (28)
router-tests/go.mod (1)
28-28: Dependency update verified for graphql-go-tools/v2 to v2.0.0-rc.206
- router-tests/go.mod (line 28) and router/go.mod (line 34) both reference
v2.0.0-rc.206- Version exists in GitHub releases
- No vulnerability alerts for this release
- Release notes only include a non-breaking bug fix (#1231: allow multiple aliases)
This RC bump is routine and safe to merge.
router/go.mod (1)
34-34: LGTM! Version consistency maintained.The dependency version update matches the corresponding change in
router-tests/go.mod, maintaining consistency across modules.demo/pkg/subgraphs/projects/cmd/service/main.go (1)
81-81: Confirm ID generation with default ProjectsServiceThe explicit
NextID: 1initializer was removed incmd/service/main.go(and similar test‐env registrations). Please ensure that, with a zero‐valueProjectsService, IDs still start at 1:
- Review the
CreateProject(and any other ID‐assigning) methods to confirm they handleNextID == 0by incrementing appropriately.- Verify existing integration and unit tests in
src/main_test.goandrouter-tests/testenv/testenv.godon’t fail due to IDs beginning at 0.- If needed, introduce a constructor (e.g.
NewProjectsService()) that setsNextID = 1or adjust the methods to default the first ID to 1.demo/pkg/subgraphs/projects/src/main_test.go (2)
18-18: LGTM! Proper import added for wrapper types.The import for
wrapperspbis correctly added to support nullable types in the test.
273-277: LGTM! Nullable types implementation aligns with PR objectives.The test correctly uses
*wrapperspb.StringValuewrapper types for optional fields, which aligns with the PR objective to support nullable types. This is a proper implementation of nullable string fields in protobuf.demo/pkg/subgraphs/projects/src/data/products.go (1)
32-48: LGTM! Well-implemented helper functions.The helper functions follow Go best practices with proper nil handling and clean, readable implementation. The functions provide a good abstraction for product and project retrieval operations.
router-tests/grpc_subgraph_test.go (3)
17-17: Good enhancement to test structure with descriptive names.Adding the
namefield to test cases improves test reporting and makes it easier to identify specific test failures when running subtests.Also applies to: 22-22, 27-27, 32-32, 37-37, 42-42
46-60: Comprehensive coverage for inline fragments and polymorphic queries.The new test cases effectively validate the GraphQL schema's support for inline fragments on the
projectResourcesfield, testing bothMilestoneandTasktypes. This aligns well with the PR's objective of supporting composite types.
68-76: Excellent use of parallel subtests for better test isolation.The modification to use
t.Run(test.name, func(t *testing.T) { t.Parallel() })improves test execution performance and provides better test isolation and reporting granularity.demo/pkg/subgraphs/projects/src/data/employee.go (4)
11-12: Good enhancement to employee data with task tracking capabilities.Adding
AssignedTasksandCompletedTasksfields provides comprehensive task relationships that align with the extended GraphQL schema. The use of helper functions to populate these fields dynamically is a clean approach.Also applies to: 17-18, 23-24, 29-30, 35-36, 41-42, 47-48, 53-54, 59-60, 65-66
71-78: Simple and effective employee lookup function.The
GetEmployeeByIdfunction provides a clean way to retrieve employees by ID with appropriate nil handling for not found cases.
81-90: Correct logic for filtering assigned tasks.The function properly filters tasks where the assignee matches and the status is not completed, which correctly represents currently assigned (active) tasks.
93-102: Appropriate filtering for completed tasks.The function correctly filters tasks where the assignee matches and the status is specifically
TASK_STATUS_COMPLETED.demo/pkg/subgraphs/projects/src/data/tasks.go (3)
8-203: Comprehensive task data with proper nullable type handling.The
ServiceTasksslice provides extensive demo data with appropriate use of protobuf wrapper types for nullable fields likeMilestoneId,Description,EstimatedHours, etc. The data structure effectively demonstrates the nullable types feature mentioned in the PR objectives.
206-213: Clean and consistent task lookup implementation.The
GetTaskByIdfunction follows the same pattern as other helper functions in the codebase, providing a straightforward way to retrieve tasks by ID with proper nil handling.
12-12: Excellent use of protobuf wrapper types for nullable fields.The consistent use of
&wrapperspb.StringValue{Value: "..."}for optional string fields and similar patterns for other types properly implements nullable type support as intended by this PR.Also applies to: 26-26, 40-40, 49-49, 77-77, 94-94
demo/pkg/subgraphs/projects/src/data/project_updates.go (3)
8-108: Comprehensive project update tracking with flexible metadata.The
ServiceProjectUpdatesslice provides extensive demo data covering various update types (status changes, milestone additions, task assignments, etc.) with flexible JSON metadata storage. This demonstrates good support for composite types with different update scenarios.
16-16: Good use of JSON metadata for flexible update information.The consistent use of
&wrapperspb.StringValue{Value:{"key": "value"}}for metadata allows for flexible storage of different types of update information while maintaining type safety through protobuf wrappers.Also applies to: 25-25, 34-34, 43-43, 52-52, 61-61, 70-70, 79-79, 88-88, 97-97, 106-106
111-118: Consistent helper function implementation.The
GetProjectUpdateByIdfunction follows the established pattern of other lookup functions in the codebase, providing clean access to project updates by ID.demo/pkg/subgraphs/projects/src/data/milestones.go (3)
8-109: Well-structured milestone data with proper nullable type implementation.The
ServiceMilestonesslice provides comprehensive demo data with appropriate use of protobuf wrapper types for nullable fields likeDescription,StartDate,EndDate, andCompletionPercentage. The data effectively demonstrates nullable type support across different milestone statuses and project associations.
13-13: Consistent nullable type patterns across milestone fields.The systematic use of
&wrapperspb.StringValue{Value: "..."}for optional string fields and&wrapperspb.DoubleValue{Value: ...}for optional numeric fields properly implements the nullable types feature throughout the milestone data structure.Also applies to: 23-23, 33-33, 43-43, 53-53, 63-63, 73-73, 83-83, 93-93, 103-103
112-119: Clean milestone lookup implementation.The
GetMilestoneByIdfunction maintains consistency with other helper functions in the codebase, providing straightforward milestone retrieval by ID with proper nil handling.router-tests/router_plugin_test.go (1)
10-11: LGTM!Import statement formatting is consistent with Go conventions.
demo/pkg/subgraphs/projects/src/data/projects.go (2)
163-174: LGTM!The helper function correctly filters employees by their associated projects.
119-128: Global Variables Verified and Helper Functions ApprovedI’ve confirmed that the referenced globals are defined in the data package:
- demo/pkg/subgraphs/projects/src/data/milestones.go (var ServiceMilestones)
- demo/pkg/subgraphs/projects/src/data/tasks.go (var ServiceTasks)
- demo/pkg/subgraphs/projects/src/data/project_updates.go (var ServiceProjectUpdates)
With these definitions in place, the helper functions are correctly implemented and can be approved.
demo/pkg/subgraphs/projects/src/schema.graphql (2)
14-29: LGTM!The new queries and mutations provide comprehensive project management capabilities.
168-172: LGTM!The union types enable flexible polymorphic queries for project resources, search results, and activities.
demo/pkg/subgraphs/projects/src/service/service.go (1)
457-484: LGTM!The lookup methods correctly maintain key order and handle missing entries by returning nil, which is essential for GraphQL federation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
demo/pkg/subgraphs/projects/src/service/service.go (2)
196-201: Add safety check for empty ServiceTasks slice.The code assumes
ServiceTasksis non-empty when accessing the last element, which could cause a panic if the slice is empty.Apply this fix to handle the empty slice case:
// Generate next ID +if len(data.ServiceTasks) == 0 { + nextID = 1 +} else { lastID := data.ServiceTasks[len(data.ServiceTasks)-1].Id nextID, err := strconv.Atoi(lastID) if err != nil { return nil, status.Errorf(codes.Internal, "failed to convert lastID to int: %v", err) } nextID++ +}
252-252: Replace hardcoded employee ID with context-derived value.The hardcoded
UpdatedById: 1should be replaced with a value extracted from the request context for proper user tracking.- UpdatedById: 1, + UpdatedById: getUserIdFromContext(ctx), // Extract from auth contextNote: You'll need to implement
getUserIdFromContextto extract the authenticated user from the request context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
demo/pkg/subgraphs/projects/src/service/service.go(7 hunks)
⏰ 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). (3)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
demo/pkg/subgraphs/projects/src/service/service.go (10)
6-6: Import additions look good.The new imports (
strings,time,wrapperspb) are properly utilized throughout the code for string manipulation, timestamp generation, and protobuf wrapper types respectively.Also applies to: 9-9, 15-15
18-30: Good refactoring of hardcoded relationships.The configurable
projectToProductMapsuccessfully addresses the previous technical debt of hardcoded project-to-product mappings, making the code more maintainable and extensible.
40-79: Well-structured relationship population logic.The helper functions provide clean separation of concerns for populating project relationships and follow good practices for creating copies to avoid modifying originals.
81-154: Lookup methods correctly maintain order and handle missing entries.The implementation properly maintains the order of requested keys and appends
nilfor missing entries, which is the correct behavior for lookup operations.
213-213: Good use of current timestamp.The use of
time.Now().Format(time.RFC3339)properly addresses the previous issue with hardcoded timestamps.
264-418: Query methods are well-implemented.The new query methods properly utilize the data helper functions, maintain consistency in relationship population, and implement appropriate search logic with case-insensitive matching.
441-461: Good improvement to maintain key order and handle missing entries.The refactored
LookupEmployeeByIdmethod now properly maintains the order of input keys and appendsnilfor missing entries, which is the correct behavior for lookup operations.
504-516: Proper initialization of empty relationships.The updated project creation correctly initializes empty slices for relationships and uses protobuf wrapper types for optional fields, which aligns with the new data model structure.
530-530: Correct use of relationship population.The modification to use
populateProjectRelationshipsensures that returned projects include all related data, which is the expected behavior for the query.
563-569: Consistent relationship population across all projects.The updated
QueryProjectsmethod ensures all returned projects have their relationships populated, providing a consistent API experience.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
demo/pkg/subgraphs/projects/src/service/service.go (1)
499-505: Add safety check for empty ServiceProjects slice.Similar to the task mutation issue, this code assumes
ServiceProjectsis non-empty when accessing the last element, which could cause a panic.Apply this fix:
- lastID := data.ServiceProjects[len(data.ServiceProjects)-1].Id - - nextID, err := strconv.Atoi(lastID) - if err != nil { - return nil, status.Errorf(codes.Internal, "failed to convert lastID to int: %v", err) - } - - nextID++ + var nextID int + if len(data.ServiceProjects) == 0 { + nextID = 1 + } else { + lastID := data.ServiceProjects[len(data.ServiceProjects)-1].Id + next, err := strconv.Atoi(lastID) + if err != nil { + return nil, status.Errorf(codes.Internal, "failed to convert lastID to int: %v", err) + } + nextID = next + 1 + }
♻️ Duplicate comments (1)
demo/pkg/subgraphs/projects/src/service/service.go (1)
257-257: Replace hardcoded user ID with context-based value.The hardcoded
UpdatedById: 1should be replaced with a value extracted from the request context for proper user attribution.This is similar to the previous review comment about hardcoded user IDs. Consider implementing a function to extract the user ID from the authentication context.
- UpdatedById: 1, + UpdatedById: getUserIdFromContext(ctx),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
demo/pkg/subgraphs/projects/src/service/service.go(7 hunks)router-tests/go.mod(1 hunks)router/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- router-tests/go.mod
- router/go.mod
⏰ 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). (9)
- GitHub Check: build-router
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
demo/pkg/subgraphs/projects/src/service/service.go (6)
18-30: Good refactoring to use configurable mapping structure.The replacement of hardcoded switch-case logic with a configurable map structure addresses the maintainability concerns from previous reviews. This makes it easier to update project-product relationships without code changes.
40-79: Well-designed relationship population functions.The helper functions properly handle relationship population with clean separation of concerns. The
populateProjectRelationshipsfunction creates a copy to avoid modifying the original, which is good practice.
81-154: Excellent implementation of lookup functions.The lookup functions properly maintain the order of keys and return
nilfor missing entries, which follows GraphQL federation best practices for batch lookups.
269-423: Well-implemented query functions with proper relationship population.The query functions properly implement relationship population and provide comprehensive functionality for milestones, tasks, project activities, resources, and search. The case-insensitive search implementation is particularly well done.
508-526: Good initialization of empty slices for new projects.The initialization of empty slices for relationships and zero progress value is appropriate for new projects. The use of protobuf wrapper types is also correct.
568-574: Excellent enhancement to populate relationships in project queries.The update to populate relationships for all projects in the query results provides more comprehensive data to clients and aligns with the relationship population pattern used throughout the service.
…465-add-support-for-nullable-base-types
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores
Checklist