feat: SBOM license filtering with refactoring for SBOM and PURL (TC-2832) #1995
+380
−102
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ces (TC-2832) Signed-off-by: mrizzi <mrizzi@redhat.com> Assisted-by: Claude Code
Reviewer's GuideThis PR centralizes the two-phase SPDX/CycloneDX license filtering logic into a new shared module and applies it to both the SBOM and PURL services, removing duplication and extending license-based filtering and its tests for SBOM. Sequence diagram for license filtering in SBOM and PURL servicessequenceDiagram
participant Client
participant Service
participant LicenseFiltering
participant Database
Client->>Service: Request with license filter
Service->>LicenseFiltering: apply_license_filtering(main_query, search_query, ...)
LicenseFiltering->>Database: Query SPDX licenses
LicenseFiltering->>Database: Query CycloneDX licenses
LicenseFiltering->>Service: Return filtered query
Service->>Database: Execute filtered query
Service->>Client: Return filtered results
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider adding integration tests for PurlService license filtering similar to the SBOM tests to ensure the new common apply_license_filtering logic works across both services.
- The special‐case translator in SbomService that maps the LICENSE field to an empty string feels hacky—consider handling license filtering more uniformly in apply_license_filtering or its helpers instead of requiring a fallback in the query translator.
- common/service.rs is becoming quite large; extracting the license‐filtering helpers into their own module could improve readability and make future maintenance easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding integration tests for PurlService license filtering similar to the SBOM tests to ensure the new common apply_license_filtering logic works across both services.
- The special‐case translator in SbomService that maps the LICENSE field to an empty string feels hacky—consider handling license filtering more uniformly in apply_license_filtering or its helpers instead of requiring a fallback in the query translator.
- common/service.rs is becoming quite large; extracting the license‐filtering helpers into their own module could improve readability and make future maintenance easier.
## Individual Comments
### Comment 1
<location> `modules/fundamental/src/common/service.rs:145-147` </location>
<code_context>
+ .map(|constraint| q(&format!("{constraint}")))
+ {
+ let license_filtering_base_query = base_query_fn();
+ let mut select_from_spdx =
+ build_spdx_license_query(license_query.clone(), license_filtering_base_query.clone())?;
+ let select_from_cyclonedx =
+ build_cyclonedx_license_query(license_query, license_filtering_base_query)?;
+
</code_context>
<issue_to_address>
**nitpick:** Avoid unnecessary cloning of license_filtering_base_query.
If cloning is necessary due to mutability, please add a clarifying comment. Otherwise, consider reusing the base query to avoid redundancy.
</issue_to_address>
### Comment 2
<location> `modules/fundamental/src/sbom/service/test.rs:255-264` </location>
<code_context>
+ assert_eq!(results.total, 2);
+ assert_eq!(results.items.len(), 2);
+
+ // Test 5: Negative test - license that doesn't exist
+ let results = service
+ .fetch_sboms(
+ q("license=NONEXISTENT_LICENSE"),
+ Paginated::default(),
+ (),
+ &ctx.db,
+ )
+ .await?;
+
+ log::debug!("Nonexistent license filter results: {results:#?}");
+ // Should return no SBOMs
+ assert_eq!(results.total, 0);
+ assert!(results.items.is_empty());
+
+ // Test 6: Empty license query
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for invalid license query syntax.
Consider adding a test case for invalid license query syntax to ensure the service handles malformed queries gracefully without crashing.
</issue_to_address>
### Comment 3
<location> `modules/fundamental/src/sbom/service/test.rs:299-318` </location>
<code_context>
+ "quay/quay-builder-qemu-rhcos-rhel8"
+ );
+
+ // Test 8: Pagination with license filtering
+ let results = service
+ .fetch_sboms(
+ q("license~GPL"),
+ Paginated {
+ offset: 0,
+ limit: 1,
+ },
+ (),
+ &ctx.db,
+ )
+ .await?;
+
+ log::debug!("Paginated license filter results: {results:#?}");
+ // Should return at most 1 item but show total count
+ // Both SBOMs contain GPL licenses, but limit to 1
+ assert_eq!(results.items.len(), 1);
+ assert_eq!(results.total, 2);
+
+ // Test 9: Verify that SBOMs without license filters still work
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for pagination with offset > 0.
Please include a test case with offset > 0 to ensure correct pagination behavior when skipping items with license filtering.
```suggestion
// Test 8: Pagination with license filtering
let results = service
.fetch_sboms(
q("license~GPL"),
Paginated {
offset: 0,
limit: 1,
},
(),
&ctx.db,
)
.await?;
log::debug!("Paginated license filter results: {results:#?}");
// Should return at most 1 item but show total count
// Both SBOMs contain GPL licenses, but limit to 1
assert_eq!(results.items.len(), 1);
assert_eq!(results.total, 2);
// Test 8b: Pagination with license filtering and offset > 0
let results_offset = service
.fetch_sboms(
q("license~GPL"),
Paginated {
offset: 1,
limit: 1,
},
(),
&ctx.db,
)
.await?;
log::debug!("Paginated license filter results with offset: {results_offset:#?}");
// Should return the second item (if order is deterministic), and total should still be 2
assert_eq!(results_offset.items.len(), 1);
assert_eq!(results_offset.total, 2);
// Optionally, check that the returned SBOM is different from the first one
// (Uncomment if order is deterministic and you want to check identity)
// assert_ne!(results.items[0].head.name, results_offset.items[0].head.name);
// Test 9: Verify that SBOMs without license filters still work
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1995 +/- ##
==========================================
+ Coverage 68.00% 68.06% +0.06%
==========================================
Files 355 356 +1
Lines 19807 19857 +50
Branches 19807 19857 +50
==========================================
+ Hits 13470 13516 +46
- Misses 5557 5560 +3
- Partials 780 781 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: mrizzi <mrizzi@redhat.com>
Signed-off-by: mrizzi <mrizzi@redhat.com>
mrizzi
force-pushed
the
sbom-filter-by-license
branch
from
1f30027 to
bcaaa32
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `modules/fundamental/src/sbom/service/sbom.rs:164-150` </location>
<code_context>
.translator(|f, op, v| match f.split_once(':') {
Some(("label", key)) => Some(format!("labels:{key}{op}{v}")),
- _ => None,
+ _ => match f {
+ // Add an empty condition (effectively TRUE) to the main SQL query
+ // since the real filtering by license happens in the license subqueries above
+ LICENSE => Some("".to_string()),
+ _ => None,
+ },
}),
</code_context>
<issue_to_address>
**nitpick:** Empty condition for license field may affect query clarity.
Consider returning None or adding a clarifying comment to make the handling of the LICENSE field more explicit and reduce potential confusion for future maintainers.
</issue_to_address>
### Comment 2
<location> `modules/fundamental/src/common/license_filtering.rs:120-40` </location>
<code_context>
+pub fn apply_license_filtering<E, BE, F, C>(
</code_context>
<issue_to_address>
**suggestion:** Generic license filtering function increases flexibility but adds complexity.
Document the expected types and usage patterns to assist future maintainers.
</issue_to_address>
### Comment 3
<location> `modules/fundamental/src/sbom/service/test.rs:270-268` </location>
<code_context>
+ assert_eq!(results.total, 0);
+ assert!(results.items.is_empty());
+
+ // Test 6: Empty license query
+ let results = service
+ .fetch_sboms(q("license="), Paginated::default(), (), &ctx.db)
+ .await?;
+
+ log::debug!("Empty license query results: {results:#?}");
+ // Should return no SBOMs or handle gracefully
+ assert_eq!(results.total, 0);
+ assert!(results.items.is_empty());
+
+ // Test 7: Combine license filter with other filters (should work together)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for a malformed or invalid license query.
Adding a test for malformed license queries, such as `license==GPL` or `license~`, will verify that the service responds appropriately and avoids unexpected behavior.
</issue_to_address>
### Comment 4
<location> `modules/fundamental/src/sbom/service/test.rs:322-331` </location>
<code_context>
+ );
+ assert_eq!(results.total, 2);
+
+ // Test 8b: Pagination with license filtering and offset > 0
+ let results_offset = service
+ .fetch_sboms(
+ q("license~GPL").sort("name:desc"),
+ Paginated {
+ offset: 1,
+ limit: 1,
+ },
+ (),
+ &ctx.db,
+ )
+ .await?;
+ log::debug!("Paginated license filter results with offset: {results_offset:#?}");
+ // Should return the second item and total should still be 2
+ assert_eq!(results_offset.items.len(), 1);
+ assert_eq!(results_offset.items[0].head.name, "MTV-2.6");
+ assert_eq!(results_offset.total, 2);
+
+ // Test 9: Verify that SBOMs without license filters still work
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for pagination with a limit of zero.
This will verify that the service returns no items but reports the correct total count when the limit is zero.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| query, | ||
| &search, | ||
| create_sbom_license_filtering_base_query, | ||
| sbom::Column::SbomId, |
There was a problem hiding this comment.
nitpick: Empty condition for license field may affect query clarity.
Consider returning None or adding a clarifying comment to make the handling of the LICENSE field more explicit and reduce potential confusion for future maintainers.
There was a problem hiding this comment.
can you better explain?
There was a problem hiding this comment.
Certainly! My nitpick refers to how the LICENSE field is handled in the query translator function (the closure passed to .translator() in your query construction). When a license filter is present, the actual filtering is performed by the subqueries created in apply_license_filtering, not by the main query itself.
Currently, when the LICENSE field is encountered in the translator, it returns an empty string (effectively a TRUE condition), which means the main query doesn't filter by license directly. This can be confusing for future maintainers because it looks like the LICENSE field is ignored or mishandled.
Why clarify?
- It helps future readers understand that license filtering is handled in a separate, dedicated subquery, not in the main query.
- It prevents misunderstandings or accidental changes that could break the intended filtering logic.
How to clarify?
- Add a comment in the translator closure explaining why LICENSE returns an empty condition.
- Alternatively, you could return
Nonefor LICENSE, but then ensure that the license filtering is always handled by the subquery logic and not missed.
Example comment:
translator(|f, op, v| match f {
// LICENSE filtering is handled by the license subquery in apply_license_filtering
LICENSE => Some("".to_string()), // No-op: main query does not filter by license directly
_ => None,
})This makes the intent clear and helps maintainers understand the separation of concerns in the filtering logic.
| }), | ||
| )? | ||
| .into_query()) | ||
| } |
There was a problem hiding this comment.
suggestion: Generic license filtering function increases flexibility but adds complexity.
Document the expected types and usage patterns to assist future maintainers.
|
/scale-test |
|
🛠️ Scale test has started! Follow the progress here: Workflow Run |
Goose ReportGoose Attack ReportPlan Overview
Request Metrics
Response Time Metrics
Status Code Metrics
Transaction Metrics
Scenario Metrics
Error Metrics
📄 Full Report (Go to "Artifacts" and download report) |
|
Successfully created backport PR for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements license filtering functionality for the SBOM service and refactors duplicate code patterns between SBOM and PURL services to improve maintainability.
Changes
modules/fundamental/src/moved to common modulecommon/license_filtering.rssome functions frompurl/service/mod.rsto be reused insbom/service/sbom.rsRelates to TC-2832
Assisted-by: Claude Code
Summary by Sourcery
Implement two-phase (SPDX and CycloneDX) license filtering for the SBOM service and refactor duplicate license filtering logic into a shared module for both SBOM and PURL services.
New Features:
Enhancements:
Tests:
Summary by Sourcery
Add license filtering support to the SBOM service and refactor duplicate SPDX/CycloneDX license filtering code into a shared module for both SBOM and PURL services
New Features:
Enhancements:
Tests: