Added Glob searching for nodes #102
Conversation
WalkthroughThe changes introduce a new method Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphServiceClient
participant Storage
participant RedisStorage
participant MockStorage
Client->>GraphServiceClient: GetNodesByGlob(pattern)
GraphServiceClient->>Storage: GetNodesByGlob(pattern)
alt if RedisStorage
Storage->>RedisStorage: GetNodesByGlob(pattern)
RedisStorage-->>Storage: Return nodes
else if MockStorage
Storage->>MockStorage: GetNodesByGlob(pattern)
MockStorage-->>Storage: Return nodes
end
Storage-->>GraphServiceClient: Return nodes
GraphServiceClient-->>Client: Return nodes
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 (
|
Codecov ReportAttention: Patch coverage is
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (12)
pkg/graph/storage.go (2)
9-9: Add documentation for the glob pattern syntax and behavior.The new method signature looks good, but it would benefit from documentation that specifies:
- The supported glob pattern syntax
- Any limitations on pattern complexity
- Performance implications for large datasets
Add a comment block above the interface or method:
type Storage interface { + // GetNodesByGlob retrieves nodes whose names match the given glob pattern. + // Pattern syntax supports standard glob operators (e.g., *, ?, []). + // Note: Complex patterns or large datasets may impact performance. GetNodesByGlob(pattern string) ([]*Node, error)
9-9: Consider performance optimization strategies.The glob searching feature could benefit from optimization strategies to handle large datasets efficiently.
Consider:
- Adding pagination support to prevent memory pressure with large result sets
- Implementing caching for frequently used patterns
- Using indexing strategies in storage implementations to optimize pattern matching
api/v1/service.proto (2)
59-65: Add field documentation for the glob pattern syntax.The
patternfield would benefit from documentation comments describing:
- The supported glob syntax and patterns
- Examples of valid patterns
- Any limitations or restrictions
Add documentation like this:
message GetNodesByGlobRequest { + // pattern is a glob pattern for matching node names. + // Supports standard glob syntax: *, ?, [seq], [!seq] + // Example: "*minefield*" matches any node containing "minefield" string pattern = 1; }
83-83: Consider streaming response for large result sets.The current implementation might face scalability issues when the glob pattern matches a large number of nodes. Consider using server-side streaming to handle large result sets more efficiently.
Example modification:
- rpc GetNodesByGlob(GetNodesByGlobRequest) returns (GetNodesByGlobResponse) {} + rpc GetNodesByGlob(GetNodesByGlobRequest) returns (stream GetNodesByGlobResponse) {}This would allow the server to stream results back to the client, reducing memory usage and improving response time for large result sets.
cmd/query/globsearch/globsearch.go (3)
22-24: Consider using a named constant for the default max output.For better maintainability and reusability, consider extracting the default value to a package-level constant.
+const defaultMaxOutput = 10 + func (o *options) AddFlags(cmd *cobra.Command) { - cmd.Flags().IntVar(&o.maxOutput, "max-output", 10, "max output length") + cmd.Flags().IntVar(&o.maxOutput, "max-output", defaultMaxOutput, "max output length") }
26-32: Consider extracting the default address to a constant.The default service address should be defined as a package-level constant for better maintainability.
+const defaultServiceAddr = "http://localhost:8089" + func (o *options) Run(cmd *cobra.Command, args []string) error { pattern := args[0] httpClient := &http.Client{} addr := os.Getenv("BITBOMDEV_ADDR") if addr == "" { - addr = "http://localhost:8089" + addr = defaultServiceAddr }
73-75: Add example usage in command description.To help users understand the glob pattern syntax, consider adding examples in the command description.
cmd := &cobra.Command{ Use: "globsearch [pattern]", - Short: "Search for nodes by glob pattern", + Short: "Search for nodes by glob pattern (e.g., *minefield*)", + Example: " globsearch *minefield*\n globsearch pkg:github/*", Args: cobra.ExactArgs(1),api/v1/service_test.go (1)
45-57: Enhance test coverage with additional test cases.While the basic test implementation is correct, consider adding more test cases to ensure comprehensive coverage of the glob functionality:
- Test various glob patterns:
- Exact match: "name1"
- Wildcard: "name1", "name?"
- Multiple patterns: "name[1-2]"
- Add negative test cases:
- Non-matching pattern
- Invalid pattern
- Empty pattern
- Test edge cases:
- Pattern matching no nodes
- Pattern matching all nodes
- Special characters in node names
Here's a suggested enhancement:
func TestGetNodesByGlob(t *testing.T) { - s := setupService() - node, err := graph.AddNode(s.storage, "type1", "metadata1", "name1") - require.NoError(t, err) - node2, err := graph.AddNode(s.storage, "type1", "metadata1", "name2") - require.NoError(t, err) - req := connect.NewRequest(&service.GetNodesByGlobRequest{Pattern: "name"}) - resp, err := s.GetNodesByGlob(context.Background(), req) - require.NoError(t, err) - for _, respNode := range resp.Msg.Nodes { - assert.Contains(t, []string{node.Name, node2.Name}, respNode.Name) - } + tests := []struct { + name string + setupNodes []string + pattern string + expectedNames []string + expectError bool + }{ + { + name: "basic glob match", + setupNodes: []string{"name1", "name2"}, + pattern: "name*", + expectedNames: []string{"name1", "name2"}, + }, + { + name: "exact match", + setupNodes: []string{"name1", "name2"}, + pattern: "name1", + expectedNames: []string{"name1"}, + }, + { + name: "no matches", + setupNodes: []string{"name1", "name2"}, + pattern: "other*", + expectedNames: []string{}, + }, + { + name: "special characters", + setupNodes: []string{"name-1", "name.2", "name[3]"}, + pattern: "name*", + expectedNames: []string{"name-1", "name.2", "name[3]"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := setupService() + + // Setup test nodes + nodes := make([]*graph.Node, 0, len(tt.setupNodes)) + for _, name := range tt.setupNodes { + node, err := graph.AddNode(s.storage, "type1", "metadata1", name) + require.NoError(t, err) + nodes = append(nodes, node) + } + + // Test glob search + req := connect.NewRequest(&service.GetNodesByGlobRequest{Pattern: tt.pattern}) + resp, err := s.GetNodesByGlob(context.Background(), req) + + if tt.expectError { + require.Error(t, err) + return + } + + require.NoError(t, err) + require.Len(t, resp.Msg.Nodes, len(tt.expectedNames)) + + actualNames := make([]string, 0, len(resp.Msg.Nodes)) + for _, node := range resp.Msg.Nodes { + actualNames = append(actualNames, node.Name) + } + + assert.ElementsMatch(t, tt.expectedNames, actualNames) + }) + } }cmd/query/query.go (1)
17-18: Add default behavior when no subcommand is provided.Currently, if a user runs
querywithout specifying any subcommands, it may result in an error or no action. To enhance user experience, consider adding a defaultRunfunction that displays the help message.Apply this diff to add a default
Runmethod:func New(storage graph.Storage) *cobra.Command { cmd := &cobra.Command{ Use: "query", Short: "Query dependencies and dependents of a project", DisableAutoGenTag: true, + Run: func(cmd *cobra.Command, args []string) { + cmd.Help() + }, } cmd.AddCommand(custom.New(storage)) cmd.AddCommand(globsearch.New(storage)) return cmd }cmd/query/custom/custom.go (3)
131-131: Fix the typo in the command's short description.There is a typo in the
Shortfield of the command. The word "Quer" should be "Query".Apply this diff to correct the typo:
- Short: "Quer dependencies and dependents of a project", + Short: "Query dependencies and dependents of a project",
35-35: Rename the--addrflag to--portfor clarity.The
--addrflag is used to specify the port number, not the full address. Renaming it to--portclarifies its purpose and prevents confusion.Apply this diff to rename the flag and update its usage:
cmd.Flags().StringVar(&o.visualizerAddr, "addr", "8081", "address to run the visualizer on") +cmd.Flags().StringVar(&o.visualizerAddr, "port", "8081", "port to run the visualizer on")Update the server initialization:
server := &http.Server{ - Addr: ":" + o.visualizerAddr, + Addr: ":" + o.visualizerAddr, }Ensure all references to
--addrare updated accordingly.Also applies to: 99-99
51-53: Consider additional fields for theQueryRequestif necessary.Depending on the service requirements, you might need to include additional parameters in the
QueryRequest, such as authentication tokens or query options.Review the
QueryRequeststructure to ensure all necessary fields are included.
📜 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 (10)
- api/v1/service.go (1 hunks)
- api/v1/service.proto (2 hunks)
- api/v1/service_test.go (1 hunks)
- cmd/query/custom/custom.go (1 hunks)
- cmd/query/globsearch/globsearch.go (1 hunks)
- cmd/query/query.go (1 hunks)
- cmd/start-service/start-service.go (1 hunks)
- pkg/graph/mockGraph.go (2 hunks)
- pkg/graph/storage.go (1 hunks)
- pkg/storages/redis_storage.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
api/v1/service.go
[warning] 87-88: api/v1/service.go#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 91-95: api/v1/service.go#L91-L95
Added lines #L91 - L95 were not covered by testspkg/graph/mockGraph.go
[warning] 54-66: pkg/graph/mockGraph.go#L54-L66
Added lines #L54 - L66 were not covered by tests
[warning] 68-68: pkg/graph/mockGraph.go#L68
Added line #L68 was not covered by testspkg/storages/redis_storage.go
[warning] 75-80: pkg/storages/redis_storage.go#L75-L80
Added lines #L75 - L80 were not covered by tests
[warning] 82-91: pkg/storages/redis_storage.go#L82-L91
Added lines #L82 - L91 were not covered by tests
[warning] 93-97: pkg/storages/redis_storage.go#L93-L97
Added lines #L93 - L97 were not covered by tests
[warning] 100-100: pkg/storages/redis_storage.go#L100
Added line #L100 was not covered by tests
🔇 Additional comments (8)
pkg/graph/storage.go (1)
9-9: LGTM! Interface addition is well-placed.The new method is a logical extension of the Storage interface, maintaining consistency with existing node retrieval methods while adding valuable search functionality.
Let's verify the implementation across storage backends:
✅ Verification successful
Let me try a different approach to verify the implementation across storage backends.
This will help us:
- Identify all storage-related files
- Find any implementations of GetNodesByGlob with some context
Implementation verified across storage backends
The search results confirm that the
GetNodesByGlobmethod is properly implemented in both storage backends:
- Mock implementation in
pkg/graph/mockGraph.go- Redis implementation in
pkg/storages/redis_storage.goAdditionally, the method is:
- Well-tested (see
api/v1/service_test.go)- Properly integrated into the service layer (
api/v1/service.go)- Correctly exposed through the API (proto definitions and generated code)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify GetNodesByGlob implementation in storage backends # Expected: Implementation in mock and redis storage # Check for implementation in storage backends ast-grep --pattern 'func ($x *$t) GetNodesByGlob(pattern string) ([]*Node, error)'Length of output: 84
Script:
#!/bin/bash # First, let's find all storage implementation files fd "storage.*\.go" # Then let's search for GetNodesByGlob in all Go files rg "GetNodesByGlob" -A 3Length of output: 13761
api/v1/service.proto (1)
Line range hint
59-83: Verify error handling consistency across similar methods.Let's ensure the error handling approach is consistent with existing node retrieval methods.
✅ Verification successful
Based on the results from the shell scripts, I can now generate the final response as the error handling patterns are clearly visible in the codebase.
Error handling is consistent across node retrieval methods
The verification shows that error handling is indeed consistent across all node retrieval methods in the GraphService:
- All methods properly propagate errors using the standard Go error pattern
- Error handling follows the same structure in
GetNode,GetNodeByName, andGetNodesByGlob:
- Direct error propagation from storage layer calls
- Consistent error wrapping when needed
- Uniform use of
connect.NewErrorfor unimplemented handlers🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check error handling patterns in service implementations # Look for error handling patterns in existing node retrieval methods echo "Checking error handling patterns in service implementations..." rg -A 5 "GetNode.*\(ctx" --type go # Look for any existing validation patterns echo "Checking validation patterns..." rg -B 2 "return.*error" --type go | grep -B 2 "GetNode"Length of output: 4986
cmd/start-service/start-service.go (1)
47-48: LGTM! Verify GraphService implementation.The addition of the GraphService handler follows the established pattern and is consistent with other service handler registrations.
Let's verify the GraphService implementation:
✅ Verification successful
Let me try a different approach to verify the GraphService implementation.
GraphService implementation verified successfully
The verification results show that the GraphService is properly implemented across the codebase:
- The service is defined in the protocol buffer (
api/v1/service.proto) with theGetNodesByGlobRPC method- The implementation exists in
api/v1/service.gowith proper request handling- The storage interface and implementations (Redis, Mock) include the
GetNodesByGlobmethod- Tests are present in
api/v1/service_test.go- Generated code (connect handlers, client interfaces) is properly set up
The handler registration in
cmd/start-service/start-service.gois correct and consistent with the implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify GraphService implementation and the new glob-related method # Test 1: Check if GraphService is properly defined in the proto/generated files ast-grep --pattern 'service GraphService { $$$ rpc GetNodesByGlob($_) returns ($_); $$$ }' # Test 2: Verify the service implementation exists ast-grep --pattern 'func (s *Service) GetNodesByGlob($_) ($_, error) { $$$ }'Length of output: 180
Script:
#!/bin/bash # Let's first check for proto definitions rg "service GraphService" -A 5 # Check for the service implementation rg "type GraphService" -A 5 # Check for imports of GraphService-related types rg "GraphService" cmd/start-service/start-service.go -A 2 -B 2 # Check for the actual implementation of GetNodesByGlob rg "GetNodesByGlob" -A 5Length of output: 20423
pkg/graph/mockGraph.go (1)
6-6: LGTM: Import addition is appropriateThe addition of the
path/filepathimport is necessary for the glob pattern matching functionality.api/v1/service.go (1)
84-84: LGTM! Method signature follows established patterns.The method signature is well-designed and consistent with other service methods, properly integrating with the connect framework.
cmd/query/query.go (2)
4-5: Imports for new subcommands are correctly added.The imports for
customandglobsearchsubcommands are properly included, facilitating the new modular design.
12-16: Command properties are appropriately set.The
Use,Short, andDisableAutoGenTagfields are correctly configured for thequerycommand.cmd/query/custom/custom.go (1)
125-139: Initialize the command with all required flags and options.The command initialization appears correct, with proper setup of options and flags.
- Added glob searching for nodes, allowing searchs for nodes like "*minefield*" and getting back two outputs "pkg:github/bitbomdev/minefield@v2" and "pkg:github/bitbomdev/minefield@v2" Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
neilnaveen
force-pushed
the
neil/addglobsearch
branch
from
2baa86c
to
640953d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
cmd/query/query.go (1)
17-18: LGTM: Well-structured command organization.The code demonstrates good separation of concerns by splitting functionality into dedicated subcommands:
customfor traditional queryingglobsearchfor the new glob pattern searchingThis modular approach aligns with the single responsibility principle and makes the codebase more maintainable.
Consider documenting the usage of these subcommands in the README.md to help users understand the new search capabilities.
cmd/start-service/start-service.go (1)
Line range hint
42-48: Consider extracting service registration into a helper function.The current implementation repeats the service registration pattern multiple times. Consider refactoring this into a helper function to improve maintainability and make future service additions more straightforward.
Here's a suggested improvement:
+func registerService(mux *http.ServeMux, newService *service.Service, handler func(*service.Service) (string, http.Handler)) { + path, h := handler(newService) + mux.Handle(path, h) +} func (o *options) Run(_ *cobra.Command, args []string) error { // ... existing code ... mux := http.NewServeMux() - path, handler := apiv1connect.NewQueryServiceHandler(newService) - mux.Handle(path, handler) - path, handler = apiv1connect.NewLeaderboardServiceHandler(newService) - mux.Handle(path, handler) - path, handler = apiv1connect.NewCacheServiceHandler(newService) - mux.Handle(path, handler) - path, handler = apiv1connect.NewGraphServiceHandler(newService) - mux.Handle(path, handler) + registerService(mux, newService, apiv1connect.NewQueryServiceHandler) + registerService(mux, newService, apiv1connect.NewLeaderboardServiceHandler) + registerService(mux, newService, apiv1connect.NewCacheServiceHandler) + registerService(mux, newService, apiv1connect.NewGraphServiceHandler)
📜 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 (10)
- api/v1/service.go (1 hunks)
- api/v1/service.proto (2 hunks)
- api/v1/service_test.go (1 hunks)
- cmd/query/custom/custom.go (1 hunks)
- cmd/query/globsearch/globsearch.go (1 hunks)
- cmd/query/query.go (1 hunks)
- cmd/start-service/start-service.go (1 hunks)
- pkg/graph/mockGraph.go (2 hunks)
- pkg/graph/storage.go (1 hunks)
- pkg/storages/redis_storage.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- api/v1/service.proto
- api/v1/service_test.go
- cmd/query/custom/custom.go
- cmd/query/globsearch/globsearch.go
- pkg/graph/storage.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
api/v1/service.go
[warning] 87-88: api/v1/service.go#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 91-95: api/v1/service.go#L91-L95
Added lines #L91 - L95 were not covered by testspkg/graph/mockGraph.go
[warning] 54-66: pkg/graph/mockGraph.go#L54-L66
Added lines #L54 - L66 were not covered by tests
[warning] 68-68: pkg/graph/mockGraph.go#L68
Added line #L68 was not covered by testspkg/storages/redis_storage.go
[warning] 75-80: pkg/storages/redis_storage.go#L75-L80
Added lines #L75 - L80 were not covered by tests
[warning] 82-91: pkg/storages/redis_storage.go#L82-L91
Added lines #L82 - L91 were not covered by tests
[warning] 93-97: pkg/storages/redis_storage.go#L93-L97
Added lines #L93 - L97 were not covered by tests
[warning] 100-100: pkg/storages/redis_storage.go#L100
Added line #L100 was not covered by tests
🔇 Additional comments (5)
cmd/query/query.go (2)
4-5: LGTM: Clean import organization for new subcommands.The imports are well-organized, using clear aliases for the subcommand packages to avoid naming conflicts.
12-14: Verify command description accuracy.The command description "Query dependencies and dependents of a project" might need updating since it now supports glob searching as well.
Consider updating the description to:
-Short: "Query dependencies and dependents of a project", +Short: "Query dependencies and search nodes using patterns",cmd/start-service/start-service.go (1)
47-48: LGTM! The GraphService handler is properly registered.The new GraphService handler is correctly registered with the HTTP multiplexer following the same pattern as other services. This addition enables the glob searching functionality while maintaining consistency with the existing codebase structure.
Let's verify the GraphService implementation and its connection with the glob searching feature:
✅ Verification successful
Let me try a different approach to verify the GraphService implementation and glob search functionality.
This should help us locate and verify the GraphService implementation and its glob search functionality across the codebase.
Based on the shell script results, I can now provide a final response as we have comprehensive evidence about the GraphService implementation and its glob search functionality.
The GraphService handler registration is correctly implemented and fully supported
The verification results show that:
- The GraphService is properly defined in the protocol buffers (
api/v1/service.proto) with theGetNodesByGlobRPC method- The implementation in
api/v1/service.gocorrectly handles glob pattern matching through the storage layer- The service registration in
cmd/start-service/start-service.gofollows the established pattern and is properly connected to the implementation- The feature is fully integrated across multiple components:
- Protocol definition (
GetNodesByGlobRequest/Response)- Service implementation (
Service.GetNodesByGlob)- Storage interface (
Storage.GetNodesByGlob)- Redis implementation (
RedisStorage.GetNodesByGlob)- Mock implementation for testing (
MockStorage.GetNodesByGlob)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the GraphService implementation and glob search functionality # Test 1: Check if the GraphService proto definition includes the glob search method rg -A 5 "service GraphService" gen/api/v1/ # Test 2: Verify the GetNodesByGlob implementation in the service ast-grep --pattern 'func (s *Service) GetNodesByGlob($_, $_) { $$$ }'Length of output: 118
Script:
#!/bin/bash # Let's try different paths and patterns # Check for proto definitions fd -e proto . | xargs rg -l "GraphService" # Look for glob-related service definitions fd . -e proto -e go | xargs rg -l "GetNodesByGlob" # Search for glob implementation patterns rg -A 5 "GetNodesByGlob" # Check service implementation files fd "service.go" | xargs catLength of output: 28762
pkg/graph/mockGraph.go (1)
6-6: LGTM!The filepath package import is correctly placed and necessary for the glob pattern matching functionality.
api/v1/service.go (1)
84-98: Add test coverage for the new endpoint.The static analysis indicates that error paths are not covered by tests. Consider adding test cases for:
- Empty pattern validation
- Storage errors
- Node conversion errors
Let's verify the current test coverage:
Would you like me to help generate comprehensive test cases for this new endpoint?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 87-88: api/v1/service.go#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 91-95: api/v1/service.go#L91-L95
Added lines #L91 - L95 were not covered by tests
Summary by CodeRabbit
Release Notes
New Features
GetNodesByGlobmethod.Bug Fixes
Tests
Chores