diff --git a/Cargo.toml b/Cargo.toml index 5f94e34..93311a5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,3 +33,7 @@ tokio = { version = "1.26", features = [ "full" ] } toml = "0.7.3" topological-sort = "0.2.2" walkdir = "2.3" + +[dev-dependencies] +proptest = "1.6.0" +test-strategy = "0.4.0" diff --git a/src/config/identifier.rs b/src/config/identifier.rs index cb2edc2..2f2344c 100644 --- a/src/config/identifier.rs +++ b/src/config/identifier.rs @@ -7,19 +7,106 @@ use std::{borrow::Cow, fmt, str::FromStr}; use serde::{Deserialize, Serialize}; use thiserror::Error; -/// A unique identifier for a configuration parameter. +macro_rules! ident_newtype { + ($id:ident) => { + impl $id { + /// Creates a new identifier at runtime. + pub fn new>(s: S) -> Result { + ConfigIdent::new(s).map(Self) + } + + /// Creates a new identifier from a static string. + pub fn new_static(s: &'static str) -> Result { + ConfigIdent::new_static(s).map(Self) + } + + /// Creates a new identifier at compile time, panicking if it is + /// invalid. + pub const fn new_const(s: &'static str) -> Self { + Self(ConfigIdent::new_const(s)) + } + + /// Returns the identifier as a string. + #[inline] + pub fn as_str(&self) -> &str { + self.0.as_str() + } + + #[inline] + #[allow(dead_code)] + pub(crate) fn as_ident(&self) -> &ConfigIdent { + &self.0 + } + } + + impl AsRef for $id { + #[inline] + fn as_ref(&self) -> &str { + self.0.as_ref() + } + } + + impl std::fmt::Display for $id { + #[inline] + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + self.0.fmt(f) + } + } + + impl FromStr for $id { + type Err = InvalidConfigIdent; + + fn from_str(s: &str) -> Result { + ConfigIdent::new(s).map(Self) + } + } + }; +} + +/// A unique identifier for a package name. /// -/// Config identifiers must be: +/// Package names must be: /// /// * non-empty /// * ASCII printable /// * first character must be a letter /// * contain only letters, numbers, underscores, and hyphens /// -/// In general, config identifiers represent Rust package and Oxide service names. +/// These generally match the rules of Rust package names. +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Deserialize, Serialize)] +#[serde(transparent)] +pub struct PackageName(ConfigIdent); +ident_newtype!(PackageName); + +/// A unique identifier for a service name. +/// +/// Package names must be: +/// +/// * non-empty +/// * ASCII printable +/// * first character must be a letter +/// * contain only letters, numbers, underscores, and hyphens +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Deserialize, Serialize)] +#[serde(transparent)] +pub struct ServiceName(ConfigIdent); +ident_newtype!(ServiceName); + +/// A unique identifier for a target preset. +/// +/// Package names must be: +/// +/// * non-empty +/// * ASCII printable +/// * first character must be a letter +/// * contain only letters, numbers, underscores, and hyphens +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Deserialize, Serialize)] +#[serde(transparent)] +pub struct PresetName(ConfigIdent); +ident_newtype!(PresetName); + #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize)] #[serde(transparent)] -pub struct ConfigIdent(Cow<'static, str>); +pub(crate) struct ConfigIdent(Cow<'static, str>); impl ConfigIdent { /// Creates a new config identifier at runtime. @@ -87,6 +174,10 @@ impl FromStr for ConfigIdent { } } +// The `Deserialize` implementation for `ConfigIdent` must be manually +// implemented, because it must go through validation. The `Serialize` +// implementation can be derived because `ConfigIdent` serializes as a regular +// string. impl<'de> Deserialize<'de> for ConfigIdent { fn deserialize(deserializer: D) -> Result where @@ -143,6 +234,10 @@ impl fmt::Display for InvalidConfigIdent { #[cfg(test)] mod tests { use super::*; + use serde_json::json; + use test_strategy::proptest; + + static IDENT_REGEX: &str = r"[a-zA-Z][a-zA-Z0-9_-]*"; #[test] fn valid_identifiers() { @@ -151,7 +246,28 @@ mod tests { ]; for &id in &valid { ConfigIdent::new(id).unwrap_or_else(|error| { - panic!("{} should have succeeded, but failed with: {:?}", id, error); + panic!( + "ConfigIdent::new for {} should have succeeded, but failed with: {:?}", + id, error + ); + }); + PackageName::new(id).unwrap_or_else(|error| { + panic!( + "PackageName::new for {} should have succeeded, but failed with: {:?}", + id, error + ); + }); + ServiceName::new(id).unwrap_or_else(|error| { + panic!( + "ServiceName::new for {} should have succeeded, but failed with: {:?}", + id, error + ); + }); + PresetName::new(id).unwrap_or_else(|error| { + panic!( + "PresetName::new for {} should have succeeded, but failed with: {:?}", + id, error + ); }); } } @@ -162,7 +278,83 @@ mod tests { "", "1", "_", "-", "1_", "-a", "_a", "a!", "a ", "a\n", "a\t", "a\r", "a\x7F", "aɑ", ]; for &id in &invalid { - ConfigIdent::new(id).expect_err(&format!("{} should have failed", id)); + ConfigIdent::new(id) + .expect_err(&format!("ConfigIdent::new for {} should have failed", id)); + PackageName::new(id) + .expect_err(&format!("PackageName::new for {} should have failed", id)); + ServiceName::new(id) + .expect_err(&format!("ServiceName::new for {} should have failed", id)); + PresetName::new(id) + .expect_err(&format!("PresetName::new for {} should have failed", id)); + + // Also ensure that deserialization fails. + let json = json!(id); + serde_json::from_value::(json.clone()).expect_err(&format!( + "ConfigIdent deserialization for {} should have failed", + id + )); + serde_json::from_value::(json.clone()).expect_err(&format!( + "PackageName deserialization for {} should have failed", + id + )); + serde_json::from_value::(json.clone()).expect_err(&format!( + "ServiceName deserialization for {} should have failed", + id + )); + serde_json::from_value::(json.clone()).expect_err(&format!( + "PresetName deserialization for {} should have failed", + id + )); } } + + #[proptest] + fn valid_identifiers_proptest(#[strategy(IDENT_REGEX)] id: String) { + ConfigIdent::new(&id).unwrap_or_else(|error| { + panic!( + "ConfigIdent::new for {} should have succeeded, but failed with: {:?}", + id, error + ); + }); + PackageName::new(&id).unwrap_or_else(|error| { + panic!( + "PackageName::new for {} should have succeeded, but failed with: {:?}", + id, error + ); + }); + ServiceName::new(&id).unwrap_or_else(|error| { + panic!( + "ServiceName::new for {} should have succeeded, but failed with: {:?}", + id, error + ); + }); + PresetName::new(&id).unwrap_or_else(|error| { + panic!( + "PresetName::new for {} should have succeeded, but failed with: {:?}", + id, error + ); + }); + } + + #[derive(Debug, Deserialize, Serialize, PartialEq, Eq)] + struct AllIdentifiers { + config: ConfigIdent, + package: PackageName, + service: ServiceName, + preset: PresetName, + } + + #[proptest] + fn valid_identifiers_proptest_serde(#[strategy(IDENT_REGEX)] id: String) { + let all = AllIdentifiers { + config: ConfigIdent::new(&id).unwrap(), + package: PackageName::new(&id).unwrap(), + service: ServiceName::new(&id).unwrap(), + preset: PresetName::new(&id).unwrap(), + }; + + let json = serde_json::to_value(&all).unwrap(); + let deserialized: AllIdentifiers = serde_json::from_value(json).unwrap(); + assert_eq!(all, deserialized); + } } diff --git a/src/config/imp.rs b/src/config/imp.rs index 7dc4ae0..d020b87 100644 --- a/src/config/imp.rs +++ b/src/config/imp.rs @@ -12,12 +12,12 @@ use std::path::Path; use thiserror::Error; use topological_sort::TopologicalSort; -use super::ConfigIdent; +use super::{PackageName, PresetName}; /// Describes a set of packages to act upon. /// /// This structure maps "package name" to "package" -pub struct PackageMap<'a>(pub BTreeMap<&'a ConfigIdent, &'a Package>); +pub struct PackageMap<'a>(pub BTreeMap<&'a PackageName, &'a Package>); // The name of a file which should be created by building a package. #[derive(Clone, Eq, Hash, Ord, PartialEq, PartialOrd)] @@ -70,12 +70,12 @@ impl<'a> PackageMap<'a> { /// /// Returns packages in batches that may be built concurrently. pub struct PackageDependencyIter<'a> { - lookup_by_output: BTreeMap, + lookup_by_output: BTreeMap, outputs: TopologicalSort, } impl<'a> Iterator for PackageDependencyIter<'a> { - type Item = Vec<(&'a ConfigIdent, &'a Package)>; + type Item = Vec<(&'a PackageName, &'a Package)>; fn next(&mut self) -> Option { if self.outputs.is_empty() { @@ -105,7 +105,7 @@ impl<'a> Iterator for PackageDependencyIter<'a> { pub struct Config { /// Packages to be built and installed. #[serde(default, rename = "package")] - pub packages: BTreeMap, + pub packages: BTreeMap, /// Target configuration. #[serde(default)] @@ -143,7 +143,7 @@ impl Config { pub struct TargetConfig { /// Preset configuration for targets. #[serde(default, rename = "preset")] - pub presets: BTreeMap, + pub presets: BTreeMap, } /// Errors which may be returned when parsing the server configuration. @@ -168,22 +168,24 @@ pub fn parse>(path: P) -> Result { #[cfg(test)] mod test { + use crate::config::ServiceName; + use super::*; #[test] fn test_order() { - let pkg_a_name = ConfigIdent::new_const("pkg-a"); + let pkg_a_name = PackageName::new_const("pkg-a"); let pkg_a = Package { - service_name: ConfigIdent::new_const("a"), + service_name: ServiceName::new_const("a"), source: PackageSource::Manual, output: PackageOutput::Tarball, only_for_targets: None, setup_hint: None, }; - let pkg_b_name = ConfigIdent::new_const("pkg-b"); + let pkg_b_name = PackageName::new_const("pkg-b"); let pkg_b = Package { - service_name: ConfigIdent::new_const("b"), + service_name: ServiceName::new_const("b"), source: PackageSource::Composite { packages: vec![pkg_a.get_output_file(&pkg_a_name)], }, @@ -213,10 +215,10 @@ mod test { #[test] #[should_panic(expected = "cyclic dependency in package manifest")] fn test_cyclic_dependency() { - let pkg_a_name = ConfigIdent::new_const("pkg-a"); - let pkg_b_name = ConfigIdent::new_const("pkg-b"); + let pkg_a_name = PackageName::new_const("pkg-a"); + let pkg_b_name = PackageName::new_const("pkg-b"); let pkg_a = Package { - service_name: ConfigIdent::new_const("a"), + service_name: ServiceName::new_const("a"), source: PackageSource::Composite { packages: vec![String::from("pkg-b.tar")], }, @@ -225,7 +227,7 @@ mod test { setup_hint: None, }; let pkg_b = Package { - service_name: ConfigIdent::new_const("b"), + service_name: ServiceName::new_const("b"), source: PackageSource::Composite { packages: vec![String::from("pkg-a.tar")], }, @@ -252,9 +254,9 @@ mod test { #[test] #[should_panic(expected = "Could not find a package which creates 'pkg-b.tar'")] fn test_missing_dependency() { - let pkg_a_name = ConfigIdent::new_const("pkg-a"); + let pkg_a_name = PackageName::new_const("pkg-a"); let pkg_a = Package { - service_name: ConfigIdent::new_const("a"), + service_name: ServiceName::new_const("a"), source: PackageSource::Composite { packages: vec![String::from("pkg-b.tar")], }, diff --git a/src/package.rs b/src/package.rs index 88ba399..d62e6a2 100644 --- a/src/package.rs +++ b/src/package.rs @@ -10,7 +10,7 @@ use crate::archive::{ }; use crate::blob::{self, BLOB}; use crate::cache::{Cache, CacheError}; -use crate::config::ConfigIdent; +use crate::config::{PackageName, ServiceName}; use crate::input::{BuildInput, BuildInputs, MappedPath, TargetDirectory, TargetPackage}; use crate::progress::{NoProgress, Progress}; use crate::target::TargetMap; @@ -169,7 +169,7 @@ pub enum PackageOutput { #[derive(Clone, Deserialize, Debug, PartialEq)] pub struct Package { /// The name of the service name to be used on the target OS. - pub service_name: ConfigIdent, + pub service_name: ServiceName, /// Identifies from where the package originates. /// @@ -194,7 +194,7 @@ pub struct Package { const DEFAULT_VERSION: semver::Version = semver::Version::new(0, 0, 0); async fn new_zone_archive_builder( - package_name: &ConfigIdent, + package_name: &PackageName, output_directory: &Utf8Path, ) -> Result>> { let tarfile = output_directory.join(format!("{}.tar.gz", package_name)); @@ -228,14 +228,19 @@ impl Default for BuildConfig<'_> { impl Package { /// The path of a package once it is built. - pub fn get_output_path(&self, id: &ConfigIdent, output_directory: &Utf8Path) -> Utf8PathBuf { + pub fn get_output_path(&self, id: &PackageName, output_directory: &Utf8Path) -> Utf8PathBuf { output_directory.join(self.get_output_file(id)) } + /// The path of the service name with respect to the install directory. + pub fn get_output_path_for_service(&self, install_directory: &Utf8Path) -> Utf8PathBuf { + install_directory.join(self.get_output_file_for_service()) + } + /// The path of a package after it has been "stamped" with a version. pub fn get_stamped_output_path( &self, - name: &ConfigIdent, + name: &PackageName, output_directory: &Utf8Path, ) -> Utf8PathBuf { output_directory @@ -244,18 +249,25 @@ impl Package { } /// The filename of a package once it is built. - pub fn get_output_file(&self, name: &ConfigIdent) -> String { + pub fn get_output_file(&self, name: &PackageName) -> String { match self.output { PackageOutput::Zone { .. } => format!("{}.tar.gz", name), PackageOutput::Tarball => format!("{}.tar", name), } } + pub fn get_output_file_for_service(&self) -> String { + match self.output { + PackageOutput::Zone { .. } => format!("{}.tar.gz", self.service_name), + PackageOutput::Tarball => format!("{}.tar", self.service_name), + } + } + #[deprecated = "Use 'Package::create', which now takes a 'BuildConfig', and implements 'Default'"] pub async fn create_for_target( &self, target: &TargetMap, - name: &ConfigIdent, + name: &PackageName, output_directory: &Utf8Path, ) -> Result { let build_config = BuildConfig { @@ -268,7 +280,7 @@ impl Package { pub async fn create( &self, - name: &ConfigIdent, + name: &PackageName, output_directory: &Utf8Path, build_config: &BuildConfig<'_>, ) -> Result { @@ -278,7 +290,7 @@ impl Package { pub async fn stamp( &self, - name: &ConfigIdent, + name: &PackageName, output_directory: &Utf8Path, version: &semver::Version, ) -> Result { @@ -351,7 +363,7 @@ impl Package { &self, progress: &impl Progress, target: &TargetMap, - name: &ConfigIdent, + name: &PackageName, output_directory: &Utf8Path, ) -> Result { let config = BuildConfig { @@ -364,7 +376,7 @@ impl Package { async fn create_internal( &self, - name: &ConfigIdent, + name: &PackageName, output_directory: &Utf8Path, config: &BuildConfig<'_>, ) -> Result { @@ -387,7 +399,7 @@ impl Package { // Adds the version file to the archive fn get_version_input( &self, - package_name: &ConfigIdent, + package_name: &PackageName, version: Option<&semver::Version>, ) -> BuildInput { match &self.output { @@ -519,7 +531,7 @@ impl Package { fn get_all_inputs( &self, - package_name: &ConfigIdent, + package_name: &PackageName, target: &TargetMap, output_directory: &Utf8Path, zoned: bool, @@ -630,7 +642,7 @@ impl Package { async fn create_zone_package( &self, timer: &mut BuildTimer, - name: &ConfigIdent, + name: &PackageName, output_directory: &Utf8Path, config: &BuildConfig<'_>, ) -> Result { @@ -769,7 +781,7 @@ impl Package { async fn create_tarball_package( &self, - name: &ConfigIdent, + name: &PackageName, output_directory: &Utf8Path, config: &BuildConfig<'_>, ) -> Result { diff --git a/tests/mod.rs b/tests/mod.rs index f16db4a..6453a57 100644 --- a/tests/mod.rs +++ b/tests/mod.rs @@ -12,13 +12,16 @@ mod test { use tar::Archive; use omicron_zone_package::blob::download; - use omicron_zone_package::config::{self, ConfigIdent}; + use omicron_zone_package::config::{self, PackageName, ServiceName}; use omicron_zone_package::package::BuildConfig; use omicron_zone_package::progress::NoProgress; use omicron_zone_package::target::TargetMap; - const MY_PACKAGE: ConfigIdent = ConfigIdent::new_const("my-package"); - const MY_SERVICE: ConfigIdent = ConfigIdent::new_const("my-service"); + const MY_PACKAGE: PackageName = PackageName::new_const("my-package"); + + /// The package name called the same as the service name. + const MY_SERVICE_PACKAGE: PackageName = PackageName::new_const("my-service"); + const MY_SERVICE: ServiceName = ServiceName::new_const("my-service"); fn entry_path<'a, R>(entry: &tar::Entry<'a, R>) -> Utf8PathBuf where @@ -61,18 +64,18 @@ mod test { async fn test_package_as_zone() { // Parse the configuration let cfg = config::parse("tests/service-a/cfg.toml").unwrap(); - let package = cfg.packages.get(&MY_SERVICE).unwrap(); + let package = cfg.packages.get(&MY_SERVICE_PACKAGE).unwrap(); // Create the packaged file let out = camino_tempfile::tempdir().unwrap(); let build_config = BuildConfig::default(); package - .create(&MY_SERVICE, out.path(), &build_config) + .create(&MY_SERVICE_PACKAGE, out.path(), &build_config) .await .unwrap(); // Verify the contents - let path = package.get_output_path(&MY_SERVICE, out.path()); + let path = package.get_output_path_for_service(out.path()); assert!(path.exists()); let gzr = flate2::read::GzDecoder::new(File::open(path).unwrap()); let mut archive = Archive::new(gzr); @@ -99,18 +102,18 @@ mod test { async fn test_rust_package_as_zone() { // Parse the configuration let cfg = config::parse("tests/service-b/cfg.toml").unwrap(); - let package = cfg.packages.get(&MY_SERVICE).unwrap(); + let package = cfg.packages.get(&MY_SERVICE_PACKAGE).unwrap(); // Create the packaged file let out = camino_tempfile::tempdir().unwrap(); let build_config = BuildConfig::default(); package - .create(&MY_SERVICE, out.path(), &build_config) + .create(&MY_SERVICE_PACKAGE, out.path(), &build_config) .await .unwrap(); // Verify the contents - let path = package.get_output_path(&MY_SERVICE, out.path()); + let path = package.get_output_path_for_service(out.path()); assert!(path.exists()); let gzr = flate2::read::GzDecoder::new(File::open(path).unwrap()); let mut archive = Archive::new(gzr); @@ -141,18 +144,18 @@ mod test { async fn test_rust_package_as_tarball() { // Parse the configuration let cfg = config::parse("tests/service-c/cfg.toml").unwrap(); - let package = cfg.packages.get(&MY_SERVICE).unwrap(); + let package = cfg.packages.get(&MY_SERVICE_PACKAGE).unwrap(); // Create the packaged file let out = camino_tempfile::tempdir().unwrap(); let build_config = BuildConfig::default(); package - .create(&MY_SERVICE, out.path(), &build_config) + .create(&MY_SERVICE_PACKAGE, out.path(), &build_config) .await .unwrap(); // Verify the contents - let path = package.get_output_path(&MY_SERVICE, out.path()); + let path = package.get_output_path_for_service(out.path()); assert!(path.exists()); let mut archive = Archive::new(File::open(path).unwrap()); let mut ents = archive.entries().unwrap(); @@ -168,7 +171,7 @@ mod test { // Try stamping it, verify the contents again let expected_semver = semver::Version::new(3, 3, 3); let path = package - .stamp(&MY_SERVICE, out.path(), &expected_semver) + .stamp(&MY_SERVICE_PACKAGE, out.path(), &expected_semver) .await .unwrap(); assert!(path.exists()); @@ -231,8 +234,8 @@ mod test { assert_eq!( batch_pkg_names, vec![ - &ConfigIdent::new_const("pkg-1"), - &ConfigIdent::new_const("pkg-2"), + &PackageName::new_const("pkg-1"), + &PackageName::new_const("pkg-2"), ] ); let build_config = BuildConfig::default(); @@ -247,7 +250,7 @@ mod test { // Build the composite package let batch = build_order.next().expect("Missing dependency batch"); let batch_pkg_names: Vec<_> = batch.iter().map(|(name, _)| *name).collect(); - let package_name = ConfigIdent::new_const("pkg-3"); + let package_name = PackageName::new_const("pkg-3"); assert_eq!(batch_pkg_names, vec![&package_name]); let package = cfg.packages.get(&package_name).unwrap(); let build_config = BuildConfig::default();