Add transaction safety and update comments from code review#2295
Merged
mrrajan merged 2 commits intoguacsec:mainfrom Mar 18, 2026
Merged
Conversation
Contributor
Reviewer's GuideIntroduces transaction-scoped SBOM ingestion and adjusts license queries for better performance and correctness, including making expanded license population transactional and replacing a LEFT JOIN + NULL filter with a NOT EXISTS subquery. Sequence diagram for transactional expanded_license populationsequenceDiagram
participant Caller
participant Db as DbConnection
participant Func as populate_expanded_license
participant Tx as Transaction
Caller->>Func: populate_expanded_license(sbom_id, Db)
activate Func
Func->>Db: begin()
activate Db
Db-->>Func: Tx
deactivate Db
activate Tx
Func->>Tx: execute(INSERT INTO expanded_license ...)
Tx-->>Func: Result
Func->>Tx: execute(WITH license_expansions AS (...) INSERT INTO sbom_license_expanded ...)
Tx-->>Func: Result
Func->>Tx: commit()
Tx-->>Func: Ok
deactivate Tx
Func-->>Caller: Ok
deactivate Func
Sequence diagram for transactional SBOM ingestion (CycloneDX/SPDX)sequenceDiagram
actor IngestJob
participant SbomContext
participant Db as DbConnection
participant Creator
participant ComponentCreator
IngestJob->>SbomContext: ingest_cyclonedx or ingest_spdx(sbom, Db)
activate SbomContext
SbomContext->>Db: preflight checks, lookups
SbomContext->>Creator: new Creator(sbom_id, components)
Creator-->>SbomContext: Creator
SbomContext->>Creator: create(Db, processors)
activate Creator
Creator->>ComponentCreator: new ComponentCreator(sbom_id, components.len())
ComponentCreator-->>Creator: ComponentCreator
Creator->>ComponentCreator: create(Db)
activate ComponentCreator
ComponentCreator->>Db: Licenses.create(Db) (requires TransactionTrait)
ComponentCreator->>Db: Purls.create(Db) (requires TransactionTrait)
ComponentCreator->>Db: Cpes.create(Db) (requires TransactionTrait)
ComponentCreator-->>Creator: Ok
deactivate ComponentCreator
Creator-->>SbomContext: Ok
deactivate Creator
SbomContext-->>IngestJob: Ok
deactivate SbomContext
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
mrrajan
force-pushed
the
fix/code-review-expanded-license-transaction
branch
from
cefa398 to
730c345
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 - I've found 1 issue, and left some high level feedback:
- The revised
populate_expanded_licensealways starts and commits its own transaction; if this is ever called inside a broader ingestion transaction, this can lead to nested-transaction/rollback surprises—consider taking an existing&Transaction(or a genericConnectionTraitthat may already be a transaction) and not unconditionally callingbegin()inside the function. - In
non_sbom_query, the NOT EXISTS subquery can be inlined directly into the.filter(Expr::exists(...).not())call (rather than pre-buildingexists_subquery), which would make the correlation onlicense::Column::Idmore obvious and the query easier to follow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The revised `populate_expanded_license` always starts and commits its own transaction; if this is ever called inside a broader ingestion transaction, this can lead to nested-transaction/rollback surprises—consider taking an existing `&Transaction` (or a generic `ConnectionTrait` that may already be a transaction) and not unconditionally calling `begin()` inside the function.
- In `non_sbom_query`, the NOT EXISTS subquery can be inlined directly into the `.filter(Expr::exists(...).not())` call (rather than pre-building `exists_subquery`), which would make the correlation on `license::Column::Id` more obvious and the query easier to follow.
## Individual Comments
### Comment 1
<location path="modules/ingestor/src/graph/sbom/common/expanded_license.rs" line_range="31-34" />
<code_context>
+///
+/// Both use the same core logic (expand_license_expression_with_mappings + md5 hash
+/// matching) but optimize for their different use cases.
+pub async fn populate_expanded_license<C>(sbom_id: Uuid, db: &C) -> Result<(), DbErr>
+where
+ C: ConnectionTrait + TransactionTrait,
+{
+ let txn = db.begin().await?;
</code_context>
<issue_to_address>
**suggestion:** Narrow the generic bound to only what this function actually needs.
Within this function, `db` is only used to start a transaction (`db.begin().await?`); all subsequent SQL calls use `txn` (which implements `ConnectionTrait`). So the function only requires `C: TransactionTrait`, not `C: ConnectionTrait`. Narrowing the bound better reflects the actual requirement and keeps the API more flexible.
```suggestion
pub async fn populate_expanded_license<C>(sbom_id: Uuid, db: &C) -> Result<(), DbErr>
where
C: TransactionTrait,
{
```
</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
reviewed
Mar 18, 2026
| where | ||
| C: ConnectionTrait + TransactionTrait, | ||
| { | ||
| let txn = db.begin().await?; |
Contributor
There was a problem hiding this comment.
Not sure why we need this. The function normally gets a transaction anyway. Creating a transaction here would actually create a savepoint. But why would a nested transaction be required here?
Contributor
Author
There was a problem hiding this comment.
Agree, I will remove this the transaction trait.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2295 +/- ##
==========================================
- Coverage 67.97% 67.68% -0.29%
==========================================
Files 430 433 +3
Lines 24731 24852 +121
Branches 24731 24852 +121
==========================================
+ Hits 16810 16822 +12
- Misses 6999 7103 +104
- Partials 922 927 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
mrrajan
force-pushed
the
fix/code-review-expanded-license-transaction
branch
2 times, most recently
from
60945c9 to
b7b3142
Compare
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
mrrajan
force-pushed
the
fix/code-review-expanded-license-transaction
branch
from
b7b3142 to
a13fd51
Compare
ctron
approved these changes
Mar 18, 2026
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.
This PR includes,
non_sbom_queryThese changes are accommodated with the backport PR - Backport 2287 to release/0.4.z #2294 on the release branch based on the sourcery review comments.
Summary by Sourcery
Ensure SBOM ingestion and expanded license population run within database transactions and optimize license lookup queries for unlinked licenses.
Bug Fixes:
Enhancements: