Refactor the cmd for custom #132
Conversation
WalkthroughThe changes in this pull request primarily involve the removal of the Changes
Possibly related PRs
Warning Rate limit exceeded@naveensrinivasan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 (
|
- refactored the custom cmd Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
naveensrinivasan
force-pushed
the
naveen/custom/cmd/refactor
branch
from
1e452ab
to
b4400e5
Compare
Codecov ReportAttention: Patch coverage is
|
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
naveensrinivasan
force-pushed
the
naveen/custom/cmd/refactor
branch
from
329e39c
to
8f47231
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: 4
🧹 Outside diff range and nitpick comments (12)
cmd/leaderboard/leaderboard.go (1)
Line range hint
13-26: Add unit tests for the leaderboard commandThe
New()function lacks test coverage. Consider adding tests to verify:
- Command initialization
- Subcommand registration
- Command properties (Use, Short, Long descriptions)
Would you like me to help create a test file with comprehensive test cases for the leaderboard command initialization?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 13-13: cmd/leaderboard/leaderboard.go#L13
Added line #L13 was not covered by testscmd/query/query.go (1)
12-14: Add test coverage for command metadataThe command metadata is well-documented with clear descriptions. However, these changes aren't covered by tests.
Would you like me to help create test cases to verify the command metadata? This could include:
- Verifying command name and aliases
- Validating help text output
- Testing command initialization
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 12-14: cmd/query/query.go#L12-L14
Added lines #L12 - L14 were not covered by testscmd/ingest/sbom/sbom.go (1)
60-61: Add test coverage for command initializationThe command initialization code lacks test coverage. Consider adding tests to verify:
- Command creation with default options
- Flag registration
- Command validation
Would you like me to help create test cases for the command initialization? This would help ensure the refactored code remains stable.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-61: cmd/ingest/sbom/sbom.go#L60-L61
Added lines #L60 - L61 were not covered by testscmd/ingest/scorecard/scorecard.go (1)
Line range hint
1-83: Good architectural improvement in progress.The removal of the storage dependency from the command layer is a positive architectural change that:
- Reduces coupling between the command layer and storage implementation
- Makes the code more maintainable and testable
- Follows the separation of concerns principle
Consider documenting these architectural decisions in the project's documentation to help future contributors understand the design choices.
pkg/tools/ingest/loader.go (2)
Line range hint
37-44: Remove duplicate error handling blockThere's a duplicate error check and append operation in the directory processing loop. The second error check is redundant as the error is already handled in the first block.
subResult, err := LoadDataFromPath(entryPath) if err != nil { errors = append(errors, fmt.Errorf("failed to load data from path %s: %w", entryPath, err)) } else { result = append(result, subResult...) } -if err != nil { - errors = append(errors, fmt.Errorf("failed to ingest data from path %s: %w", entryPath, err)) -}
19-19: Update function documentation to reflect changesThe function comment should be updated to reflect the removal of the storage parameter and clearly document the function's current responsibilities:
-// LoadDataFromPath takes in a directory or file path and processes the data into the storage. -// The data can either be JSON files or ZIP files containing JSON files. +// LoadDataFromPath processes data from a given file path, supporting both JSON files and ZIP archives +// containing JSON files. It recursively processes directories and returns the collected data. +// Returns a slice of Data containing the file paths and their contents, or an error if any operation fails.cmd/query/custom/custom.go (2)
18-25: LGTM! Good architectural improvementsThe refactoring of the options struct shows good separation of concerns:
- Removal of storage dependency aligns with the service-oriented architecture
- Addition of queryServiceClient enables better testing through dependency injection
- New output options provide better flexibility for different use cases
Consider adding documentation for the output formats and valid values in the struct tags or package documentation.
28-34: Add validation for the output format flagWhile the flag setup is good, consider adding validation for the output format during flag parsing to fail fast if an invalid format is provided.
func (o *options) AddFlags(cmd *cobra.Command) { cmd.Flags().IntVar(&o.maxOutput, "max-output", 10, "maximum number of results to display") cmd.Flags().BoolVar(&o.showInfo, "show-info", true, "display the info column") cmd.Flags().StringVar(&o.addr, "addr", "http://localhost:8089", "address of the minefield server") - cmd.Flags().StringVar(&o.output, "output", "table", "output format (table or json)") + cmd.Flags().StringVar(&o.output, "output", "table", "output format (table or json)") + cmd.RegisterFlagCompletionFunc("output", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + return []string{"table", "json"}, cobra.ShellCompDirectiveNoFileComp + }) }pkg/tools/ingest/vuln_test.go (1)
Line range hint
36-54: Consider making the test more resilient to test data changesWhile the removal of the
storageparameter is handled correctly, the test assumes a very specific outcome (exactly 3 new nodes). This might make the test brittle if the test data changes.Consider:
- Adding a comment explaining why exactly 3 new nodes are expected
- Making the test more resilient by either:
- Calculating the expected number of nodes based on the test data
- Using a range check instead of an exact number
- if len(keys) != numberOfNodes+3 { - t.Fatalf("Expected number of nodes to be %d, got %d", numberOfNodes+3, len(keys)) + // Add comment explaining the expected node increase + expectedMinIncrease := 1 // minimum number of new vulnerability nodes + if len(keys) <= numberOfNodes || len(keys) > numberOfNodes+10 { + t.Fatalf("Expected number of nodes to increase by at least %d, got an increase of %d", + expectedMinIncrease, len(keys)-numberOfNodes) + }api/v1/service_test.go (1)
Line range hint
76-124: Consider breaking down the test into smaller, focused test cases.The
TestQueriesIngestAndCachefunction is testing multiple operations in a single test case, which can make it difficult to maintain and debug. Consider splitting it into separate test cases for each operation:
TestSBOMIngestionTestVulnerabilityIngestionTestCustomLeaderboardTestDependencyQueriesTestCacheOperationsThis would improve:
- Test isolation
- Failure debugging
- Maintenance
- Readability
Example refactor for the SBOM ingestion test:
func TestSBOMIngestion(t *testing.T) { s := setupService() // Load and ingest SBOM result, err := ingest.LoadDataFromPath("../../testdata/osv-sboms/google_agi.sbom.json") require.NoError(t, err) for _, data := range result { sbomReq := connect.NewRequest(&service.IngestSBOMRequest{ Sbom: data.Data, }) _, err = s.IngestSBOM(context.Background(), sbomReq) require.NoError(t, err) } // Verify ingestion graphReq := connect.NewRequest(&service.GetNodeByNameRequest{ Name: "pkg:pypi/astroid@2.11.7", }) resp, err := s.GetNodeByName(context.Background(), graphReq) require.NoError(t, err) assert.NotNil(t, resp.Msg.Node) }cmd/query/custom/custom_test.go (2)
17-62: Consider refactoring to use table-driven testsThe test could be more maintainable and concise using a table-driven approach with the assertion library.
Here's a suggested refactoring:
func TestOptions_AddFlags(t *testing.T) { o := &options{} cmd := &cobra.Command{} o.AddFlags(cmd) - - // Test "max-output" flag - maxOutputFlag := cmd.Flags().Lookup("max-output") - if maxOutputFlag == nil { - t.Error("Expected 'max-output' flag to be defined") - } else { - if maxOutputFlag.DefValue != "10" { - t.Errorf("Expected default value of 'max-output' to be '10', got '%s'", maxOutputFlag.DefValue) - } - } - // ... similar blocks for other flags + + tests := []struct { + flagName string + defValue string + }{ + {"max-output", "10"}, + {"show-info", "true"}, + {"addr", "http://localhost:8089"}, + {"output", "table"}, + } + + for _, tt := range tests { + t.Run(tt.flagName, func(t *testing.T) { + flag := cmd.Flags().Lookup(tt.flagName) + assert.NotNil(t, flag, "Expected '%s' flag to be defined", tt.flagName) + assert.Equal(t, tt.defValue, flag.DefValue, + "Expected default value of '%s' to be '%s'", tt.flagName, tt.defValue) + }) + }
64-115: Consider refactoring command property checksSimilar to the previous suggestion, the command property checks could be more maintainable using table-driven tests.
Here's a suggested refactoring for the command properties section:
func TestNewCommand(t *testing.T) { cmd := New() - if cmd == nil { - t.Fatal("Expected New() to return a non-nil command") - } + assert.NotNil(t, cmd, "Expected New() to return a non-nil command") - // Check the command's basic properties - if cmd.Use != "custom [script]" { - t.Errorf("Expected Use: 'custom [script]', got: '%s'", cmd.Use) - } + properties := []struct { + property string + expected string + actual string + }{ + {"Use", "custom [script]", cmd.Use}, + {"Short", "Execute a custom query script", cmd.Short}, + {"Long", "Execute a custom query script to perform tailored queries against the project's dependencies and dependents.", cmd.Long}, + } - // ... similar blocks for other properties + for _, prop := range properties { + assert.Equal(t, prop.expected, prop.actual, + "Expected %s: '%s', got: '%s'", prop.property, prop.expected, prop.actual) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
api/v1/service_test.go(1 hunks)cmd/ingest/ingest.go(1 hunks)cmd/ingest/osv/osv.go(3 hunks)cmd/ingest/sbom/sbom.go(3 hunks)cmd/ingest/scorecard/scorecard.go(3 hunks)cmd/leaderboard/leaderboard.go(1 hunks)cmd/query/custom/custom.go(1 hunks)cmd/query/custom/custom_test.go(1 hunks)cmd/query/query.go(1 hunks)cmd/root/root.go(1 hunks)codecov.yml(0 hunks)pkg/storages/e2e_test.go(1 hunks)pkg/tools/ingest/loader.go(5 hunks)pkg/tools/ingest/sbom_test.go(2 hunks)pkg/tools/ingest/vuln_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- codecov.yml
🧰 Additional context used
🪛 GitHub Check: codecov/patch
cmd/ingest/ingest.go
[warning] 15-15: cmd/ingest/ingest.go#L15
Added line #L15 was not covered by tests
[warning] 23-25: cmd/ingest/ingest.go#L23-L25
Added lines #L23 - L25 were not covered by tests
cmd/ingest/osv/osv.go
[warning] 38-38: cmd/ingest/osv/osv.go#L38
Added line #L38 was not covered by tests
[warning] 57-58: cmd/ingest/osv/osv.go#L57-L58
Added lines #L57 - L58 were not covered by tests
cmd/ingest/sbom/sbom.go
[warning] 39-39: cmd/ingest/sbom/sbom.go#L39
Added line #L39 was not covered by tests
[warning] 60-61: cmd/ingest/sbom/sbom.go#L60-L61
Added lines #L60 - L61 were not covered by tests
cmd/ingest/scorecard/scorecard.go
[warning] 39-39: cmd/ingest/scorecard/scorecard.go#L39
Added line #L39 was not covered by tests
[warning] 60-61: cmd/ingest/scorecard/scorecard.go#L60-L61
Added lines #L60 - L61 were not covered by tests
cmd/leaderboard/leaderboard.go
[warning] 13-13: cmd/leaderboard/leaderboard.go#L13
Added line #L13 was not covered by tests
cmd/query/custom/custom.go
[warning] 45-50: cmd/query/custom/custom.go#L45-L50
Added lines #L45 - L50 were not covered by tests
[warning] 71-72: cmd/query/custom/custom.go#L71-L72
Added lines #L71 - L72 were not covered by tests
cmd/query/query.go
[warning] 10-10: cmd/query/query.go#L10
Added line #L10 was not covered by tests
[warning] 12-14: cmd/query/query.go#L12-L14
Added lines #L12 - L14 were not covered by tests
[warning] 18-19: cmd/query/query.go#L18-L19
Added lines #L18 - L19 were not covered by tests
[warning] 22-22: cmd/query/query.go#L22
Added line #L22 was not covered by tests
cmd/root/root.go
[warning] 34-35: cmd/root/root.go#L34-L35
Added lines #L34 - L35 were not covered by tests
[warning] 37-37: cmd/root/root.go#L37
Added line #L37 was not covered by tests
pkg/tools/ingest/loader.go
[warning] 49-49: pkg/tools/ingest/loader.go#L49
Added line #L49 was not covered by tests
[warning] 71-71: pkg/tools/ingest/loader.go#L71
Added line #L71 was not covered by tests
[warning] 103-103: pkg/tools/ingest/loader.go#L103
Added line #L103 was not covered by tests
🔇 Additional comments (31)
cmd/ingest/ingest.go (2)
15-15: Verify the impact of removing the storage parameter
The removal of the storage parameter aligns with the broader refactoring pattern across the codebase. However, we should verify that the command's functionality remains intact without this dependency.
Let's check if storage initialization has been moved elsewhere:
✅ Verification successful
Storage dependency removal is consistent with service-based architecture
The removal of the storage parameter is correct and aligns with the codebase's architecture. The commands now use ingestServiceClient for data operations instead of direct storage access, following a service-based pattern. All subcommands (osv, sbom, scorecard) consistently initialize their client connections in their respective Run methods, making the storage parameter unnecessary at the command initialization level.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for storage initialization patterns in the codebase
# Look for storage initialization in parent commands or middleware
# Search for storage initialization patterns
rg -A 5 "graph\.Storage|storage\." cmd/
# Check for any middleware or initialization patterns
ast-grep --pattern 'func PreRun($$$) { $$ }'
Length of output: 2800
Script:
#!/bin/bash
# Let's check for storage initialization patterns more thoroughly
# Check for storage initialization in the Run methods
rg -A 5 "func.*Run" cmd/ingest/
# Check for storage-related imports and usage
rg "\".*storage\"" cmd/ingest/
# Look for storage initialization in the parent command
cat cmd/ingest/ingest.go
Length of output: 1753
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 15-15: cmd/ingest/ingest.go#L15
Added line #L15 was not covered by tests
23-25: Ensure consistent refactoring across subcommands
The subcommand initialization has been updated consistently to remove the storage parameter. However, test coverage is missing for these changes.
Let's verify the consistency of this pattern:
Would you like me to help generate test cases for the command initialization? This would help address the code coverage warnings from the static analysis.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-25: cmd/ingest/ingest.go#L23-L25
Added lines #L23 - L25 were not covered by tests
cmd/leaderboard/leaderboard.go (1)
13-13: Verify removal of storage parameter across dependent packages
The removal of the storage parameter aligns with the broader refactoring pattern. However, let's verify that the dependent packages (keys and custom) have been updated accordingly.
✅ Verification successful
Storage parameter removal is consistent across packages
The verification shows that both dependent packages (keys and custom) have already been updated to use the new signature without the storage parameter. Their New() functions match the signature change in the parent package, and no storage-related code was found in any of the leaderboard packages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that dependent packages have also removed storage parameter
# and there are no lingering references
# Check for any remaining storage parameter usage in dependent packages
echo "Checking for storage parameter in keys package..."
ast-grep --pattern 'func New(storage $_) $_' cmd/leaderboard/keys/
echo "Checking for storage parameter in custom package..."
ast-grep --pattern 'func New(storage $_) $_' cmd/leaderboard/custom/
# Check for any direct storage field references that might need updating
echo "Checking for storage field references..."
rg "storage\." cmd/leaderboard/
Length of output: 486
Script:
#!/bin/bash
# Let's try a different approach to verify the changes
# First, let's check the actual function signatures in these packages
echo "Checking function signatures in keys package..."
rg "^func New" cmd/leaderboard/keys/ -A 2
echo "Checking function signatures in custom package..."
rg "^func New" cmd/leaderboard/custom/ -A 2
# Let's also check for any storage-related imports or variables
echo "Checking for storage-related code in all leaderboard packages..."
rg "storage" cmd/leaderboard/ -A 1
Length of output: 786
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 13-13: cmd/leaderboard/leaderboard.go#L13
Added line #L13 was not covered by tests
cmd/query/query.go (2)
10-10: Verify storage parameter removal across dependent packages
The removal of the storage parameter aligns with the broader refactoring effort. Let's verify this change is consistently applied across related packages.
✅ Verification successful
Storage parameter removal is part of a consistent pattern
The removal of the storage parameter from query.New() is part of a broader architectural change where most commands (ingest, cache, leaderboard, and their subcommands) don't require direct storage access. Only the server command retains the storage parameter, which is passed down from the root command, indicating a deliberate design where storage access is centralized through the server component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining storage parameter usage in New() functions
# across related packages to ensure consistent refactoring
# Search for New functions that still accept storage parameter
ast-grep --pattern 'func New(storage $_) $_'
# Search for any remaining direct storage parameter usage
rg -l "storage.*graph\.Storage"
Length of output: 1884
Script:
#!/bin/bash
# Let's check the implementation of query.New() to understand how it handles storage now
ast-grep --pattern 'func New() *cobra.Command {
$$$
}'
# Also check for any storage-related code in the query package
rg -l "storage" cmd/query/
# Check how other commands that removed storage parameter are handling it
ast-grep --pattern 'func New() *cobra.Command {
$$$
}' cmd/ingest/ cmd/cache/ cmd/leaderboard/
Length of output: 16297
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-10: cmd/query/query.go#L10
Added line #L10 was not covered by tests
18-22: Add integration tests for subcommand registration
While the subcommand registration looks correct, we should ensure proper integration testing.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 18-19: cmd/query/query.go#L18-L19
Added lines #L18 - L19 were not covered by tests
[warning] 22-22: cmd/query/query.go#L22
Added line #L22 was not covered by tests
cmd/root/root.go (1)
34-35: Verify the architectural change in storage handling
The removal of the storage parameter from query.New(), ingest.New(), and leaderboard.New() while retaining it for server.New(storage) creates an inconsistent pattern. This architectural change raises several concerns:
- How is storage now being handled in these commands?
- Is this inconsistency intentional?
- Could this lead to potential runtime issues if these commands still need storage access?
Let's verify how storage is now being handled in these commands:
Also applies to: 37-37
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-35: cmd/root/root.go#L34-L35
Added lines #L34 - L35 were not covered by tests
cmd/ingest/sbom/sbom.go (4)
17-17: LGTM! Clean struct definition
The simplified struct with essential fields improves code maintainability.
39-39: LGTM! Simplified function call
The simplified LoadDataFromPath call aligns with the refactoring objectives while maintaining functionality.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-39: cmd/ingest/sbom/sbom.go#L39
Added line #L39 was not covered by tests
60-61: Verify command registration after signature change
The removal of the storage parameter from New() might affect command registration in other parts of the codebase.
#!/bin/bash
# Search for references to this command's New function
rg "sbom\.New\(" --type go
# Look for similar command registrations
rg "cobra\.Command.*sbom" --type go🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-61: cmd/ingest/sbom/sbom.go#L60-L61
Added lines #L60 - L61 were not covered by tests
39-39: Add test coverage for SBOM ingestion
This line represents a critical path in the SBOM ingestion process but lacks test coverage.
Would you like me to help create a test case for this functionality? I can generate a test that covers:
- Success case with valid SBOM data
- Error handling for invalid paths
- Error handling for malformed SBOM data
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-39: cmd/ingest/sbom/sbom.go#L39
Added line #L39 was not covered by tests
cmd/ingest/scorecard/scorecard.go (4)
16-19: LGTM! Good simplification of the options struct.
The removal of the storage field makes the struct more focused and follows the single responsibility principle better.
39-39: LGTM! Clean refactoring of LoadDataFromPath call.
The simplified function call without the storage parameter aligns well with the refactoring objectives.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-39: cmd/ingest/scorecard/scorecard.go#L39
Added line #L39 was not covered by tests
39-39: Add test coverage for the refactored code path.
The static analysis indicates that this line lacks test coverage. Consider adding tests to verify the behavior of LoadDataFromPath integration.
Let's check the existing test coverage:
#!/bin/bash
# Description: Check for existing test files and coverage
# Look for test files that might need updating
fd -e go -e _test.go scorecard cmd/ingest
# Check if there are any existing test cases for LoadDataFromPath
rg -A 5 "LoadDataFromPath" --type go --type testWould you like me to help generate test cases for this code path?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-39: cmd/ingest/scorecard/scorecard.go#L39
Added line #L39 was not covered by tests
Line range hint 60-71: Add test coverage for the command initialization.
The command initialization looks good, but lacks test coverage. Consider adding tests to verify:
- Command creation with default options
- Flag initialization
- Argument validation
Let's check for existing command tests:
Would you like me to help generate test cases for command initialization?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-61: cmd/ingest/scorecard/scorecard.go#L60-L61
Added lines #L60 - L61 were not covered by tests
cmd/ingest/osv/osv.go (3)
17-17: LGTM! Storage field removal aligns with refactoring goals
The simplification of the options struct by removing the storage dependency is consistent with the broader refactoring effort.
57-58: Consider adding integration tests for command initialization
The simplified New() function aligns with the refactoring goals. However, it lacks test coverage. Consider adding integration tests to verify proper command initialization and flag setup.
Let's check for existing command testing patterns:
#!/bin/bash
# Search for existing command initialization tests
rg -t go "func Test.*Command" -g "*test.go"
# Look for cobra command testing patterns
rg -t go "cobra.*testing" -g "*test.go"Would you like me to help create integration tests for the OSV command initialization?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-58: cmd/ingest/osv/osv.go#L57-L58
Added lines #L57 - L58 were not covered by tests
38-38: Add test coverage for vulnerability data loading
While the simplified LoadDataFromPath call aligns with the refactoring, this critical path for vulnerability ingestion lacks test coverage.
Let's check existing test coverage for similar functionality:
Would you like me to help create test cases for the vulnerability data loading functionality?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 38-38: cmd/ingest/osv/osv.go#L38
Added line #L38 was not covered by tests
pkg/tools/ingest/sbom_test.go (2)
48-48: LGTM: Proper error handling block closure
The addition of the closing brace properly structures the error handling block.
39-39: Verify consistent removal of storage parameter
The removal of the storage parameter from LoadDataFromPath aligns with the broader refactoring effort. However, let's ensure this change is consistently applied across the codebase.
✅ Verification successful
Storage parameter removal is consistently applied
Based on the comprehensive search results:
- The function signature
LoadDataFromPathconsistently accepts only the path parameter across its definition and all usage sites - All calls to the function across the codebase (in test files, commands, and core functionality) have been updated to the new signature
- No interfaces or types define this method, confirming it's a standalone function
- The implementation in
loader.goshows the function operates independently of storage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that LoadDataFromPath is consistently called without storage parameter
# and that there are no remaining references to the old signature
# Check for any remaining calls with storage parameter
ast-grep --pattern 'LoadDataFromPath($storage, $_)'
# Check the function definition to confirm signature change
ast-grep --pattern 'func LoadDataFromPath($_) $_'
# Look for any comments or documentation that might need updating
rg -l "LoadDataFromPath.*storage" --type md
Length of output: 3055
Script:
#!/bin/bash
# Let's search for all calls to LoadDataFromPath to ensure consistency
rg "LoadDataFromPath\(" -A 2 -B 2
# Also check for any potential interface definitions or related types
ast-grep --pattern 'type $_ interface {
$$$
LoadDataFromPath($$$)
$$$
}'
# Check for any test files that might be using this function
fd test.go --exec grep -l "LoadDataFromPath"
Length of output: 3748
cmd/query/custom/custom.go (4)
82-116: LGTM! Well-structured table formatting
The table formatting logic is well-organized with:
- Clear separation of concerns
- Proper handling of optional columns
- Good use of the tablewriter package
118-130: LGTM! Clean command setup
The command setup is well-structured with:
- Clear documentation
- Proper argument validation
- Clean initialization
67-79: 🛠️ Refactor suggestion
Add test coverage for error handling paths
The error handling for JSON formatting lacks test coverage.
#!/bin/bash
# Check for existing error handling tests
rg -l "TestRun.*Error" cmd/query/custom/Consider adding test cases for:
- Invalid JSON formatting scenarios
- Unknown output format handling
- Empty node list handling
Would you like me to help create these test cases?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-72: cmd/query/custom/custom.go#L71-L72
Added lines #L71 - L72 were not covered by tests
43-50:
Add test coverage and improve error handling
The client initialization logic lacks test coverage and error handling for invalid addresses.
Consider adding:
- Error handling for malformed addresses:
if o.queryServiceClient == nil {
+ if !strings.HasPrefix(o.addr, "http://") && !strings.HasPrefix(o.addr, "https://") {
+ return fmt.Errorf("invalid address format: %s. Must start with http:// or https://", o.addr)
+ }
o.queryServiceClient = apiv1connect.NewQueryServiceClient(
http.DefaultClient,
o.addr,- Test cases for:
- Invalid address format
- Client initialization with custom HTTP client
- Client initialization with default HTTP client
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-50: cmd/query/custom/custom.go#L45-L50
Added lines #L45 - L50 were not covered by tests
pkg/tools/ingest/vuln_test.go (1)
Line range hint 15-34: Verify consistent removal of storage parameter across test files
The removal of the storage parameter from LoadDataFromPath appears to be part of a larger refactoring effort. The test logic remains sound, maintaining proper validation of SBOM ingestion.
Let's verify the consistency of this change across related test files:
✅ Verification successful
The previous results show that LoadDataFromPath is being used consistently without the storage parameter across multiple test files, but let's verify the actual function definition and implementation to ensure complete consistency.
LoadDataFromPath parameter removal is consistent across the codebase
The function definition and all its usages across the codebase have been verified to be consistent. The LoadDataFromPath function:
- Has a single parameter
path string - Returns
([]Data, error) - Is used consistently without a storage parameter in all test files
- Implementation shows it's designed to only load data, with storage operations handled separately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of LoadDataFromPath with storage parameter
# and verify test coverage in related files
# Search for any remaining instances of LoadDataFromPath with storage parameter
rg -p "LoadDataFromPath\([^)]*storage[^)]*\)" --type go
# Find all test files that might be affected by this change
fd -e go -g "*_test.go" | while read -r file; do
echo "=== $file ==="
rg "LoadDataFromPath" "$file" -C 2
done
Length of output: 4788
Script:
#!/bin/bash
# Let's examine the LoadDataFromPath function definition and its usage pattern
# to verify the consistency of the parameter changes
# Check the function definition
ast-grep --pattern 'func LoadDataFromPath($_) $$$'
# Look for the implementation and any recent changes
rg -p "func LoadDataFromPath" --type go -C 5
# Check for any imports of the ingest package to see usage patterns
rg "\".*ingest\"" --type go
Length of output: 4047
pkg/storages/e2e_test.go (3)
23-29: LGTM! The refactored LoadDataFromPath call maintains correct functionality.
The removal of the redisStorage parameter from LoadDataFromPath simplifies the data loading process while maintaining proper error handling and data ingestion flow.
Line range hint 30-36: LGTM! Consistent refactoring applied to vulnerability data loading.
The changes maintain consistency with the SBOM data loading approach above, following the same pattern of simplified LoadDataFromPath usage.
Line range hint 23-36: Verify comprehensive refactoring across the codebase.
Let's ensure all callers of LoadDataFromPath have been updated consistently.
✅ Verification successful
Based on the search results, I can now generate the final response:
LoadDataFromPath refactoring appears consistent across the codebase
The verification shows that all calls to LoadDataFromPath have been updated consistently:
- No instances found where
storageparameter is being passed - All callers are using the new signature that only takes a
pathparameter - The function is used consistently across test files and implementation code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all callers of LoadDataFromPath have been updated to remove the storage parameter
# Search for any remaining calls to LoadDataFromPath that might still pass storage
rg -p "LoadDataFromPath\([^)]*storage"
# Find all current usages of LoadDataFromPath for manual verification
rg -p "LoadDataFromPath\(" -A 2
Length of output: 1845
api/v1/service_test.go (1)
76-76: LGTM! Verify test data path existence.
The removal of the storage parameter from LoadDataFromPath reduces coupling and improves the function's design.
✅ Verification successful
Test data path is valid and properly formatted
The test data file testdata/osv-sboms/google_agi.sbom.json exists and contains valid JSON content, ensuring reliable test execution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of test data file and its format
# Test: Check if the SBOM test file exists and is valid JSON
if [ -f "testdata/osv-sboms/google_agi.sbom.json" ]; then
jq empty "testdata/osv-sboms/google_agi.sbom.json" 2>/dev/null
if [ $? -eq 0 ]; then
echo "✓ Test data file exists and is valid JSON"
else
echo "✗ Test data file exists but is not valid JSON"
fi
else
echo "✗ Test data file not found"
fi
Length of output: 214
cmd/query/custom/custom_test.go (3)
117-170: Well-structured test implementation!
Good use of sub-tests and assertions to verify table formatting functionality. The test cases effectively cover both normal operation and output limits.
172-180: Clean and effective mock implementation!
The mock client implementation is minimal yet flexible enough for testing different scenarios.
181-315: Comprehensive test coverage!
Excellent test cases covering various scenarios including success cases, error handling, and edge cases. The table-driven approach is well implemented.
| cmd.AddCommand(query.New()) | ||
| cmd.AddCommand(ingest.New()) |
There was a problem hiding this comment.
Add tests for the modified command initialization
The static analysis indicates that the modified command initialization lines are not covered by tests. This is particularly important given the architectural changes in how these commands handle storage.
Would you like me to help create test cases for these modified command initializations? The tests should verify:
- Commands are properly initialized without storage
- Commands can still access required storage functionality
- Integration with the rest of the system works as expected
Also applies to: 37-37
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-35: cmd/root/root.go#L34-L35
Added lines #L34 - L35 were not covered by tests
| @@ -48,7 +46,7 @@ | |||
| } else { | |||
| switch filepath.Ext(path) { | |||
| case ".zip": | |||
| subResult, err := processZipFile(storage, path) | |||
| subResult, err := processZipFile(path) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for ZIP file processing
The ZIP file processing functionality lacks test coverage. This is critical functionality that should be thoroughly tested, including error cases such as:
- Invalid ZIP files
- Corrupted internal files
- Permission issues during extraction
Would you like me to help create comprehensive test cases for the ZIP file processing functionality? I can provide a test suite that covers both success and error scenarios.
Also applies to: 71-71, 103-103
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 49-49: pkg/tools/ingest/loader.go#L49
Added line #L49 was not covered by tests
| @@ -18,7 +16,7 @@ | |||
| Data []byte | |||
| } | |||
|
|
|||
| func LoadDataFromPath(storage graph.Storage, path string) ([]Data, error) { | |||
| func LoadDataFromPath(path string) ([]Data, error) { | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent storage parameter removal detected
The removal of the storage parameter from LoadDataFromPath appears inconsistent with other functions in the package. The following functions still use graph.Storage:
SBOM(storage graph.Storage, data []byte)Scorecards(storage graph.Storage, data []byte)Vulnerabilities(storage graph.Storage, data []byte)
This suggests an incomplete architectural change that could lead to inconsistent patterns within the same package.
🔗 Analysis chain
Verify consistent removal of storage dependency across the codebase
The removal of the storage parameter aligns with the broader refactoring effort. Let's verify this architectural change is consistently applied.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct storage dependencies in the ingest package
rg -l "graph\.Storage" "pkg/tools/ingest/"
# Search for any calls to the old function signature
ast-grep --pattern 'LoadDataFromPath($storage, $path)'
Length of output: 179
Script:
#!/bin/bash
# Check the actual usage of Storage in the identified files
rg "graph\.Storage" "pkg/tools/ingest/vuln.go" "pkg/tools/ingest/scorecard.go" "pkg/tools/ingest/sbom.go" -B 2 -A 2
# Check if these files have similar function signatures that were updated
ast-grep --pattern 'func $name($storage graph.Storage, $rest) $ret'
Length of output: 1068
| cmd := &cobra.Command{} | ||
| cmd.SetOut(new(bytes.Buffer)) // Capture output for assertions | ||
| outputBuf := &bytes.Buffer{} | ||
| cmd.SetOut(outputBuf) | ||
| cmd.SetContext(context.Background()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant buffer creation
The output buffer is set twice, and the first one is never used.
-cmd.SetOut(new(bytes.Buffer)) // Capture output for assertions
outputBuf := &bytes.Buffer{}
cmd.SetOut(outputBuf)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cmd := &cobra.Command{} | |
| cmd.SetOut(new(bytes.Buffer)) // Capture output for assertions | |
| outputBuf := &bytes.Buffer{} | |
| cmd.SetOut(outputBuf) | |
| cmd.SetContext(context.Background()) | |
| cmd := &cobra.Command{} | |
| outputBuf := &bytes.Buffer{} | |
| cmd.SetOut(outputBuf) | |
| cmd.SetContext(context.Background()) |
|
@coderabbitai generate unit testing code for missing coverage. |
|
Tip For best results, initiate chat on the files or code changes. I've identified several areas in the codebase where unit tests can be added to improve coverage. Below are unit tests for the // cmd/ingest/osv/osv_test.go
package osv
import (
"testing"
"github.com/spf13/cobra"
)
func TestNewCommand(t *testing.T) {
cmd := New()
if cmd == nil {
t.Fatal("Expected non-nil command")
}
if cmd.Use != "osv" {
t.Errorf("Expected command use to be 'osv', got '%s'", cmd.Use)
}
if cmd.Short == "" {
t.Error("Expected Short description to be set")
}
if cmd.RunE == nil {
t.Error("Expected RunE to be set")
}
}// cmd/ingest/sbom/sbom_test.go
package sbom
import (
"testing"
"github.com/spf13/cobra"
)
func TestNewCommand(t *testing.T) {
cmd := New()
if cmd == nil {
t.Fatal("Expected non-nil command")
}
if cmd.Use != "sbom" {
t.Errorf("Expected command use to be 'sbom', got '%s'", cmd.Use)
}
if cmd.Short == "" {
t.Error("Expected Short description to be set")
}
if cmd.RunE == nil {
t.Error("Expected RunE to be set")
}
}// cmd/ingest/scorecard/scorecard_test.go
package scorecard
import (
"testing"
"github.com/spf13/cobra"
)
func TestNewCommand(t *testing.T) {
cmd := New()
if cmd == nil {
t.Fatal("Expected non-nil command")
}
if cmd.Use != "scorecard" {
t.Errorf("Expected command use to be 'scorecard', got '%s'", cmd.Use)
}
if cmd.Short == "" {
t.Error("Expected Short description to be set")
}
if cmd.RunE == nil {
t.Error("Expected RunE to be set")
}
}Additionally, since the // pkg/tools/ingest/loader_test.go
package ingest
import (
"io/ioutil"
"os"
"path/filepath"
"testing"
)
func TestLoadDataFromPath_File(t *testing.T) {
// Create a temporary JSON file
data := []byte(`{"test": "data"}`)
tmpfile, err := ioutil.TempFile("", "test*.json")
if err != nil {
t.Fatal(err)
}
defer os.Remove(tmpfile.Name())
if _, err := tmpfile.Write(data); err != nil {
t.Fatal(err)
}
tmpfile.Close()
// Test LoadDataFromPath with the temporary file
result, err := LoadDataFromPath(tmpfile.Name())
if err != nil {
t.Errorf("Expected no error, got %v", err)
}
if len(result) != 1 {
t.Errorf("Expected 1 data item, got %d", len(result))
}
}
func TestLoadDataFromPath_Directory(t *testing.T) {
// Create a temporary directory
dir, err := ioutil.TempDir("", "testdir")
if err != nil {
t.Fatal(err)
}
defer os.RemoveAll(dir)
// Create a temporary JSON file in the directory
data := []byte(`{"test": "data"}`)
tmpfile := filepath.Join(dir, "test.json")
if err := ioutil.WriteFile(tmpfile, data, 0644); err != nil {
t.Fatal(err)
}
// Test LoadDataFromPath with the directory
result, err := LoadDataFromPath(dir)
if err != nil {
t.Errorf("Expected no error, got %v", err)
}
if len(result) != 1 {
t.Errorf("Expected 1 data item, got %d", len(result))
}
}
func TestProcessZipFile(t *testing.T) {
// TODO: Implement tests for processZipFile function
}These tests ensure that the key functions are properly validated and will help improve the overall code coverage. Please let me know if you'd like me to generate additional tests for other areas. |
Summary by CodeRabbit
Release Notes
New Features
--addrand--output.storageparameter.Bug Fixes
Documentation
Tests