feat: license filter consistent for SBOM packages tab #2011
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
Reviewer's GuideImplements consistent license filtering for SBOM packages by adopting a SeaORM‐based approach with unified SPDX expression expansion, deprecating legacy mapping fields, and updating tests and the OpenAPI spec for expanded license expressions. Entity relationship diagram for deprecated LicenseRefMapping and expanded license filteringerDiagram
SBOM_PACKAGE ||--o{ SBOM_PACKAGE_LICENSE : has
SBOM_PACKAGE_LICENSE }o--|| LICENSE : references
SBOM_PACKAGE_LICENSE }o--|| LICENSING_INFOS : references
SBOM_PACKAGE_PURL_REF }o--|| SBOM_PACKAGE : references
SBOM_PACKAGE_PURL_REF }o--|| LICENSE : references
PURL_DETAILS {
licenses LicenseInfo
licenses_ref_mapping LicenseRefMapping [deprecated]
}
LICENSE {
text string
spdx_licenses string[]
}
LICENSING_INFOS {
name string
license_id string
sbom_id uuid
}
File-Level Changes
Possibly linked issues
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:
- Review the decision to deprecate
licenses_ref_mappingand always return an empty array—ensure no downstream clients still depend on that mapping or plan a migration path before removing it entirely. - The added
#![recursion_limit = "256"]may hide overly deep JSON structures; consider refactoring the parser or expected‐result definitions to avoid increasing the recursion limit. - The SBOM and package license filtering functions share a lot of query building logic—consider extracting common parts to reduce duplication and make maintenance easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Review the decision to deprecate `licenses_ref_mapping` and always return an empty array—ensure no downstream clients still depend on that mapping or plan a migration path before removing it entirely.
- The added `#![recursion_limit = "256"]` may hide overly deep JSON structures; consider refactoring the parser or expected‐result definitions to avoid increasing the recursion limit.
- The SBOM and package license filtering functions share a lot of query building logic—consider extracting common parts to reduce duplication and make maintenance easier.
## Individual Comments
### Comment 1
<location> `modules/fundamental/src/sbom/service/test.rs:383-386` </location>
<code_context>
+ log::debug!("All packages count: {}", all_packages.total);
+ assert_eq!(all_packages.total, 5388, "Should have packages in the SBOM");
+
+ // Test 2: Filter by specific license that exists
+ let license_filtered = service
+ .fetch_sbom_packages(
+ sbom_id,
+ q("license=GPLv2 AND GPLv2+ AND CC-BY"),
+ Paginated::default(),
+ &ctx.db,
+ )
+ .await?;
+
+ log::debug!("License filtered packages: {license_filtered:#?}");
+ // Should find packages with this specific license
+ // This validates that the license filtering is applied correctly
+ assert_eq!(license_filtered.total, 14);
+
+ // Test 3: Filter by partial license match
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding assertions to check the actual package contents for license filtering.
Adding assertions for the license field values in the filtered packages will verify that the filter returns the correct data, not just the expected count.
```suggestion
log::debug!("License filtered packages: {license_filtered:#?}");
// Should find packages with this specific license
// This validates that the license filtering is applied correctly
assert_eq!(license_filtered.total, 14);
// Assert that all returned packages have the expected license values
let expected_licenses = ["GPLv2", "GPLv2+", "CC-BY"];
for pkg in &license_filtered.packages {
assert!(
pkg.license.is_some(),
"Package {:?} should have a license field",
pkg
);
let license = pkg.license.as_ref().unwrap();
assert!(
expected_licenses.contains(&license.as_str()),
"Package {:?} has unexpected license: {}",
pkg,
license
);
}
```
</issue_to_address>
### Comment 2
<location> `modules/fundamental/src/sbom/service/test.rs:431-451` </location>
<code_context>
+ // Should apply both license and name filters
+ assert_eq!(combined_filter.total, 11);
+
+ // Test 6: Pagination with license filtering
+ if partial_license_filtered.total > 1 {
+ let paginated = service
+ .fetch_sbom_packages(
+ sbom_id,
+ q("license~GPL"),
+ Paginated {
+ offset: 0,
+ limit: 1,
+ },
+ &ctx.db,
+ )
+ .await?;
+
+ log::debug!("Paginated license filtered packages: {paginated:#?}");
+ assert_eq!(paginated.items.len(), 1, "Should respect pagination limit");
+ assert_eq!(
+ paginated.total, partial_license_filtered.total,
+ "Total should match full query"
+ );
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Pagination logic is tested, but consider adding a test for offset > 0.
Adding a test with offset > 0 will help verify that items are correctly skipped during pagination.
```suggestion
// Test 6: Pagination with license filtering
if partial_license_filtered.total > 1 {
// Fetch first item (offset 0)
let paginated_first = service
.fetch_sbom_packages(
sbom_id,
q("license~GPL"),
Paginated {
offset: 0,
limit: 1,
},
&ctx.db,
)
.await?;
log::debug!("Paginated license filtered packages (offset 0): {paginated_first:#?}");
assert_eq!(paginated_first.items.len(), 1, "Should respect pagination limit");
assert_eq!(
paginated_first.total, partial_license_filtered.total,
"Total should match full query"
);
// Fetch second item (offset 1)
let paginated_second = service
.fetch_sbom_packages(
sbom_id,
q("license~GPL"),
Paginated {
offset: 1,
limit: 1,
},
&ctx.db,
)
.await?;
log::debug!("Paginated license filtered packages (offset 1): {paginated_second:#?}");
assert_eq!(paginated_second.items.len(), 1, "Should respect pagination limit for offset > 0");
assert_eq!(
paginated_second.total, partial_license_filtered.total,
"Total should match full query for offset > 0"
);
assert_ne!(
paginated_first.items[0], paginated_second.items[0],
"Items at offset 0 and offset 1 should be different"
);
}
```
</issue_to_address>
### Comment 3
<location> `modules/fundamental/src/sbom/endpoints/test.rs:355` </location>
<code_context>
+ "license_id": "Zlib AND Sendmail AND LGPLv2+"
}
]);
+ log::debug!("{:#}", json!(response));
assert!(expected_result.contains_subset(response.clone()));
- log::debug!("{response:#?}");
</code_context>
<issue_to_address>
**suggestion (testing):** Test expectations updated for expanded license expressions.
Consider adding assertions to ensure the response does not include unexpected licenses or that its length matches expectations, to better detect over-inclusion errors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+383
to
+386
| log::debug!("License filtered packages: {license_filtered:#?}"); | ||
| // Should find packages with this specific license | ||
| // This validates that the license filtering is applied correctly | ||
| assert_eq!(license_filtered.total, 14); |
There was a problem hiding this comment.
suggestion (testing): Consider adding assertions to check the actual package contents for license filtering.
Adding assertions for the license field values in the filtered packages will verify that the filter returns the correct data, not just the expected count.
Suggested change
| log::debug!("License filtered packages: {license_filtered:#?}"); | |
| // Should find packages with this specific license | |
| // This validates that the license filtering is applied correctly | |
| assert_eq!(license_filtered.total, 14); | |
| log::debug!("License filtered packages: {license_filtered:#?}"); | |
| // Should find packages with this specific license | |
| // This validates that the license filtering is applied correctly | |
| assert_eq!(license_filtered.total, 14); | |
| // Assert that all returned packages have the expected license values | |
| let expected_licenses = ["GPLv2", "GPLv2+", "CC-BY"]; | |
| for pkg in &license_filtered.packages { | |
| assert!( | |
| pkg.license.is_some(), | |
| "Package {:?} should have a license field", | |
| pkg | |
| ); | |
| let license = pkg.license.as_ref().unwrap(); | |
| assert!( | |
| expected_licenses.contains(&license.as_str()), | |
| "Package {:?} has unexpected license: {}", | |
| pkg, | |
| license | |
| ); | |
| } |
Comment on lines
+431
to
+451
| // Test 6: Pagination with license filtering | ||
| if partial_license_filtered.total > 1 { | ||
| let paginated = service | ||
| .fetch_sbom_packages( | ||
| sbom_id, | ||
| q("license~GPL"), | ||
| Paginated { | ||
| offset: 0, | ||
| limit: 1, | ||
| }, | ||
| &ctx.db, | ||
| ) | ||
| .await?; | ||
|
|
||
| log::debug!("Paginated license filtered packages: {paginated:#?}"); | ||
| assert_eq!(paginated.items.len(), 1, "Should respect pagination limit"); | ||
| assert_eq!( | ||
| paginated.total, partial_license_filtered.total, | ||
| "Total should match full query" | ||
| ); | ||
| } |
There was a problem hiding this comment.
suggestion (testing): Pagination logic is tested, but consider adding a test for offset > 0.
Adding a test with offset > 0 will help verify that items are correctly skipped during pagination.
Suggested change
| // Test 6: Pagination with license filtering | |
| if partial_license_filtered.total > 1 { | |
| let paginated = service | |
| .fetch_sbom_packages( | |
| sbom_id, | |
| q("license~GPL"), | |
| Paginated { | |
| offset: 0, | |
| limit: 1, | |
| }, | |
| &ctx.db, | |
| ) | |
| .await?; | |
| log::debug!("Paginated license filtered packages: {paginated:#?}"); | |
| assert_eq!(paginated.items.len(), 1, "Should respect pagination limit"); | |
| assert_eq!( | |
| paginated.total, partial_license_filtered.total, | |
| "Total should match full query" | |
| ); | |
| } | |
| // Test 6: Pagination with license filtering | |
| if partial_license_filtered.total > 1 { | |
| // Fetch first item (offset 0) | |
| let paginated_first = service | |
| .fetch_sbom_packages( | |
| sbom_id, | |
| q("license~GPL"), | |
| Paginated { | |
| offset: 0, | |
| limit: 1, | |
| }, | |
| &ctx.db, | |
| ) | |
| .await?; | |
| log::debug!("Paginated license filtered packages (offset 0): {paginated_first:#?}"); | |
| assert_eq!(paginated_first.items.len(), 1, "Should respect pagination limit"); | |
| assert_eq!( | |
| paginated_first.total, partial_license_filtered.total, | |
| "Total should match full query" | |
| ); | |
| // Fetch second item (offset 1) | |
| let paginated_second = service | |
| .fetch_sbom_packages( | |
| sbom_id, | |
| q("license~GPL"), | |
| Paginated { | |
| offset: 1, | |
| limit: 1, | |
| }, | |
| &ctx.db, | |
| ) | |
| .await?; | |
| log::debug!("Paginated license filtered packages (offset 1): {paginated_second:#?}"); | |
| assert_eq!(paginated_second.items.len(), 1, "Should respect pagination limit for offset > 0"); | |
| assert_eq!( | |
| paginated_second.total, partial_license_filtered.total, | |
| "Total should match full query for offset > 0" | |
| ); | |
| assert_ne!( | |
| paginated_first.items[0], paginated_second.items[0], | |
| "Items at offset 0 and offset 1 should be different" | |
| ); | |
| } |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2011 +/- ##
==========================================
+ Coverage 68.20% 68.30% +0.10%
==========================================
Files 359 359
Lines 19947 19940 -7
Branches 19947 19940 -7
==========================================
+ Hits 13604 13620 +16
+ Misses 5561 5536 -25
- Partials 782 784 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jcrossley3
reviewed
Oct 3, 2025
Comment on lines
+233
to
+239
| .translator(|field, _operator, _value| { | ||
| match field { | ||
| // 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, |
There was a problem hiding this comment.
Is this really necessary? Seems like the result would be the same if you just eliminated the .translator(...) call entirely, no?
There was a problem hiding this comment.
It is necessary otherwise the result, if eliminated, would be
Error: Query syntax error: 'license' is an invalid field. Try [cpe_id, group, id, license_id, license_type, name, namespace, node_id, qualified_purl_id, sbom_id, spdx_license_exceptions, spdx_licenses, text, type, version]
There was a problem hiding this comment.
I didn't understand your comment until I looked more deeply at the code and realized that #1987 kinda snuck by me because I don't love exposing Constraint in the query api. I'm more in the "tell don't ask" camp of api design. 🥲
I feel like the union of the spdx and cyclonedx queries is an expression that should be added here, thereby obviating the need for the translator. But I wouldn't want to take on that refactoring in this PR.
There was a problem hiding this comment.
Basically this PR is all about ensuring consistency for the SBOM packages license filter with all the other filters by license.
mrizzi
force-pushed
the
sbom-packages-license-filter
branch
2 times, most recently
from
3f701f3 to
08f8e9c
Compare
|
@jcrossley3 - Jim this is the last piece of the puzzle regarding the License Search changes. We had to make some compromises with the as we didn't want to change the data model in this release and introduce data migration challenges. Instead Marco did the best he could with the existing, flawed, data model (it should be SBOM specification agnostic, but isn't). So with that said, is there anything preventing this PR from getting merged? |
Signed-off-by: mrizzi <[email protected]> Assisted-by: Claude Code
mrizzi
force-pushed
the
sbom-packages-license-filter
branch
from
08f8e9c to
7d398f8
Compare
Not to my knowledge. Merge away! |
jcrossley3
approved these changes
Oct 8, 2025
|
Successfully created backport PR for |
|
/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) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Add SBOM package license filtering with expanded SPDX LicenseRef resolution to achieve consistency with license filtering approach used in SBOM and Packages lists.
The SBOM package license filter BEFORE this change looks like this when filtering licenses for
gpl+The SBOM package license filter WITH this change looks like this when filtering licenses for
gpl+Changes
case_license_text_sbom_idfunction adding reusable get_case_license_text_sbom_id() helper functionfetch_unique_licensesSummary by Sourcery
Implement consistent license filtering for SBOM packages by expanding SPDX expressions, refactor the license service to use SeaORM and shared helpers, deprecate the
licenses_ref_mappingfield, and update tests and OpenAPI spec accordinglyNew Features:
Enhancements:
licenses_ref_mappingacross SBOM and PURL endpoints, returning an empty arraycase_license_text_sbom_idPL/SQL function via a reusable helperDocumentation:
licenses_ref_mappingas deprecated in the OpenAPI specificationTests:
fetch_unique_licensesand PURL endpoint tests to expect expanded license expressions and emptylicenses_ref_mapping