Fixed unit32 conversion #149
Conversation
WalkthroughThe changes in this pull request introduce a new function Changes
Possibly related PRs
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
🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (12)
pkg/utils/convert.go (1)
8-11: Enhance function documentation with examplesWhile the function is well-implemented, consider adding an example in the documentation to demonstrate usage and expected output.
-// Uint32ToStr converts uint32 to string +// Uint32ToStr converts uint32 to string +// +// Example: +// +// Uint32ToStr(123) // returns "123"cmd/helpers/common_test.go (1)
Line range hint
7-71: Consider adding more edge cases to the test suiteThe current test suite is well-structured with good coverage of basic scenarios and edge cases. Consider adding these additional test cases to make it even more robust:
- Unicode characters and emojis
- Very large maxLength values
- Negative maxLength values (if applicable)
Here's a suggested addition to the test cases:
func TestTruncatePath(t *testing.T) { tests := []struct { name string id string maxLength int want string }{ // ... existing test cases ... + { + name: "Path with unicode characters", + id: "path/with/🚀/emoji/⭐/chars/⚡", + maxLength: 15, + want: "...emoji/⭐/chars/⚡", + }, + { + name: "Very large maxLength", + id: "normal/path", + maxLength: 1000, + want: "normal/path", + }, + { + name: "Negative maxLength", + id: "test/path", + maxLength: -1, + want: "", // or handle according to your requirements + }, }cmd/ingest/scorecard/scorecard.go (1)
51-51: LGTM! Consider making the truncation length configurable.The usage of
helpers.TruncateStringis appropriate here. However, since the truncation length of 50 is used in multiple places, consider making it a named constant for better maintainability.const ( DefaultAddr = "http://localhost:8089" // Default address of the minefield server + MaxDisplayLength = 50 // Maximum length for truncated strings in display )cmd/ingest/osv/osv.go (1)
69-69: LGTM, consider making truncation length a constantThe change to use
helpers.TruncateStringis correct. Since the truncation length of 50 is used in multiple places, consider defining it as a package-level constant for better maintainability.const ( DefaultAddr = "http://localhost:8089" // Default address of the minefield server + MaxPathLength = 50 // Maximum length for truncated paths in progress messages )pkg/utils/convert_test.go (2)
7-26: Consider adding more edge cases to TestUint32ToStrWhile the current test cases cover basic scenarios, consider adding more edge cases such as:
- Powers of 10 (100, 1000, etc.)
- Values close to max uint32 (4294967294)
tests := []struct { name string input uint32 expected string }{ {"zero value", 0, "0"}, {"small number", 42, "42"}, {"max uint32", 4294967295, "4294967295"}, + {"power of ten", 1000, "1000"}, + {"near max", 4294967294, "4294967294"}, }
1-6: Add package documentation for the test fileConsider adding a package comment to document the purpose of these conversion functions and their test coverage.
package utils + +// This file contains tests for uint32 conversion utilities that handle +// conversions between uint32, string, and int types with proper +// bounds checking and error handling. import ( "testing"pkg/graph/cache.go (4)
51-65: Enhance error messages with more contextWhile the conversion logic is correct, the error messages could be more specific about the operation being performed.
Consider updating the error messages to provide more context:
-return fmt.Errorf("error converting child key %s: %w", childId, err) +return fmt.Errorf("error converting child key %s to uint32: %w", childId, err) -return fmt.Errorf("error getting value for key %s: %w", childId, err) +return fmt.Errorf("error retrieving cached parent value for key %s: %w", childId, err)
Line range hint
262-283: Consider extracting repeated bitmap retrieval logicThe function contains repeated patterns for retrieving and error handling bitmap operations.
Consider extracting the common pattern into a helper function:
+func getBitmapForKey(cache *NativeKeyManagement, key string) (roaring.Bitmap, error) { + bitmap, err := cache.Get(key) + if err != nil { + return roaring.Bitmap{}, fmt.Errorf("error retrieving bitmap for key %s: %w", key, err) + } + return bitmap, nil +} func getTodoAndFutureNodes(children, parents *NativeKeyManagement, curNode *Node, direction Direction) ([]uint32, []uint32, error) { var todoNodes, futureNodes []uint32 nodeIDStr := utils.Uint32ToStr(curNode.ID) if direction == ChildrenDirection { - todoNodesBitmap, err := children.Get(nodeIDStr) + todoNodesBitmap, err := getBitmapForKey(children, nodeIDStr) if err != nil { return nil, nil, err } - futureNodesBitmap, err := parents.Get(nodeIDStr) + futureNodesBitmap, err := getBitmapForKey(parents, nodeIDStr) if err != nil { return nil, nil, err } todoNodes, futureNodes = todoNodesBitmap.ToArray(), futureNodesBitmap.ToArray() } else { // Similar changes for the else block }
290-306: Remove unnecessary empty line and improve error messagesThe function has correct type conversions but could benefit from some cleanup.
Consider these improvements:
func addToCache(bm *NativeKeyManagement, curElem, todoElem uint32) error { curElemStr := utils.Uint32ToStr(curElem) todoElemStr := utils.Uint32ToStr(todoElem) - curElemVal, err := bm.Get(curElemStr) if err != nil { - return fmt.Errorf("error getting value for curElem key from value %d: %w", curElem, err) + return fmt.Errorf("error retrieving bitmap for current element %d: %w", curElem, err) } todoVal, err := bm.Get(todoElemStr) if err != nil { - return fmt.Errorf("error getting value for todoElem key %d: %w", todoElem, err) + return fmt.Errorf("error retrieving bitmap for todo element %d: %w", todoElem, err) }
53-53: Standardize error message patternsWhile error handling is generally good, the error messages could follow a more consistent pattern.
Consider standardizing error messages to follow this pattern:
- Start with the operation being performed
- Include relevant identifiers
- Wrap the underlying error
Example:
-return fmt.Errorf("error converting child key %s: %w", childId, err) +return fmt.Errorf("failed to convert node ID %s to uint32: %w", childId, err) -return fmt.Errorf("error getting value for key %s: %w", childId, err) +return fmt.Errorf("failed to retrieve cached value for node ID %s: %w", childId, err)Also applies to: 61-61, 295-295, 300-300
pkg/storages/redis_storage.go (2)
44-45: Good improvement, consider applying consistentlyThe change to use
utils.Uint32ToStrimproves type safety and eliminates unnecessary type conversions. However, similar string conversions exist in other methods (e.g.,GetAllKeys,ToBeCached) that could benefit from the same treatment.Consider updating these methods to use the new utility function:
// In GetAllKeys method: -id, err := strconv.ParseUint(strings.TrimPrefix(key, NodeKeyPrefix), 10, 32) +id, err := utils.StrToUint32(strings.TrimPrefix(key, NodeKeyPrefix)) // In ToBeCached method: -id, err := strconv.ParseUint(item, 10, 32) +id, err := utils.StrToUint32(item)
Line range hint
1-1: Consider comprehensive type safety improvementsWhile the current changes improve type safety for some operations, consider a more comprehensive approach:
- Create constants for Redis key prefixes to avoid string literals
- Implement consistent type conversion across all ID handling methods
- Consider adding validation for uint32 overflow in ID generation
This would further improve maintainability and type safety across the entire storage implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
cmd/helpers/common.go(1 hunks)cmd/helpers/common_test.go(1 hunks)cmd/helpers/getAdditionalInfo.go(1 hunks)cmd/ingest/osv/osv.go(2 hunks)cmd/ingest/sbom/sbom.go(1 hunks)cmd/ingest/scorecard/scorecard.go(2 hunks)pkg/graph/cache.go(5 hunks)pkg/storages/redis_storage.go(3 hunks)pkg/tools/common.go(0 hunks)pkg/tools/ingest/vuln.go(1 hunks)pkg/utils/convert.go(1 hunks)pkg/utils/convert_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/tools/common.go
✅ Files skipped from review due to trivial changes (1)
- pkg/tools/ingest/vuln.go
🔇 Additional comments (13)
cmd/helpers/common.go (1)
3-3: Verify all usages have been updated
Let's verify that all references to the old tools.TruncateString have been updated and check for potentially problematic usage patterns.
✅ Verification successful
Let me try a different search pattern to find all usages of TruncateString, regardless of the package prefix.
Let me try a more specific search to find all usages of TruncateString and check the imports:
Based on the search results, I can now provide a final response:
All TruncateString usages have been properly updated
All calls to TruncateString are using the correct helpers.TruncateString import and implementation. The function is used consistently across the codebase with a maxLength parameter of 50, which is a safe value. No instances of the old tools.TruncateString remain.
- Function is properly defined in
cmd/helpers/common.go - All imports correctly reference
github.com/bitbomdev/minefield/cmd/helpers - Used safely in osv, sbom, and scorecard packages for path truncation
- Test coverage exists in
cmd/helpers/common_test.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to tools.TruncateString
# and verify the usage patterns of helpers.TruncateString
# Check for any remaining references to tools.TruncateString
echo "Checking for remaining references to tools.TruncateString:"
rg "tools\.TruncateString"
# Find all calls to helpers.TruncateString to review usage patterns
echo -e "\nChecking usage patterns of helpers.TruncateString:"
ast-grep --pattern 'TruncateString($str, $length)'
Length of output: 325
Script:
#!/bin/bash
# Search for any TruncateString function calls with context
echo "Searching for TruncateString function calls:"
rg "TruncateString\(" -A 1
# Also check for import statements related to this
echo -e "\nChecking related imports:"
rg "import.*\".*tools\""
rg "import.*\".*helpers\""
Length of output: 132637
Script:
#!/bin/bash
# Search for import statements related to TruncateString
echo "Checking imports:"
rg "\".*/(tools|helpers)\"" --type go
# Search for all TruncateString usages with more context
echo -e "\nChecking TruncateString usages:"
rg "TruncateString" -B 2 -A 2 --type go
Length of output: 3557
pkg/utils/convert.go (3)
1-7: LGTM: Clean package structure and imports
The package structure follows Go conventions with appropriate imports.
13-20: LGTM: Robust string to uint32 conversion
The function implements proper error handling and safe conversion using appropriate string parsing options.
1-28: Verify conversion function usage
Let's verify the usage of these new conversion functions across the codebase to ensure consistent type handling.
✅ Verification successful
All conversion functions are properly used across the codebase
The verification shows that:
- All instances of direct
strconvconversions in Redis storage have been replaced with the new utility functions - The graph cache is using the new utility functions consistently
- The remaining direct
strconvusages incmd/packages are appropriate as they handle uint64 values, not uint32 - No instances of unsafe type conversions were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct uint32/string conversions that should use these new functions
rg --type go 'strconv\.(FormatUint|ParseUint|Atoi)' --glob '!pkg/utils/convert*'
# Search for places where these new functions are used
rg --type go 'utils\.(Uint32ToStr|StrToUint32|IntToUint32)'
Length of output: 1779
cmd/helpers/common_test.go (1)
Line range hint 1-71: Verify complete migration from tools package
Let's ensure all references to tools.TruncateString have been updated to use helpers.TruncateString.
✅ Verification successful
Let me search for any references to the function in other files to ensure we have a complete picture.
Let me search for any references to the old tools package to ensure we have a complete picture.
Based on the search results, I can now provide a final response:
Migration to helpers package is complete and correct
The function has been properly migrated from the tools package to the helpers package. All references to TruncateString are now using the helpers package:
cmd/ingest/sbom/sbom.go:helpers.TruncateStringcmd/ingest/osv/osv.go:helpers.TruncateStringcmd/ingest/scorecard/scorecard.go:helpers.TruncateString
No references to tools.TruncateString remain in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to tools.TruncateString
# Search for any remaining references to tools.TruncateString
echo "Checking for remaining references to tools.TruncateString:"
rg "tools\.TruncateString"
# Search for import statements of the old tools package
echo -e "\nChecking for remaining imports of the tools package:"
rg "\".*tools\""
Length of output: 124316
Script:
#!/bin/bash
# Search for TruncateString function usage across the codebase
echo "Searching for TruncateString function usage:"
rg "TruncateString\(" -A 2
# Search for files importing the helpers package
echo -e "\nSearching for helpers package imports:"
rg "\".*helpers\""
Length of output: 153609
Script:
#!/bin/bash
# Search for any files importing the tools package
echo "Searching for tools package imports:"
rg "\".*tools\""
# Search for any references to tools package in Go files
echo -e "\nSearching for tools package references in Go files:"
rg "tools\." --type go
Length of output: 102660
cmd/helpers/getAdditionalInfo.go (2)
6-8: LGTM! Import organization looks good.
The import statement organization is clean and follows standard Go conventions.
Line range hint 1-65: Verify package dependencies after tools package removal.
Since the AI summary mentions that the tools package has been removed, let's verify that the remaining imports are still valid and required.
cmd/ingest/sbom/sbom.go (1)
52-52: LGTM! Verify helper function behavior.
The change from tools.TruncateString to helpers.TruncateString looks correct. Let's verify the new implementation maintains the same behavior.
✅ Verification successful
Implementation verified and behavior is correct
The new helpers.TruncateString implementation in cmd/helpers/common.go follows the same logic for string truncation:
- Returns the original string if its length is within the max length
- For longer strings, adds "..." prefix and keeps the last (maxLength-3) characters
The function maintains the same behavior as before and is properly placed in the helpers package. The change is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of helpers.TruncateString matches the expected behavior
# Check the implementation of the new helper function
ast-grep --pattern 'func TruncateString($str, $maxLen) {
$$$
}'
# Look for any test cases
rg -l "TestTruncateString"
Length of output: 94
Script:
#!/bin/bash
# Let's try a broader search for the TruncateString function
rg -p "func.*TruncateString"
# Also search for the file containing the function
fd -t f "helpers.go"
Length of output: 159
Script:
#!/bin/bash
# Let's examine both implementations to compare them
echo "=== New Implementation in cmd/helpers/common.go ==="
rg -A 10 "func TruncateString" cmd/helpers/common.go
echo -e "\n=== Previous Implementation in pkg/tools/ingest/helpers.go ==="
rg -A 10 "func TruncateString" pkg/tools/ingest/helpers.go
Length of output: 514
cmd/ingest/scorecard/scorecard.go (1)
51-51: Verify the implementation of helpers.TruncateString.
Let's verify that the new helpers.TruncateString function is properly implemented and consistently used across the codebase.
Also applies to: 73-73
✅ Verification successful
Implementation of helpers.TruncateString is correct and consistently used
The helper function is properly implemented in cmd/helpers/common.go with correct string truncation logic, and it's being used consistently across the codebase for truncating paths in progress messages. No instances of the old tools.TruncateString remain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of TruncateString
# Check the implementation
echo "Checking TruncateString implementation:"
ast-grep --pattern 'func TruncateString($str, $maxLen) {
$$$
}'
# Check for any remaining references to tools.TruncateString
echo -e "\nChecking for remaining tools.TruncateString references:"
rg "tools\.TruncateString"
# Check for consistent usage of the new helper
echo -e "\nChecking helpers.TruncateString usage:"
rg "helpers\.TruncateString"
Length of output: 1279
Script:
#!/bin/bash
# Let's check the actual implementation of TruncateString in the helpers package
# First, find the file containing TruncateString
echo "Finding TruncateString implementation file:"
fd "helpers" --type f --exec rg -l "TruncateString"
# Then get the actual implementation
echo -e "\nChecking the implementation:"
rg -A 10 "func TruncateString"
Length of output: 529
cmd/ingest/osv/osv.go (1)
Line range hint 1-70: PR objective mismatch
The changes in this file involve migrating from tools.TruncateString to helpers.TruncateString, which appears unrelated to the PR's stated objective of fixing uint32 conversion. Please clarify if there are additional changes pending or if the PR title should be updated to reflect these changes.
pkg/utils/convert_test.go (1)
28-57: Well-structured and comprehensive test cases!
The test cases effectively cover:
- Valid conversions
- Error handling for invalid inputs
- Boundary conditions (overflow)
- Proper error validation
pkg/graph/cache.go (1)
8-8: LGTM: Import addition is appropriate
The new import aligns with the changes to use utility functions for type-safe conversions.
pkg/storages/redis_storage.go (1)
10-10: LGTM: Import statement follows Go conventions
The utils package import is properly placed and grouped with other internal imports.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
cmd/helpers/common.go(1 hunks)cmd/ingest/scorecard/scorecard.go(2 hunks)pkg/utils/convert.go(1 hunks)pkg/utils/convert_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/helpers/common.go
- cmd/ingest/scorecard/scorecard.go
- pkg/utils/convert_test.go
🧰 Additional context used
🪛 GitHub Check: build
pkg/utils/convert.go
[failure] 32-32:
syntax error: non-declaration statement outside function body
[failure] 36-36:
syntax error: imports must appear before other declarations
🔇 Additional comments (3)
pkg/utils/convert.go (3)
8-11: LGTM!
The implementation is correct and efficient. The function properly handles the conversion from uint32 to string using appropriate type conversion and base 10 formatting.
13-20: LGTM!
The implementation is robust with proper error handling and type safety checks. The use of ParseUint with bitSize 32 ensures values stay within uint32 range.
22-31: LGTM! Safety checks properly implemented
The function implementation includes all necessary safety checks:
- Negative value validation
- Overflow protection using math.MaxUint32
- Clear error messages for both cases
This addresses the previous review comment about int overflow protection.
Signed-off-by: neilnaveen <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: naveensrinivasan <[email protected]>
naveensrinivasan
force-pushed
the
neil/fixIntConversion
branch
from
e131e07 to
712a71c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cmd/helpers/common_test.go (1)
Line range hint
7-60: Fix inconsistent function names in test.The test contains naming inconsistencies:
- The test function is named
TestTruncatePathbut it testsTruncateString- The error message uses
truncatePath()but should useTruncateString()Apply this diff to fix the inconsistencies:
-func TestTruncatePath(t *testing.T) { +func TestTruncateString(t *testing.T) { // ... test cases ... for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { if got := TruncateString(tt.id, tt.maxLength); got != tt.want { - t.Errorf("truncatePath() = %v, want %v", got, tt.want) + t.Errorf("TruncateString() = %v, want %v", got, tt.want) } }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
cmd/helpers/common.go(1 hunks)cmd/helpers/common_test.go(1 hunks)cmd/helpers/getAdditionalInfo.go(1 hunks)cmd/ingest/osv/osv.go(2 hunks)cmd/ingest/sbom/sbom.go(1 hunks)cmd/ingest/scorecard/scorecard.go(2 hunks)pkg/graph/cache.go(6 hunks)pkg/storages/redis_storage.go(4 hunks)pkg/tools/common.go(0 hunks)pkg/tools/ingest/vuln.go(1 hunks)pkg/utils/convert.go(1 hunks)pkg/utils/convert_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/tools/common.go
🚧 Files skipped from review as they are similar to previous changes (10)
- cmd/helpers/common.go
- cmd/helpers/getAdditionalInfo.go
- cmd/ingest/osv/osv.go
- cmd/ingest/sbom/sbom.go
- cmd/ingest/scorecard/scorecard.go
- pkg/graph/cache.go
- pkg/storages/redis_storage.go
- pkg/tools/ingest/vuln.go
- pkg/utils/convert.go
- pkg/utils/convert_test.go
🔇 Additional comments (2)
cmd/helpers/common_test.go (2)
1-5: LGTM! Package change and imports look good.
The package rename from tools to helpers is consistent with the codebase refactoring, and the imports are appropriately minimal.
Line range hint 1-60: Verify TruncateString usage across the codebase.
Let's verify that all references to the old tools.TruncateString have been updated to use helpers.TruncateString.
✅ Verification successful
All references to TruncateString have been properly updated
The verification shows that:
- No instances of
tools.TruncateStringremain in the codebase - All usages in the ingest commands (
osv,sbom,scorecard) have been properly updated to usehelpers.TruncateString
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to tools.TruncateString and verify helpers.TruncateString usage
# Check for any remaining references to tools.TruncateString
echo "Checking for remaining tools.TruncateString references..."
rg "tools\.TruncateString"
# Verify the new helpers.TruncateString usage
echo "Verifying helpers.TruncateString usage..."
rg "helpers\.TruncateString" "cmd/ingest/"
Length of output: 1137
Summary by CodeRabbit
Release Notes
New Features
TruncateStringfunction for better string handling, allowing for truncation with ellipsis.Uint32ToStr,StrToUint32, andIntToUint32.Bug Fixes
Refactor
toolspackage to the newhelperspackage for theTruncateStringfunction across various components.SanitizeFilenamefunction to simplify the codebase.Tests