diff --git a/crates/dropshot-api-manager/src/cmd/check.rs b/crates/dropshot-api-manager/src/cmd/check.rs index 6b9c2d7..af9e812 100644 --- a/crates/dropshot-api-manager/src/cmd/check.rs +++ b/crates/dropshot-api-manager/src/cmd/check.rs @@ -30,7 +30,8 @@ pub(crate) fn check_impl( let (local_files, errors) = env.local_source.load(apis, &styles)?; display_load_problems(&errors, &styles)?; - let (blessed, errors) = blessed_source.load(apis, &styles)?; + let (blessed, errors) = + blessed_source.load(&env.repo_root, apis, &styles)?; display_load_problems(&errors, &styles)?; let resolved = Resolved::new(env, apis, &blessed, &generated, &local_files); diff --git a/crates/dropshot-api-manager/src/cmd/debug.rs b/crates/dropshot-api-manager/src/cmd/debug.rs index 89aeffa..d899116 100644 --- a/crates/dropshot-api-manager/src/cmd/debug.rs +++ b/crates/dropshot-api-manager/src/cmd/debug.rs @@ -30,7 +30,8 @@ pub(crate) fn debug_impl( dump_structure(&local_files, &errors); // Print information about what we found in Git. - let (blessed, errors) = blessed_source.load(apis, &styles)?; + let (blessed, errors) = + blessed_source.load(&env.repo_root, apis, &styles)?; dump_structure(&blessed, &errors); // Print information about generated files. diff --git a/crates/dropshot-api-manager/src/cmd/generate.rs b/crates/dropshot-api-manager/src/cmd/generate.rs index d2ac1c0..c47ec78 100644 --- a/crates/dropshot-api-manager/src/cmd/generate.rs +++ b/crates/dropshot-api-manager/src/cmd/generate.rs @@ -49,7 +49,8 @@ pub(crate) fn generate_impl( let (local_files, errors) = env.local_source.load(apis, &styles)?; display_load_problems(&errors, &styles)?; - let (blessed, errors) = blessed_source.load(apis, &styles)?; + let (blessed, errors) = + blessed_source.load(&env.repo_root, apis, &styles)?; display_load_problems(&errors, &styles)?; let resolved = Resolved::new(env, apis, &blessed, &generated, &local_files); diff --git a/crates/dropshot-api-manager/src/environment.rs b/crates/dropshot-api-manager/src/environment.rs index ea8952a..ccc72bb 100644 --- a/crates/dropshot-api-manager/src/environment.rs +++ b/crates/dropshot-api-manager/src/environment.rs @@ -202,6 +202,7 @@ impl BlessedSource { /// Load the blessed OpenAPI documents pub fn load( &self, + repo_root: &Utf8Path, apis: &ManagedApis, styles: &Styles, ) -> anyhow::Result<(BlessedFiles, ErrorAccumulator)> { @@ -227,6 +228,7 @@ impl BlessedSource { ); Ok(( BlessedFiles::load_from_git_parent_branch( + repo_root, revision, directory, apis, diff --git a/crates/dropshot-api-manager/src/git.rs b/crates/dropshot-api-manager/src/git.rs index bf7622b..dc53a91 100644 --- a/crates/dropshot-api-manager/src/git.rs +++ b/crates/dropshot-api-manager/src/git.rs @@ -20,9 +20,10 @@ NewtypeFrom! { () pub struct GitRevision(String); } /// Given a revision, return its merge base with HEAD pub fn git_merge_base_head( + repo_root: &Utf8Path, revision: &GitRevision, ) -> anyhow::Result { - let mut cmd = git_start(); + let mut cmd = git_start(repo_root); cmd.arg("merge-base").arg("--all").arg("HEAD").arg(revision.as_str()); let label = cmd_label(&cmd); let stdout = do_run(&mut cmd)?; @@ -39,10 +40,11 @@ pub fn git_merge_base_head( /// List files recursively under some path `path` in Git revision `revision`. pub fn git_ls_tree( + repo_root: &Utf8Path, revision: &GitRevision, directory: &Utf8Path, ) -> anyhow::Result> { - let mut cmd = git_start(); + let mut cmd = git_start(repo_root); cmd.arg("ls-tree") .arg("-r") .arg("-z") @@ -75,19 +77,22 @@ pub fn git_ls_tree( /// Returns the contents of the file at the given path `path` in Git revision /// `revision`. pub fn git_show_file( + repo_root: &Utf8Path, revision: &GitRevision, path: &Utf8Path, ) -> anyhow::Result> { - let mut cmd = git_start(); + let mut cmd = git_start(repo_root); cmd.arg("cat-file").arg("blob").arg(format!("{}:{}", revision, path)); let stdout = do_run(&mut cmd)?; Ok(stdout.into_bytes()) } /// Begin assembling an invocation of git(1) -fn git_start() -> Command { +fn git_start(repo_root: &Utf8Path) -> Command { let git = std::env::var("GIT").ok().unwrap_or_else(|| String::from("git")); - Command::new(&git) + let mut command = Command::new(&git); + command.current_dir(repo_root); + command } /// Runs an assembled git(1) command, returning stdout on success and an error diff --git a/crates/dropshot-api-manager/src/resolved.rs b/crates/dropshot-api-manager/src/resolved.rs index c1ba57e..2bc7154 100644 --- a/crates/dropshot-api-manager/src/resolved.rs +++ b/crates/dropshot-api-manager/src/resolved.rs @@ -679,37 +679,82 @@ fn resolve_api<'a>( } else { // latest_local is different from latest_generated. // - // We never want to update blessed documents. But - // latest_generated might have wire-compatible changes which - // would cause the hash to change. + // We never want to update the local copies of blessed + // documents. But latest_generated might have + // wire-compatible (trivial) changes which would cause the + // hash to change, so we need to handle this case with care. // - // There are two possibilities: + // The possibilities are: // - // 1. latest_local is blessed and latest_generated has - // wire-compatible changes. In that case, we don't update - // the symlink. - // 2. latest_local is blessed and latest_generated has + // 1. latest_local is blessed, latest_generated has the same + // version as latest_local, and it has wire-compatible + // changes. In that case, don't update the symlink. + // 2. latest_local is blessed, latest_generated has the same + // version as latest_local, and latest_generated has // wire-*incompatible* changes. In that case, we'd have // returned errors in the by_version map above, and we // wouldn't want to update the symlink in any case. - // 3. latest_local is not blessed. In that case, we do + // 3. latest_local is blessed, and latest_generated is + // blessed but a *different* version. This means that + // the latest version was retired. In this case, + // we want to update the symlink to the blessed hash + // corresponding to the latest generated version. + // 4. latest_local is not blessed. In that case, we do // want to update the symlink. - // - // This boils down to simply checking if latest_generated is - // a blessed version. - let version = latest_generated + let generated_version = latest_generated + .version() + .expect("versioned APIs have a version"); + let local_version = latest_local .version() .expect("versioned APIs have a version"); - if let Some(resolution) = by_version.get(version) { + if let Some(resolution) = by_version.get(generated_version) + { match resolution.kind() { ResolutionKind::Lockstep => { unreachable!("this is a versioned API"); } - ResolutionKind::Blessed => { - // latest_generated is blessed, so don't update - // the symlink. + // Case 1 and 2 above. + ResolutionKind::Blessed + if generated_version == local_version => + { + // latest_generated is blessed and the same + // version as latest_local, so don't update the + // symlink. None } + ResolutionKind::Blessed => { + // latest_generated is blessed, and has a + // different version from latest_local. In this + // case, we want to update the symlink to the + // blessed version matching latest_generated + // (not latest_generated, in case it's different + // from the blessed version in a wire-compatible + // way!) + let api_blessed = + api_blessed.unwrap_or_else(|| { + panic!( + "for {}, Blessed means \ + api_blessed exists", + api.ident() + ) + }); + let blessed = api_blessed + .versions() + .get(generated_version) + .unwrap_or_else(|| { + panic!( + "for {} v{}, Blessed means \ + generated_version exists", + api.ident(), + generated_version + ); + }); + Some(Problem::LatestLinkStale { + api_ident: api.ident().clone(), + link: blessed.spec_file_name(), + found: latest_local, + }) + } ResolutionKind::NewLocally => { // latest_generated is not blessed, so update // the symlink. diff --git a/crates/dropshot-api-manager/src/spec_files_blessed.rs b/crates/dropshot-api-manager/src/spec_files_blessed.rs index 4af5ff1..9fc3e95 100644 --- a/crates/dropshot-api-manager/src/spec_files_blessed.rs +++ b/crates/dropshot-api-manager/src/spec_files_blessed.rs @@ -95,13 +95,15 @@ impl BlessedFiles { /// `M1` for blessed documents because you haven't yet merged in commits M2, /// M3, and M4. pub fn load_from_git_parent_branch( + repo_root: &Utf8Path, branch: &GitRevision, directory: &Utf8Path, apis: &ManagedApis, error_accumulator: &mut ErrorAccumulator, ) -> anyhow::Result { - let revision = git_merge_base_head(branch)?; + let revision = git_merge_base_head(repo_root, branch)?; Self::load_from_git_revision( + repo_root, &revision, directory, apis, @@ -111,6 +113,7 @@ impl BlessedFiles { /// Load OpenAPI documents from the given Git revision and directory. pub fn load_from_git_revision( + repo_root: &Utf8Path, commit: &GitRevision, directory: &Utf8Path, apis: &ManagedApis, @@ -118,7 +121,7 @@ impl BlessedFiles { ) -> anyhow::Result { let mut api_files: ApiSpecFilesBuilder = ApiSpecFilesBuilder::new(apis, error_accumulator); - let files_found = git_ls_tree(commit, directory)?; + let files_found = git_ls_tree(repo_root, commit, directory)?; for f in files_found { // We should be looking at either a single-component path // ("api.json") or a file inside one level of directory hierarchy @@ -134,7 +137,8 @@ impl BlessedFiles { // Read the contents. Use "/" rather than "\" on Windows. let file_name = format!("{directory}/{f}"); - let contents = git_show_file(commit, file_name.as_ref())?; + let contents = + git_show_file(repo_root, commit, file_name.as_ref())?; if parts.len() == 1 { if let Some(file_name) = api_files.lockstep_file_name(parts[0]) { diff --git a/crates/integration-tests/src/common/fixtures.rs b/crates/integration-tests/src/common/fixtures.rs index 910fdac..4a09df8 100644 --- a/crates/integration-tests/src/common/fixtures.rs +++ b/crates/integration-tests/src/common/fixtures.rs @@ -451,3 +451,43 @@ pub mod versioned_user { pub permissions: Vec, } } + +/// Reduced versioned health API for testing version removal scenarios. +pub mod versioned_health_reduced { + use super::*; + use dropshot_api_manager_types::api_versions; + + api_versions!([(2, WITH_DETAILED_STATUS), (1, INITIAL),]); + + #[dropshot::api_description] + pub trait VersionedHealthApi { + type Context; + + /// Check if the service is healthy (all versions). + #[endpoint { + method = GET, + path = "/health", + operation_id = "health_check", + versions = "1.0.0".. + }] + async fn health_check( + rqctx: RequestContext, + ) -> Result, HttpError>; + + /// Get detailed health status (v2+). + #[endpoint { + method = GET, + path = "/health/detailed", + operation_id = "detailed_health_check", + versions = "2.0.0".. + }] + async fn detailed_health_check( + rqctx: RequestContext, + ) -> Result, HttpError>; + } + + // Reuse the same response types from the main versioned_health module. + pub use super::versioned_health::{ + DependencyStatus, DetailedHealthStatus, HealthStatusV1, + }; +} diff --git a/crates/integration-tests/src/common/mod.rs b/crates/integration-tests/src/common/mod.rs index 7c8a2a1..141b002 100644 --- a/crates/integration-tests/src/common/mod.rs +++ b/crates/integration-tests/src/common/mod.rs @@ -43,7 +43,10 @@ impl TestEnvironment { let documents_dir = workspace_root.child("documents"); // Initialize git repository in workspace root. - Self::run_git_command(&workspace_root, &["init"])?; + Self::run_git_command( + &workspace_root, + &["init", "--initial-branch", "main"], + )?; Self::run_git_command( &workspace_root, &["config", "user.name", "Test User"], @@ -70,7 +73,11 @@ impl TestEnvironment { "test-openapi-manager", workspace_root.as_path(), "documents", - )?; + )? + // Use "main" rather than the default "origin/main" since we're not + // pushing to an upstream. A commit to main automatically marks the + // document blessed. + .with_default_git_branch("main"); Ok(Self { temp_dir, workspace_root, documents_dir, environment }) } @@ -110,6 +117,15 @@ impl TestEnvironment { .with_context(|| format!("failed to read file: {}", path)) } + pub fn read_link( + &self, + relative_path: impl AsRef, + ) -> Result { + let path = self.workspace_root.join(relative_path); + path.read_link_utf8() + .with_context(|| format!("failed to read link: {}", path)) + } + /// Check if a file exists in the workspace. pub fn file_exists(&self, relative_path: impl AsRef) -> bool { self.workspace_root.join(relative_path.as_ref()).exists() @@ -199,10 +215,13 @@ impl TestEnvironment { &self, api_ident: &str, ) -> Result { - self.read_file(format!( - "documents/{}/{}-latest.json", - api_ident, api_ident - )) + // Try reading the link to ensure it's a symlink. + let file_name = + format!("documents/{}/{}-latest.json", api_ident, api_ident); + let target = self.read_link(&file_name)?; + eprintln!("** symlink target: {}", target); + + self.read_file(&file_name) } /// Commit documents to git (for blessed document workflow testing). @@ -456,3 +475,42 @@ pub fn create_mixed_test_apis() -> Result { ]; ManagedApis::new(configs).context("failed to create mixed ManagedApis") } + +/// Create versioned health API with a trivial change (title/metadata updated). +pub fn create_versioned_health_test_apis_with_trivial_change() +-> Result { + // Create a modified API config that would produce different OpenAPI documents. + let mut config = versioned_health_test_api(); + + // Modify the title to create a different document signature. + config.title = "Modified Versioned Health API"; + config.metadata.description = + Some("A versioned health API with breaking changes"); + + ManagedApis::new(vec![config]) + .context("failed to create trivial change versioned health ManagedApis") +} + +/// Create versioned health API with reduced versions (simulating version removal). +pub fn create_versioned_health_test_apis_reduced_versions() +-> Result { + // Create a configuration similar to versioned health but with fewer versions. + // We'll create a new fixture for this. + let config = ManagedApiConfig { + ident: "versioned-health", + versions: Versions::Versioned { + // Use a subset of versions (only 1.0.0 and 2.0.0, not 3.0.0). + supported_versions: fixtures::versioned_health_reduced::supported_versions(), + }, + title: "Versioned Health API", + metadata: ManagedApiMetadata { + description: Some("A versioned health API with reduced versions"), + ..Default::default() + }, + api_description: fixtures::versioned_health_reduced::versioned_health_api_mod::stub_api_description, + extra_validation: None, + }; + + ManagedApis::new(vec![config]) + .context("failed to create reduced versioned health ManagedApis") +} diff --git a/crates/integration-tests/tests/integration/versioned.rs b/crates/integration-tests/tests/integration/versioned.rs index 4a0475f..d7f5365 100644 --- a/crates/integration-tests/tests/integration/versioned.rs +++ b/crates/integration-tests/tests/integration/versioned.rs @@ -7,6 +7,7 @@ //! and must remain stable across changes. use anyhow::Result; +use dropshot_api_manager::test_util::{CheckResult, check_apis_up_to_date}; use integration_tests::common::*; use openapiv3::OpenAPI; @@ -228,3 +229,206 @@ fn test_git_commit_documents() -> Result<()> { Ok(()) } + +/// Test blessed document lifecycle - generate, commit, then verify check passes. +#[test] +fn test_blessed_document_lifecycle() -> Result<()> { + let env = TestEnvironment::new()?; + let apis = create_versioned_health_test_apis()?; + + // Initially, APIs should fail the up-to-date check (no documents exist). + let result = check_apis_up_to_date(env.environment(), &apis)?; + assert_eq!(result, CheckResult::NeedsUpdate); + + // Generate the documents. + env.generate_documents(&apis)?; + + // After generation, for new APIs, they are considered fresh/up-to-date. + let result = check_apis_up_to_date(env.environment(), &apis)?; + assert_eq!(result, CheckResult::Success); + + // Commit the documents to "bless" them. + env.commit_documents()?; + + // Should still pass after committing. + let result = check_apis_up_to_date(env.environment(), &apis)?; + assert_eq!(result, CheckResult::Success); + + Ok(()) +} + +/// Test that API changes require regeneration and recommitting. +#[test] +fn test_blessed_api_changes_should_not_do_trivial_updates() -> Result<()> { + let env = TestEnvironment::new()?; + let apis = create_versioned_health_test_apis()?; + + // Generate and commit initial documents. + env.generate_documents(&apis)?; + env.commit_documents()?; + + // Verify initial state is up-to-date. + let result = check_apis_up_to_date(env.environment(), &apis)?; + assert_eq!(result, CheckResult::Success); + + // Create a modified API with trivial changes (different title/description). + let modified_apis = + create_versioned_health_test_apis_with_trivial_change()?; + + // The check should indicate that *no updates are needed* to the blessed version. + let result = check_apis_up_to_date(env.environment(), &modified_apis)?; + assert_eq!(result, CheckResult::Success); + + Ok(()) +} + +/// Test multiple versioned APIs with mixed blessed document states. +#[test] +fn test_mixed_blessed_document_states() -> Result<()> { + let env = TestEnvironment::new()?; + + // Start with combined APIs to establish the proper context. + let combined_apis = create_multi_versioned_test_apis()?; + + // Initially, combined APIs should need update. + let result = check_apis_up_to_date(env.environment(), &combined_apis)?; + assert_eq!(result, CheckResult::NeedsUpdate); + + // Generate only health API documents first. + let health_apis = create_versioned_health_test_apis()?; + env.generate_documents(&health_apis)?; + env.commit_documents()?; + + // Combined APIs should still need update (user API missing). + let result = check_apis_up_to_date(env.environment(), &combined_apis)?; + assert_eq!(result, CheckResult::NeedsUpdate); + + // Generate and commit all APIs documents. + env.generate_documents(&combined_apis)?; + env.commit_documents()?; + + // Now combined APIs should pass. + let result = check_apis_up_to_date(env.environment(), &combined_apis)?; + assert_eq!(result, CheckResult::Success); + + Ok(()) +} + +/// Test that removing API versions fails the check. +#[test] +fn test_removing_api_version_fails_check() -> Result<()> { + let env = TestEnvironment::new()?; + let apis = create_versioned_health_test_apis()?; + + // Generate and commit initial documents (3 versions). + env.generate_documents(&apis)?; + env.commit_documents()?; + + // Verify all versions exist. + assert!(env.versioned_document_exists("versioned-health", "1.0.0")); + assert!(env.versioned_document_exists("versioned-health", "2.0.0")); + assert!(env.versioned_document_exists("versioned-health", "3.0.0")); + + // Create API with fewer versions (simulating version removal). + let reduced_apis = create_versioned_health_test_apis_reduced_versions()?; + + // The check should result in NeedsUpdate when versions are removed. + let result = check_apis_up_to_date(env.environment(), &reduced_apis)?; + assert_eq!(result, CheckResult::NeedsUpdate); + + Ok(()) +} + +/// Test that adding new API versions passes the check. +#[test] +fn test_adding_new_api_version_passes_check() -> Result<()> { + let env = TestEnvironment::new()?; + + // Start with reduced version API. + let reduced_apis = create_versioned_health_test_apis_reduced_versions()?; + env.generate_documents(&reduced_apis)?; + env.commit_documents()?; + + // Should pass check with reduced versions. + let result = check_apis_up_to_date(env.environment(), &reduced_apis)?; + assert_eq!(result, CheckResult::Success); + + // Add more versions. + let expanded_apis = create_versioned_health_test_apis()?; + + // Adding versions should require update (new documents to generate). + let result = check_apis_up_to_date(env.environment(), &expanded_apis)?; + assert_eq!(result, CheckResult::NeedsUpdate); + + // Generate the new versions. + env.generate_documents(&expanded_apis)?; + + // Should now pass with all versions. + let result = check_apis_up_to_date(env.environment(), &expanded_apis)?; + assert_eq!(result, CheckResult::Success); + + Ok(()) +} + +/// Test retirement of the latest blessed API version. +#[test] +fn test_retiring_latest_blessed_version() -> Result<()> { + let env = TestEnvironment::new()?; + + // Start with the full versioned health API (3 versions). + let full_apis = create_versioned_health_test_apis()?; + + // Generate and commit the initial "blessed" documents. + env.generate_documents(&full_apis)?; + env.commit_documents()?; + + // Verify initial state is up-to-date. + let result = check_apis_up_to_date(env.environment(), &full_apis)?; + assert_eq!(result, CheckResult::Success); + + // Verify all 3 versions exist in the blessed state. + assert!(env.versioned_document_exists("versioned-health", "1.0.0")); + assert!(env.versioned_document_exists("versioned-health", "2.0.0")); + assert!(env.versioned_document_exists("versioned-health", "3.0.0")); + + // Now remove version 3.0.0 by switching to the reduced API. + // This simulates a developer deciding to remove a version that was previously blessed. + let reduced_apis = create_versioned_health_test_apis_reduced_versions()?; + + // This check should return NeedsUpdate because the v3.0.0 document exists + // and needs to be removed. + let result = check_apis_up_to_date(env.environment(), &reduced_apis)?; + assert_eq!(result, CheckResult::NeedsUpdate); + + // Generate documents with the retired version. + env.generate_documents(&reduced_apis)?; + + // After generation, should be up-to-date with the new API definition. + let result = check_apis_up_to_date(env.environment(), &reduced_apis)?; + assert_eq!(result, CheckResult::Success); + + // Verify the v3.0.0 document was removed and v1/v2 were updated. + assert!(env.versioned_document_exists("versioned-health", "1.0.0")); + assert!(env.versioned_document_exists("versioned-health", "2.0.0")); + assert!(!env.versioned_document_exists("versioned-health", "3.0.0")); + + // Verify the latest document now points to v2.0.0 (the new highest version). + let latest_content = + env.read_versioned_latest_document("versioned-health")?; + let latest_spec: OpenAPI = serde_json::from_str(&latest_content)?; + assert_eq!(latest_spec.info.version, "2.0.0"); + + // Commit the retired version to "approve" it. + env.commit_documents()?; + + // Should still pass after committing the retired change. + let result = check_apis_up_to_date(env.environment(), &reduced_apis)?; + assert_eq!(result, CheckResult::Success); + + // Verify we can no longer use the old full API against the new blessed + // documents. + let result = check_apis_up_to_date(env.environment(), &full_apis)?; + assert_eq!(result, CheckResult::NeedsUpdate); + + Ok(()) +}