Refactor getmetada cmd #116
Conversation
Signed-off-by: naveensrinivasan <[email protected]>
Signed-off-by: naveensrinivasan <[email protected]>
- refactor getmetadata Signed-off-by: naveensrinivasan <[email protected]>
WalkthroughThe changes in this pull request involve significant updates to the Changes
Possibly related PRs
Poem
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 ReportAll modified and coverable lines are covered by tests ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
cmd/query/globsearch/globsearch.go (1)
86-90: Consider making the table columns configurableThe table structure is currently hardcoded. For better maintainability and flexibility, consider making the columns configurable, perhaps through a configuration struct or slice of column definitions. This would make it easier to add or modify columns in the future without changing the core formatting logic.
Example approach:
type Column struct { Header string ValueFunc func(*apiv1.Node) string } func formatTable(w io.Writer, nodes []*apiv1.Node, maxOutput int, columns []Column) error { table := tablewriter.NewWriter(w) headers := make([]string, len(columns)) for i, col := range columns { headers[i] = col.Header } table.SetHeader(headers) // ... rest of the implementation }cmd/query/getMetadata/getMetadata_test.go (3)
19-146: Consider adding more edge cases to TestFormatNodeJSON.While the test coverage is good, consider adding these additional test cases to make it more robust:
- Test with extremely large metadata to verify handling of size limits
- Test with metadata containing nested arrays
- Test with empty strings in node fields (name, type)
tests := []struct { // ... existing test cases ... + { + name: "large metadata", + node: &apiv1.Node{ + Name: "node6", + Type: "type6", + Id: 6, + Metadata: []byte(`{ + "largeField": "` + strings.Repeat("x", 10000) + `" + }`), + }, + }, + { + name: "metadata with nested arrays", + node: &apiv1.Node{ + Name: "node7", + Type: "type7", + Id: 7, + Metadata: []byte(`{ + "array": [1, 2, ["nested", "array"]] + }`), + }, + }, + { + name: "empty string fields", + node: &apiv1.Node{ + Name: "", + Type: "", + Id: 8, + Metadata: []byte(`{"key": "value"}`), + }, + }, }
178-303: Enhance TestRun with additional test scenarios and parallel execution.Consider the following improvements:
- Add test case for context cancellation
- Validate request parameters in mock client
- Enable parallel test execution for faster tests
func TestRun(t *testing.T) { + // Enable parallel test execution + t.Parallel() + tests := []struct { // ... existing fields ... + expectedRequest *apiv1.GetNodeByNameRequest }{ // ... existing test cases ... + { + name: "context cancellation", + nodeName: "node1", + output: "json", + mockResponse: &apiv1.GetNodeByNameResponse{ + Node: &apiv1.Node{Name: "node1"}, + }, + expectedRequest: &apiv1.GetNodeByNameRequest{ + Name: "node1", + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Create cancelled context for cancellation test + ctx := context.Background() + if tt.name == "context cancellation" { + var cancel context.CancelFunc + ctx, cancel = context.WithCancel(ctx) + cancel() + } + mockClient := &mockGraphServiceClient{ GetNodeByNameFunc: func(ctx context.Context, req *connect.Request[apiv1.GetNodeByNameRequest]) (*connect.Response[apiv1.GetNodeByNameResponse], error) { + // Validate request parameters + if tt.expectedRequest != nil { + assert.Equal(t, tt.expectedRequest.Name, req.Msg.Name) + } + + // Handle context cancellation + if ctx.Err() != nil { + return nil, ctx.Err() + } + if tt.mockError != nil { return nil, tt.mockError } return connect.NewResponse(tt.mockResponse), nil }, } // ... rest of the test ... - cmd.SetContext(context.Background()) + cmd.SetContext(ctx) }) } }
305-333: Enhance flag testing with more comprehensive validation.The current flag tests are basic. Consider adding:
- Validation of flag default values
- Validation of flag types
- Testing of flag value parsing
func TestAddFlags(t *testing.T) { o := &options{} cmd := &cobra.Command{} o.AddFlags(cmd) - flags := []string{"output-file", "addr", "output"} + tests := []struct { + name string + flag string + expectedType string + defaultValue string + testValue string + }{ + { + name: "output flag", + flag: "output", + expectedType: "string", + defaultValue: "table", + testValue: "json", + }, + { + name: "addr flag", + flag: "addr", + expectedType: "string", + defaultValue: "http://localhost:8080", + testValue: "http://example.com", + }, + { + name: "output-file flag", + flag: "output-file", + expectedType: "string", + defaultValue: "", + testValue: "test.out", + }, + } - for _, flag := range flags { + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { - if cmd.Flags().Lookup(flag) == nil { - t.Errorf("Flag %q not found", flag) + flag := cmd.Flags().Lookup(tt.flag) + if flag == nil { + t.Fatalf("Flag %q not found", tt.flag) } + + // Verify flag type + assert.Equal(t, tt.expectedType, flag.Value.Type()) + + // Verify default value + assert.Equal(t, tt.defaultValue, flag.DefValue) + + // Test value parsing + err := cmd.Flags().Set(tt.flag, tt.testValue) + assert.NoError(t, err) + assert.Equal(t, tt.testValue, flag.Value.String()) + }) } }cmd/query/getMetadata/getMetadata.go (2)
42-55: Suggestion: Initializeoptionswith default valuesConsider initializing the
optionsstruct with default values foraddrandoutputto ensure they have valid defaults even if flags are not provided.Apply this diff to initialize default values:
func New() *cobra.Command { - o := &options{} + o := &options{ + addr: defaultAddr, + output: outputFormatJSON, + } cmd := &cobra.Command{ Use: "get-metadata [node name]", Short: "Outputs the node with its metadata",
138-161: Consider including metadata in the table outputCurrently, the
formatTablefunction does not display the node's metadata. If the metadata is important to the end-user, consider adding it to the table output or providing an option to include it.Apply this diff to include metadata in the table:
func formatTable(w io.Writer, node *apiv1.Node) error { table := tablewriter.NewWriter(w) - table.SetHeader([]string{"Name", "Type", "ID"}) + table.SetHeader([]string{"Name", "Type", "ID", "Metadata"}) table.SetAutoWrapText(false) table.SetAutoFormatHeaders(true) + var metadata string + if len(node.Metadata) > 0 { + metadata = string(node.Metadata) + } else { + metadata = "N/A" + } + table.Append([]string{ node.Name, node.Type, - strconv.FormatUint(uint64(node.Id), 10), + strconv.FormatUint(uint64(node.Id), 10), + metadata, }) table.Render()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
cmd/query/getMetadata/getMetadata.go(1 hunks)cmd/query/getMetadata/getMetadata_test.go(1 hunks)cmd/query/globsearch/globsearch.go(1 hunks)cmd/query/globsearch/globsearch_test.go(1 hunks)cmd/query/query.go(1 hunks)
🔇 Additional comments (13)
cmd/query/query.go (1)
19-19: LGTM! The change aligns with the refactoring objective.
The removal of the storage parameter from getMetadata.New() is consistent with the package's refactoring to use a new options-based initialization approach.
Consider applying similar refactoring to other commands (custom, globsearch) if they could benefit from the same options-based initialization pattern for consistency across the codebase.
Let's verify the proper initialization in the getMetadata package:
✅ Verification successful
Let me gather more information about the getMetadata package initialization by checking its options struct and Run implementation.
Let me check the getMetadata package's options struct and initialization details.
Let me check the complete options struct definition and client initialization in getMetadata.
Let me check the complete file content to see the full implementation.
LGTM! The refactoring is properly implemented with client initialization handled within the package.
The getMetadata package now correctly manages its dependencies:
- Options struct contains necessary fields (addr, output, graphServiceClient)
- Client initialization is handled lazily in fetchNode method
- No storage dependency is needed as it uses the GraphService client
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify getMetadata package initialization
# Expected: Find initialization of required dependencies (client, options) in getMetadata package
# Search for options struct and initialization
ast-grep --pattern 'type options struct {
$$$
}'
# Search for New function implementation
ast-grep --pattern 'func New() *cobra.Command {
$$$
}'
Length of output: 5063
Script:
#!/bin/bash
# Search for getMetadata options struct
ast-grep --pattern 'type options struct {
$$$
}' cmd/query/getMetadata/getMetadata.go
# Search for Run method implementation to verify client initialization
ast-grep --pattern 'func (o *options) Run($$$) error {
$$$
}'
Length of output: 29234
Script:
#!/bin/bash
# Search for getMetadata options struct and its fields
rg -A 5 "type options struct" cmd/query/getMetadata/getMetadata.go
# Search for initialization related code in getMetadata
rg -A 5 "func \(o \*options\) AddFlags|func \(o \*options\) fetchNode" cmd/query/getMetadata/getMetadata.go
Length of output: 686
Script:
#!/bin/bash
# Get the complete options struct
rg -U "type options struct \{.*?\}" -A 5 cmd/query/getMetadata/getMetadata.go
# Get the client initialization code
rg -A 10 "if o.graphServiceClient == nil" cmd/query/getMetadata/getMetadata.go
Length of output: 376
Script:
#!/bin/bash
# Get the complete getMetadata.go file
cat cmd/query/getMetadata/getMetadata.go
Length of output: 5196
cmd/query/globsearch/globsearch.go (2)
69-69: LGTM! Clean removal of the showAdditionalInfo parameter
The update to the formatTable call is consistent with the simplified function signature.
76-81: LGTM! Clean function signature and table setup
The simplified function signature and basic table structure are well-implemented.
cmd/query/globsearch/globsearch_test.go (2)
Line range hint 19-42: Verify table header behavior after parameter removal.
The test cases cover basic functionality, but explicitly verify that table headers are still displayed correctly since the removed boolean parameter might have been related to header control. Consider adding a specific test case that validates header presence/format.
Let's check the implementation to understand the header behavior:
#!/bin/bash
# Find the formatTable implementation to verify header handling
ast-grep --pattern 'func formatTable($$$) {
$$$
}'63-63: LGTM! Function call updated to match new signature.
The removal of the boolean parameter from the formatTable call aligns with the simplified function signature. The test cases remain comprehensive, covering empty input, output limits, and basic functionality.
Let's verify that all calls to formatTable have been updated consistently:
✅ Verification successful
Function signature mismatch detected between packages
The verification revealed inconsistent formatTable function signatures across different packages:
cmd/query/globsearch/globsearch.go:formatTable(w io.Writer, nodes []*apiv1.Node, maxOutput int)cmd/query/getMetadata/getMetadata.go:formatTable(w io.Writer, node *apiv1.Node)
While the review comment's specific change is correct for its package, there appears to be similar but distinct formatTable functions in different packages. This is not an issue as they serve different purposes:
- globsearch: Handles multiple nodes with output limiting
- getMetadata: Formats a single node's metadata
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining calls to formatTable with the old signature
rg "formatTable.*,.*true|formatTable.*,.*false" --type go
Length of output: 148
Script:
#!/bin/bash
# Search for formatTable function definition and its usages
rg "formatTable" -A 5 --type go
Length of output: 3677
cmd/query/getMetadata/getMetadata_test.go (1)
3-18: LGTM! Well-organized imports.
The imports are properly organized and all are being utilized in the test file.
cmd/query/getMetadata/getMetadata.go (7)
4-7: Approved: Necessary imports added
The imports added are appropriate for the new functionality, including handling JSON, HTTP requests, and table formatting.
19-33: Approved: New struct definitions are well-defined
The options and nodeOutput structs are appropriately defined to support the command's options and output formatting.
35-41: Approved: Constants defined for output formats and default values
Defining constants for output formats and default values enhances code readability and maintainability.
76-104: Approved: fetchNode method handles node retrieval and errors appropriately
The fetchNode method correctly handles the retrieval of node data and provides clear error messages for missing node names and not found nodes.
106-117: Approved: Output formatting logic is cleanly separated
The formatAndDisplayOutput method cleanly separates output formatting logic based on the specified format, enhancing code maintainability.
119-137: Approved: JSON output handling is implemented correctly
The handleJSONOutput method appropriately processes the node data into JSON format and handles writing to a file or displaying to stdout.
42-55: Verify that flags are correctly parsed and utilized
Ensure that command-line flags for --addr and --output are correctly parsed and override the default values when provided.
Run the following script to confirm that flags are properly handled:
✅ Verification successful
Flags are properly implemented and thoroughly tested
The code review concern has been thoroughly investigated and the flags are correctly implemented:
-
The flags are properly defined with default values:
--addrdefaults to "http://localhost:8089"--outputdefaults to "json"--output-filedefaults to empty string
-
The implementation includes comprehensive test coverage that verifies:
- Flag registration and default values
- Flag parsing and usage in the Run method
- Different output formats (json/table)
- Error handling for invalid flag values
-
The test suite includes specific test cases for all flag combinations and edge cases, ensuring robust flag handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Test the command with and without flags to verify that defaults and overrides work as expected.
# Test 1: Run without flags and check if default values are used.
./get-metadata node1
# Test 2: Run with custom address and output format.
./get-metadata node1 --addr "http://customserver:8080" --output "table"
Length of output: 272
Script:
#!/bin/bash
# Let's examine the options struct and flag definitions
# First, check the options struct and flag setup
rg -A 10 "type options struct" cmd/query/getMetadata/
# Then, look for AddFlags method implementation
ast-grep --pattern 'func (o *options) AddFlags($$$) {
$$$
}'
# Also check for any flag usage in the Run method
ast-grep --pattern 'func (o *options) Run($$$) {
$$$
}'
Length of output: 4908
Script:
#!/bin/bash
# Let's examine the options struct definition and the Run method implementation
# First, check the options struct
rg -A 5 "type options struct" cmd/query/getMetadata/getMetadata.go
# Then check the Run method and how it uses these flags
rg -A 20 "func \(o \*options\) Run" cmd/query/getMetadata/getMetadata.go
# Also check for any default constants
rg "const.*default" cmd/query/getMetadata/getMetadata.go
Length of output: 851
Script:
#!/bin/bash
# Let's check the entire file content to see the complete implementation
cat cmd/query/getMetadata/getMetadata.go
# Also check for any test files that might verify the flag handling
fd "getMetadata.*test\.go" cmd/query/getMetadata/
Length of output: 5290
Script:
#!/bin/bash
# Let's check the test file to see how flags are verified
cat cmd/query/getMetadata/getMetadata_test.go
Length of output: 8339
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores