test(analysis): create a reproducer test for TC-3286 #2171
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 GuideAdds a regression-style test to reproduce TC-3286, ensuring DB and in-memory query logic stay aligned, refactors AnalysisService graph-query helpers to reuse the internal graph cache instead of passing it around, and exposes a helper to load graphs directly for tests. Sequence diagram for updated run_graph_query using internal graph_cachesequenceDiagram
actor Test as TestCode
participant AS as AnalysisService
participant ASI as AnalysisServiceInner
participant DB as ConnectionTrait
participant PG as PackageGraph
participant GC as GraphMap
Test->>AS: run_graph_query(query, options, graphs, connection)
activate AS
AS->>DB: use connection with graphs
loop for each graph in graphs
AS->>ASI: access graph_cache
ASI-->>AS: Arc_GraphMap_
AS->>GC: lookup relationships for node
GC-->>AS: cached_result or miss
AS->>PG: traverse graph with relationships
PG-->>AS: matching_nodes
end
AS-->>Test: Vec_Node_
deactivate AS
Class diagram for updated AnalysisService graph query helpersclassDiagram
class AnalysisService {
+concurrency: usize
-inner: AnalysisServiceInner
+run_graph_query<C>(query: GraphQuery, options: QueryOptions, graphs: Vec_PackageGraph_, connection: C) Result_Vec_Node__Error_
+load_graphs_query(connection: ConnectionTrait, query: GraphQuery) Result_Vec_PackageGraph__Error_
}
class AnalysisServiceInner {
+graph_cache: Arc_GraphMap_
+load_graphs_query(connection: ConnectionTrait, query: GraphQuery) Result_Vec_PackageGraph__Error_
}
class GraphQuery {
}
class QueryOptions {
+relationships: Relationships
}
class PackageGraph {
}
class GraphMap {
}
class Node {
}
class Relationships {
}
class ConnectionTrait {
}
class Error {
}
class Uuid {
}
AnalysisService --> AnalysisServiceInner : inner
AnalysisServiceInner --> GraphMap : graph_cache
AnalysisService --> PackageGraph : uses
AnalysisService --> GraphQuery : uses
AnalysisService --> QueryOptions : uses
AnalysisService --> ConnectionTrait : uses
AnalysisService --> Node : returns
AnalysisService --> Error : returns
AnalysisService --> Uuid : uses
AnalysisServiceInner --> ConnectionTrait : uses
AnalysisServiceInner --> PackageGraph : returns
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 there - I've reviewed your changes - here's some feedback:
- The new helper
escape_qis only used in tests; consider moving it into the test module (or making it#[cfg(test)]) to avoid expanding the public API surface unnecessarily. - The test
test_circular_deps_cyclonedx_service_countname no longer reflects what it checks (DB vs in-memoryqhandling); renaming it to something likedb_and_in_memory_query_alignmentwould make its purpose clearer. - Now that
run_graph_queryalways usesself.inner.graph_cache, you might add a brief comment on why bypassing a passed-in cache is intentional to avoid future refactors reintroducing divergence between implementations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new helper `escape_q` is only used in tests; consider moving it into the test module (or making it `#[cfg(test)]`) to avoid expanding the public API surface unnecessarily.
- The test `test_circular_deps_cyclonedx_service_count` name no longer reflects what it checks (DB vs in-memory `q` handling); renaming it to something like `db_and_in_memory_query_alignment` would make its purpose clearer.
- Now that `run_graph_query` always uses `self.inner.graph_cache`, you might add a brief comment on why bypassing a passed-in cache is intentional to avoid future refactors reintroducing divergence between implementations.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/repro_1
branch
from
f793c45 to
223fa78
Compare
jcrossley3
reviewed
Dec 8, 2025
Contributor
jcrossley3
left a comment
jcrossley3
left a comment
There was a problem hiding this comment.
Thanks for the tests! I have questions...
jcrossley3
force-pushed
the
feature/repro_1
branch
from
3f013d3 to
1b0cd6d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2171 +/- ##
=======================================
Coverage ? 68.37%
=======================================
Files ? 377
Lines ? 21282
Branches ? 21282
=======================================
Hits ? 14552
Misses ? 5858
Partials ? 872 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jcrossley3
force-pushed
the
feature/repro_1
branch
from
687ca32 to
7383235
Compare
Contributor
|
/scale-test |
|
🛠️ Scale test has started! Follow the progress here: Workflow Run |
jcrossley3
force-pushed
the
feature/repro_1
branch
from
7383235 to
8e336ac
Compare
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) |
jcrossley3
force-pushed
the
feature/repro_1
branch
from
8e336ac to
b8c4b50
Compare
jcrossley3
approved these changes
Dec 10, 2025
jcrossley3
reviewed
Dec 10, 2025
Comment on lines
19
to
26
| #[case( // purl partial search | ||
| Req { what: What::Q("pkg:oci/quay-builder-qemu-rhcos-rhel8"), ancestors: Some(10), ..Req::default() }, | ||
| 8 | ||
| 18 | ||
| )] | ||
| #[case( // purl partial search latest | ||
| Req { what: What::Q("pkg:oci/quay-builder-qemu-rhcos-rhel8"), ancestors: Some(10), latest: true, ..Req::default() }, | ||
| 2 | ||
| 5 | ||
| )] |
Contributor
There was a problem hiding this comment.
@ctron @JimFuller-RedHat Before we merge this PR, I'd like you guys to confirm this change to expected results, given that we've restored the ability to find matches of PURL's in full-text searches. Thanks!
Contributor
Author
There was a problem hiding this comment.
I do not think this is what we want. Testing for purl~ should be good enough. Also for performance reasons.
Contributor
There was a problem hiding this comment.
Interesting. If I prefix the above queries with purl~ I get 16 for the first one and 5 for the second. Why do you expect 8 and 2 for full-text searches of a purl? Shouldn't both be 0?
Contributor
Author
There was a problem hiding this comment.
Because in the case of purl~, you're actually searching PURLs. While in the case of not providing any field, it's not searching PURLs. But finding "purl like content" in other fields (like name).
I think this shows that using q without a field is highly confusing, to all of us.
Contributor
There was a problem hiding this comment.
I think this shows that using
qwithout a field is highly confusing, to all of us.
All of who? 'q' supports two types of searches: full-text and field. It's conceptually very simple and not confusing at all for a large number of use cases.
I would describe what you're asking for as a "partial text search". That's beyond the scope of 'q' and you should probably use something else.
ctron
commented
Dec 11, 2025
Contributor
Author
ctron
left a comment
ctron
left a comment
There was a problem hiding this comment.
As said before, I an against add this complexity for no reason. purl~ seems to work just fine. We advise anyway to go away from the default field search because it's to indeterministic for the user and has performance impacts. The ask is to align both implementation, which can also be done by just dropping purl from the default fields of the in-memory implementation.
Comment on lines
19
to
26
| #[case( // purl partial search | ||
| Req { what: What::Q("pkg:oci/quay-builder-qemu-rhcos-rhel8"), ancestors: Some(10), ..Req::default() }, | ||
| 8 | ||
| 18 | ||
| )] | ||
| #[case( // purl partial search latest | ||
| Req { what: What::Q("pkg:oci/quay-builder-qemu-rhcos-rhel8"), ancestors: Some(10), latest: true, ..Req::default() }, | ||
| 2 | ||
| 5 | ||
| )] |
Contributor
Author
There was a problem hiding this comment.
I do not think this is what we want. Testing for purl~ should be good enough. Also for performance reasons.
TC-3286 is about the fact that processing of `q` works differently for the DB implementation than it does for the in-memory implementation. This lead to weird behavior in the past. Both implementations must be aligned to prevent this in the future. This test gives an example. The test is expected to fail with the current codebase.
We can add whatever else we need to the context as the need arises
This fixes the alignment test, but it changed some expected results in a couple of other tests.
…L's" This reverts commit b8c4b50.
jcrossley3
force-pushed
the
feature/repro_1
branch
from
136ce65 to
b6eb579
Compare
ctron
commented
Dec 12, 2025
Contributor
Author
ctron
left a comment
ctron
left a comment
There was a problem hiding this comment.
I'm ok merging this. I can't approve as it's my own PR.
I'd also want @JimFuller-RedHat to perform a review.
This required allowing optional table name prefixes in field queries. We also DRY'd up a bit more of the common CPE types
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.
TC-3286 is about the fact that processing of
qworks differently for the DB implementation than it does for the in-memory implementation. This lead to weird behavior in the past. Both implementations must be aligned to prevent this in the future.This test gives an example. The test is expected to fail with the current codebase.
Summary by Sourcery
Align graph query handling with shared graph cache and add regression test ensuring DB and in-memory
qquery logic stay consistent.Enhancements:
qparameters.Tests:
qfiltering and in-memory graph query to prevent divergence between implementations.