Merge pull request #639 from justahero/sebastian/refactor-validation

Refactor validation logic to be more concise

Author: Shnatsel

Cargo.lock

@@ -168,8 +168,8 @@ impl Args {
             accept_named: HashSet::from_iter(self.license_accept_named.clone()),
         });
 
-        let describe = self.describe.clone();
-        let spec_version = self.spec_version.clone();
+        let describe = self.describe;
+        let spec_version = self.spec_version;
 
         Ok(SbomConfig {
             format: self.format,

cargo-cyclonedx/src/cli.rs

@@ -55,11 +55,8 @@ impl SbomConfig {
                 .clone()
                 .map(|other| self.license_parser.clone().unwrap_or_default().merge(other))
                 .or_else(|| self.license_parser.clone()),
-            describe: other.describe.clone().or_else(|| self.describe.clone()),
-            spec_version: other
-                .spec_version
-                .clone()
-                .or_else(|| self.spec_version.clone()),
+            describe: other.describe.or(self.describe),
+            spec_version: other.spec_version.or(self.spec_version),
         }
     }
 

cargo-cyclonedx/src/config.rs

@@ -50,7 +50,6 @@ use cyclonedx_bom::models::metadata::MetadataError;
 use cyclonedx_bom::models::organization::OrganizationalContact;
 use cyclonedx_bom::models::tool::{Tool, Tools};
 use cyclonedx_bom::validation::Validate;
-use cyclonedx_bom::validation::ValidationResult;
 use once_cell::sync::Lazy;
 use regex::Regex;
 
@@ -714,8 +713,11 @@ impl GeneratedSbom {
         // If running in debug mode, validate that the SBOM is self-consistent and well-formed
         if cfg!(debug_assertions) {
             let result = bom.validate();
-            if let ValidationResult::Failed { reasons } = result {
-                panic!("The generated SBOM failed validation: {:?}", &reasons);
+            if result.has_errors() {
+                panic!(
+                    "The generated SBOM failed validation: {:?}",
+                    result.errors()
+                );
             }
         }
 
@@ -796,7 +798,7 @@ impl GeneratedSbom {
 
     fn filename(&self, binary_name: Option<&str>, target_kind: &[String]) -> String {
         let output_options = self.sbom_config.output_options();
-        let describe = self.sbom_config.describe.clone().unwrap_or_default();
+        let describe = self.sbom_config.describe.unwrap_or_default();
 
         let mut prefix = match describe {
             Describe::Crate => self.package_name.clone(),

cargo-cyclonedx/src/generator.rs

@@ -1,7 +1,7 @@
 use std::str::FromStr;
 
 use cargo_metadata::{camino::Utf8Path, Package};
-use cyclonedx_bom::prelude::Purl as CdxPurl;
+use cyclonedx_bom::{external_models::uri::validate_purl, prelude::Purl as CdxPurl};
 use pathdiff::diff_utf8_paths;
 use purl::{PackageError, PackageType, PurlBuilder};
 
@@ -81,8 +81,7 @@ fn to_purl_subpath(path: &Utf8Path) -> String {
 }
 
 fn assert_validation_passes(purl: &CdxPurl) {
-    use cyclonedx_bom::validation::{Validate, ValidationResult};
-    assert_eq!(purl.validate(), ValidationResult::Passed);
+    assert!(validate_purl(purl).is_ok());
 }
 
 #[cfg(test)]

cargo-cyclonedx/src/purl.rs

@@ -16,6 +16,7 @@ rust-version.workspace = true
 [dependencies]
 base64 = "0.21.2"
 fluent-uri = "0.1.4"
+indexmap = "2.2.2"
 once_cell = "1.18.0"
 ordered-float = { version = "4.2.0", default-features = false }
 packageurl = "0.3.0"

cyclonedx-bom/Cargo.toml

@@ -21,7 +21,7 @@ use std::convert::TryFrom;
 use thiserror::Error;
 use time::{format_description::well_known::Iso8601, OffsetDateTime};
 
-use crate::validation::{Validate, ValidationContext, ValidationResult};
+use crate::validation::ValidationError;
 
 /// For the purposes of CycloneDX SBOM documents, `DateTime` is a ISO8601 formatted timestamp
 ///
@@ -41,6 +41,13 @@ use crate::validation::{Validate, ValidationContext, ValidationResult};
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct DateTime(pub(crate) String);
 
+pub fn validate_date_time(date_time: &DateTime) -> Result<(), ValidationError> {
+    if OffsetDateTime::parse(&date_time.0, &Iso8601::DEFAULT).is_err() {
+        return Err("DateTime does not conform to ISO 8601".into());
+    }
+    Ok(())
+}
+
 impl DateTime {
     pub fn now() -> Result<Self, DateTimeError> {
         let now = OffsetDateTime::now_utc()
@@ -64,15 +71,6 @@ impl TryFrom<String> for DateTime {
     }
 }
 
-impl Validate for DateTime {
-    fn validate_with_context(&self, context: ValidationContext) -> ValidationResult {
-        match OffsetDateTime::parse(&self.0.to_string(), &Iso8601::DEFAULT) {
-            Ok(_) => ValidationResult::Passed,
-            Err(_) => ValidationResult::failure("DateTime does not conform to ISO 8601", context),
-        }
-    }
-}
-
 impl ToString for DateTime {
     fn to_string(&self) -> String {
         self.0.clone()
@@ -90,26 +88,25 @@ pub enum DateTimeError {
 
 #[cfg(test)]
 mod test {
-    use super::*;
     use pretty_assertions::assert_eq;
 
+    use crate::{external_models::validate_date_time, prelude::DateTime};
+
     #[test]
     fn valid_datetimes_should_pass_validation() {
-        let validation_result = DateTime("1969-06-28T01:20:00.00-04:00".to_string()).validate();
+        let validation_result =
+            validate_date_time(&DateTime("1969-06-28T01:20:00.00-04:00".to_string()));
 
-        assert_eq!(validation_result, ValidationResult::Passed)
+        assert!(validation_result.is_ok());
     }
 
     #[test]
     fn invalid_datetimes_should_fail_validation() {
-        let validation_result = DateTime("invalid date".to_string()).validate();
+        let validation_result = validate_date_time(&DateTime("invalid date".to_string()));
 
         assert_eq!(
             validation_result,
-            ValidationResult::failure(
-                "DateTime does not conform to ISO 8601",
-                ValidationContext::default()
-            )
-        )
+            Err("DateTime does not conform to ISO 8601".into()),
+        );
     }
 }

cyclonedx-bom/src/external_models/date_time.rs

@@ -20,3 +20,5 @@ pub mod date_time;
 pub mod normalized_string;
 pub mod spdx;
 pub mod uri;
+
+pub(crate) use date_time::validate_date_time;

cyclonedx-bom/src/external_models/mod.rs

@@ -16,7 +16,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-use crate::validation::{Validate, ValidationContext, ValidationResult};
+use crate::validation::ValidationError;
 use std::fmt::Display;
 use std::ops::Deref;
 
@@ -53,6 +53,12 @@ impl Deref for NormalizedString {
     }
 }
 
+impl AsRef<NormalizedString> for NormalizedString {
+    fn as_ref(&self) -> &NormalizedString {
+        self
+    }
+}
+
 impl AsRef<str> for NormalizedString {
     fn as_ref(&self) -> &str {
         &self.0
@@ -65,21 +71,21 @@ impl Display for NormalizedString {
     }
 }
 
-impl Validate for NormalizedString {
-    fn validate_with_context(&self, context: ValidationContext) -> ValidationResult {
-        if self.0.contains("\r\n")
-            || self.0.contains('\r')
-            || self.0.contains('\n')
-            || self.0.contains('\t')
-        {
-            return ValidationResult::failure(
-                "NormalizedString contains invalid characters \\r \\n \\t or \\r\\n",
-                context,
-            );
-        }
-
-        ValidationResult::Passed
+/// Validates a [`NormalizedString`].
+pub fn validate_normalized_string(
+    normalized_string: &NormalizedString,
+) -> Result<(), ValidationError> {
+    if normalized_string.contains("\r\n")
+        || normalized_string.contains('\r')
+        || normalized_string.contains('\n')
+        || normalized_string.contains('\t')
+    {
+        return Err(ValidationError::new(
+            "NormalizedString contains invalid characters \\r \\n \\t or \\r\\n",
+        ));
     }
+
+    Ok(())
 }
 
 #[cfg(test)]
@@ -105,21 +111,18 @@ mod test {
 
     #[test]
     fn it_should_pass_validation() {
-        let validation_result = NormalizedString("no_whitespace".to_string()).validate();
-
-        assert_eq!(validation_result, ValidationResult::Passed);
+        assert!(validate_normalized_string(&NormalizedString("no_whitespace".to_string())).is_ok());
     }
 
     #[test]
     fn it_should_fail_validation() {
-        let validation_result = NormalizedString("spaces and\ttabs".to_string()).validate();
+        let result = validate_normalized_string(&NormalizedString("spaces and\ttabs".to_string()));
 
         assert_eq!(
-            validation_result,
-            ValidationResult::failure(
+            result,
+            Err(ValidationError::new(
                 "NormalizedString contains invalid characters \\r \\n \\t or \\r\\n",
-                ValidationContext::default()
-            )
+            ))
         );
     }
 }

cyclonedx-bom/src/external_models/normalized_string.rs

@@ -21,7 +21,7 @@ use std::convert::TryFrom;
 use spdx::{Expression, ParseMode};
 use thiserror::Error;
 
-use crate::validation::{Validate, ValidationResult};
+use crate::validation::ValidationError;
 
 /// An identifier for a single, specific license
 ///
@@ -80,15 +80,10 @@ impl ToString for SpdxIdentifier {
     }
 }
 
-impl Validate for SpdxIdentifier {
-    fn validate_with_context(
-        &self,
-        context: crate::validation::ValidationContext,
-    ) -> ValidationResult {
-        match Self::try_from(self.0.clone()) {
-            Ok(_) => ValidationResult::Passed,
-            Err(_) => ValidationResult::failure("SPDX identifier is not valid", context),
-        }
+pub fn validate_spdx_identifier(identifier: &SpdxIdentifier) -> Result<(), ValidationError> {
+    match SpdxIdentifier::try_from(identifier.0.to_string()) {
+        Err(_error) => Err(ValidationError::new("SPDX identifier is not valid")),
+        _ => Ok(()),
     }
 }
 
@@ -175,16 +170,11 @@ impl ToString for SpdxExpression {
     }
 }
 
-impl Validate for SpdxExpression {
-    fn validate_with_context(
-        &self,
-        context: crate::validation::ValidationContext,
-    ) -> ValidationResult {
-        match SpdxExpression::try_from(self.0.clone()) {
-            Ok(_) => ValidationResult::Passed,
-            Err(_) => ValidationResult::failure("SPDX expression is not valid", context),
-        }
+pub fn validate_spdx_expression(expression: &SpdxExpression) -> Result<(), ValidationError> {
+    if Expression::parse(&expression.0).is_err() {
+        return Err(ValidationError::new("SPDX expression is not valid"));
     }
+    Ok(())
 }
 
 #[derive(Debug, Error, PartialEq, Eq)]
@@ -198,8 +188,6 @@ pub enum SpdxExpressionError {
 
 #[cfg(test)]
 mod test {
-    use crate::validation::{ValidationContext, ValidationResult};
-
     use super::*;
     use pretty_assertions::assert_eq;
 
@@ -247,18 +235,19 @@ mod test {
 
     #[test]
     fn valid_spdx_identifiers_should_pass_validation() {
-        let validation_result = SpdxIdentifier("MIT".to_string()).validate();
+        let validaton_result = validate_spdx_identifier(&SpdxIdentifier("MIT".to_string()));
 
-        assert_eq!(validation_result, ValidationResult::Passed);
+        assert!(validaton_result.is_ok());
     }
 
     #[test]
     fn invalid_spdx_identifiers_should_fail_validation() {
-        let validation_result = SpdxIdentifier("MIT OR Apache-2.0".to_string()).validate();
+        let validation_result =
+            validate_spdx_identifier(&SpdxIdentifier("MIT OR Apache-2.0".to_string()));
 
         assert_eq!(
             validation_result,
-            ValidationResult::failure("SPDX identifier is not valid", ValidationContext::default()),
+            Err("SPDX identifier is not valid".into()),
         );
     }
 
@@ -288,18 +277,20 @@ mod test {
 
     #[test]
     fn valid_spdx_expressions_should_pass_validation() {
-        let validation_result = SpdxExpression("MIT OR Apache-2.0".to_string()).validate();
+        let validation_result =
+            validate_spdx_expression(&SpdxExpression("MIT OR Apache-2.0".to_string()));
 
-        assert_eq!(validation_result, ValidationResult::Passed);
+        assert!(validation_result.is_ok());
     }
 
     #[test]
     fn invalid_spdx_expressions_should_fail_validation() {
-        let validation_result = SpdxExpression("not a real license".to_string()).validate();
+        let validation_result =
+            validate_spdx_expression(&SpdxExpression("not a real license".to_string()));
 
         assert_eq!(
             validation_result,
-            ValidationResult::failure("SPDX expression is not valid", ValidationContext::default())
+            Err("SPDX expression is not valid".into()),
         );
     }
 }