fix: source document id must be non-null

ctron · ctron · commit d105547ab070 · 2025-10-13T11:14:59.000+02:00
(cherry picked from commit 877a6b8)
diff --git a/entity/src/advisory.rs b/entity/src/advisory.rs
@@ -31,7 +31,7 @@ pub struct Model {
     pub withdrawn: Option<OffsetDateTime>,
     pub title: Option<String>,
     pub labels: Labels,
-    pub source_document_id: Option<Uuid>,
+    pub source_document_id: Uuid,
 }
 
 #[cfg(feature = "async-graphql")]
diff --git a/entity/src/sbom.rs b/entity/src/sbom.rs
@@ -19,7 +19,7 @@ pub struct Model {
     pub suppliers: Vec<String>,
     pub data_licenses: Vec<String>,
 
-    pub source_document_id: Option<Uuid>,
+    pub source_document_id: Uuid,
 
     #[cfg_attr(
         feature = "async-graphql",
diff --git a/migration/src/lib.rs b/migration/src/lib.rs
@@ -28,6 +28,7 @@ mod m0001130_gover_cmp;
 mod m0001140_expand_spdx_licenses_function;
 mod m0001150_case_license_text_sbom_id_function;
 mod m0001160_improve_expand_spdx_licenses_function;
+mod m0001170_non_null_source_document_id;
 
 pub struct Migrator;
 
@@ -63,6 +64,7 @@ impl MigratorTrait for Migrator {
             Box::new(m0001140_expand_spdx_licenses_function::Migration),
             Box::new(m0001150_case_license_text_sbom_id_function::Migration),
             Box::new(m0001160_improve_expand_spdx_licenses_function::Migration),
+            Box::new(m0001170_non_null_source_document_id::Migration),
         ]
     }
 }
diff --git a/migration/src/m0001170_non_null_source_document_id.rs b/migration/src/m0001170_non_null_source_document_id.rs
@@ -0,0 +1,63 @@
+use sea_orm_migration::prelude::*;
+
+#[derive(DeriveMigrationName)]
+pub struct Migration;
+
+#[async_trait::async_trait]
+impl MigrationTrait for Migration {
+    async fn up(&self, manager: &SchemaManager) -> Result<(), DbErr> {
+        manager
+            .alter_table(
+                Table::alter()
+                    .table(Advisory::Table)
+                    .modify_column(ColumnDef::new(Advisory::SourceDocumentId).not_null())
+                    .to_owned(),
+            )
+            .await?;
+
+        manager
+            .alter_table(
+                Table::alter()
+                    .table(Sbom::Table)
+                    .modify_column(ColumnDef::new(Sbom::SourceDocumentId).not_null())
+                    .to_owned(),
+            )
+            .await?;
+
+        Ok(())
+    }
+
+    async fn down(&self, manager: &SchemaManager) -> Result<(), DbErr> {
+        manager
+            .alter_table(
+                Table::alter()
+                    .table(Sbom::Table)
+                    .modify_column(ColumnDef::new(Sbom::SourceDocumentId).null())
+                    .to_owned(),
+            )
+            .await?;
+
+        manager
+            .alter_table(
+                Table::alter()
+                    .table(Advisory::Table)
+                    .modify_column(ColumnDef::new(Advisory::SourceDocumentId).null())
+                    .to_owned(),
+            )
+            .await?;
+
+        Ok(())
+    }
+}
+
+#[derive(Iden)]
+enum Advisory {
+    Table,
+    SourceDocumentId,
+}
+
+#[derive(Iden)]
+enum Sbom {
+    Table,
+    SourceDocumentId,
+}
diff --git a/modules/fundamental/src/advisory/endpoints/mod.rs b/modules/fundamental/src/advisory/endpoints/mod.rs
@@ -157,7 +157,7 @@ pub async fn delete(
             false => Ok(HttpResponse::NotFound().finish()),
             true => {
                 tx.commit().await?;
-                if let Err(e) = delete_doc(v.source_document.as_ref(), i.storage()).await {
+                if let Err(e) = delete_doc(&v.source_document, i.storage()).await {
                     log::warn!("Ignoring {e}");
                 }
                 Ok(HttpResponse::Ok().json(v))
@@ -253,19 +253,15 @@ pub async fn download(
         return Ok(HttpResponse::NotFound().finish());
     };
 
-    if let Some(doc) = &advisory.source_document {
-        let stream = ingestor
-            .storage()
-            .retrieve(doc.try_into()?)
-            .await
-            .map_err(Error::Storage)?
-            .map(|stream| stream.map_err(Error::Storage));
+    let stream = ingestor
+        .storage()
+        .retrieve(advisory.source_document.try_into()?)
+        .await
+        .map_err(Error::Storage)?
+        .map(|stream| stream.map_err(Error::Storage));
 
-        Ok(match stream {
-            Some(s) => HttpResponse::Ok().streaming(s),
-            None => HttpResponse::NotFound().finish(),
-        })
-    } else {
-        Ok(HttpResponse::NotFound().finish())
-    }
+    Ok(match stream {
+        Some(s) => HttpResponse::Ok().streaming(s),
+        None => HttpResponse::NotFound().finish(),
+    })
 }
diff --git a/modules/fundamental/src/advisory/endpoints/test.rs b/modules/fundamental/src/advisory/endpoints/test.rs
@@ -560,11 +560,7 @@ async fn delete_advisory(ctx: &TrustifyContext) -> Result<(), anyhow::Error> {
         .call_and_read_body_json(TestRequest::get().uri("/api/v2/advisory").to_request())
         .await;
     assert_eq!(advisory_list.total, 1);
-    let key: StorageKey = advisory_list.items[0]
-        .source_document
-        .as_ref()
-        .expect("valid document")
-        .try_into()?;
+    let key: StorageKey = advisory_list.items[0].source_document.clone().try_into()?;
     assert!(storage.retrieve(key.clone()).await?.is_some());
 
     // first delete should succeed
diff --git a/modules/fundamental/src/advisory/model/details/mod.rs b/modules/fundamental/src/advisory/model/details/mod.rs
@@ -17,7 +17,7 @@ pub struct AdvisoryDetails {
     pub head: AdvisoryHead,
 
     #[serde(flatten)]
-    pub source_document: Option<SourceDocument>,
+    pub source_document: SourceDocument,
 
     /// Vulnerabilities addressed within this advisory.
     pub vulnerabilities: Vec<AdvisoryVulnerabilitySummary>,
@@ -60,10 +60,7 @@ impl AdvisoryDetails {
                 tx,
             )
             .await?,
-            source_document: advisory
-                .source_document
-                .as_ref()
-                .map(SourceDocument::from_entity),
+            source_document: SourceDocument::from_entity(&advisory.source_document),
             vulnerabilities,
             average_severity: None,
             average_score: None,
diff --git a/modules/fundamental/src/advisory/model/summary.rs b/modules/fundamental/src/advisory/model/summary.rs
@@ -17,7 +17,7 @@ pub struct AdvisorySummary {
 
     /// Information pertaning to the underlying source document, if any.
     #[serde(flatten)]
-    pub source_document: Option<SourceDocument>,
+    pub source_document: SourceDocument,
 
     /// Average (arithmetic mean) severity of the advisory aggregated from *all* related vulnerability assertions.
     #[schema(required)]
@@ -69,10 +69,7 @@ impl AdvisorySummary {
                     tx,
                 )
                 .await?,
-                source_document: each
-                    .source_document
-                    .as_ref()
-                    .map(SourceDocument::from_entity),
+                source_document: SourceDocument::from_entity(&each.source_document),
                 average_severity: None,
                 average_score: None,
                 vulnerabilities,
diff --git a/modules/fundamental/src/advisory/service/mod.rs b/modules/fundamental/src/advisory/service/mod.rs
@@ -199,7 +199,7 @@ impl AdvisoryService {
 
 #[derive(Debug)]
 pub struct AdvisoryCatcher {
-    pub source_document: Option<source_document::Model>,
+    pub source_document: source_document::Model,
     pub advisory: advisory::Model,
     pub issuer: Option<organization::Model>,
 }
@@ -211,7 +211,8 @@ impl FromQueryResult for AdvisoryCatcher {
                 res,
                 "",
                 source_document::Entity,
-            )?,
+            )?
+            .ok_or_else(|| DbErr::Custom("Missing source document".to_string()))?,
             advisory: Self::from_query_result_multi_model(res, "", advisory::Entity)?,
             issuer: Self::from_query_result_multi_model_optional(res, "", organization::Entity)?,
         })
diff --git a/modules/fundamental/src/advisory/service/test.rs b/modules/fundamental/src/advisory/service/test.rs
@@ -152,12 +152,12 @@ async fn single_advisory(ctx: &TrustifyContext) -> Result<(), anyhow::Error> {
             fetched,
             Some(AdvisoryDetails {
                 head: AdvisoryHead { .. },
-            source_document: Some(SourceDocument {
+            source_document: SourceDocument {
                 sha256,
                 sha384,
                 sha512,
                 ..
-            }),
+            },
             average_severity: None,
                 ..
             })
@@ -168,14 +168,13 @@ async fn single_advisory(ctx: &TrustifyContext) -> Result<(), anyhow::Error> {
             fetched,
             Some(AdvisoryDetails {
                 head: AdvisoryHead { .. },
-            source_document: Some(SourceDocument {
+            source_document: SourceDocument {
                 sha256,
                 sha384,
                 sha512,
                 ..
-            }),
+            },
             average_severity: None,
-
                 ..
             })
         if sha256 == jenny256.to_string() && sha384 == jenny384.to_string() && sha512 == jenny512.to_string()));
diff --git a/modules/fundamental/src/common/service.rs b/modules/fundamental/src/common/service.rs
@@ -66,21 +66,15 @@ fn escape(text: String) -> String {
 /// Delete the original raw json doc from storage. An appropriate
 /// message is returned in the event of an error, but it's up to the
 /// caller to either log the message or return failure to its caller.
-pub async fn delete_doc(
-    doc: Option<&SourceDocument>,
-    storage: impl DocumentDelete,
-) -> Result<(), Error> {
-    match doc {
-        Some(doc) => {
-            let key = doc.try_into()?;
-            storage.delete(key).await
-        }
-        None => Ok(()),
-    }
+pub async fn delete_doc(doc: &SourceDocument, storage: impl DocumentDelete) -> Result<(), Error> {
+    let key = doc.try_into()?;
+    storage.delete(key).await
 }
+
 pub trait DocumentDelete {
     fn delete(&self, key: StorageKey) -> impl Future<Output = Result<(), Error>>;
 }
+
 impl DocumentDelete for &DispatchBackend {
     async fn delete(&self, key: StorageKey) -> Result<(), Error> {
         (*self).delete(key).await.map_err(Error::Storage)
@@ -137,13 +131,9 @@ mod test {
             }
         }
 
-        // Deleting no doc is fine, error or not
-        let msg = delete_doc(None, FailingDelete {}).await;
-        assert!(msg.is_ok());
-
         // Failing to delete an invalid doc from storage should log an error
         let doc = SourceDocument::default();
-        match delete_doc(Some(&doc), FailingDelete {}).await {
+        match delete_doc(&doc, FailingDelete {}).await {
             Ok(_) => panic!("expected error"),
             Err(e) => assert!(e.to_string().contains("Missing prefix")),
         };
@@ -155,7 +145,7 @@ mod test {
             ),
             ..Default::default()
         };
-        match delete_doc(Some(&doc), FailingDelete {}).await {
+        match delete_doc(&doc, FailingDelete {}).await {
             Ok(_) => panic!("expected error"),
             Err(e) => assert!(e.to_string().contains("Delete failed")),
         };
diff --git a/modules/fundamental/src/sbom/endpoints/mod.rs b/modules/fundamental/src/sbom/endpoints/mod.rs
@@ -328,7 +328,7 @@ pub async fn delete(
             false => Ok(HttpResponse::NotFound().finish()),
             true => {
                 tx.commit().await?;
-                if let Err(e) = delete_doc(v.source_document.as_ref(), i.storage()).await {
+                if let Err(e) = delete_doc(&v.source_document, i.storage()).await {
                     log::warn!("Ignoring {e}");
                 }
                 Ok(HttpResponse::Ok().json(v))
@@ -505,21 +505,15 @@ pub async fn download(
         return Ok(HttpResponse::NotFound().finish());
     };
 
-    if let Some(doc) = &sbom.source_document {
-        let storage_key = doc.try_into()?;
-
-        let stream = ingestor
-            .storage()
-            .retrieve(storage_key)
-            .await
-            .map_err(Error::Storage)?
-            .map(|stream| stream.map_err(Error::Storage));
-
-        Ok(match stream {
-            Some(s) => HttpResponse::Ok().streaming(s),
-            None => HttpResponse::NotFound().finish(),
-        })
-    } else {
-        Ok(HttpResponse::NotFound().finish())
-    }
+    let stream = ingestor
+        .storage()
+        .retrieve(sbom.source_document.try_into()?)
+        .await
+        .map_err(Error::Storage)?
+        .map(|stream| stream.map_err(Error::Storage));
+
+    Ok(match stream {
+        Some(s) => HttpResponse::Ok().streaming(s),
+        None => HttpResponse::NotFound().finish(),
+    })
 }
diff --git a/modules/fundamental/src/sbom/endpoints/test.rs b/modules/fundamental/src/sbom/endpoints/test.rs
@@ -1181,8 +1181,7 @@ async fn download_sbom(ctx: &TrustifyContext) -> Result<(), anyhow::Error> {
     let sbom = app.call_and_read_body_json::<SbomSummary>(req).await;
     assert_eq!(Id::Uuid(sbom.head.id), result.id);
 
-    assert!(sbom.source_document.is_some());
-    let doc = sbom.source_document.unwrap();
+    let doc = sbom.source_document;
 
     let hashes = vec![doc.sha256, doc.sha384, doc.sha512];
 
diff --git a/modules/fundamental/src/sbom/model/mod.rs b/modules/fundamental/src/sbom/model/mod.rs
@@ -78,7 +78,7 @@ pub struct SbomSummary {
     pub head: SbomHead,
 
     #[serde(flatten)]
-    pub source_document: Option<SourceDocument>,
+    pub source_document: SourceDocument,
 
     pub described_by: Vec<SbomPackage>,
 }
@@ -93,12 +93,16 @@ impl SbomSummary {
         // TODO: consider improving the n-select issues here
         let described_by = service.describes_packages(sbom.sbom_id, (), db).await?;
 
-        let source_document = sbom.find_related(source_document::Entity).one(db).await?;
+        let source_document = sbom
+            .find_related(source_document::Entity)
+            .one(db)
+            .await?
+            .ok_or_else(|| Error::NotFound("Missing source document".to_string()))?;
 
         Ok(match node {
             Some(_) => Some(SbomSummary {
                 head: SbomHead::from_entity(&sbom, node, db).await?,
-                source_document: source_document.as_ref().map(SourceDocument::from_entity),
+                source_document: SourceDocument::from_entity(&source_document),
                 described_by,
             }),
             None => None,
diff --git a/modules/fundamental/tests/dataset.rs b/modules/fundamental/tests/dataset.rs
@@ -48,10 +48,6 @@ async fn ingest(ctx: TrustifyContext) -> anyhow::Result<()> {
 
     let source_doc = sbom_summary.source_document;
 
-    assert!(source_doc.is_some());
-
-    let source_doc = source_doc.unwrap();
-
     let storage_key = (&source_doc).try_into()?;
 
     let stream = storage.retrieve(storage_key).await?;
diff --git a/modules/fundamental/tests/sbom/spdx.rs b/modules/fundamental/tests/sbom/spdx.rs
@@ -181,10 +181,7 @@ async fn ingested_timestamp(ctx: &TrustifyContext) -> Result<(), anyhow::Error>
                 .await?
                 .expect("must find the document");
 
-            let ingested = sbom
-                .source_document
-                .expect("must have a source document")
-                .ingested;
+            let ingested = sbom.source_document.ingested;
             let now = OffsetDateTime::now_utc();
             assert!(ingested > start && ingested < now);
 
diff --git a/modules/fundamental/tests/sbom/spdx/corner_cases.rs b/modules/fundamental/tests/sbom/spdx/corner_cases.rs
diff --git a/modules/ingestor/src/graph/advisory/mod.rs b/modules/ingestor/src/graph/advisory/mod.rs
diff --git a/modules/ingestor/src/graph/sbom/mod.rs b/modules/ingestor/src/graph/sbom/mod.rs
diff --git a/openapi.yaml b/openapi.yaml

Original file line number	Diff line number	Diff line change
`@@ -31,7 +31,7 @@ pub struct Model {`
`31`	`31`	`pub withdrawn: Option<OffsetDateTime>,`
`32`	`32`	`pub title: Option<String>,`
`33`	`33`	`pub labels: Labels,`
`34`		`- pub source_document_id: Option<Uuid>,`
	`34`	`+ pub source_document_id: Uuid,`
`35`	`35`	`}`
`36`	`36`
`37`	`37`	`#[cfg(feature = "async-graphql")]`
Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,7 @@ mod m0001130_gover_cmp;`
`28`	`28`	`mod m0001140_expand_spdx_licenses_function;`
`29`	`29`	`mod m0001150_case_license_text_sbom_id_function;`
`30`	`30`	`mod m0001160_improve_expand_spdx_licenses_function;`
	`31`	`+mod m0001170_non_null_source_document_id;`
`31`	`32`
`32`	`33`	`pub struct Migrator;`
`33`	`34`
`@@ -63,6 +64,7 @@ impl MigratorTrait for Migrator {`
`63`	`64`	`Box::new(m0001140_expand_spdx_licenses_function::Migration),`
`64`	`65`	`Box::new(m0001150_case_license_text_sbom_id_function::Migration),`
`65`	`66`	`Box::new(m0001160_improve_expand_spdx_licenses_function::Migration),`
	`67`	`+ Box::new(m0001170_non_null_source_document_id::Migration),`
`66`	`68`	`]`
`67`	`69`	`}`
`68`	`70`	`}`