perf(analysis): paginate before hydration to fix component search tim…#2411
Open
mrrajan wants to merge 1 commit into
Open
perf(analysis): paginate before hydration to fix component search tim…#2411mrrajan wants to merge 1 commit into
mrrajan wants to merge 1 commit into
Conversation
Contributor
Reviewer's GuideApply pagination at the graph node index level so only the requested page of nodes is hydrated, while ensuring deterministic ordering across SBOM graphs and exposing pagination-aware retrieval endpoints with tests. Sequence diagram for paginated graph query with pre-hydration paginationsequenceDiagram
actor Client
participant Endpoint as AnalysisEndpoint
participant Service as AnalysisService
participant DB as ConnectionTrait
participant Loader as GraphLoader
participant Collector
Client->>Endpoint: retrieve / retrieve_single / retrieve_latest
Endpoint->>Service: run_graph_query_paged(query, options, graphs, paginated, connection)
Service->>Service: count_matching_nodes(query, graphs)
Service->>Service: collect_graph_paged(query, graphs, concurrency, offset, limit, create)
loop for each paged node
Service->>Loader: GraphLoader::new(self.clone())
Service->>Collector: Collector::new(..., Direction::Incoming, ...).collect()
Service->>Collector: Collector::new(..., Direction::Outgoing, ...).collect()
Collector-->>Service: ancestors, descendants
Service-->Service: build Node { base, ancestors, descendants, warnings }
end
Service-->>Endpoint: PaginatedResults<Node> { items, total }
Endpoint-->>Client: HTTP response with items and total
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Both
count_matching_nodesandcollect_graph_pagedindependently iterate and filter all nodes across graphs; consider factoring out a shared iterator or precomputed list so you don’t double-walk and re-applyfilterfor the same query. collect_graph_pagedsorts the matching nodes but then usesbuffer_unordered, which can re-order the hydrated results; if callers depend on deterministic(sbom_id, node_id)ordering per page, consider using an ordered concurrency pattern or re-sorting after hydration.- The
limit == 0branch incollect_graph_pagedeffectively means 'no limit'; if that semantic is relied on, it may be worth making it explicit in the type or docs, or normalizingPaginatedearlier solimitis always non-zero when pagination is intended.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Both `count_matching_nodes` and `collect_graph_paged` independently iterate and filter all nodes across graphs; consider factoring out a shared iterator or precomputed list so you don’t double-walk and re-apply `filter` for the same query.
- `collect_graph_paged` sorts the matching nodes but then uses `buffer_unordered`, which can re-order the hydrated results; if callers depend on deterministic `(sbom_id, node_id)` ordering per page, consider using an ordered concurrency pattern or re-sorting after hydration.
- The `limit == 0` branch in `collect_graph_paged` effectively means 'no limit'; if that semantic is relied on, it may be worth making it explicit in the type or docs, or normalizing `Paginated` earlier so `limit` is always non-zero when pagination is intended.
## Individual Comments
### Comment 1
<location path="modules/analysis/src/service/test/mod.rs" line_range="856-886" />
<code_context>
+/// Verify that pagination limits the number of hydrated nodes.
+#[test_context(TrustifyContext)]
+#[test(tokio::test)]
+async fn test_pagination_limits_hydration(ctx: &TrustifyContext) -> Result<(), anyhow::Error> {
+ ctx.ingest_documents([
+ "spdx/quarkus-bom-3.2.11.Final-redhat-00001.json",
+ "spdx/quarkus-bom-3.2.12.Final-redhat-00002.json",
+ ])
+ .await?;
+
+ let service = AnalysisService::new(AnalysisConfig::default(), ctx.db.clone());
+
+ // When: request with limit=1
+ let result = service
+ .retrieve(
+ &Query::q("spymemcached"),
+ QueryOptions::ancestors(),
+ Paginated {
+ offset: 0,
+ limit: 1,
+ },
+ &ctx.db,
+ )
+ .await?;
+
+ // Then: only 1 item returned but total reflects all matches
+ assert_eq!(result.items.len(), 1);
+ assert_eq!(result.total, 2);
+
+ Ok(())
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a service-level test for offset beyond total and limit=0 behavior
These tests don’t yet cover the branches where `offset >= total` and `limit == 0` in `run_graph_query_paged`. Please add a case where a second call uses an `offset` ≥ `total` and asserts `items.is_empty()` while `total` is unchanged, and another case with `Paginated { offset: 0, limit: 0 }` that asserts all matching nodes are returned and `total` is correct.
```suggestion
/// Verify that pagination limits the number of hydrated nodes.
#[test_context(TrustifyContext)]
#[test(tokio::test)]
async fn test_pagination_limits_hydration(ctx: &TrustifyContext) -> Result<(), anyhow::Error> {
ctx.ingest_documents([
"spdx/quarkus-bom-3.2.11.Final-redhat-00001.json",
"spdx/quarkus-bom-3.2.12.Final-redhat-00002.json",
])
.await?;
let service = AnalysisService::new(AnalysisConfig::default(), ctx.db.clone());
// When: request with limit=1
let first_page = service
.retrieve(
&Query::q("spymemcached"),
QueryOptions::ancestors(),
Paginated {
offset: 0,
limit: 1,
},
&ctx.db,
)
.await?;
// Then: only 1 item returned but total reflects all matches
assert_eq!(first_page.items.len(), 1);
assert_eq!(first_page.total, 2);
// And when: requesting a page with offset beyond total
let beyond_total_page = service
.retrieve(
&Query::q("spymemcached"),
QueryOptions::ancestors(),
Paginated {
offset: first_page.total as i64,
limit: 1,
},
&ctx.db,
)
.await?;
// Then: no items are returned but total remains unchanged
assert!(beyond_total_page.items.is_empty());
assert_eq!(beyond_total_page.total, first_page.total);
Ok(())
}
/// Verify that limit=0 returns all hydrated nodes while respecting total.
#[test_context(TrustifyContext)]
#[test(tokio::test)]
async fn test_pagination_limit_zero_returns_all(
ctx: &TrustifyContext,
) -> Result<(), anyhow::Error> {
ctx.ingest_documents([
"spdx/quarkus-bom-3.2.11.Final-redhat-00001.json",
"spdx/quarkus-bom-3.2.12.Final-redhat-00002.json",
])
.await?;
let service = AnalysisService::new(AnalysisConfig::default(), ctx.db.clone());
// When: request with limit=0
let result = service
.retrieve(
&Query::q("spymemcached"),
QueryOptions::ancestors(),
Paginated {
offset: 0,
limit: 0,
},
&ctx.db,
)
.await?;
// Then: all matching nodes are returned and total is correct
assert_eq!(result.items.len(), 2);
assert_eq!(result.total, 2);
Ok(())
}
```
</issue_to_address>
### Comment 2
<location path="modules/analysis/src/service/test/mod.rs" line_range="891-853" />
<code_context>
+async fn test_pagination_offset(ctx: &TrustifyContext) -> Result<(), anyhow::Error> {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a test asserting deterministic ordering across repeated paginated calls
Given the new deterministic ordering via `(sbom_id, node_id)` and ordered SBOM loading, it would be helpful to add a test that calls `retrieve` twice with the same `Paginated { offset: 0, limit: 2 }` and asserts the same items, in the same order, are returned both times. This will catch regressions where ordering starts to depend on insertion order or map iteration.
Suggested implementation:
```rust
let service = AnalysisService::new(AnalysisConfig::default(), ctx.db.clone());
// Given: page 1 (offset=0, limit=1)
let page1 = service
```
To implement the deterministic ordering test you requested, add a new async test next to `test_pagination_offset` (reusing the same ingestion and service setup). The new test should:
```rust
#[test_context(TrustifyContext)]
#[test(tokio::test)]
async fn test_pagination_offset_deterministic_ordering(
ctx: &TrustifyContext,
) -> Result<(), anyhow::Error> {
// Given: same documents and service setup as test_pagination_offset
ctx.ingest_documents([
"spdx/quarkus-bom-3.2.11.Final-redhat-00001.json",
"spdx/quarkus-bom-3.2.12.Final-redhat-00002.json",
])
.await?;
let service = AnalysisService::new(AnalysisConfig::default(), ctx.db.clone());
// When: retrieve the first page twice with identical pagination
let page1_first = service
.retrieve(
/* whatever query/filter arguments you use in test_pagination_offset, e.g.: */
/* &some_query, */
Paginated {
offset: 0,
limit: 2,
},
/* and any additional params such as sort/filter, if present */
)
.await?;
let page1_second = service
.retrieve(
/* same arguments as above */
/* &some_query, */
Paginated {
offset: 0,
limit: 2,
},
)
.await?;
// Then: both calls should return the same total and the same items in the same order
assert_eq!(page1_first.total, page1_second.total);
assert_eq!(page1_first.items.len(), page1_second.items.len());
assert_eq!(page1_first.items, page1_second.items);
Ok(())
}
```
You will need to:
1. Copy the exact `retrieve` invocation used in `test_pagination_offset` (same query/filter arguments, additional parameters, etc.) for both `page1_first` and `page1_second` so that the only varying factor is the repeated call.
2. If `retrieve` requires additional context (e.g., a `query` struct, `AdvisoryQuery`, `ComponentQuery`, or feature flags), construct those exactly as in `test_pagination_offset` to ensure the test exercises the same path.
3. If `items` is not directly `Eq` (e.g., contains non-`Eq` types), instead compare the relevant deterministic fields (e.g., `(sbom_id, node_id)` for each item) in order:
```rust
let first_keys: Vec<_> = page1_first.items.iter().map(|item| (item.sbom_id, item.node_id)).collect();
let second_keys: Vec<_> = page1_second.items.iter().map(|item| (item.sbom_id, item.node_id)).collect();
assert_eq!(first_keys, second_keys);
```
4. Place this new test function in `modules/analysis/src/service/test/mod.rs` near `test_pagination_offset` to keep pagination-related tests grouped.
</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
+856
to
+886
| /// Verify that pagination limits the number of hydrated nodes. | ||
| #[test_context(TrustifyContext)] | ||
| #[test(tokio::test)] | ||
| async fn test_pagination_limits_hydration(ctx: &TrustifyContext) -> Result<(), anyhow::Error> { | ||
| ctx.ingest_documents([ | ||
| "spdx/quarkus-bom-3.2.11.Final-redhat-00001.json", | ||
| "spdx/quarkus-bom-3.2.12.Final-redhat-00002.json", | ||
| ]) | ||
| .await?; | ||
|
|
||
| let service = AnalysisService::new(AnalysisConfig::default(), ctx.db.clone()); | ||
|
|
||
| // When: request with limit=1 | ||
| let result = service | ||
| .retrieve( | ||
| &Query::q("spymemcached"), | ||
| QueryOptions::ancestors(), | ||
| Paginated { | ||
| offset: 0, | ||
| limit: 1, | ||
| }, | ||
| &ctx.db, | ||
| ) | ||
| .await?; | ||
|
|
||
| // Then: only 1 item returned but total reflects all matches | ||
| assert_eq!(result.items.len(), 1); | ||
| assert_eq!(result.total, 2); | ||
|
|
||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a service-level test for offset beyond total and limit=0 behavior
These tests don’t yet cover the branches where offset >= total and limit == 0 in run_graph_query_paged. Please add a case where a second call uses an offset ≥ total and asserts items.is_empty() while total is unchanged, and another case with Paginated { offset: 0, limit: 0 } that asserts all matching nodes are returned and total is correct.
Suggested change
| /// Verify that pagination limits the number of hydrated nodes. | |
| #[test_context(TrustifyContext)] | |
| #[test(tokio::test)] | |
| async fn test_pagination_limits_hydration(ctx: &TrustifyContext) -> Result<(), anyhow::Error> { | |
| ctx.ingest_documents([ | |
| "spdx/quarkus-bom-3.2.11.Final-redhat-00001.json", | |
| "spdx/quarkus-bom-3.2.12.Final-redhat-00002.json", | |
| ]) | |
| .await?; | |
| let service = AnalysisService::new(AnalysisConfig::default(), ctx.db.clone()); | |
| // When: request with limit=1 | |
| let result = service | |
| .retrieve( | |
| &Query::q("spymemcached"), | |
| QueryOptions::ancestors(), | |
| Paginated { | |
| offset: 0, | |
| limit: 1, | |
| }, | |
| &ctx.db, | |
| ) | |
| .await?; | |
| // Then: only 1 item returned but total reflects all matches | |
| assert_eq!(result.items.len(), 1); | |
| assert_eq!(result.total, 2); | |
| Ok(()) | |
| } | |
| /// Verify that pagination limits the number of hydrated nodes. | |
| #[test_context(TrustifyContext)] | |
| #[test(tokio::test)] | |
| async fn test_pagination_limits_hydration(ctx: &TrustifyContext) -> Result<(), anyhow::Error> { | |
| ctx.ingest_documents([ | |
| "spdx/quarkus-bom-3.2.11.Final-redhat-00001.json", | |
| "spdx/quarkus-bom-3.2.12.Final-redhat-00002.json", | |
| ]) | |
| .await?; | |
| let service = AnalysisService::new(AnalysisConfig::default(), ctx.db.clone()); | |
| // When: request with limit=1 | |
| let first_page = service | |
| .retrieve( | |
| &Query::q("spymemcached"), | |
| QueryOptions::ancestors(), | |
| Paginated { | |
| offset: 0, | |
| limit: 1, | |
| }, | |
| &ctx.db, | |
| ) | |
| .await?; | |
| // Then: only 1 item returned but total reflects all matches | |
| assert_eq!(first_page.items.len(), 1); | |
| assert_eq!(first_page.total, 2); | |
| // And when: requesting a page with offset beyond total | |
| let beyond_total_page = service | |
| .retrieve( | |
| &Query::q("spymemcached"), | |
| QueryOptions::ancestors(), | |
| Paginated { | |
| offset: first_page.total as i64, | |
| limit: 1, | |
| }, | |
| &ctx.db, | |
| ) | |
| .await?; | |
| // Then: no items are returned but total remains unchanged | |
| assert!(beyond_total_page.items.is_empty()); | |
| assert_eq!(beyond_total_page.total, first_page.total); | |
| Ok(()) | |
| } | |
| /// Verify that limit=0 returns all hydrated nodes while respecting total. | |
| #[test_context(TrustifyContext)] | |
| #[test(tokio::test)] | |
| async fn test_pagination_limit_zero_returns_all( | |
| ctx: &TrustifyContext, | |
| ) -> Result<(), anyhow::Error> { | |
| ctx.ingest_documents([ | |
| "spdx/quarkus-bom-3.2.11.Final-redhat-00001.json", | |
| "spdx/quarkus-bom-3.2.12.Final-redhat-00002.json", | |
| ]) | |
| .await?; | |
| let service = AnalysisService::new(AnalysisConfig::default(), ctx.db.clone()); | |
| // When: request with limit=0 | |
| let result = service | |
| .retrieve( | |
| &Query::q("spymemcached"), | |
| QueryOptions::ancestors(), | |
| Paginated { | |
| offset: 0, | |
| limit: 0, | |
| }, | |
| &ctx.db, | |
| ) | |
| .await?; | |
| // Then: all matching nodes are returned and total is correct | |
| assert_eq!(result.items.len(), 2); | |
| assert_eq!(result.total, 2); | |
| Ok(()) | |
| } |
| @@ -852,3 +852,100 @@ async fn resolve_sbom_cdx_rh_variant_external_node_sbom( | |||
|
|
|||
| Ok(()) | |||
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider a test asserting deterministic ordering across repeated paginated calls
Given the new deterministic ordering via (sbom_id, node_id) and ordered SBOM loading, it would be helpful to add a test that calls retrieve twice with the same Paginated { offset: 0, limit: 2 } and asserts the same items, in the same order, are returned both times. This will catch regressions where ordering starts to depend on insertion order or map iteration.
Suggested implementation:
let service = AnalysisService::new(AnalysisConfig::default(), ctx.db.clone());
// Given: page 1 (offset=0, limit=1)
let page1 = serviceTo implement the deterministic ordering test you requested, add a new async test next to test_pagination_offset (reusing the same ingestion and service setup). The new test should:
#[test_context(TrustifyContext)]
#[test(tokio::test)]
async fn test_pagination_offset_deterministic_ordering(
ctx: &TrustifyContext,
) -> Result<(), anyhow::Error> {
// Given: same documents and service setup as test_pagination_offset
ctx.ingest_documents([
"spdx/quarkus-bom-3.2.11.Final-redhat-00001.json",
"spdx/quarkus-bom-3.2.12.Final-redhat-00002.json",
])
.await?;
let service = AnalysisService::new(AnalysisConfig::default(), ctx.db.clone());
// When: retrieve the first page twice with identical pagination
let page1_first = service
.retrieve(
/* whatever query/filter arguments you use in test_pagination_offset, e.g.: */
/* &some_query, */
Paginated {
offset: 0,
limit: 2,
},
/* and any additional params such as sort/filter, if present */
)
.await?;
let page1_second = service
.retrieve(
/* same arguments as above */
/* &some_query, */
Paginated {
offset: 0,
limit: 2,
},
)
.await?;
// Then: both calls should return the same total and the same items in the same order
assert_eq!(page1_first.total, page1_second.total);
assert_eq!(page1_first.items.len(), page1_second.items.len());
assert_eq!(page1_first.items, page1_second.items);
Ok(())
}You will need to:
- Copy the exact
retrieveinvocation used intest_pagination_offset(same query/filter arguments, additional parameters, etc.) for bothpage1_firstandpage1_secondso that the only varying factor is the repeated call. - If
retrieverequires additional context (e.g., aquerystruct,AdvisoryQuery,ComponentQuery, or feature flags), construct those exactly as intest_pagination_offsetto ensure the test exercises the same path. - If
itemsis not directlyEq(e.g., contains non-Eqtypes), instead compare the relevant deterministic fields (e.g.,(sbom_id, node_id)for each item) in order:let first_keys: Vec<_> = page1_first.items.iter().map(|item| (item.sbom_id, item.node_id)).collect(); let second_keys: Vec<_> = page1_second.items.iter().map(|item| (item.sbom_id, item.node_id)).collect(); assert_eq!(first_keys, second_keys);
- Place this new test function in
modules/analysis/src/service/test/mod.rsneartest_pagination_offsetto keep pagination-related tests grouped.
Contributor
|
/scale-test |
|
🛠️ Scale test has started! Follow the progress here: Workflow Run |
Contributor
|
note: scale-tests use datasets that are prob tied to main (and will not work against release branch) |
…eout Apply LIMIT/OFFSET at the graph node filtering stage so only the requested page of nodes runs through expensive ancestor/descendant hydration. Previously all matching nodes were fully hydrated then sliced with paginate_array. - Add count_matching_nodes, collect_graph_paged, run_graph_query_paged - Use BTreeSet for sorted unique SBOM IDs and order_by_asc for deterministic pagination ordering - Update retrieve, retrieve_single, retrieve_latest to use paged path - Add pagination endpoint and service-level tests Implements TC-4365 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code
mrrajan
force-pushed
the
TC-4365-release-0.4.z
branch
from
4b07204 to
8a56de8
Compare
Contributor
|
I think the 0.4.z ship has sailed ... @mrrajan can this be applied to main branch ? |
Contributor
Author
|
@rh-jfuller Raised a PR based on |
Contributor
|
can this be closed then ? |
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
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.
Apply LIMIT/OFFSET at the graph node filtering stage so only the requested page of nodes runs through expensive ancestor/descendant hydration. Previously all matching nodes were fully hydrated then sliced with paginate_array.
Implements TC-4365
Assisted-by: Claude Code
Summary by Sourcery
Apply server-side pagination at the graph node filtering stage to avoid hydrating non-requested nodes and ensure deterministic ordering across SBOM graphs.
New Features:
Enhancements:
Tests: