Pull out analisys test improvements from draft PR #2125
Merged
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
Contributor
Reviewer's GuideThis PR isolates analysis test improvements from a draft PR by parameterizing endpoint tests with rstest and a common Req helper, refactoring Collector and service code to propagate errors and extract subroutines, overhauling external SBOM resolution and recursive graph loading logic, and bumping the Rust toolchain and related dependencies. Sequence diagram for recursive graph loading with external SBOM resolutionsequenceDiagram
participant AnalysisService
participant InnerService
participant sbom_external_node_Entity
participant Collector
participant resolve_external_sbom
participant load_graph
participant load_graphs_inner
AnalysisService->>InnerService: load_graphs_inner(connection, sbom_ids, seen)
loop for each sbom_id
InnerService->>sbom_external_node_Entity: find external nodes for sbom_id
sbom_external_node_Entity-->>InnerService: external_sboms
loop for each external_sbom
InnerService->>resolve_external_sbom: resolve external sbom
resolve_external_sbom-->>InnerService: Option<ResolvedSbom>
alt resolved
InnerService->>load_graph: load graph for resolved sbom
load_graph-->>InnerService: Arc<PackageGraph>
InnerService->>InnerService: load_graphs_inner(connection, [resolved_sbom_id], seen)
else not resolved
InnerService-->>InnerService: skip
end
end
end
Sequence diagram for Collector::collect with error propagation and subroutinessequenceDiagram
participant Collector
participant "collect_external()"
participant "collect_package()"
participant "collect_graph()"
Collector->>Collector: collect()
alt depth == 0 or already visited
Collector-->>Collector: return Ok(None)
else node is ExternalNode
Collector->>"collect_external()": collect_external(external_node)
"collect_external()"-->>Collector: Result<Option<Vec<Node>>, Error>
else node is PackageNode
Collector->>"collect_package()": collect_package(current_node)
"collect_package()"-->>Collector: Result<Option<Vec<Node>>, Error>
else other node
Collector->>"collect_graph()": collect_graph()
"collect_graph()"-->>Collector: Result<Vec<Node>, Error>
Collector-->>Collector: Ok(Some(nodes))
end
Class diagram for refactored Collector and related service functionsclassDiagram
class Collector {
+collect() Result<Option<Vec<Node>>, Error>
+collect_graph() Result<Vec<Node>, Error>
+collect_external(external_node: &ExternalNode) Result<Option<Vec<Node>>, Error>
+collect_package(current_node: &PackageNode) Result<Option<Vec<Node>>, Error>
}
class AnalysisService {
+collect_components() Result<Vec<Node>, Error>
+collect_relationships() Result<Vec<Node>, Error>
+collect_ancestors() Result<Vec<Node>, Error>
+collect_descendants() Result<Vec<Node>, Error>
+collect_graphs_inner() Result<Vec<(String, Arc<PackageGraph>)>, Error>
+load_graphs_inner() Result<Vec<(String, Arc<PackageGraph>)>, Error>
}
Collector --> AnalysisService : uses
AnalysisService --> Collector : creates
Collector --> Node
Collector --> ExternalNode
Collector --> PackageNode
AnalysisService --> PackageGraph
AnalysisService --> Node
AnalysisService --> Error
Class diagram for new and refactored SBOM resolution helpersclassDiagram
class ResolvedSbom {
+sbom_id: Uuid
+node_id: String
}
class ExternalType {
<<enum>>
SPDX
CycloneDx
RedHatProductComponent
}
class DiscriminatorType {
<<enum>>
Sha256
// ...
}
class sbom_external_node_Entity {
+find()
+discriminator_type
+discriminator_value
+external_type
+external_node_ref
+external_doc_ref
+sbom_id
+node_id
}
class sbom_node_checksum_Entity {
+find()
+value
+sbom_id
+node_id
}
ResolvedSbom <.. sbom_external_node_Entity
ResolvedSbom <.. sbom_node_checksum_Entity
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ctron
force-pushed
the
feature/fix_tc_3170_2
branch
from
3a1eec7 to
54b972c
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider refactoring the URI construction in
req.rsto use a builder or a dedicated URI helper instead of manual string concatenation to ensure proper encoding and reduce boilerplate. - The new
collect_externalandcollect_packagemethods inCollectorwould benefit from brief doc comments explaining their inputs, outputs, and error behavior to improve maintainability. - In
load_graphs_inner, you could extract the external SBOM resolution and loading logic into a separate helper function to flatten the loop nesting and make the flow clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider refactoring the URI construction in `req.rs` to use a builder or a dedicated URI helper instead of manual string concatenation to ensure proper encoding and reduce boilerplate.
- The new `collect_external` and `collect_package` methods in `Collector` would benefit from brief doc comments explaining their inputs, outputs, and error behavior to improve maintainability.
- In `load_graphs_inner`, you could extract the external SBOM resolution and loading logic into a separate helper function to flatten the loop nesting and make the flow clearer.
## Individual Comments
### Comment 1
<location> `modules/analysis/src/service/mod.rs:225-241` </location>
<code_context>
+
+ log::debug!("Found {} nodes by checksum", matches.len());
+
+ Ok(matches
+ .into_iter()
+ // The use of .find here ensures we never match on cdx top level metadata component
+ // which has not defined a bom-ref - we can 'sniff' this because such nodes always
+ // are ingested with a uuid node-id.
+ .find(|model| {
+ if Uuid::parse_str(&model.node_id).is_err() {
+ // failed to parse, we keep it
+ true
+ } else {
+ log::debug!("Dropping suspected top-level node ID: {}", model.node_id);
+ false
}
- }
- _ => None,
- }
+ })
+ .map(|matched_model| ResolvedSbom {
+ sbom_id: matched_model.sbom_id,
+ node_id: matched_model.node_id,
</code_context>
<issue_to_address>
**suggestion:** The logic for filtering top-level nodes by UUID parsing may be fragile.
If the assumption that only top-level nodes have UUIDs changes, this filter may fail. Consider documenting this dependency or using a dedicated marker for top-level nodes.
```suggestion
// NOTE: The following filter relies on the assumption that only top-level nodes have UUID node-ids.
// If this assumption changes, this logic may become invalid.
// For future maintainability, consider using a dedicated marker or field to identify top-level nodes.
Ok(matches
.into_iter()
.find(|model| {
if Uuid::parse_str(&model.node_id).is_err() {
// failed to parse, we keep it
true
} else {
log::debug!("Dropping suspected top-level node ID: {}", model.node_id);
false
}
})
.map(|matched_model| ResolvedSbom {
sbom_id: matched_model.sbom_id,
node_id: matched_model.node_id,
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ctron
force-pushed
the
feature/fix_tc_3170_2
branch
2 times, most recently
from
98fa4ba to
5ed02a2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2125 +/- ##
==========================================
- Coverage 68.16% 68.08% -0.09%
==========================================
Files 371 371
Lines 20838 20753 -85
Branches 20838 20753 -85
==========================================
- Hits 14204 14129 -75
+ Misses 5797 5778 -19
- Partials 837 846 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ctron
force-pushed
the
feature/fix_tc_3170_2
branch
2 times, most recently
from
a359b20 to
ec672fa
Compare
dejanb
reviewed
Nov 21, 2025
| use serde_json::Value; | ||
|
|
||
| #[derive(Default, Copy, Clone)] | ||
| pub struct Req<'a> { |
Contributor
There was a problem hiding this comment.
I'd add some comments here and maybe some more descriptive naming especially for Loc enum and its members. It'd make it more readable
Contributor
Author
There was a problem hiding this comment.
I'll do that. With the exception of the Req one. Normally I'd be all for full names. However, in this specific case, which is limited to the analysis tests, I'd like to keep it short to not make #[case] attributes more cluttered.
|
|
||
| let mut uri = match loc { | ||
| Loc::None => { | ||
| format!("/api/v2/analysis/{latest}component?",) |
Contributor
There was a problem hiding this comment.
Can we use /api/v2/analysis/ as a shared prefix? Or use some URI build pattern?
Assisted-By: Cursor
ctron
force-pushed
the
feature/fix_tc_3170_2
branch
from
95b3e64 to
27a92d8
Compare
Contributor
Author
|
Addressed the comments and rebased. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
This extracts a few improvements of the analysis tests from PR #2118
Notably:
rstest(requires Rust 1.91)Summary by Sourcery
Extract analysis tests improvements by parameterizing endpoint tests and centralizing request logic, and refactor core service modules for better error propagation, modularity, and SBOM graph loading.
New Features:
Enhancements:
Build:
Tests: