diff --git a/crates/dropshot-api-manager/src/cmd/diff.rs b/crates/dropshot-api-manager/src/cmd/diff.rs new file mode 100644 index 0000000..b0ccde3 --- /dev/null +++ b/crates/dropshot-api-manager/src/cmd/diff.rs @@ -0,0 +1,252 @@ +// Copyright 2025 Oxide Computer Company + +use crate::{ + apis::ManagedApis, + environment::{BlessedSource, ResolvedEnv}, + output::{OutputOpts, Styles, display_load_problems, write_diff}, + spec_files_blessed::BlessedApiSpecFile, + spec_files_local::LocalApiSpecFile, +}; +use anyhow::{Context, bail}; +use camino::Utf8Path; +use dropshot_api_manager_types::ApiIdent; +use owo_colors::OwoColorize; +use similar::TextDiff; +use std::{ + collections::{BTreeMap, BTreeSet}, + io::Write, +}; + +/// Compare local OpenAPI documents against blessed (upstream) versions. +/// +/// For each API with differences, shows the diff between what's on disk locally +/// and what's blessed in the upstream branch (typically origin/main). +/// +/// Diff output is written directly to `writer`. Headers describing each changed +/// file are written to stderr. +pub(crate) fn diff_impl( + apis: &ManagedApis, + env: &ResolvedEnv, + blessed_source: &BlessedSource, + output: &OutputOpts, + writer: &mut W, +) -> anyhow::Result<()> { + let mut styles = Styles::default(); + if output.use_color(supports_color::Stream::Stdout) { + styles.colorize(); + } + + // Load files and display any errors/warnings. We proceed with the diff for + // files that loaded successfully (treating unloadable files as missing). + let (local_files, errors) = env.local_source.load(apis, &styles)?; + display_load_problems(&errors, &styles)?; + + // Build maps from version to single file, validating that no version has + // multiple local files. Multiple files can happen if two developers create + // the same version with different content (resulting in different hashes). + let mut local_by_api: BTreeMap<&ApiIdent, BTreeMap<_, _>> = BTreeMap::new(); + for (ident, api_files) in local_files.iter() { + let mut version_map = BTreeMap::new(); + for (version, files) in api_files.versions() { + if files.len() > 1 { + let file_names: Vec<_> = files + .iter() + .map(|f| f.spec_file_name().path().to_string()) + .collect(); + bail!( + "{} v{}: found {} local files for the same version \ + ({}); run `generate` to resolve this conflict", + ident, + version, + files.len(), + file_names.join(", "), + ); + } + if let Some(file) = files.first() { + version_map.insert(version, file); + } + } + local_by_api.insert(ident, version_map); + } + + let (blessed_files, errors) = + blessed_source.load(&env.repo_root, apis, &styles)?; + display_load_problems(&errors, &styles)?; + + let mut any_diff = false; + + for api in apis.iter_apis() { + let ident = api.ident(); + let empty = BTreeMap::new(); + let local_versions = local_by_api.get(ident).unwrap_or(&empty); + let blessed_versions: BTreeMap<_, _> = blessed_files + .get(ident) + .into_iter() + .flat_map(|api| api.versions()) + .collect(); + + let has_diff = diff_api( + ident, + local_versions, + &blessed_versions, + &styles, + writer, + )?; + any_diff |= has_diff; + } + + if !any_diff { + eprintln!("No differences from blessed."); + } + + Ok(()) +} + +fn diff_api( + ident: &ApiIdent, + local_versions: &BTreeMap<&semver::Version, &LocalApiSpecFile>, + blessed_versions: &BTreeMap<&semver::Version, &BlessedApiSpecFile>, + styles: &Styles, + writer: &mut W, +) -> anyhow::Result { + if local_versions.is_empty() && blessed_versions.is_empty() { + return Ok(false); + } + + let mut has_diff = false; + + // Collect unique versions from both sources to handle all combinations: + // local only (added), blessed only (removed), or both (potentially + // modified). We need a set because chaining the iterators would visit + // versions present in both maps twice. + let all_versions: BTreeSet<_> = + local_versions.keys().chain(blessed_versions.keys()).collect(); + + for version in all_versions { + let local_file = local_versions.get(version).copied(); + let blessed_file = blessed_versions.get(version).copied(); + + match (blessed_file, local_file) { + (None, Some(local)) => { + // New version added locally. Diff against the previous blessed + // version to show what actually changed in the schema. + let prev_blessed = blessed_versions + .range::(..*version) + .next_back() + .map(|(_, file)| *file); + + let local_content = std::str::from_utf8(local.contents()) + .context("local file is not valid UTF-8")?; + let local_path = local.spec_file_name().path(); + + if let Some(prev) = prev_blessed { + let base_content = std::str::from_utf8(prev.contents()) + .context("blessed file is not valid UTF-8")?; + let base_path = prev.spec_file_name().path(); + + // Skip if no actual diff (shouldn't happen, but be safe). + if base_content == local_content { + continue; + } + + eprintln!( + "\n{} v{}: {} (new locally)", + ident.style(styles.filename), + version, + "added".style(styles.success_header), + ); + let diff = + TextDiff::from_lines(base_content, local_content); + write_diff( + &diff, + base_path.as_ref(), // old path + local_path.as_ref(), // new path + styles, + 3, // context lines + true, // show missing newline hint + writer, + )?; + } else { + // No previous version to compare against - show full file. + eprintln!( + "\n{} v{}: {} (new locally)", + ident.style(styles.filename), + version, + "added".style(styles.success_header), + ); + let diff = TextDiff::from_lines("", local_content); + write_diff( + &diff, + Utf8Path::new("/dev/null"), // old path + local_path.as_ref(), // new path + styles, + 3, // context lines + true, // show missing newline hint + writer, + )?; + } + has_diff = true; + } + (Some(blessed), None) => { + // Version removed locally + eprintln!( + "\n{} v{}: {} (removed locally)", + ident.style(styles.filename), + version, + "removed".style(styles.failure_header), + ); + let blessed_content = + std::str::from_utf8(blessed.contents()) + .context("blessed file is not valid UTF-8")?; + let diff = TextDiff::from_lines(blessed_content, ""); + let blessed_path = blessed.spec_file_name().path(); + write_diff( + &diff, + blessed_path.as_ref(), // old path + Utf8Path::new("/dev/null"), // new path + styles, + 3, // context lines + true, // show missing newline hint + writer, + )?; + has_diff = true; + } + (Some(blessed), Some(local)) => { + let blessed_content = + std::str::from_utf8(blessed.contents()) + .context("blessed file is not valid UTF-8")?; + let local_content = std::str::from_utf8(local.contents()) + .context("local file is not valid UTF-8")?; + + if blessed_content != local_content { + eprintln!( + "\n{} v{}: {}", + ident.style(styles.filename), + version, + "modified".style(styles.warning_header), + ); + let diff = + TextDiff::from_lines(blessed_content, local_content); + let blessed_path = blessed.spec_file_name().path(); + let local_path = local.spec_file_name().path(); + write_diff( + &diff, + blessed_path.as_ref(), // old path + local_path.as_ref(), // new path + styles, + 3, // context lines + true, // show missing newline hint + writer, + )?; + has_diff = true; + } + } + (None, None) => { + // Shouldn't happen since we collected versions from both + unreachable!() + } + } + } + + Ok(has_diff) +} diff --git a/crates/dropshot-api-manager/src/cmd/dispatch.rs b/crates/dropshot-api-manager/src/cmd/dispatch.rs index 7421d7d..f2d6d78 100644 --- a/crates/dropshot-api-manager/src/cmd/dispatch.rs +++ b/crates/dropshot-api-manager/src/cmd/dispatch.rs @@ -3,8 +3,8 @@ use crate::{ apis::ManagedApis, cmd::{ - check::check_impl, debug::debug_impl, generate::generate_impl, - list::list_impl, + check::check_impl, debug::debug_impl, diff::diff_impl, + generate::generate_impl, list::list_impl, }, environment::{BlessedSource, Environment, GeneratedSource, ResolvedEnv}, git::GitRevision, @@ -33,6 +33,7 @@ impl App { pub fn exec(self, env: &Environment, apis: &ManagedApis) -> ExitCode { let result = match self.command { Command::Debug(args) => args.exec(env, apis, &self.output_opts), + Command::Diff(args) => args.exec(env, apis, &self.output_opts), Command::List(args) => args.exec(apis, &self.output_opts), Command::Generate(args) => args.exec(env, apis, &self.output_opts), Command::Check(args) => args.exec(env, apis, &self.output_opts), @@ -53,6 +54,9 @@ pub enum Command { /// Dump debug information about everything the tool knows Debug(DebugArgs), + /// Show differences between local and blessed OpenAPI documents. + Diff(DiffArgs), + /// List managed APIs. /// /// Returns information purely from code without consulting JSON files on @@ -202,6 +206,29 @@ impl ListArgs { } } +#[derive(Debug, Args)] +pub struct DiffArgs { + #[clap(flatten)] + local: LocalSourceArgs, + #[clap(flatten)] + blessed: BlessedSourceArgs, +} + +impl DiffArgs { + fn exec( + self, + env: &Environment, + apis: &ManagedApis, + output: &OutputOpts, + ) -> anyhow::Result { + let env = env.resolve(self.local.dir)?; + let blessed_source = self.blessed.to_blessed_source(&env)?; + let mut stdout = std::io::stdout(); + diff_impl(apis, &env, &blessed_source, output, &mut stdout)?; + Ok(ExitCode::SUCCESS) + } +} + #[derive(Debug, Args)] pub struct GenerateArgs { #[clap(flatten)] diff --git a/crates/dropshot-api-manager/src/cmd/mod.rs b/crates/dropshot-api-manager/src/cmd/mod.rs index 455171a..dba8c4d 100644 --- a/crates/dropshot-api-manager/src/cmd/mod.rs +++ b/crates/dropshot-api-manager/src/cmd/mod.rs @@ -9,5 +9,6 @@ pub mod dispatch; // subcommands pub(crate) mod check; mod debug; +pub(crate) mod diff; mod generate; mod list; diff --git a/crates/dropshot-api-manager/src/test_util.rs b/crates/dropshot-api-manager/src/test_util.rs index 9b2e683..ddb2af3 100644 --- a/crates/dropshot-api-manager/src/test_util.rs +++ b/crates/dropshot-api-manager/src/test_util.rs @@ -5,10 +5,7 @@ pub use crate::output::CheckResult; use crate::{ apis::ManagedApis, - cmd::{ - check::check_impl, - dispatch::{BlessedSourceArgs, GeneratedSourceArgs}, - }, + cmd::{check::check_impl, diff::diff_impl, dispatch::BlessedSourceArgs}, environment::{Environment, GeneratedSource}, output::OutputOpts, }; @@ -28,9 +25,32 @@ pub fn check_apis_up_to_date( let blessed_source = BlessedSourceArgs { blessed_from_git: None, blessed_from_dir: None } .to_blessed_source(&env)?; - let generated_source = - GeneratedSource::from(GeneratedSourceArgs { generated_from_dir: None }); + let generated_source = GeneratedSource::Generated; let output = OutputOpts { color: clap::ColorChoice::Auto }; check_impl(apis, &env, &blessed_source, &generated_source, &output) } + +/// Generate the diff output as a string. Used for testing. +/// +/// Returns the diff output that would be written to stdout by the diff command. +pub fn get_diff_output( + env: &Environment, + apis: &ManagedApis, +) -> Result { + let env = env.resolve(None)?; + + let blessed_source = + BlessedSourceArgs { blessed_from_git: None, blessed_from_dir: None } + .to_blessed_source(&env)?; + let output = OutputOpts { color: clap::ColorChoice::Auto }; + + let mut buffer = Vec::new(); + diff_impl(apis, &env, &blessed_source, &output, &mut buffer)?; + + // Normalize path separators for cross-platform consistency in tests. + let result = String::from_utf8(buffer).map_err(|e| { + anyhow::anyhow!("diff output is not valid UTF-8: {}", e) + })?; + Ok(result.replace('\\', "/")) +} diff --git a/crates/integration-tests/src/environment.rs b/crates/integration-tests/src/environment.rs index b5cd76d..9fa13e5 100644 --- a/crates/integration-tests/src/environment.rs +++ b/crates/integration-tests/src/environment.rs @@ -365,6 +365,14 @@ impl TestEnvironment { } } + /// Get the diff output as a string. + pub fn get_diff_output(&self, apis: &ManagedApis) -> Result { + dropshot_api_manager::test_util::get_diff_output( + &self.environment, + apis, + ) + } + fn collect_files_recursive( &self, dir: &Utf8Path, diff --git a/crates/integration-tests/tests/integration/versioned.rs b/crates/integration-tests/tests/integration/versioned.rs index c7482ea..46c8c1c 100644 --- a/crates/integration-tests/tests/integration/versioned.rs +++ b/crates/integration-tests/tests/integration/versioned.rs @@ -774,6 +774,313 @@ fn test_blessed_version_extra_local_spec() -> Result<()> { Ok(()) } +// ============================================================================ +// Diff command tests +// ============================================================================ + +/// Test that diff produces no output when local matches blessed. +#[test] +fn test_diff_empty_when_up_to_date() -> Result<()> { + let env = TestEnvironment::new()?; + let apis = versioned_health_apis()?; + + // Generate and commit documents to make them blessed. + env.generate_documents(&apis)?; + env.commit_documents()?; + + // Diff should produce no output. + let diff_output = env.get_diff_output(&apis)?; + assert!(diff_output.is_empty(), "diff should be empty when up-to-date"); + + Ok(()) +} + +/// Test that diff shows changes when a new version is added locally. +/// +/// When a new version is added, the diff should compare against the previous +/// blessed version to show what actually changed in the schema. +#[test] +fn test_diff_new_version_compares_to_previous() -> Result<()> { + let env = TestEnvironment::new()?; + + // Start with reduced versions (1.0.0 and 2.0.0) and bless them. + let reduced_apis = versioned_health_reduced_apis()?; + env.generate_documents(&reduced_apis)?; + env.commit_documents()?; + + // Generate with full APIs (adding version 3.0.0). + let full_apis = versioned_health_apis()?; + env.generate_documents(&full_apis)?; + + // Get the diff output. + let diff_output = env.get_diff_output(&full_apis)?; + + // Diff output should compare 2.0.0 to 3.0.0 and only show the new /metrics + // endpoint, not the full schema. If /health appeared as added, it would + // mean we're dumping the entire file instead of diffing properly. + assert!( + diff_output.contains("--- a/versioned-health/versioned-health-2.0.0-"), + "diff should show 2.0.0 as base:\n{}", + diff_output + ); + assert!( + diff_output.contains("+++ b/versioned-health/versioned-health-3.0.0-"), + "diff should show 3.0.0 as target:\n{}", + diff_output + ); + assert!( + diff_output.contains( + r#"- "version": "2.0.0" ++ "description": "A versioned health API for testing version evolution", ++ "version": "3.0.0""# + ), + "diff should show version change:\n{}", + diff_output + ); + assert!( + diff_output.contains( + r#"+ "/metrics": { ++ "get": { ++ "summary": "Get service metrics (v3+).","# + ), + "diff should show /metrics addition:\n{}", + diff_output + ); + + // The /health endpoint exists in both versions, so it should not appear as + // added content. + let health_added = diff_output + .lines() + .any(|line| line.starts_with('+') && line.contains("\"/health\"")); + assert!(!health_added, "diff shows /health as added:\n{}", diff_output); + + Ok(()) +} + +/// Test that diff shows removed content when a version is removed locally. +#[test] +fn test_diff_shows_removed_version() -> Result<()> { + let env = TestEnvironment::new()?; + + // Start with full versions and bless them. + let full_apis = versioned_health_apis()?; + env.generate_documents(&full_apis)?; + env.commit_documents()?; + + // Generate with reduced APIs (removing version 3.0.0). + let reduced_apis = versioned_health_reduced_apis()?; + env.generate_documents(&reduced_apis)?; + + // Get the diff output. + let diff_output = env.get_diff_output(&reduced_apis)?; + + // The diff should show the removed version's content with - prefix. + assert!( + diff_output.contains("versioned-health-3.0.0"), + "diff should reference removed version (3.0.0), got: {}", + diff_output + ); + assert!( + diff_output.contains("/dev/null"), + "diff should show /dev/null as target (file removed), got: {}", + diff_output + ); + + Ok(()) +} + +/// Test that diff shows full content when no blessed documents exist. +#[test] +fn test_diff_shows_full_content_when_no_blessed() -> Result<()> { + let env = TestEnvironment::new()?; + let apis = versioned_health_apis()?; + + // Generate documents but don't commit (no blessed documents exist). + env.generate_documents(&apis)?; + + // Get the diff output. + let diff_output = env.get_diff_output(&apis)?; + + // Should show full file content since there's nothing to compare against. + assert!( + diff_output.contains("/dev/null"), + "diff should show /dev/null as source when no blessed exists, got: {}", + diff_output + ); + // Should contain OpenAPI content. + assert!( + diff_output.contains("openapi"), + "diff should show OpenAPI content, got: {}", + diff_output + ); + + Ok(()) +} + +/// Test that diff shows removed content when local documents are deleted. +#[test] +fn test_diff_shows_removed_when_local_deleted() -> Result<()> { + let env = TestEnvironment::new()?; + let apis = versioned_health_apis()?; + + // Generate and commit to create blessed documents. + env.generate_documents(&apis)?; + env.commit_documents()?; + + // Delete local documents. + let docs = env.list_versioned_documents("versioned-health")?; + for doc in docs { + let full_path = env.workspace_root().join(&doc); + std::fs::remove_file(&full_path)?; + } + + // Get the diff output. + let diff_output = env.get_diff_output(&apis)?; + + // Should show files being removed (blessed exists, local missing). + assert!( + diff_output.contains("/dev/null"), + "diff should show /dev/null as target (files removed), got: {}", + diff_output + ); + // Should show content being removed (- lines). + assert!( + diff_output.contains('-'), + "diff should contain - lines for removed content, got: {}", + diff_output + ); + + Ok(()) +} + +/// Test that diff shows changes when a blessed version is modified locally. +#[test] +fn test_diff_shows_modified_version() -> Result<()> { + let env = TestEnvironment::new()?; + let apis = versioned_health_apis()?; + + // Generate and commit documents to make them blessed. + env.generate_documents(&apis)?; + env.commit_documents()?; + + // Generate different content for the same version in a second environment. + // The incompat API has /health/detailed instead of /status for v2. + let env2 = TestEnvironment::new()?; + let incompat_apis = versioned_health_incompat_apis()?; + env2.generate_documents(&incompat_apis)?; + + // Replace env's v2 file with env2's v2 file (different content, same + // version). + let env_path = env + .find_versioned_document_path("versioned-health", "2.0.0")? + .expect("should find v2.0.0 document in env"); + let env2_path = env2 + .find_versioned_document_path("versioned-health", "2.0.0")? + .expect("should find v2.0.0 document in env2"); + + let src = env2.workspace_root().join(&env2_path); + let old_dst = env.workspace_root().join(&env_path); + let new_dst = env + .documents_dir() + .join("versioned-health") + .join(env2_path.file_name().unwrap()); + + std::fs::remove_file(&old_dst)?; + std::fs::copy(&src, &new_dst)?; + + // Get the diff output. + let diff_output = env.get_diff_output(&apis)?; + + // Should show the modification (description differs between the APIs). + assert!( + diff_output.contains("- \"description\": \"A versioned health API for testing version evolution\""), + "diff should show removed description, got: {}", + diff_output + ); + assert!( + diff_output.contains("+ \"description\": \"A versioned health API with incompatible changes\""), + "diff should show added description, got: {}", + diff_output + ); + + // The v2 diff should only appear once (regression test for duplicate + // iteration bug where versions in both maps were visited twice). + let diff_header_count = diff_output + .matches("--- a/versioned-health/versioned-health-2.0.0") + .count(); + assert_eq!( + diff_header_count, 1, + "v2.0.0 diff should appear exactly once, got: {}", + diff_output + ); + + Ok(()) +} + +/// Test that diff fails when there are multiple local files for the same +/// version. +/// +/// This can happen when two developers create the same API version in separate +/// branches with different content, resulting in files with different hashes. +#[test] +fn test_diff_fails_with_duplicate_local_versions() -> Result<()> { + let env = TestEnvironment::new()?; + let apis = versioned_health_apis()?; + + // Generate and commit initial documents to make them blessed. + env.generate_documents(&apis)?; + env.commit_documents()?; + + // Generate with the incompatible APIs in a second environment. This + // produces files with different hashes for the same version. + let env2 = TestEnvironment::new()?; + let incompatible_apis = versioned_health_incompat_apis()?; + env2.generate_documents(&incompatible_apis)?; + + // Ensure that the v3 documents are actually different between env and env2. + let env_path = env + .find_versioned_document_path("versioned-health", "3.0.0")? + .expect("should find v3.0.0 document"); + let env2_path = env2 + .find_versioned_document_path("versioned-health", "3.0.0")? + .expect("should find v3.0.0 document"); + assert_ne!( + env_path, env2_path, + "incompatible APIs should lead to different hashes" + ); + + // Copy env2's document into env's documents directory. Now env has two + // files for version 3.0.0. + let src = env2.workspace_root().join(&env2_path); + let dst = env + .documents_dir() + .join("versioned-health") + .join(env2_path.file_name().unwrap()); + std::fs::copy(&src, &dst) + .with_context(|| format!("failed to copy {} to {}", src, dst))?; + + // Diff should fail because there are two local files for the same version. + let result = env.get_diff_output(&apis); + let err = result.expect_err("diff should fail with duplicate local files"); + let err_msg = err.to_string(); + assert!( + err_msg.contains("found 2 local files for the same version"), + "error should mention duplicate files, got: {}", + err_msg + ); + assert!( + err_msg.contains("versioned-health") && err_msg.contains("3.0.0"), + "error should identify the API and version, got: {}", + err_msg + ); + + Ok(()) +} + +// ============================================================================ +// Extra validation tests +// ============================================================================ + struct VersionValidationPair { first: ValidationCall, second: ValidationCall,