[SP-3557] fix: sort results by ascending order #31
Conversation
WalkthroughThis PR changes scan result ordering to sort by the minimum Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as ScanServiceImpl
participant Deduper as deduplicateComponents
participant Sorter as sortByBestComponentOrder
participant Result as ScanResult[]
Note over Caller,Deduper: Existing deduplication step
Caller->>Deduper: deduplicateComponents(results)
Deduper-->>Result: deduplicated results (ordering unchanged)
rect rgb(223,242,191)
Note over Caller,Sorter: New post-processing sort
Caller->>Sorter: sortByBestComponentOrder(results)
Sorter-->>Result: results sorted by min(ComponentGroup.Order) ascending
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
CHANGELOG.md (1)
8-10: Consider recategorizing the changelog entry.The entry describes a fix ("Fix sorting order of results") but is categorized under "Added". According to Keep a Changelog conventions, this should be under "Fixed" or "Changed" since it modifies existing behavior rather than adding new functionality.
Apply this diff to recategorize:
## [0.7.1] - 2025-10-28 -### Added +### Fixed - Fix sorting order of results. Now sorting by `Order` field in ascending order, instead of `PathId`.internal/service/scan_service_impl.go (1)
229-243: Consider usingmath.MaxInt32for clarity.The expression
int32(^uint32(0) >> 1)correctly computes the maximum int32 value, but it's not immediately obvious. Using the standard library constantmath.MaxInt32would improve readability.Apply this diff:
+import ( + "math" +) + // sortByBestComponentOrder sorts the results by the lowest order of the component groups (lower order is better). func sortByBestComponentOrder(results []*entities.ScanResult) { sort.Slice(results, func(i, j int) bool { - minOrderI := int32(^uint32(0) >> 1) // Max int32 + minOrderI := int32(math.MaxInt32) for _, group := range results[i].ComponentGroups { minOrderI = min(minOrderI, group.Order) } - minOrderJ := int32(^uint32(0) >> 1) // Max int32 + minOrderJ := int32(math.MaxInt32) for _, group := range results[j].ComponentGroups { minOrderJ = min(minOrderJ, group.Order) } return minOrderI < minOrderJ }) }internal/service/scan_service_impl_test.go (1)
25-206: Add test case for empty ComponentGroups.Consider adding a test case where a
ScanResulthas an emptyComponentGroupsslice to explicitly verify the edge-case behavior (such results should sort last with max int32 as their minimum order).Add this test case:
{ name: "Result with empty ComponentGroups sorts last", input: []*entities.ScanResult{ { PathID: "/path/with-groups", ComponentGroups: []*entities.ComponentGroup{ {Order: 10}, }, }, { PathID: "/path/empty-groups", ComponentGroups: []*entities.ComponentGroup{}, }, { PathID: "/path/with-low-order", ComponentGroups: []*entities.ComponentGroup{ {Order: 1}, }, }, }, expected: []string{"/path/with-low-order", "/path/with-groups", "/path/empty-groups"}, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(2 hunks)internal/service/scan_service_impl.go(1 hunks)internal/service/scan_service_impl_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/service/scan_service_impl.go (1)
internal/domain/entities/scan_response.go (1)
ScanResult(26-31)
internal/service/scan_service_impl_test.go (3)
internal/domain/entities/scan_response.go (1)
ScanResult(26-31)internal/domain/entities/component.go (2)
ComponentGroup(21-28)Version(31-34)internal/service/scan_service_impl.go (1)
ScanServiceImpl(31-33)
🔇 Additional comments (5)
internal/service/scan_service_impl_test.go (4)
25-206: Excellent test coverage for sorting logic!The test suite comprehensively covers empty slices, single/multiple results, ascending/descending order, minimum order selection across component groups, and complex scenarios. The approach of copying input to avoid mutation (line 186-188) is a good practice.
208-288: LGTM! Comprehensive threshold testing.The test cases thoroughly cover empty groups, boundary conditions (below, at, and above threshold), and multiple group scenarios.
290-401: LGTM! Thorough component group processing tests.The tests validate filtering by
minAcceptedScore, correct path propagation, and accurate result/version counts across various scenarios including empty groups, threshold filtering, and mixed results.
403-539: LGTM! Excellent deduplication test coverage.The test suite covers empty results, unique components, duplicate handling with score-based selection, and multi-path deduplication. The comment at lines 516-518 clearly explains the expected behavior, which aids understanding.
internal/service/scan_service_impl.go (1)
229-243: Remove this review comment.The code cannot encounter the scenario described. The
deduplicateComponentsfunction (lines 216-221) includes a guard conditionif len(components) > 0that ensures onlyScanResultobjects with non-emptyComponentGroupsare added todeduplicatedResults. ThesortByBestComponentOrderfunction is then called exclusively ondeduplicatedResults(line 224), guaranteeing it never receives aScanResultwith emptyComponentGroups. The concern is based on a false premise that this scenario is possible.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(2 hunks)internal/service/scan_service_impl.go(2 hunks)internal/service/scan_service_impl_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/service/scan_service_impl.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/service/scan_service_impl_test.go (3)
internal/domain/entities/scan_response.go (1)
ScanResult(26-31)internal/domain/entities/component.go (2)
ComponentGroup(21-28)Version(31-34)internal/service/scan_service_impl.go (1)
ScanServiceImpl(32-34)
🪛 GitHub Actions: Go Unit Test
internal/service/scan_service_impl_test.go
[error] 549-549: go test: internal/service/scan_service_impl_test.go:549:5: (*testing.common).Errorf format %v reads arg #3, but call has 2 args
🪛 GitHub Actions: Golang CI Lint
internal/service/scan_service_impl_test.go
[error] 549-549: golangci-lint: printf: (*testing.common).Errorf format %v reads arg #3, but call has 2 args (govet)
🪛 GitHub Check: build
internal/service/scan_service_impl_test.go
[failure] 549-549:
printf: (*testing.common).Errorf format %v reads arg #3, but call has 2 args (govet)
🔇 Additional comments (5)
CHANGELOG.md (1)
8-11: LGTM! Changelog entry follows project conventions.The version 0.7.1 entry clearly documents the sorting behavior change and follows the Keep a Changelog format. The reference link is properly added.
Also applies to: 53-53
internal/service/scan_service_impl_test.go (4)
25-228: Comprehensive test coverage for sorting behavior.The test cases effectively validate sorting by minimum component order across various scenarios including edge cases (empty slices, empty component groups) and complex multi-group situations. The test structure is clear and maintainable.
Note: The test case at lines 153-169 regarding equal minimum orders has a previous review comment about sort stability that you may want to address.
230-310: Good test coverage for threshold validation.The test cases appropriately cover edge cases and boundary conditions, including the important equality case at line 272-282. The multi-group scenario ensures the function correctly checks across all component groups.
312-423: Solid coverage of component group processing.The test cases validate both filtering behavior and data propagation (PathID, version counts). The multi-group mixed scenario at lines 378-395 is particularly valuable for ensuring correct handling of partially qualifying groups.
425-448: Well-structured deduplication tests.The test cases effectively validate the deduplication logic, including the important scenario of selecting higher-scoring components when duplicates are found. The inline comment at lines 538-540 helpfully explains the expected behavior after deduplication and sorting.
Also applies to: 452-561
matiasdaloia
deleted the
bugfix/mdaloia/SP-3557-SCANOSS-HFH-Sort-response-components-by-ascending-order
branch
Summary by CodeRabbit
Improvements
Documentation
Tests