Add more endpoints Add AddNode and SetDependency methods to Service struct in API #158
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce two new methods, Changes
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 buf (1.47.2)api/v1/service.protoAn unexpected error occurred while running buf. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
api/v1/service.proto (3)
75-78: Fix inconsistent field naming in SetDependencyRequestThe field naming is inconsistent:
nodeIdvsdependencyID. Consider using consistent casing across all fields.message SetDependencyRequest { - uint32 nodeId = 1; - uint32 dependencyID = 2; + uint32 node_id = 1; + uint32 dependency_id = 2; }
67-78: Add documentation comments for the new message typesConsider adding documentation comments to describe the purpose and usage of these new message types. This will help API consumers understand how to use them correctly.
+// AddNodeRequest represents a request to add a new node to the graph. message AddNodeRequest { Node node = 1; } +// AddNodeResponse contains the node that was added, including any server-generated fields. message AddNodeResponse { Node node = 1; } +// SetDependencyRequest represents a request to establish a dependency relationship +// between two nodes in the graph. message SetDependencyRequest { uint32 node_id = 1; uint32 dependency_id = 2; }
114-115: Add documentation comments for the new RPC methodsConsider adding documentation comments to describe the behavior, error cases, and usage of these new methods.
+ // AddNode adds a new node to the graph. + // Returns the added node with any server-generated fields populated. + // Errors: + // - ALREADY_EXISTS if a node with the same name already exists + // - INVALID_ARGUMENT if the node is missing required fields rpc AddNode(AddNodeRequest) returns (AddNodeResponse) {} + + // SetDependency establishes a dependency relationship between two nodes. + // Errors: + // - NOT_FOUND if either node doesn't exist + // - INVALID_ARGUMENT if attempting to create a circular dependency rpc SetDependency(SetDependencyRequest) returns (google.protobuf.Empty) {}api/v1/service_test.go (1)
189-201: Enhance test coverage with additional test casesWhile the basic happy path is covered, consider adding these test cases for robustness:
- Duplicate node creation
- Invalid node names/types
- Verification of all node fields (type, metadata, etc.)
- Cleanup after test execution
Here's a suggested enhancement:
func TestAddNode(t *testing.T) { s := setupService() + t.Run("successful node creation", func(t *testing.T) { addNodeReq := connect.NewRequest(&service.AddNodeRequest{ Node: &service.Node{Name: "test_node", Type: "type1"}, }) _, err := s.AddNode(context.Background(), addNodeReq) require.NoError(t, err) getNodeReq := connect.NewRequest(&service.GetNodeByNameRequest{Name: "test_node"}) resp, err := s.GetNodeByName(context.Background(), getNodeReq) require.NoError(t, err) assert.NotNil(t, resp.Msg.Node) assert.Equal(t, "test_node", resp.Msg.Node.Name) + assert.Equal(t, "type1", resp.Msg.Node.Type) }) + + t.Run("duplicate node creation", func(t *testing.T) { + addNodeReq := connect.NewRequest(&service.AddNodeRequest{ + Node: &service.Node{Name: "test_node", Type: "type1"}, + }) + _, err := s.AddNode(context.Background(), addNodeReq) + assert.Error(t, err) + }) }cmd/query/globsearch/globsearch_test.go (2)
167-168: Consider grouping related mock functions togetherWhile the implementation is correct, consider grouping related mock functions together for better maintainability. For example, group node-related functions (GetNode, GetNodeByName, AddNode) together, followed by dependency-related functions.
type mockGraphServiceClient struct { GetNodesByGlobFunc func(ctx context.Context, req *connect.Request[apiv1.GetNodesByGlobRequest]) (*connect.Response[apiv1.GetNodesByGlobResponse], error) GetNodeFunc func(ctx context.Context, req *connect.Request[apiv1.GetNodeRequest]) (*connect.Response[apiv1.GetNodeResponse], error) GetNodeByNameFunc func(ctx context.Context, req *connect.Request[apiv1.GetNodeByNameRequest]) (*connect.Response[apiv1.GetNodeByNameResponse], error) + AddNodeFunc func(ctx context.Context, req *connect.Request[apiv1.AddNodeRequest]) (*connect.Response[apiv1.AddNodeResponse], error) SetDependencyFunc func(ctx context.Context, req *connect.Request[apiv1.SetDependencyRequest]) (*connect.Response[emptypb.Empty], error) - AddNodeFunc func(ctx context.Context, req *connect.Request[apiv1.AddNodeRequest]) (*connect.Response[apiv1.AddNodeResponse], error) }
183-189: Add test cases for new mock methodsWhile the implementation of AddNode and SetDependency methods is correct and follows the established pattern, the TestRun test suite should be extended to cover these new methods.
Consider adding test cases like:
{ name: "add node success", pattern: "node*", output: "json", mockResponse: &apiv1.GetNodesByGlobResponse{ Nodes: []*apiv1.Node{ {Name: "newNode", Type: "type1", Id: 1}, }, }, }, { name: "set dependency error", pattern: "node*", output: "json", mockError: errors.New("dependency error"), expectError: true, expectedErrorString: "failed to set dependency: dependency error", }api/v1/service.go (1)
92-102: Add input validation and utilize context.The implementation looks functionally correct but could be improved in a few areas:
- Add validation for required fields (type, name) before calling storage
- Propagate the context to the storage layer for potential timeout/cancellation handling
Consider this improvement:
func (s *Service) AddNode(ctx context.Context, req *connect.Request[service.AddNodeRequest]) (*connect.Response[service.AddNodeResponse], error) { + if req.Msg.Node.Type == "" { + return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("node type is required")) + } + if req.Msg.Node.Name == "" { + return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("node name is required")) + } - resultNode, err := graph.AddNode(s.storage, req.Msg.Node.Type, req.Msg.Node.Metadata, req.Msg.Node.Name) + resultNode, err := graph.AddNode(ctx, s.storage, req.Msg.Node.Type, req.Msg.Node.Metadata, req.Msg.Node.Name) if err != nil { return nil, err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
gen/api/v1/apiv1connect/service.connect.gois excluded by!**/gen/**gen/api/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (5)
api/v1/service.go(1 hunks)api/v1/service.proto(2 hunks)api/v1/service_test.go(1 hunks)cmd/query/getMetadata/getMetadata_test.go(3 hunks)cmd/query/globsearch/globsearch_test.go(3 hunks)
🔇 Additional comments (5)
api/v1/service.proto (1)
114-115: Verify error handling patterns across the service
The new methods should follow consistent error handling patterns with existing methods in the service.
✅ Verification successful
Let me gather more information about the error handling patterns by examining the request/response message definitions and any error-related comments in the proto file.
Let me check the actual message definitions to see the fields and error handling patterns in detail.
Based on the analysis of service.proto, I can now provide the final response:
Error handling follows standard gRPC patterns
The new methods AddNode and SetDependency follow the consistent error handling pattern used across the service, where:
- All RPCs use standard gRPC status codes for error handling
- No explicit error fields are defined in response messages
- Methods returning
google.protobuf.Empty(likeSetDependency) align with other similar methods in the service (e.g.,IngestSBOM,Cache) - Methods returning specific responses (like
AddNode) follow the same pattern as other getter methods (e.g.,GetNode,GetNodeByName)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling patterns in existing RPC methods
# Look for error documentation patterns in existing methods
rg -B 2 "rpc.*returns" api/v1/service.proto
# Check implementation files for error handling patterns
ast-grep --pattern 'func \(.*\) \$_(.*\) \(.*\) \(.*\) error {
$$$
return $$$
}'
Length of output: 1308
Script:
#!/bin/bash
# Look for error-related fields and comments in request/response messages
rg -B 2 "message.*Response|message.*Request" api/v1/service.proto
# Check for any error-related fields or status codes
rg -i "error|status|code" api/v1/service.proto
Length of output: 844
Script:
#!/bin/bash
# Get the complete content of service.proto to analyze message definitions
cat api/v1/service.proto
# Also check for any common error handling utilities or helpers
fd -e proto | xargs rg -l "import.*error"
Length of output: 2624
cmd/query/getMetadata/getMetadata_test.go (2)
15-15: LGTM: Required import for empty protobuf responses
The addition of emptypb import is necessary to support the new SetDependency method's empty response type.
52-53: LGTM: Mock function fields follow established patterns
The new function fields are correctly typed and follow the existing naming conventions.
cmd/query/globsearch/globsearch_test.go (1)
18-18: LGTM: Required import for empty response type
The addition of emptypb import is necessary to support the SetDependency method's empty response type.
api/v1/service.go (1)
104-118: 🛠️ Refactor suggestion
Add dependency validation checks.
The implementation needs additional validation to prevent invalid dependency configurations:
- Check for self-dependencies (node depending on itself)
- Consider adding circular dependency detection
- Propagate context to storage operations
Consider these improvements:
func (s *Service) SetDependency(ctx context.Context, req *connect.Request[service.SetDependencyRequest]) (*connect.Response[emptypb.Empty], error) {
+ if req.Msg.NodeId == req.Msg.DependencyID {
+ return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("self-dependencies are not allowed"))
+ }
- fromNode, err := s.storage.GetNode(req.Msg.NodeId)
+ fromNode, err := s.storage.GetNode(ctx, req.Msg.NodeId)
if err != nil {
- return nil, err
+ return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("source node not found: %w", err))
}
- toNode, err := s.storage.GetNode(req.Msg.DependencyID)
+ toNode, err := s.storage.GetNode(ctx, req.Msg.DependencyID)
if err != nil {
- return nil, err
+ return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("target node not found: %w", err))
}
- err = fromNode.SetDependency(s.storage, toNode)
+ err = fromNode.SetDependency(ctx, s.storage, toNode)
if err != nil {
return nil, err
}Let's verify if circular dependency detection is already implemented in the storage layer:
Codecov ReportAttention: Patch coverage is
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
pkg/tools/ingest/helpers.go (1)
76-76: Consider enhancing the error message with more contextWhile the current error wrapping is good, we could make it more helpful for debugging by including the invalid purl in the error message.
Here's a suggested improvement:
- return PackageInfo{}, fmt.Errorf("failed to parse purl: %w", err) + return PackageInfo{}, fmt.Errorf("failed to parse package URL %q: %w", purl, err)This change would make it easier to identify which specific package URL caused the parsing failure.
pkg/tools/ingest/vuln.go (2)
Line range hint
141-144: Consider enhancing error collection logic.The error handling in the vulnerability processing loop could be improved. Currently, errors are collected but only some lead to immediate returns while others are accumulated.
Consider standardizing the error handling approach:
vulnData, err := json.Marshal(vuln) if err != nil { - errors = append(errors, err) - continue + return fmt.Errorf("failed to marshal vulnerability data: %w", err) }
Line range hint
198-201: Consider adding ecosystem-specific version comparison.The
compareEcosystemVersionsfunction currently uses a simple string comparison as a placeholder. This might not handle semantic versioning correctly for all ecosystems.Consider implementing proper version comparison logic for different package ecosystems (npm, pip, maven, etc.) to ensure accurate vulnerability matching.
api/v1/service.go (1)
92-102: Add input validation and improve error handlingWhile the implementation is functional, consider these improvements:
- Add validation for required fields (type, name)
- Add checks for duplicate nodes
- Make error messages more specific for different failure cases
- Add method documentation
Consider this enhanced implementation:
+// AddNode creates a new node in the graph with the specified type, metadata, and name. +// Returns the created node or an error if the operation fails. func (s *Service) AddNode(ctx context.Context, req *connect.Request[service.AddNodeRequest]) (*connect.Response[service.AddNodeResponse], error) { + if req.Msg.Node.Type == "" { + return nil, fmt.Errorf("node type is required") + } + if req.Msg.Node.Name == "" { + return nil, fmt.Errorf("node name is required") + } + + // Check for existing node with same name + if _, err := s.storage.NameToID(req.Msg.Node.Name); err == nil { + return nil, fmt.Errorf("node with name %q already exists", req.Msg.Node.Name) + } + resultNode, err := graph.AddNode(s.storage, req.Msg.Node.Type, req.Msg.Node.Metadata, req.Msg.Node.Name) if err != nil { - return nil, fmt.Errorf("failed to add node: %w", err) + return nil, fmt.Errorf("failed to add node %q of type %q: %w", req.Msg.Node.Name, req.Msg.Node.Type, err) } serviceNode, err := NodeToServiceNode(resultNode) if err != nil { return nil, fmt.Errorf("failed to convert node to service node: %w", err) } return connect.NewResponse(&service.AddNodeResponse{Node: serviceNode}), nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
api/v1/service.go(4 hunks)api/v1/service_test.go(1 hunks)pkg/tools/ingest/helpers.go(2 hunks)pkg/tools/ingest/vuln.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/v1/service_test.go
🔇 Additional comments (6)
pkg/tools/ingest/helpers.go (2)
3-7: LGTM: Clean import organization
The addition of the fmt package import is appropriate for the enhanced error handling implementation.
Line range hint 73-99: Consider comprehensive ecosystem testing for new endpoints
Since this parsing function handles various ecosystem-specific naming conventions (Maven, Debian, Alpine, etc.), and the PR adds new node management endpoints, it would be valuable to ensure that the new AddNode and SetDependency endpoints are tested with package URLs from different ecosystems to verify correct handling of namespace conventions.
Let's verify the test coverage:
pkg/tools/ingest/vuln.go (2)
106-106: LGTM! Error handling improvement is appropriate.
The enhanced error message provides better context about the failure point, which is helpful for debugging. This change aligns with good error handling practices.
Line range hint 109-111: Consider consistent error wrapping pattern.
While the error at line 106 is now properly wrapped, the subsequent error handling follows the same pattern. This is good for consistency.
api/v1/service.go (2)
51-51: LGTM: Improved error handling across methods
The error handling improvements follow best practices by:
- Adding descriptive context to errors
- Using consistent error message format
- Properly wrapping errors with
%wverb for error chain preservation
Also applies to: 55-55, 63-63, 67-67, 71-71, 79-79, 85-85, 123-123, 131-131, 242-242, 246-246, 253-253, 269-269, 274-274, 279-279, 283-283, 287-287, 292-292, 299-299, 318-318, 326-326, 334-334
92-118: Verify integration with existing Graph service functionality
The implementation aligns with PR objectives but let's verify integration with existing functionality.
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have comprehensive information about the integration points and usage of the new methods:
Implementation properly integrates with existing Graph service functionality
The implementation aligns well with the existing codebase:
- The
AddNodemethod correctly uses the underlyinggraph.AddNodefunction which properly initializes node structures with Children/Parents bitmaps - The
SetDependencymethod integrates with the existing node relationship model by:- Using storage's GetNode for retrieving nodes
- Utilizing Node.SetDependency which maintains bidirectional relationships via Children/Parents bitmaps
- Both methods are already being used in multiple components:
- Test suites (api/v1/service_test.go)
- Ingest tools (pkg/tools/ingest/*)
- Risk calculation (pkg/tools/weightedNACD/*)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify integration points with existing Graph service functionality
# Check for existing node retrieval methods that might need updates
echo "Checking for node retrieval methods that might need updates:"
rg -A 2 "func.*GetNode" --type go
# Check for existing dependency-related functionality
echo "Checking for existing dependency-related functionality:"
rg -A 2 "Dependency|Children|Parents" --type go
# Check for potential usage of new methods
echo "Checking for potential usage of new methods:"
rg -l "AddNode|SetDependency" --type go
Length of output: 53004
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- Added AddNode and SetDependency to the Graph service Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
naveensrinivasan
force-pushed
the
neil/addmoreendpoints
branch
from
7f7bc7c to
4cee5a0
Compare
coderabbitai
bot
changed the title
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests