diff --git a/crates/dropshot-api-manager-types/src/validation.rs b/crates/dropshot-api-manager-types/src/validation.rs index dd9a91c..b9a97e1 100644 --- a/crates/dropshot-api-manager-types/src/validation.rs +++ b/crates/dropshot-api-manager-types/src/validation.rs @@ -100,183 +100,362 @@ pub trait ValidationBackend { fn record_file_contents(&mut self, path: Utf8PathBuf, contents: Vec); } -/// Describes the path to an OpenAPI document file, relative to some root where -/// similar documents are found +/// A lockstep API spec filename. +/// +/// Lockstep APIs have a single OpenAPI document with no versioning. The +/// filename is simply `{ident}.json`. #[derive(Clone, Debug, Ord, PartialOrd, Eq, PartialEq)] -pub struct ApiSpecFileName { +pub struct LockstepApiSpecFileName { ident: ApiIdent, - kind: ApiSpecFileNameKind, } -impl fmt::Display for ApiSpecFileName { +impl LockstepApiSpecFileName { + /// Creates a new lockstep API spec filename. + pub fn new(ident: ApiIdent) -> Self { + Self { ident } + } + + /// Returns the API identifier. + pub fn ident(&self) -> &ApiIdent { + &self.ident + } + + /// Returns the path of this file relative to the root of the OpenAPI + /// documents. + pub fn path(&self) -> Utf8PathBuf { + Utf8PathBuf::from(self.basename()) + } + + /// Returns the base name of this file path. + pub fn basename(&self) -> String { + format!("{}.json", self.ident) + } +} + +impl fmt::Display for LockstepApiSpecFileName { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.write_str(self.path().as_str()) } } -impl ApiSpecFileName { - // Only used by the OpenAPI manager -- not part of the public API. - #[doc(hidden)] - pub fn new(ident: ApiIdent, kind: ApiSpecFileNameKind) -> ApiSpecFileName { - ApiSpecFileName { ident, kind } +/// A versioned API spec filename. +/// +/// Versioned APIs can have multiple versions coexisting. The filename includes +/// the version and a content hash: `{ident}/{ident}-{version}-{hash}.json` (or +/// `.json.gitref` for git ref storage). +#[derive(Clone, Debug, Ord, PartialOrd, Eq, PartialEq)] +pub struct VersionedApiSpecFileName { + ident: ApiIdent, + version: semver::Version, + hash: String, + kind: VersionedApiSpecKind, +} + +impl VersionedApiSpecFileName { + /// Creates a new versioned API spec filename (JSON format). + pub fn new( + ident: ApiIdent, + version: semver::Version, + hash: String, + ) -> Self { + Self { ident, version, hash, kind: VersionedApiSpecKind::Json } + } + + /// Creates a new versioned API spec filename (git ref format). + pub fn new_git_ref( + ident: ApiIdent, + version: semver::Version, + hash: String, + ) -> Self { + Self { ident, version, hash, kind: VersionedApiSpecKind::GitRef } } + /// Returns the API identifier. pub fn ident(&self) -> &ApiIdent { &self.ident } - pub fn kind(&self) -> &ApiSpecFileNameKind { - &self.kind + /// Returns the version. + pub fn version(&self) -> &semver::Version { + &self.version + } + + /// Returns the hash. + pub fn hash(&self) -> &str { + &self.hash + } + + /// Returns the storage kind (JSON or git ref). + pub fn kind(&self) -> VersionedApiSpecKind { + self.kind + } + + /// Returns true if this is a git ref file. + pub fn is_git_ref(&self) -> bool { + self.kind == VersionedApiSpecKind::GitRef } /// Returns the path of this file relative to the root of the OpenAPI /// documents. pub fn path(&self) -> Utf8PathBuf { - match &self.kind { - ApiSpecFileNameKind::Lockstep => { - Utf8PathBuf::from_iter([self.basename()]) + Utf8PathBuf::from_iter([self.ident.deref().clone(), self.basename()]) + } + + /// Returns the base name of this file path. + pub fn basename(&self) -> String { + match self.kind { + VersionedApiSpecKind::Json => { + format!("{}-{}-{}.json", self.ident, self.version, self.hash) } - ApiSpecFileNameKind::Versioned { .. } - | ApiSpecFileNameKind::VersionedGitRef { .. } => { - Utf8PathBuf::from_iter([ - self.ident.deref().clone(), - self.basename(), - ]) + VersionedApiSpecKind::GitRef => { + format!( + "{}-{}-{}.json.gitref", + self.ident, self.version, self.hash + ) } } } + /// Converts this filename to its JSON equivalent. + /// + /// If already JSON, returns a clone of self. + pub fn to_json(&self) -> Self { + Self { + ident: self.ident.clone(), + version: self.version.clone(), + hash: self.hash.clone(), + kind: VersionedApiSpecKind::Json, + } + } + + /// Converts this filename to its git ref equivalent. + /// + /// If already a git ref, returns a clone of self. + pub fn to_git_ref(&self) -> Self { + Self { + ident: self.ident.clone(), + version: self.version.clone(), + hash: self.hash.clone(), + kind: VersionedApiSpecKind::GitRef, + } + } + + /// Returns the basename as a git ref filename. + /// + /// - If already a git ref, returns `basename()` directly. + /// - If JSON, returns `basename() + ".gitref"`. + pub fn git_ref_basename(&self) -> String { + match self.kind { + VersionedApiSpecKind::GitRef => self.basename(), + VersionedApiSpecKind::Json => format!("{}.gitref", self.basename()), + } + } + + /// Returns the basename as a JSON filename. + /// + /// - If already JSON, returns `basename()` directly. + /// - If git ref, returns the basename without `.gitref`. + pub fn json_basename(&self) -> String { + match self.kind { + VersionedApiSpecKind::Json => self.basename(), + VersionedApiSpecKind::GitRef => { + format!("{}-{}-{}.json", self.ident, self.version, self.hash) + } + } + } +} + +impl fmt::Display for VersionedApiSpecFileName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.path().as_str()) + } +} + +/// Describes how a versioned API spec file is stored. +#[derive(Clone, Copy, Debug, Ord, PartialOrd, Eq, PartialEq)] +pub enum VersionedApiSpecKind { + /// The spec is stored as a JSON file containing the full OpenAPI document. + Json, + /// The spec is stored as a git ref file. + /// + /// Instead of storing the full JSON content, a `.gitref` file contains a + /// reference in the format `commit:path` that can be used to retrieve the + /// content via `git show`. + GitRef, +} + +/// Describes the path to an OpenAPI document file, relative to some root where +/// similar documents are found. +#[derive(Clone, Debug, Ord, PartialOrd, Eq, PartialEq)] +pub enum ApiSpecFileName { + /// A lockstep API: single OpenAPI document, no versioning. + Lockstep(LockstepApiSpecFileName), + /// A versioned API: multiple versions can coexist. + Versioned(VersionedApiSpecFileName), +} + +impl fmt::Display for ApiSpecFileName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.path().as_str()) + } +} + +impl ApiSpecFileName { + /// Returns the API identifier. + pub fn ident(&self) -> &ApiIdent { + match self { + ApiSpecFileName::Lockstep(l) => l.ident(), + ApiSpecFileName::Versioned(v) => v.ident(), + } + } + + /// Returns the path of this file relative to the root of the OpenAPI + /// documents. + pub fn path(&self) -> Utf8PathBuf { + match self { + ApiSpecFileName::Lockstep(l) => l.path(), + ApiSpecFileName::Versioned(v) => v.path(), + } + } + /// Returns the base name of this file path. pub fn basename(&self) -> String { - match &self.kind { - ApiSpecFileNameKind::Lockstep => format!("{}.json", self.ident), - ApiSpecFileNameKind::Versioned { version, hash } => { - format!("{}-{}-{}.json", self.ident, version, hash) - } - ApiSpecFileNameKind::VersionedGitRef { version, hash } => { - format!("{}-{}-{}.json.gitref", self.ident, version, hash) - } + match self { + ApiSpecFileName::Lockstep(l) => l.basename(), + ApiSpecFileName::Versioned(v) => v.basename(), } } /// For versioned APIs, returns the version part of the filename. pub fn version(&self) -> Option<&semver::Version> { - match &self.kind { - ApiSpecFileNameKind::Lockstep => None, - ApiSpecFileNameKind::Versioned { version, .. } - | ApiSpecFileNameKind::VersionedGitRef { version, .. } => { - Some(version) - } + match self { + ApiSpecFileName::Lockstep(_) => None, + ApiSpecFileName::Versioned(v) => Some(v.version()), } } /// For versioned APIs, returns the hash part of the filename. pub fn hash(&self) -> Option<&str> { - match &self.kind { - ApiSpecFileNameKind::Lockstep => None, - ApiSpecFileNameKind::Versioned { hash, .. } - | ApiSpecFileNameKind::VersionedGitRef { hash, .. } => Some(hash), + match self { + ApiSpecFileName::Lockstep(_) => None, + ApiSpecFileName::Versioned(v) => Some(v.hash()), } } /// Returns true if this is a git ref file. pub fn is_git_ref(&self) -> bool { - matches!(self.kind, ApiSpecFileNameKind::VersionedGitRef { .. }) + match self { + ApiSpecFileName::Lockstep(_) => false, + ApiSpecFileName::Versioned(v) => v.is_git_ref(), + } } - /// Converts a `VersionedGitRef` to its `Versioned` equivalent. + /// For versioned APIs, returns the kind of storage. + pub fn versioned_kind(&self) -> Option { + match self { + ApiSpecFileName::Lockstep(_) => None, + ApiSpecFileName::Versioned(v) => Some(v.kind()), + } + } + + /// Converts a git ref filename to its JSON equivalent. /// /// For non-git ref files, returns a clone of self. pub fn to_json_filename(&self) -> ApiSpecFileName { - match &self.kind { - ApiSpecFileNameKind::VersionedGitRef { version, hash } => { - ApiSpecFileName::new( - self.ident.clone(), - ApiSpecFileNameKind::Versioned { - version: version.clone(), - hash: hash.clone(), - }, - ) + match self { + ApiSpecFileName::Lockstep(_) => self.clone(), + ApiSpecFileName::Versioned(v) => { + ApiSpecFileName::Versioned(v.to_json()) } - _ => self.clone(), } } - /// Converts a `Versioned` to its `VersionedGitRef` equivalent. + /// Converts a JSON filename to its git ref equivalent. /// - /// For `VersionedGitRef` files, returns a clone of self. - /// For `Lockstep` files, returns a clone of self (lockstep files are not + /// For git ref files, returns a clone of self. + /// For lockstep files, returns a clone of self (lockstep files are not /// converted to git refs). pub fn to_git_ref_filename(&self) -> ApiSpecFileName { - match &self.kind { - ApiSpecFileNameKind::Versioned { version, hash } => { - ApiSpecFileName::new( - self.ident.clone(), - ApiSpecFileNameKind::VersionedGitRef { - version: version.clone(), - hash: hash.clone(), - }, - ) + match self { + ApiSpecFileName::Lockstep(_) => self.clone(), + ApiSpecFileName::Versioned(v) => { + ApiSpecFileName::Versioned(v.to_git_ref()) } - _ => self.clone(), } } /// Returns the basename for this file as a git ref. /// - /// - If this is already a `VersionedGitRef`, returns `basename()` directly. - /// - If this is a `Versioned`, returns `basename() + ".gitref"`. - /// - For `Lockstep`, returns `basename()` (lockstep files are not converted + /// - If this is already a git ref, returns `basename()` directly. + /// - If this is a versioned JSON file, returns `basename() + ".gitref"`. + /// - For lockstep, returns `basename()` (lockstep files are not converted /// to git refs). pub fn git_ref_basename(&self) -> String { - match &self.kind { - ApiSpecFileNameKind::VersionedGitRef { .. } => self.basename(), - ApiSpecFileNameKind::Versioned { .. } => { - format!("{}.gitref", self.basename()) - } - ApiSpecFileNameKind::Lockstep => self.basename(), + match self { + ApiSpecFileName::Lockstep(l) => l.basename(), + ApiSpecFileName::Versioned(v) => v.git_ref_basename(), } } /// Returns the basename for this file as a JSON file. /// - /// - If this is a `VersionedGitRef`, returns the basename without the - /// `.gitref` suffix. + /// - If this is a git ref, returns the basename without the `.gitref` + /// suffix. /// - Otherwise, returns `basename()` directly. pub fn json_basename(&self) -> String { - match &self.kind { - ApiSpecFileNameKind::Versioned { .. } => self.basename(), - ApiSpecFileNameKind::VersionedGitRef { version, hash } => { - format!("{}-{}-{}.json", self.ident, version, hash) - } - ApiSpecFileNameKind::Lockstep => self.basename(), + match self { + ApiSpecFileName::Lockstep(l) => l.basename(), + ApiSpecFileName::Versioned(v) => v.json_basename(), + } + } + + /// Returns a reference to the inner `VersionedApiSpecFileName` if this is + /// a versioned API, or `None` if this is a lockstep API. + pub fn as_versioned(&self) -> Option<&VersionedApiSpecFileName> { + match self { + ApiSpecFileName::Lockstep(_) => None, + ApiSpecFileName::Versioned(v) => Some(v), + } + } + + /// Consumes `self` and returns the inner `VersionedApiSpecFileName` if + /// this is a versioned API, or `None` if this is a lockstep API. + pub fn into_versioned(self) -> Option { + match self { + ApiSpecFileName::Lockstep(_) => None, + ApiSpecFileName::Versioned(v) => Some(v), + } + } + + /// Returns a reference to the inner `LockstepApiSpecFileName` if this is + /// a lockstep API, or `None` if this is a versioned API. + pub fn as_lockstep(&self) -> Option<&LockstepApiSpecFileName> { + match self { + ApiSpecFileName::Lockstep(l) => Some(l), + ApiSpecFileName::Versioned(_) => None, + } + } + + /// Consumes `self` and returns the inner `LockstepApiSpecFileName` if + /// this is a lockstep API, or `None` if this is a versioned API. + pub fn into_lockstep(self) -> Option { + match self { + ApiSpecFileName::Lockstep(l) => Some(l), + ApiSpecFileName::Versioned(_) => None, } } } -/// Describes how a particular OpenAPI document is named. -#[derive(Clone, Debug, Ord, PartialOrd, Eq, PartialEq)] -pub enum ApiSpecFileNameKind { - /// The file's path implies a lockstep API. - Lockstep, - /// The file's path implies a versioned API. - Versioned { - /// The version of the API this document describes. - version: semver::Version, - /// The hash of the file contents. - hash: String, - }, - /// The file's path implies a versioned API stored as a git ref. - /// - /// Instead of storing the full JSON content, a `.gitref` file contains a - /// reference in the format `commit:path` that can be used to retrieve the - /// content via `git show`. - VersionedGitRef { - /// The version of the API this document describes. - version: semver::Version, - /// The hash of the file contents (from the original file). - hash: String, - }, +impl From for ApiSpecFileName { + fn from(l: LockstepApiSpecFileName) -> Self { + ApiSpecFileName::Lockstep(l) + } +} + +impl From for ApiSpecFileName { + fn from(v: VersionedApiSpecFileName) -> Self { + ApiSpecFileName::Versioned(v) + } } /// Newtype for API identifiers diff --git a/crates/dropshot-api-manager/src/resolved.rs b/crates/dropshot-api-manager/src/resolved.rs index c04f5a0..cd8268e 100644 --- a/crates/dropshot-api-manager/src/resolved.rs +++ b/crates/dropshot-api-manager/src/resolved.rs @@ -19,7 +19,9 @@ use crate::{ }; use anyhow::{Context, anyhow}; use camino::{Utf8Path, Utf8PathBuf}; -use dropshot_api_manager_types::{ApiIdent, ApiSpecFileName}; +use dropshot_api_manager_types::{ + ApiIdent, ApiSpecFileName, VersionedApiSpecFileName, +}; use std::{ collections::{BTreeMap, BTreeSet, HashSet}, fmt::{Debug, Display}, @@ -143,7 +145,7 @@ pub enum Problem<'a> { merged with upstream and had to change your local version number. \ In either case, this tool can remove the unused file for you." )] - LocalSpecFileOrphaned { spec_file_name: ApiSpecFileName }, + LocalSpecFileOrphaned { spec_file_name: VersionedApiSpecFileName }, #[error( "A local OpenAPI document could not be parsed: {}. \ @@ -163,7 +165,7 @@ pub enum Problem<'a> { versions in Rust. If you didn't, restore the file from git: \ {spec_file_name}" )] - BlessedVersionMissingLocal { spec_file_name: ApiSpecFileName }, + BlessedVersionMissingLocal { spec_file_name: VersionedApiSpecFileName }, #[error( "For this blessed version, found an extra OpenAPI document that does \ @@ -174,7 +176,7 @@ pub enum Problem<'a> { number (when you merged the list of supported versions in Rust) and \ this file is vestigial. This tool can remove the unused file for you." )] - BlessedVersionExtraLocalSpec { spec_file_name: ApiSpecFileName }, + BlessedVersionExtraLocalSpec { spec_file_name: VersionedApiSpecFileName }, #[error( "error comparing OpenAPI document generated from current code with \ @@ -231,7 +233,9 @@ pub enum Problem<'a> { "Extra (incorrect) OpenAPI documents were found for locally-added \ version: {spec_file_names}. This tool can remove the files for you." )] - LocalVersionExtra { spec_file_names: DisplayableVec }, + LocalVersionExtra { + spec_file_names: DisplayableVec, + }, #[error( "For this locally-added version, the OpenAPI document generated \ @@ -273,7 +277,10 @@ pub enum Problem<'a> { }, #[error("\"Latest\" symlink for versioned API {api_ident:?} is missing")] - LatestLinkMissing { api_ident: ApiIdent, link: &'a ApiSpecFileName }, + LatestLinkMissing { + api_ident: ApiIdent, + link: &'a VersionedApiSpecFileName, + }, #[error( "\"Latest\" symlink for versioned API {api_ident:?} is stale: points \ @@ -283,8 +290,8 @@ pub enum Problem<'a> { )] LatestLinkStale { api_ident: ApiIdent, - found: &'a ApiSpecFileName, - link: &'a ApiSpecFileName, + found: &'a VersionedApiSpecFileName, + link: &'a VersionedApiSpecFileName, }, #[error( @@ -332,7 +339,7 @@ pub enum Problem<'a> { // ". )] GitRefFirstCommitUnknown { - spec_file_name: ApiSpecFileName, + spec_file_name: VersionedApiSpecFileName, #[source] source: anyhow::Error, }, @@ -347,13 +354,13 @@ impl<'a> Problem<'a> { match self { Problem::LocalSpecFileOrphaned { spec_file_name } => { Some(Fix::DeleteFiles { - files: DisplayableVec(vec![spec_file_name.clone()]), + files: DisplayableVec(vec![spec_file_name.clone().into()]), }) } Problem::BlessedVersionMissingLocal { .. } => None, Problem::BlessedVersionExtraLocalSpec { spec_file_name } => { Some(Fix::DeleteFiles { - files: DisplayableVec(vec![spec_file_name.clone()]), + files: DisplayableVec(vec![spec_file_name.clone().into()]), }) } Problem::BlessedVersionCompareError { .. } => None, @@ -370,7 +377,16 @@ impl<'a> Problem<'a> { }) } Problem::LocalVersionExtra { spec_file_names } => { - Some(Fix::DeleteFiles { files: spec_file_names.clone() }) + Some(Fix::DeleteFiles { + files: DisplayableVec( + spec_file_names + .0 + .iter() + .cloned() + .map(Into::into) + .collect(), + ), + }) } Problem::LocalVersionStale { spec_files, generated } => { Some(Fix::UpdateVersionedFiles { @@ -437,7 +453,7 @@ pub enum Fix<'a> { }, UpdateSymlink { api_ident: &'a ApiIdent, - link: &'a ApiSpecFileName, + link: &'a VersionedApiSpecFileName, }, /// Convert a full JSON file to a git ref file. ConvertToGitRef { @@ -504,7 +520,7 @@ impl Display for Fix<'_> { writeln!( f, "update symlink to point to {}", - link.to_json_filename().basename() + link.json_basename() )?; } Fix::ConvertToGitRef { local_file, .. } => { @@ -651,7 +667,7 @@ impl Fix<'_> { // same directory so that it's correct no matter where it's // resolved from. If the link target is a gitref, convert it to // the JSON filename (the symlink should always point to JSON). - let target = link.to_json_filename().basename(); + let target = link.json_basename(); match fs_err::remove_file(&path) { Ok(_) => (), Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} @@ -955,7 +971,9 @@ fn resolve_orphaned_local_specs<'a>( BTreeSet<&'a semver::Version>, >, local: &'a LocalFiles, -) -> impl Iterator + 'a { +) -> impl Iterator + 'a { + // Orphaned specs are always versioned: lockstep APIs have exactly one file, + // so orphans can't exist for them. local.iter().flat_map(|(ident, api_files)| { let set = supported_versions_by_api.get(ident); api_files @@ -963,7 +981,11 @@ fn resolve_orphaned_local_specs<'a>( .iter() .filter_map(move |(version, files)| match set { Some(set) if !set.contains(version) => { - Some(files.iter().map(|f| f.spec_file_name())) + Some(files.iter().map(|f| { + f.spec_file_name() + .as_versioned() + .expect("orphaned specs are versioned") + })) } _ => None, }) @@ -1019,7 +1041,7 @@ fn resolve_api<'a>( let blessed_file = api_blessed .and_then(|b| b.versions().get(latest_version)); let spec_file_name = blessed_file - .map(|f| f.spec_file_name().clone()); + .map(|f| f.versioned_spec_file_name().clone()); ( LatestFirstCommit::BlessedError, Some((spec_file_name, error)), @@ -1071,14 +1093,12 @@ fn resolve_api<'a>( // If there was an error computing the first commit for the latest // version, add the error to the latest version's resolution. - if let Some((spec_file_name, error)) = latest_first_commit_error { + if let Some((Some(spec_file_name), error)) = latest_first_commit_error { if let Some(resolution) = by_version.get_mut(latest_version) { - if let Some(spec_file_name) = spec_file_name { - resolution.add_problem(Problem::GitRefFirstCommitUnknown { - spec_file_name, - source: error, - }); - } + resolution.add_problem(Problem::GitRefFirstCommitUnknown { + spec_file_name, + source: error, + }); } } @@ -1086,8 +1106,7 @@ fn resolve_api<'a>( let latest_generated = api_generated.latest_link().expect( "\"generated\" source should always have a \"latest\" link", ); - let generated_version = - latest_generated.version().expect("versioned APIs have a version"); + let generated_version = latest_generated.version(); let resolution = by_version.get(generated_version).unwrap_or_else(|| { panic!( @@ -1130,9 +1149,7 @@ fn resolve_api<'a>( // // 4. latest_local is not blessed. In that case, we do // want to update the symlink. - let local_version = latest_local - .version() - .expect("versioned APIs have a version"); + let local_version = latest_local.version(); match resolution.kind() { ResolutionKind::Lockstep => { unreachable!("this is a versioned API"); @@ -1175,7 +1192,7 @@ fn resolve_api<'a>( }); Some(Problem::LatestLinkStale { api_ident: api.ident().clone(), - link: blessed.spec_file_name(), + link: blessed.versioned_spec_file_name(), found: latest_local, }) } @@ -1219,7 +1236,7 @@ fn resolve_api<'a>( }); Some(Problem::LatestLinkMissing { api_ident: api.ident().clone(), - link: blessed.spec_file_name(), + link: blessed.versioned_spec_file_name(), }) } ResolutionKind::NewLocally => { @@ -1465,7 +1482,9 @@ fn resolve_api_version_blessed<'a>( } Err(error) => { problems.push(Problem::GitRefFirstCommitUnknown { - spec_file_name: blessed.spec_file_name().clone(), + spec_file_name: blessed + .versioned_spec_file_name() + .clone(), source: error, }); VersionStorageFormat::Error @@ -1478,7 +1497,7 @@ fn resolve_api_version_blessed<'a>( if matching.is_empty() && corrupted.is_empty() { // No valid or corrupted local files match the blessed version. problems.push(Problem::BlessedVersionMissingLocal { - spec_file_name: blessed.spec_file_name().clone(), + spec_file_name: blessed.versioned_spec_file_name().clone(), }); } else if !use_git_ref_storage || is_latest { // Fast path: git ref storage disabled or this is the latest version. @@ -1516,7 +1535,11 @@ fn resolve_api_version_blessed<'a>( problems.extend(non_matching.into_iter().map(|s| { Problem::BlessedVersionExtraLocalSpec { - spec_file_name: s.spec_file_name().clone(), + spec_file_name: s + .spec_file_name() + .as_versioned() + .expect("blessed extra spec is versioned") + .clone(), } })); } else { @@ -1594,7 +1617,11 @@ fn resolve_api_version_blessed<'a>( problems.extend(non_matching.into_iter().map(|s| { Problem::BlessedVersionExtraLocalSpec { - spec_file_name: s.spec_file_name().clone(), + spec_file_name: s + .spec_file_name() + .as_versioned() + .expect("blessed extra spec is versioned") + .clone(), } })); } @@ -1635,7 +1662,15 @@ fn resolve_api_version_local<'a>( // There was a matching spec, but also some non-matching ones. // These are superfluous. (It's not clear how this could happen.) let spec_file_names = DisplayableVec( - non_matching.iter().map(|s| s.spec_file_name().clone()).collect(), + non_matching + .iter() + .map(|s| { + s.spec_file_name() + .as_versioned() + .expect("local specs in versioned API are versioned") + .clone() + }) + .collect(), ); problems.push(Problem::LocalVersionExtra { spec_file_names }); } diff --git a/crates/dropshot-api-manager/src/spec_files_blessed.rs b/crates/dropshot-api-manager/src/spec_files_blessed.rs index a0a6c94..b563573 100644 --- a/crates/dropshot-api-manager/src/spec_files_blessed.rs +++ b/crates/dropshot-api-manager/src/spec_files_blessed.rs @@ -17,7 +17,9 @@ use crate::{ }; use anyhow::{anyhow, bail}; use camino::{Utf8Path, Utf8PathBuf}; -use dropshot_api_manager_types::{ApiIdent, ApiSpecFileName}; +use dropshot_api_manager_types::{ + ApiIdent, ApiSpecFileName, VersionedApiSpecFileName, +}; use std::{collections::BTreeMap, ops::Deref}; /// Newtype wrapper around [`ApiSpecFile`] to describe OpenAPI documents from @@ -26,14 +28,62 @@ use std::{collections::BTreeMap, ops::Deref}; /// The blessed source contains the documents that are not allowed to be changed /// locally because they've been committed-to upstream. /// -/// Note that this type can represent documents for both lockstep APIs and -/// versioned APIs, but it's meaningless for lockstep APIs. Any documents for -/// versioned APIs are blessed by definition. -pub struct BlessedApiSpecFile(ApiSpecFile); -NewtypeDebug! { () pub struct BlessedApiSpecFile(ApiSpecFile); } -NewtypeDeref! { () pub struct BlessedApiSpecFile(ApiSpecFile); } -NewtypeDerefMut! { () pub struct BlessedApiSpecFile(ApiSpecFile); } -NewtypeFrom! { () pub struct BlessedApiSpecFile(ApiSpecFile); } +/// This type only represents versioned APIs, not lockstep APIs. Lockstep APIs +/// don't have a meaningful "blessed" source since they're always regenerated. +/// The type system enforces this invariant: construction will panic if given a +/// lockstep spec. +pub struct BlessedApiSpecFile { + inner: ApiSpecFile, + /// Cached versioned filename, avoiding repeated conversion. + versioned_name: VersionedApiSpecFileName, +} + +impl std::fmt::Debug for BlessedApiSpecFile { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("BlessedApiSpecFile") + .field("inner", &self.inner) + .finish() + } +} + +impl Deref for BlessedApiSpecFile { + type Target = ApiSpecFile; + fn deref(&self) -> &Self::Target { + &self.inner + } +} + +impl BlessedApiSpecFile { + /// Creates a new `BlessedApiSpecFile` from an `ApiSpecFile`. + /// + /// # Panics + /// + /// Panics if the spec file is for a lockstep API. Blessed files only exist + /// for versioned APIs. + pub fn new(inner: ApiSpecFile) -> Self { + let versioned_name = inner + .spec_file_name() + .as_versioned() + .unwrap_or_else(|| { + panic!( + "BlessedApiSpecFile requires a versioned API spec, \ + got lockstep: {}", + inner.spec_file_name() + ) + }) + .clone(); + Self { inner, versioned_name } + } + + /// Returns the versioned spec file name. + /// + /// Unlike `spec_file_name()` which returns `&ApiSpecFileName`, this method + /// returns the more specific `&VersionedApiSpecFileName` since blessed + /// files are always versioned. + pub fn versioned_spec_file_name(&self) -> &VersionedApiSpecFileName { + &self.versioned_name + } +} // Trait impls that allow us to use `ApiFiles` // @@ -45,7 +95,7 @@ impl ApiLoad for BlessedApiSpecFile { type Unparseable = std::convert::Infallible; fn make_item(raw: ApiSpecFile) -> Self { - BlessedApiSpecFile(raw) + BlessedApiSpecFile::new(raw) } fn try_extend(&mut self, item: ApiSpecFile) -> anyhow::Result<()> { @@ -135,7 +185,8 @@ impl BlessedGitRef { /// any API-level validation. enum BlessedPathKind<'a> { /// Single-component path (e.g., "api.json"). Potential lockstep file. - Lockstep { basename: &'a str }, + /// These are skipped since blessed files only exist for versioned APIs. + Lockstep, /// Two-component path with `.json.gitref` extension. Potential versioned /// git ref file. @@ -154,7 +205,7 @@ impl<'a> BlessedPathKind<'a> { fn parse(path: &'a Utf8Path) -> Result { let parts: Vec<_> = path.iter().collect(); match parts.as_slice() { - [basename] => Ok(BlessedPathKind::Lockstep { basename }), + [_basename] => Ok(BlessedPathKind::Lockstep), [api_dir, basename] if basename.ends_with(".json.gitref") => { Ok(BlessedPathKind::GitRefFile { api_dir, basename }) } @@ -269,19 +320,20 @@ impl BlessedFiles { } }; + // Lockstep files are not loaded from blessed sources. They're + // always regenerated from the current code, so there's no + // "blessed" version to compare against. + if matches!(kind, BlessedPathKind::Lockstep) { + continue; + } + // Read the contents. Use "/" rather than "\" on Windows. let git_path = format!("{directory}/{f}"); let contents = git_show_file(repo_root, commit, git_path.as_ref())?; match kind { - BlessedPathKind::Lockstep { basename } => { - if let Some(spec_file_name) = - api_files.lockstep_file_name(basename) - { - api_files.load_contents(spec_file_name, contents); - // Lockstep files don't need git refs since they're - // always regenerated. - } + BlessedPathKind::Lockstep => { + unreachable!("handled above"); } BlessedPathKind::VersionedFile { api_dir, basename } => { diff --git a/crates/dropshot-api-manager/src/spec_files_generated.rs b/crates/dropshot-api-manager/src/spec_files_generated.rs index 12270fb..defa22a 100644 --- a/crates/dropshot-api-manager/src/spec_files_generated.rs +++ b/crates/dropshot-api-manager/src/spec_files_generated.rs @@ -13,7 +13,8 @@ use crate::{ }; use anyhow::{anyhow, bail}; use dropshot_api_manager_types::{ - ApiIdent, ApiSpecFileName, ApiSpecFileNameKind, + ApiIdent, ApiSpecFileName, LockstepApiSpecFileName, + VersionedApiSpecFileName, }; use std::{collections::BTreeMap, ops::Deref}; @@ -111,11 +112,10 @@ impl GeneratedFiles { ))) } Ok(contents) => { - let file_name = ApiSpecFileName::new( + let file_name = LockstepApiSpecFileName::new( api.ident().clone(), - ApiSpecFileNameKind::Lockstep, ); - api_files.load_contents(file_name, contents); + api_files.load_contents(file_name.into(), contents); } } } @@ -137,15 +137,13 @@ impl GeneratedFiles { ))) } Ok(contents) => { - let file_name = ApiSpecFileName::new( + let file_name = VersionedApiSpecFileName::new( api.ident().clone(), - ApiSpecFileNameKind::Versioned { - version: version.clone(), - hash: hash_contents(&contents), - }, + version.clone(), + hash_contents(&contents), ); latest = Some(file_name.clone()); - api_files.load_contents(file_name, contents); + api_files.load_contents(file_name.into(), contents); } } } diff --git a/crates/dropshot-api-manager/src/spec_files_generic.rs b/crates/dropshot-api-manager/src/spec_files_generic.rs index 1fdccfa..73b3f23 100644 --- a/crates/dropshot-api-manager/src/spec_files_generic.rs +++ b/crates/dropshot-api-manager/src/spec_files_generic.rs @@ -8,7 +8,8 @@ use anyhow::anyhow; use camino::{Utf8Path, Utf8PathBuf}; use debug_ignore::DebugIgnore; use dropshot_api_manager_types::{ - ApiIdent, ApiSpecFileName, ApiSpecFileNameKind, + ApiIdent, ApiSpecFileName, LockstepApiSpecFileName, + VersionedApiSpecFileName, VersionedApiSpecKind, }; use openapiv3::OpenAPI; use sha2::{Digest, Sha256}; @@ -30,15 +31,14 @@ pub struct UnparseableFile { pub path: Utf8PathBuf, } -/// Attempts to parse the given file basename as an ApiSpecFileName of kind -/// `Versioned` +/// Attempts to parse the given file basename as a `VersionedApiSpecFileName`. /// /// These look like: `ident-SEMVER-HASH.json`. fn parse_versioned_file_name( apis: &ManagedApis, ident: &str, basename: &str, -) -> Result { +) -> Result { let ident = ApiIdent::from(ident.to_string()); let Some(api) = apis.api(&ident) else { return Err(BadVersionedFileName::NoSuchApi); @@ -105,21 +105,18 @@ fn parse_versioned_file_name( }); } - Ok(ApiSpecFileName::new( - ident, - ApiSpecFileNameKind::Versioned { version, hash: hash.to_string() }, - )) + Ok(VersionedApiSpecFileName::new(ident, version, hash.to_string())) } -/// Attempts to parse the given file basename as an ApiSpecFileName of kind -/// `VersionedGitRef`. +/// Attempts to parse the given file basename as a `VersionedApiSpecFileName` +/// with git ref storage. /// /// These look like: `ident-SEMVER-HASH.json.gitref`. fn parse_versioned_git_ref_file_name( apis: &ManagedApis, ident: &str, basename: &str, -) -> Result { +) -> Result { // The file name must end with .json.gitref. let json_basename = basename.strip_suffix(".gitref").ok_or_else(|| { BadVersionedFileName::UnexpectedName { @@ -131,28 +128,15 @@ fn parse_versioned_git_ref_file_name( // Parse the underlying versioned name to get the version and hash. let versioned = parse_versioned_file_name(apis, ident, json_basename)?; - match versioned.kind() { - ApiSpecFileNameKind::Versioned { version, hash } => { - Ok(ApiSpecFileName::new( - versioned.ident().clone(), - ApiSpecFileNameKind::VersionedGitRef { - version: version.clone(), - hash: hash.clone(), - }, - )) - } - other => unreachable!( - "parse_versioned_file_name always returns Versioned, found {other:?}" - ), - } + // Convert to git ref format. + Ok(versioned.to_git_ref()) } -/// Attempts to parse the given file basename as an ApiSpecFileName of kind -/// `Lockstep` +/// Attempts to parse the given file basename as a `LockstepApiSpecFileName`. fn parse_lockstep_file_name( apis: &ManagedApis, basename: &str, -) -> Result { +) -> Result { let ident = ApiIdent::from( basename .strip_suffix(".json") @@ -166,7 +150,7 @@ fn parse_lockstep_file_name( return Err(BadLockstepFileName::NotLockstep); } - Ok(ApiSpecFileName::new(ident, ApiSpecFileNameKind::Lockstep)) + Ok(LockstepApiSpecFileName::new(ident)) } /// Describes a failure to parse a file name for a lockstep API @@ -298,9 +282,9 @@ impl ApiSpecFile { } }; - match spec_file_name.kind() { - ApiSpecFileNameKind::Versioned { version, hash } => { - if *version != parsed_version { + match &spec_file_name { + ApiSpecFileName::Versioned(v) => { + if *v.version() != parsed_version { return Err(( ApiSpecFileParseError::VersionMismatch { path: spec_file_name.path().to_owned(), @@ -310,33 +294,23 @@ impl ApiSpecFile { )); } - let expected_hash = hash_contents(&contents_buf); - if expected_hash != *hash { - return Err(( - ApiSpecFileParseError::HashMismatch { - path: spec_file_name.path().to_owned(), - expected: expected_hash, - actual: hash.clone(), - }, - contents_buf, - )); - } - } - ApiSpecFileNameKind::VersionedGitRef { version, .. } => { - // Git ref files: validate that the version matches, but skip - // hash check. The content came from git, so the git ref itself - // is the source of truth. - if *version != parsed_version { - return Err(( - ApiSpecFileParseError::VersionMismatch { - path: spec_file_name.path().to_owned(), - file_version: parsed_version, - }, - contents_buf, - )); + // Only check hash for JSON files. Git ref files use the git ref + // itself as the source of truth. + if v.kind() == VersionedApiSpecKind::Json { + let expected_hash = hash_contents(&contents_buf); + if expected_hash != v.hash() { + return Err(( + ApiSpecFileParseError::HashMismatch { + path: spec_file_name.path().to_owned(), + expected: expected_hash, + actual: v.hash().to_owned(), + }, + contents_buf, + )); + } } } - ApiSpecFileNameKind::Lockstep => {} + ApiSpecFileName::Lockstep(_) => {} } Ok(ApiSpecFile { @@ -516,7 +490,7 @@ impl<'a, T: ApiLoad + AsRawFiles> ApiSpecFilesBuilder<'a, T> { ); None } - Ok(file_name) => Some(file_name), + Ok(file_name) => Some(file_name.into()), } } @@ -568,7 +542,7 @@ impl<'a, T: ApiLoad + AsRawFiles> ApiSpecFilesBuilder<'a, T> { basename: &str, ) -> Option { match parse_versioned_file_name(self.apis, ident, basename) { - Ok(file_name) => Some(file_name), + Ok(file_name) => Some(file_name.into()), Err( warning @ (BadVersionedFileName::NoSuchApi | BadVersionedFileName::NotVersioned), @@ -608,7 +582,7 @@ impl<'a, T: ApiLoad + AsRawFiles> ApiSpecFilesBuilder<'a, T> { basename: &str, ) -> Option { match parse_versioned_git_ref_file_name(self.apis, ident, basename) { - Ok(file_name) => Some(file_name), + Ok(file_name) => Some(file_name.into()), Err( warning @ (BadVersionedFileName::NoSuchApi | BadVersionedFileName::NotVersioned), @@ -643,7 +617,7 @@ impl<'a, T: ApiLoad + AsRawFiles> ApiSpecFilesBuilder<'a, T> { symlink_path: &Utf8Path, ident: &ApiIdent, basename: &str, - ) -> Option { + ) -> Option { match parse_versioned_file_name(self.apis, ident, basename) { Ok(file_name) => Some(file_name), Err( @@ -835,7 +809,7 @@ impl<'a, T: ApiLoad + AsRawFiles> ApiSpecFilesBuilder<'a, T> { pub fn load_latest_link( &mut self, ident: &ApiIdent, - links_to: ApiSpecFileName, + links_to: VersionedApiSpecFileName, ) { let Some(api) = self.apis.api(ident) else { let error = @@ -891,7 +865,7 @@ impl<'a, T: ApiLoad + AsRawFiles> ApiSpecFilesBuilder<'a, T> { #[derive(Debug)] pub struct ApiFiles { spec_files: BTreeMap, - latest_link: Option, + latest_link: Option, /// Files that exist on disk but couldn't be parsed. These are tracked so /// that generate can delete them and create correct files in their place. unparseable_files: Vec, @@ -910,7 +884,7 @@ impl ApiFiles { &self.spec_files } - pub fn latest_link(&self) -> Option<&ApiSpecFileName> { + pub fn latest_link(&self) -> Option<&VersionedApiSpecFileName> { self.latest_link.as_ref() } @@ -1062,10 +1036,7 @@ mod test { let name = parse_lockstep_file_name(&apis, "lockstep.json").unwrap(); assert_eq!( name, - ApiSpecFileName::new( - ApiIdent::from("lockstep".to_owned()), - ApiSpecFileNameKind::Lockstep, - ) + LockstepApiSpecFileName::new(ApiIdent::from("lockstep".to_owned())) ); } @@ -1080,12 +1051,10 @@ mod test { .unwrap(); assert_eq!( name, - ApiSpecFileName::new( + VersionedApiSpecFileName::new( ApiIdent::from("versioned".to_owned()), - ApiSpecFileNameKind::Versioned { - version: Version::new(1, 2, 3), - hash: "feedface".to_owned(), - }, + Version::new(1, 2, 3), + "feedface".to_owned(), ) ); } @@ -1174,12 +1143,10 @@ mod test { .unwrap(); assert_eq!( name, - ApiSpecFileName::new( + VersionedApiSpecFileName::new_git_ref( ApiIdent::from("versioned".to_owned()), - ApiSpecFileNameKind::VersionedGitRef { - version: Version::new(1, 2, 3), - hash: "feedface".to_owned(), - }, + Version::new(1, 2, 3), + "feedface".to_owned(), ) ); } diff --git a/crates/integration-tests/tests/integration/versioned.rs b/crates/integration-tests/tests/integration/versioned.rs index 869d795..609326f 100644 --- a/crates/integration-tests/tests/integration/versioned.rs +++ b/crates/integration-tests/tests/integration/versioned.rs @@ -1,4 +1,4 @@ -// Copyright 2025 Oxide Computer Company +// Copyright 2026 Oxide Computer Company //! Integration tests for versioned APIs in dropshot-api-manager. //! @@ -991,3 +991,71 @@ fn test_extra_validation_with_extra_file() -> Result<()> { Ok(()) } + +/// Test that a malformed "latest" symlink pointing to a non-versioned file is +/// handled gracefully (not with a panic). +/// +/// This simulates a situation where someone accidentally creates a symlink like +/// `versioned-health-latest.json -> versioned-health.json` (a non-versioned +/// target). +#[test] +fn test_malformed_latest_symlink_nonversioned_target() -> Result<()> { + let env = TestEnvironment::new()?; + let apis = versioned_health_apis()?; + + env.generate_documents(&apis)?; + + // Verify the symlink exists and points to a versioned file. + assert!(env.versioned_latest_document_exists("versioned-health")); + let original_target = env + .read_link("documents/versioned-health/versioned-health-latest.json")?; + assert!( + original_target.as_str().contains("-3.0.0-"), + "original symlink should point to v3.0.0 file, got: {}", + original_target + ); + + // Delete the valid symlink and create a malformed one pointing to a + // non-versioned target. + env.delete_versioned_latest_symlink("versioned-health")?; + + let symlink_path = env + .documents_dir() + .join("versioned-health/versioned-health-latest.json"); + + // Create a symlink pointing to a non-versioned file name. The target + // doesn't need to exist: we're testing that the symlink parsing handles + // this gracefully. + #[cfg(unix)] + std::os::unix::fs::symlink("versioned-health.json", &symlink_path) + .context("failed to create malformed symlink")?; + #[cfg(windows)] + std::os::windows::fs::symlink_file("versioned-health.json", &symlink_path) + .context("failed to create malformed symlink")?; + + // The check should not panic. It should report that updates are needed + // (because the "latest" symlink is effectively missing/malformed). + let result = check_apis_up_to_date(env.environment(), &apis)?; + assert_eq!( + result, + CheckResult::NeedsUpdate, + "malformed symlink should be detected as needing update" + ); + + // Generate should fix the symlink. + env.generate_documents(&apis)?; + + let result = check_apis_up_to_date(env.environment(), &apis)?; + assert_eq!(result, CheckResult::Success); + + // The symlink should now point to the correct versioned file. + let new_target = env + .read_link("documents/versioned-health/versioned-health-latest.json")?; + assert!( + new_target.as_str().contains("-3.0.0-"), + "regenerated symlink should point to v3.0.0 file, got: {}", + new_target + ); + + Ok(()) +}