From 0fb3db1669037a5e57ac1a34d743e9004450a92f Mon Sep 17 00:00:00 2001 From: mrrajan <86094767+mrrajan@users.noreply.github.com.> Date: Wed, 18 Mar 2026 14:04:03 +0530 Subject: [PATCH 1/2] add transaction safety and update comments from code review Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.> --- .../fundamental/src/license/service/mod.rs | 29 ++++++++++++------- .../src/graph/sbom/common/expanded_license.rs | 20 ++++++++++--- modules/ingestor/src/graph/sbom/cyclonedx.rs | 23 +++++++++------ modules/ingestor/src/graph/sbom/spdx.rs | 9 ++++-- 4 files changed, 54 insertions(+), 27 deletions(-) diff --git a/modules/fundamental/src/license/service/mod.rs b/modules/fundamental/src/license/service/mod.rs index b44a3d742..63e7174ba 100644 --- a/modules/fundamental/src/license/service/mod.rs +++ b/modules/fundamental/src/license/service/mod.rs @@ -297,22 +297,29 @@ impl LicenseService { .distinct() .column_as(expanded_license::Column::ExpandedText, LICENSE_TEXT); - // Build query for non-expanded licenses: includes both + // Build query for licenses not yet linked to any SBOM: includes both // (a) pre-loaded SPDX dictionary entries with no SBOM connection yet, AND - // (b) CycloneDX licenses that exist in sbom_package_license but were never expanded. - // A LEFT JOIN on sbom_package_license (instead of INNER JOIN) ensures pre-loaded licenses - // with no SBOM attachment are included. Then filtering for sbom_license_expanded IS NULL - // removes SPDX licenses that have already been expanded (they appear in spdx_query instead). + // (b) licenses from older SBOMs ingested before license expansion was implemented. + // Use NOT EXISTS instead of LEFT JOIN + IS NULL to find licenses without SBOMs. + // On large tables, LEFT JOIN scans all rows while NOT EXISTS + // uses a Nested Loop Anti Join with index-only scan. + let exists_subquery = sea_query::Query::select() + .expr(Expr::val(1)) + .from(sbom_license_expanded::Entity) + .and_where( + Expr::col(( + sbom_license_expanded::Entity, + sbom_license_expanded::Column::LicenseId, + )) + .equals((license::Entity, license::Column::Id)), + ) + .to_owned(); + let mut non_sbom_query = license::Entity::find() .select_only() .distinct() .column_as(license::Column::Text, LICENSE_TEXT) - .join(JoinType::LeftJoin, license::Relation::PackageLicense.def()) - .join( - JoinType::LeftJoin, - sbom_license_expanded::Relation::License.def().rev(), - ) - .filter(sbom_license_expanded::Column::LicenseId.is_null()); + .filter(Expr::exists(exists_subquery).not()); // Apply filtering to both queries (without sorting - that's applied to the UNION result) let filter_only = Query { diff --git a/modules/ingestor/src/graph/sbom/common/expanded_license.rs b/modules/ingestor/src/graph/sbom/common/expanded_license.rs index 6fb55765a..78489de19 100644 --- a/modules/ingestor/src/graph/sbom/common/expanded_license.rs +++ b/modules/ingestor/src/graph/sbom/common/expanded_license.rs @@ -16,10 +16,22 @@ use uuid::Uuid; /// /// While SeaORM could express this via custom expressions, it would be significantly /// more verbose and harder to maintain than the raw SQL. -pub async fn populate_expanded_license( - sbom_id: Uuid, - db: &impl ConnectionTrait, -) -> Result<(), DbErr> { +/// +/// # Differences from Migration Backfill +/// +/// The migration in m0002120_normalize_expanded_license/up.sql performs a similar +/// operation but with key differences: +/// - Migration: Pre-deduplicates by (text, sbom_id) and uses WHERE NOT EXISTS to skip +/// already-backfilled SBOMs. Optimized for one-time bulk processing. +/// - Ingestion: Filters by specific sbom_id parameter for single-SBOM processing. +/// Uses ON CONFLICT for idempotent re-ingestion of the same SBOM. +/// +/// 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(sbom_id: Uuid, db: &C) -> Result<(), DbErr> +where + C: ConnectionTrait, +{ // Step 1: Insert into expanded_license dictionary db.execute(Statement::from_sql_and_values( db.get_database_backend(), diff --git a/modules/ingestor/src/graph/sbom/cyclonedx.rs b/modules/ingestor/src/graph/sbom/cyclonedx.rs index 5ae3a824e..5f0166b47 100644 --- a/modules/ingestor/src/graph/sbom/cyclonedx.rs +++ b/modules/ingestor/src/graph/sbom/cyclonedx.rs @@ -21,7 +21,7 @@ use sbom_walker::{ model::sbom::serde_cyclonedx::Sbom, report::{ReportSink, check}, }; -use sea_orm::ConnectionTrait; +use sea_orm::{ConnectionTrait, TransactionTrait}; use serde_cyclonedx::cyclonedx::v_1_6::{ Component, ComponentEvidenceIdentity, CycloneDx, LicenseChoiceUrl, OrganizationalContact, }; @@ -138,12 +138,15 @@ impl<'a> From> for SbomInformation { impl SbomContext { #[instrument(skip(connection, sbom, warnings), err(level=tracing::Level::INFO))] - pub async fn ingest_cyclonedx( + pub async fn ingest_cyclonedx( &self, mut sbom: Box, warnings: &dyn ReportSink, connection: &C, - ) -> Result<(), Error> { + ) -> Result<(), Error> + where + C: ConnectionTrait + TransactionTrait, + { // pre-flight checks check::serde_cyclonedx::all(warnings, &Sbom::V1_6(Cow::Borrowed(&sbom))); @@ -285,11 +288,10 @@ impl<'a> Creator<'a> { } #[instrument(skip(self, db, processors), err(level=tracing::Level::INFO))] - pub async fn create( - self, - db: &impl ConnectionTrait, - processors: &mut [Box], - ) -> Result<(), Error> { + pub async fn create(self, db: &C, processors: &mut [Box]) -> Result<(), Error> + where + C: ConnectionTrait + TransactionTrait, + { let mut creator = ComponentCreator::new(self.sbom_id, self.components.len()); for comp in self.components { @@ -576,7 +578,10 @@ impl ComponentCreator { // order matters to prevent cross-table deadlocks when running // concurrent SBOM ingestions. All SBOM loaders must use the same // table insertion order. - async fn create(self, db: &impl ConnectionTrait) -> Result<(), Error> { + async fn create(self, db: &C) -> Result<(), Error> + where + C: ConnectionTrait + TransactionTrait, + { self.licenses.create(db).await?; self.purls.create(db).await?; self.cpes.create(db).await?; diff --git a/modules/ingestor/src/graph/sbom/spdx.rs b/modules/ingestor/src/graph/sbom/spdx.rs index 2a1e564d5..219e68c56 100644 --- a/modules/ingestor/src/graph/sbom/spdx.rs +++ b/modules/ingestor/src/graph/sbom/spdx.rs @@ -16,7 +16,7 @@ use crate::{ service::Error, }; use sbom_walker::report::{ReportSink, check}; -use sea_orm::ConnectionTrait; +use sea_orm::{ConnectionTrait, TransactionTrait}; use spdx_rs::models::{RelationshipType, SPDX}; use std::collections::HashSet; use std::str::FromStr; @@ -103,12 +103,15 @@ impl<'a> From> for SbomInformation { impl SbomContext { #[instrument(skip(db, sbom_data, warnings), ret(level=tracing::Level::DEBUG))] - pub async fn ingest_spdx( + pub async fn ingest_spdx( &self, sbom_data: SPDX, warnings: &dyn ReportSink, db: &C, - ) -> Result<(), Error> { + ) -> Result<(), Error> + where + C: ConnectionTrait + TransactionTrait, + { // pre-flight checks check::spdx::all(warnings, &sbom_data); From a13fd51e7ca73e85366130a43a4f743e23733a40 Mon Sep 17 00:00:00 2001 From: mrrajan <86094767+mrrajan@users.noreply.github.com.> Date: Wed, 18 Mar 2026 16:08:45 +0530 Subject: [PATCH 2/2] Remove unnecessary transactiontraits Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.> --- .../src/graph/sbom/common/expanded_license.rs | 8 +++--- modules/ingestor/src/graph/sbom/cyclonedx.rs | 25 ++++++++----------- modules/ingestor/src/graph/sbom/spdx.rs | 11 +++----- 3 files changed, 18 insertions(+), 26 deletions(-) diff --git a/modules/ingestor/src/graph/sbom/common/expanded_license.rs b/modules/ingestor/src/graph/sbom/common/expanded_license.rs index 78489de19..341e74fe2 100644 --- a/modules/ingestor/src/graph/sbom/common/expanded_license.rs +++ b/modules/ingestor/src/graph/sbom/common/expanded_license.rs @@ -28,10 +28,10 @@ use uuid::Uuid; /// /// 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(sbom_id: Uuid, db: &C) -> Result<(), DbErr> -where - C: ConnectionTrait, -{ +pub async fn populate_expanded_license( + sbom_id: Uuid, + db: &impl ConnectionTrait, +) -> Result<(), DbErr> { // Step 1: Insert into expanded_license dictionary db.execute(Statement::from_sql_and_values( db.get_database_backend(), diff --git a/modules/ingestor/src/graph/sbom/cyclonedx.rs b/modules/ingestor/src/graph/sbom/cyclonedx.rs index 5f0166b47..fe0019d69 100644 --- a/modules/ingestor/src/graph/sbom/cyclonedx.rs +++ b/modules/ingestor/src/graph/sbom/cyclonedx.rs @@ -21,7 +21,7 @@ use sbom_walker::{ model::sbom::serde_cyclonedx::Sbom, report::{ReportSink, check}, }; -use sea_orm::{ConnectionTrait, TransactionTrait}; +use sea_orm::ConnectionTrait; use serde_cyclonedx::cyclonedx::v_1_6::{ Component, ComponentEvidenceIdentity, CycloneDx, LicenseChoiceUrl, OrganizationalContact, }; @@ -138,15 +138,12 @@ impl<'a> From> for SbomInformation { impl SbomContext { #[instrument(skip(connection, sbom, warnings), err(level=tracing::Level::INFO))] - pub async fn ingest_cyclonedx( + pub async fn ingest_cyclonedx( &self, mut sbom: Box, warnings: &dyn ReportSink, - connection: &C, - ) -> Result<(), Error> - where - C: ConnectionTrait + TransactionTrait, - { + connection: &impl ConnectionTrait, + ) -> Result<(), Error> { // pre-flight checks check::serde_cyclonedx::all(warnings, &Sbom::V1_6(Cow::Borrowed(&sbom))); @@ -288,10 +285,11 @@ impl<'a> Creator<'a> { } #[instrument(skip(self, db, processors), err(level=tracing::Level::INFO))] - pub async fn create(self, db: &C, processors: &mut [Box]) -> Result<(), Error> - where - C: ConnectionTrait + TransactionTrait, - { + pub async fn create( + self, + db: &impl ConnectionTrait, + processors: &mut [Box], + ) -> Result<(), Error> { let mut creator = ComponentCreator::new(self.sbom_id, self.components.len()); for comp in self.components { @@ -578,10 +576,7 @@ impl ComponentCreator { // order matters to prevent cross-table deadlocks when running // concurrent SBOM ingestions. All SBOM loaders must use the same // table insertion order. - async fn create(self, db: &C) -> Result<(), Error> - where - C: ConnectionTrait + TransactionTrait, - { + async fn create(self, db: &impl ConnectionTrait) -> Result<(), Error> { self.licenses.create(db).await?; self.purls.create(db).await?; self.cpes.create(db).await?; diff --git a/modules/ingestor/src/graph/sbom/spdx.rs b/modules/ingestor/src/graph/sbom/spdx.rs index 219e68c56..53d6e65da 100644 --- a/modules/ingestor/src/graph/sbom/spdx.rs +++ b/modules/ingestor/src/graph/sbom/spdx.rs @@ -16,7 +16,7 @@ use crate::{ service::Error, }; use sbom_walker::report::{ReportSink, check}; -use sea_orm::{ConnectionTrait, TransactionTrait}; +use sea_orm::ConnectionTrait; use spdx_rs::models::{RelationshipType, SPDX}; use std::collections::HashSet; use std::str::FromStr; @@ -103,15 +103,12 @@ impl<'a> From> for SbomInformation { impl SbomContext { #[instrument(skip(db, sbom_data, warnings), ret(level=tracing::Level::DEBUG))] - pub async fn ingest_spdx( + pub async fn ingest_spdx( &self, sbom_data: SPDX, warnings: &dyn ReportSink, - db: &C, - ) -> Result<(), Error> - where - C: ConnectionTrait + TransactionTrait, - { + db: &impl ConnectionTrait, + ) -> Result<(), Error> { // pre-flight checks check::spdx::all(warnings, &sbom_data);