Merge pull request #1306 from spkenv/ident-parts-buf-eq

Fix IdentPartsBuf not respecting version equality semantics

Author: jrray

crates/spk-schema/crates/foundation/src/ident/ident_any.rs

@@ -191,37 +191,12 @@ impl FromStr for AnyIdent {
     }
 }
 
-impl TryFrom<&IdentPartsBuf> for AnyIdent {
-    type Error = crate::ident::Error;
-
-    fn try_from(parts: &IdentPartsBuf) -> Result<Self> {
-        if parts.repository_name.is_some() {
-            return Err("Ident may not have a repository name".into());
-        }
-
-        let name = parts.pkg_name.parse::<PkgNameBuf>()?;
-        let version = parts
-            .version_str
-            .as_ref()
-            .map(|v| v.parse::<Version>())
-            .transpose()?
-            .unwrap_or_default();
-        let build = parts
-            .build_str
-            .as_ref()
-            .map(|v| v.parse::<Build>())
-            .transpose()?;
-
-        Ok(VersionIdent::new(name, version).into_any_ident(build))
-    }
-}
-
 impl From<&AnyIdent> for IdentPartsBuf {
     fn from(ident: &AnyIdent) -> Self {
         IdentPartsBuf {
             repository_name: None,
             pkg_name: ident.name().to_string(),
-            version_str: Some(ident.version().to_string()),
+            version_str: Some(ident.version().into()),
             build_str: ident.build().map(|b| b.to_string()),
         }
     }
@@ -240,15 +215,6 @@ impl From<PkgNameBuf> for Box<AnyIdent> {
     }
 }
 
-impl PartialEq<&AnyIdent> for IdentPartsBuf {
-    fn eq(&self, other: &&AnyIdent) -> bool {
-        self.repository_name.is_none()
-            && self.pkg_name == other.name().as_str()
-            && self.version_str == Some(other.version().to_string())
-            && self.build_str == other.build().map(|b| b.to_string())
-    }
-}
-
 impl TryFrom<RangeIdent> for AnyIdent {
     type Error = crate::ident::Error;
 

crates/spk-schema/crates/foundation/src/ident/ident_build.rs

@@ -59,7 +59,7 @@ impl TryFrom<EmbeddedSourcePackage> for BuildIdent {
             };
         };
         Ok(Self::new(
-            VersionIdent::new(pkg_name.try_into()?, version.try_into()?),
+            VersionIdent::new(pkg_name.try_into()?, version.as_str().try_into()?),
             build.try_into()?,
         ))
     }
@@ -198,7 +198,7 @@ impl TryFrom<&IdentPartsBuf> for BuildIdent {
         let version = parts
             .version_str
             .as_ref()
-            .map(|v| v.parse::<Version>())
+            .map(|v| v.as_str().parse::<Version>())
             .transpose()?
             .unwrap_or_default();
         let build = parts
@@ -238,7 +238,7 @@ impl From<&BuildIdent> for IdentPartsBuf {
         IdentPartsBuf {
             repository_name: None,
             pkg_name: ident.name().to_string(),
-            version_str: Some(ident.version().to_string()),
+            version_str: Some(ident.version().into()),
             build_str: Some(ident.build().to_string()),
         }
     }
@@ -248,7 +248,10 @@ impl PartialEq<&BuildIdent> for IdentPartsBuf {
     fn eq(&self, other: &&BuildIdent) -> bool {
         self.repository_name.is_none()
             && self.pkg_name == other.name().as_str()
-            && self.version_str == Some(other.version().to_string())
+            && self
+                .version_str
+                .as_ref()
+                .is_some_and(|s| *s == other.version().into())
             && self.build_str == Some(other.build().to_string())
     }
 }

crates/spk-schema/crates/foundation/src/ident_build/build.rs

@@ -63,7 +63,7 @@ impl EmbeddedSourcePackage {
             repository_name: None,
             name: self.ident.pkg_name.parse()?,
             version: VersionFilter::single(VersionRange::DoubleEquals(DoubleEqualsVersion::from(
-                version_str.parse::<Version>()?,
+                version_str.as_str().parse::<Version>()?,
             ))),
             components: self.components.clone(),
             build: Some(build_str.parse()?),

crates/spk-schema/crates/foundation/src/ident_build/parsing/build.rs

@@ -96,13 +96,15 @@ where
         + FromExternalError<&'b str, std::num::ParseIntError>
         + TagError<&'b str, &'static str>,
 {
-    map(
+    map_res(
         delimited(tag("["), cut(ident_parts_with_components), cut(tag("]"))),
         |(ident, components)| {
-            EmbeddedSource::Package(Box::new(EmbeddedSourcePackage {
-                ident: ident.to_owned(),
-                components,
-            }))
+            Ok::<_, crate::version::Error>(EmbeddedSource::Package(Box::new(
+                EmbeddedSourcePackage {
+                    ident: ident.to_owned()?,
+                    components,
+                },
+            )))
         },
     )(input)
 }

crates/spk-schema/crates/foundation/src/ident_ops/parsing/ident.rs

@@ -21,8 +21,11 @@ use crate::ident_build::parsing::build;
 use crate::ident_component::Component;
 use crate::name::RepositoryName;
 use crate::name::parsing::package_name;
+use crate::version::Version;
 use crate::version::parsing::version_str;
 
+// This type should _not_ derive Eq because `version_str` is not guaranteed to
+// be normalized.
 #[derive(Debug)]
 pub struct IdentParts<'s> {
     pub repository_name: Option<&'s str>,
@@ -31,22 +34,49 @@ pub struct IdentParts<'s> {
     pub build_str: Option<&'s str>,
 }
 
+/// A version string that is normalized.
+///
+/// A normalized version string is required for equality comparisons.
+#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
+pub struct NormalizedVersionString(String);
+
+impl NormalizedVersionString {
+    /// Access to the inner string.
+    pub fn as_str(&self) -> &str {
+        &self.0
+    }
+}
+
+impl std::fmt::Display for NormalizedVersionString {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.0.fmt(f)
+    }
+}
+
+impl From<&Version> for NormalizedVersionString {
+    fn from(version: &Version) -> Self {
+        NormalizedVersionString(version.to_storage_string())
+    }
+}
+
 #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
 pub struct IdentPartsBuf {
     pub repository_name: Option<String>,
     pub pkg_name: String,
-    pub version_str: Option<String>,
+    pub version_str: Option<NormalizedVersionString>,
     pub build_str: Option<String>,
 }
 
 impl IdentParts<'_> {
-    pub fn to_owned(&self) -> IdentPartsBuf {
-        IdentPartsBuf {
+    pub fn to_owned(&self) -> crate::version::Result<IdentPartsBuf> {
+        let version: Option<Version> = self.version_str.map(|s| s.parse()).transpose()?;
+
+        Ok(IdentPartsBuf {
             repository_name: self.repository_name.map(|o| o.to_owned()),
             pkg_name: self.pkg_name.to_owned(),
-            version_str: self.version_str.map(|o| o.to_owned()),
+            version_str: version.as_ref().map(Into::into),
             build_str: self.build_str.map(|o| o.to_owned()),
-        }
+        })
     }
 }
 

crates/spk-schema/crates/foundation/src/version/mod.rs

@@ -586,14 +586,6 @@ impl TryFrom<&str> for Version {
     }
 }
 
-impl TryFrom<String> for Version {
-    type Error = Error;
-
-    fn try_from(value: String) -> Result<Self> {
-        parse_version(value)
-    }
-}
-
 impl FromStr for Version {
     type Err = Error;
 

crates/spk-storage/src/storage/repository_test.rs

@@ -6,9 +6,10 @@ use std::str::FromStr;
 
 use rstest::rstest;
 use spk_schema::foundation::ident_component::Component;
-use spk_schema::foundation::pkg_name;
 use spk_schema::foundation::spec_ops::Named;
+use spk_schema::foundation::{pkg_name, version_ident};
 use spk_schema::ident::{AsVersionIdent, parse_build_ident, parse_version_ident};
+use spk_schema::ident_build::{Build, EmbeddedSource};
 use spk_schema::{
     Deprecate,
     DeprecateMut,
@@ -389,6 +390,60 @@ async fn test_repo_publish_package_creates_embed_stubs(#[case] repo: RepoKind) {
     );
 }
 
+/// If an embedded package is declared with a non-normalized version,
+/// the buildid created for the stub should contain a normalized version.
+///
+/// This is necessary to be able to compare build ids correctly.
+#[rstest]
+#[case::mem(RepoKind::Mem)]
+#[case::spfs(RepoKind::Spfs)]
+#[tokio::test]
+async fn test_embedded_stub_build_version_is_normalized(#[case] repo: RepoKind) {
+    let repo = make_repo(repo).await;
+    let recipe = recipe!({
+        "pkg": "my-pkg/1.0.0",
+        "install": {
+            "embedded": [
+                {"pkg": "my-embedded-pkg/1.0.0.0.0"}
+            ]
+        }
+    });
+    repo.publish_recipe(&recipe).await.unwrap();
+    let spec = spec!({
+        "pkg": "my-pkg/1.0.0/3I42H3S6",
+        "install": {
+            "embedded": [
+                {"pkg": "my-embedded-pkg/1.0.0.0.0/embedded"}
+            ]
+        }
+    });
+    repo.publish_package(
+        &spec,
+        &vec![(Component::Run, empty_layer_digest())]
+            .into_iter()
+            .collect(),
+    )
+    .await
+    .unwrap();
+    assert_eq!(
+        repo.list_package_builds(&version_ident!("my-embedded-pkg/1.0.0"))
+            .await
+            .unwrap()
+            .into_iter()
+            .filter_map(|build_ident| match build_ident.build() {
+                Build::Embedded(EmbeddedSource::Package(embedded_source)) =>
+                // Checking the "raw" version string that was encoded into
+                // the build id.
+                    embedded_source.ident.version_str.to_owned(),
+                _ => None,
+            })
+            .next()
+            .expect("should have at least one (embedded) build")
+            .as_str(),
+        "1.0.0"
+    );
+}
+
 #[rstest]
 #[case::mem(RepoKind::Mem)]
 #[case::spfs(RepoKind::Spfs)]