From 86ccf54586508c9286d998c563d8272649d72df6 Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 15 May 2026 16:44:17 -0700 Subject: [PATCH 1/2] [spr] changes to main this commit is based on Created using spr 1.3.6-beta.1 [skip ci] --- AGENTS.md | 6 +- Cargo.lock | 1 + crates/dropshot-api-manager/Cargo.toml | 1 + .../dropshot-api-manager/src/cmd/generate.rs | 19 +- .../src/compatibility/detect.rs | 812 +++++----- .../src/compatibility/display.rs | 1339 +++++++++++++++++ .../src/compatibility/mod.rs | 7 +- .../src/compatibility/types.rs | 464 ++++++ crates/dropshot-api-manager/src/output.rs | 185 ++- crates/dropshot-api-manager/src/resolved.rs | 34 +- .../output/compatibility/abbreviated.ansi | 6 + .../output/compatibility/abbreviated.txt | 6 + .../abbreviated_multi_change.ansi | 6 + .../abbreviated_multi_change.txt | 6 + .../compatibility/deep_branching_tree.ansi | 8 + .../compatibility/deep_branching_tree.txt | 8 + .../output/compatibility/endpoint_added.ansi | 2 + .../output/compatibility/endpoint_added.txt | 2 + .../compatibility/endpoint_only_change.ansi | 2 + .../compatibility/endpoint_only_change.txt | 2 + .../output/compatibility/in_context.ansi | 9 + .../tests/output/compatibility/in_context.txt | 9 + .../compatibility/multi_sibling_tree.ansi | 8 + .../compatibility/multi_sibling_tree.txt | 8 + .../multiple_changes_in_one_component.ansi | 5 + .../multiple_changes_in_one_component.txt | 5 + .../output/compatibility/renamed_base.ansi | 2 + .../output/compatibility/renamed_base.txt | 2 + .../output/compatibility/subpath_renamed.ansi | 2 + .../output/compatibility/subpath_renamed.txt | 2 + .../output/compatibility/unhandled_class.ansi | 2 + .../output/compatibility/unhandled_class.txt | 2 + .../integration/blessed_version_broken.txt | 156 +- .../blessed_version_endpoint_removed.txt | 156 +- .../output/integration/cross_api_dedup.txt | 259 +--- 35 files changed, 2768 insertions(+), 775 deletions(-) create mode 100644 crates/dropshot-api-manager/src/compatibility/display.rs create mode 100644 crates/dropshot-api-manager/src/compatibility/types.rs create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/abbreviated.ansi create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/abbreviated.txt create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/abbreviated_multi_change.ansi create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/abbreviated_multi_change.txt create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/deep_branching_tree.ansi create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/deep_branching_tree.txt create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/endpoint_added.ansi create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/endpoint_added.txt create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/endpoint_only_change.ansi create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/endpoint_only_change.txt create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/in_context.ansi create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/in_context.txt create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/multi_sibling_tree.ansi create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/multi_sibling_tree.txt create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/multiple_changes_in_one_component.ansi create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/multiple_changes_in_one_component.txt create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/renamed_base.ansi create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/renamed_base.txt create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/subpath_renamed.ansi create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/subpath_renamed.txt create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/unhandled_class.ansi create mode 100644 crates/dropshot-api-manager/tests/output/compatibility/unhandled_class.txt diff --git a/AGENTS.md b/AGENTS.md index d22a1c6..d24108b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -224,7 +224,11 @@ crates/ │ │ ├── cmd/ # CLI commands (dispatch, check, generate, list, debug) │ │ ├── environment.rs # Environment configuration and resolution │ │ ├── resolved.rs # Resolution logic, VersionProblem / NonVersionProblem / Fix enums -│ │ ├── compatibility.rs # Wire compatibility checking via drift +│ │ ├── compatibility/ # Wire compatibility checking via drift +│ │ │ ├── mod.rs # Module re-exports +│ │ │ ├── types.rs # ApiCompatIssue and related data model +│ │ │ ├── detect.rs # Bridge from drift output into the data model +│ │ │ └── display.rs # Styled CLI rendering of compatibility issues │ │ ├── validation.rs # OpenAPI document validation │ │ ├── vcs/ # VCS abstraction (RepoVcs: Git/Jujutsu dispatch) │ │ │ ├── mod.rs # Module re-exports diff --git a/Cargo.lock b/Cargo.lock index 31b3e5b..feab823 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -534,6 +534,7 @@ dependencies = [ "drift", "dropshot", "dropshot-api-manager-types", + "expectorate", "fs-err", "git-stub", "git-stub-vcs", diff --git a/crates/dropshot-api-manager/Cargo.toml b/crates/dropshot-api-manager/Cargo.toml index dfe9fcf..8eba61f 100644 --- a/crates/dropshot-api-manager/Cargo.toml +++ b/crates/dropshot-api-manager/Cargo.toml @@ -38,6 +38,7 @@ thiserror.workspace = true [dev-dependencies] assert_matches.workspace = true +expectorate.workspace = true [lints] workspace = true diff --git a/crates/dropshot-api-manager/src/cmd/generate.rs b/crates/dropshot-api-manager/src/cmd/generate.rs index 1c0b2e4..0b1995b 100644 --- a/crates/dropshot-api-manager/src/cmd/generate.rs +++ b/crates/dropshot-api-manager/src/cmd/generate.rs @@ -3,11 +3,13 @@ use crate::{ FAILURE_EXIT_CODE, apis::ManagedApis, + compatibility::CompatIssueLocation, environment::{BlessedSource, GeneratedSource, ResolvedEnv}, output::{ - CheckResult, OutputOpts, Styles, display_api_spec_version, - display_load_problems, display_non_version_problems, - display_resolution, display_version_problems, + CheckResult, CompatDisplayContext, OutputOpts, Styles, + display_api_spec_version, display_load_problems, + display_non_version_problems, display_resolution, + display_version_problems, headers::{self, *}, plural, }, @@ -216,6 +218,8 @@ fn generate_impl_inner( display_load_problems(writer, &errors, styles)?; let resolved = Resolved::new(env, apis, &blessed, &generated, &local_files_recheck); + let dedupe = resolved.build_compat_dedupe_map(); + let orphaned_and_unparseable: Vec<_> = resolved.orphaned_and_unparseable().collect(); nproblems += orphaned_and_unparseable.len(); @@ -237,7 +241,13 @@ fn generate_impl_inner( (this is a bug)", ident, version )?; - display_version_problems(writer, env, problems, styles)?; + let compat_ctx = CompatDisplayContext { + dedupe: &dedupe, + current: CompatIssueLocation { api: ident, version }, + }; + display_version_problems( + writer, env, problems, styles, compat_ctx, + )?; } } @@ -259,6 +269,7 @@ fn generate_impl_inner( // Release borrows held by `resolved`, then drop all source // collections in parallel. Each contains many parsed OpenAPI // documents whose sequential drops are costly. + drop(dedupe); drop(resolved); std::thread::scope(|s| { s.spawn(|| drop(blessed)); diff --git a/crates/dropshot-api-manager/src/compatibility/detect.rs b/crates/dropshot-api-manager/src/compatibility/detect.rs index d684478..cacf730 100644 --- a/crates/dropshot-api-manager/src/compatibility/detect.rs +++ b/crates/dropshot-api-manager/src/compatibility/detect.rs @@ -1,134 +1,208 @@ // Copyright 2026 Oxide Computer Company -//! Determine if one OpenAPI document is a subset of another - -use drift::{Change, ChangeClass, ChangeInfo}; -use std::{collections::BTreeMap, fmt}; - -/// A compatibility error between two OpenAPI documents, indexed by the blessed -/// and generated paths. -#[derive(Debug)] -pub struct ApiCompatIssue { - // Blessed and generated pointers in JSON Pointer format (e.g. - // "#/paths/~1thing3/get") - blessed_pointer: String, - generated_pointer: String, - data: CompatIssueData, -} +//! Detect non-trivial differences between two OpenAPI documents. +//! +//! The data types passed in and out (`ApiCompatIssue`, `PathTree`, …) live in +//! [`super::types`]; this module just bridges drift's output into them. + +use super::types::{ + ApiCompatIssue, CompatIssueLocation, DocumentBasePath, DocumentPath, + OperationIdMap, PathTree, PathTreeKey, SubpathChange, + unescape_pointer_component, +}; +use drift::{Change, ChangeClass, ChangeInfo, ChangePath}; impl ApiCompatIssue { - fn best_pointer(&self) -> ApiCompatPointer<'_> { - ApiCompatPointer::best_pointer( - &self.blessed_pointer, - &self.generated_pointer, - ) - } + fn new( + blessed_spec: &serde_json::Value, + generated_spec: &serde_json::Value, + paths: Vec, + change_infos: Vec, + blessed_op_ids: &OperationIdMap<'_>, + generated_op_ids: &OperationIdMap<'_>, + ) -> Self { + // Every path within a single `Change` shares the same base, so we + // can take the first one. Drift guarantees `paths` is non-empty. + let first = paths.first().expect("non-empty paths from drift"); + let (blessed_base_ptr, _) = first.old.base_and_subpath(); + let (generated_base_ptr, _) = first.new.base_and_subpath(); + + let blessed_base = DocumentBasePath::classify( + DocumentPath::parse(blessed_base_ptr), + blessed_op_ids, + ); + let generated_base = DocumentBasePath::classify( + DocumentPath::parse(generated_base_ptr), + generated_op_ids, + ); - pub(crate) fn blessed_json(&self) -> String { - to_json_pretty(self.data.blessed_value.as_ref()) + // For an added/removed endpoint, drift points the missing side at + // the `.paths` container — fetching its JSON value there would + // drag in *every* unrelated endpoint and produce a giant + // uninformative diff. Skip the fetch on the `PathsRoot` side so + // the diff renders as plain additions (or deletions) of the one + // endpoint that actually changed. + let blessed_value = (!blessed_base.is_paths_root()) + .then(|| get_json_value(blessed_base_ptr, blessed_spec)) + .flatten(); + let generated_value = (!generated_base.is_paths_root()) + .then(|| get_json_value(generated_base_ptr, generated_spec)) + .flatten(); + + let changes = + change_infos.into_iter().map(SubpathChange::from_info).collect(); + + // Tree refs are blessed-side (see `PathTree::build`), so endpoint + // leaves resolve their op id in the blessed map. + let tree = PathTree::build(&paths, blessed_op_ids); + + Self { + blessed_base, + generated_base, + changes, + tree, + blessed_value, + generated_value, + } } +} - pub(crate) fn generated_json(&self) -> String { - to_json_pretty(self.data.generated_value.as_ref()) - } +/// Tracks which compatibility issue was first reported by which API/version, so +/// that subsequent renderings can elide the JSON diff and just show a +/// back-reference to the original. +#[derive(Debug, Default)] +pub(crate) struct CompatDedupeMap<'a> { + // The list of first occurrences of each unique issue. + // + // We store the items as a `Vec` and do linear scans for lookups. This is + // because `serde_json::Value` doesn't implement `Hash` (since it can + // represent floats), and the number of distinct compatibility issues across + // a check run is expected to be reasonably small. + entries: Vec<(&'a ApiCompatIssue, CompatIssueLocation<'a>)>, } -impl fmt::Display for ApiCompatIssue { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self.best_pointer() { - ApiCompatPointer::Same(p) - | ApiCompatPointer::Blessed(p) - | ApiCompatPointer::Generated(p) => { - write!(f, "at {}:", json_pointer_to_jq(p))?; - } - ApiCompatPointer::Rename { blessed_pointer, generated_pointer } => { - write!( - f, - "at {} -> {}:", - json_pointer_to_jq(blessed_pointer), - json_pointer_to_jq(generated_pointer), - )?; - } +impl<'a> CompatDedupeMap<'a> { + /// Record an `(issue, location)` pair. The first call for a given issue + /// records its location as the canonical first occurrence; later calls + /// with an equivalent issue are no-ops, so subsequent [`Self::lookup`]s + /// of that issue from a different location report as duplicates. + pub(crate) fn insert( + &mut self, + location: CompatIssueLocation<'a>, + issue: &'a ApiCompatIssue, + ) { + if !self.entries.iter().any(|(seen, _)| seen.is_same_change_as(issue)) { + self.entries.push((issue, location)); } + } - if self.data.changes.len() == 1 { - let ChangeInfo { - message, - old_subpath: _, - new_subpath: _, - class, - details: _, - } = &self.data.changes[0]; - write!(f, " {}change: {}", change_class_str(class), message)?; - } else { - writeln!(f)?; - for error in &self.data.changes { - let ChangeInfo { - message, - old_subpath: _, - new_subpath: _, - class, - details: _, - } = error; - writeln!( - f, - "- {}change: {}", - change_class_str(class), - message - )?; + /// Look up an issue against this dedupe map. + /// + /// Returns [`DedupeStatus::Duplicate`] only when the issue has been seen + /// previously *and* the supplied `current` location is not the one that + /// first reported it. The first-occurrence location renders normally. + pub(crate) fn lookup( + &self, + issue: &ApiCompatIssue, + current: CompatIssueLocation<'_>, + ) -> DedupeStatus<'_> { + match self + .entries + .iter() + .find(|(seen, _)| seen.is_same_change_as(issue)) + { + Some(&(_, loc)) if loc != current => { + DedupeStatus::Duplicate { first_occurrence: loc } } + _ => DedupeStatus::FirstOccurrence, } - - Ok(()) } } +/// Result of consulting a [`CompatDedupeMap`] for a particular issue at a +/// particular API/version. #[derive(Debug)] -struct CompatIssueData { - blessed_value: Option, - generated_value: Option, - changes: Vec, +pub(crate) enum DedupeStatus<'a> { + /// This is the first place the issue is being reported. + FirstOccurrence, + /// The same issue was already reported elsewhere. Render an abbreviated + /// form that references the first occurrence. + Duplicate { first_occurrence: CompatIssueLocation<'a> }, } -impl CompatIssueData { - fn new( - blessed_spec: &serde_json::Value, - blessed_pointer: &str, - generated_spec: &serde_json::Value, - generated_pointer: &str, - ) -> Self { - let (blessed_value, generated_value) = - match ApiCompatPointer::best_pointer( - blessed_pointer, - generated_pointer, - ) { - ApiCompatPointer::Same(pointer) => ( - get_json_value(pointer, blessed_spec), - get_json_value(pointer, generated_spec), - ), - ApiCompatPointer::Blessed(pointer) => { - // If the blessed version is the best, then it means that - // the generated version isn't relevant to this - // determination (typically because the generated version - // removed a path or schema that the blessed version has). - // In that case, store only the blessed value. - (get_json_value(pointer, blessed_spec), None) - } - ApiCompatPointer::Generated(pointer) => { - // Same logic as above, but for the generated version. - (None, get_json_value(pointer, generated_spec)) - } - ApiCompatPointer::Rename { - blessed_pointer, - generated_pointer, - } => ( - get_json_value(blessed_pointer, blessed_spec), - get_json_value(generated_pointer, generated_spec), - ), - }; +impl SubpathChange { + fn from_info(info: ChangeInfo) -> Self { + Self { + class: info.class, + message: info.message, + old_subpath: DocumentPath::parse(&info.old_subpath), + new_subpath: DocumentPath::parse(&info.new_subpath), + } + } +} - Self { blessed_value, generated_value, changes: Vec::new() } +impl PathTree { + fn build(paths: &[ChangePath], op_ids: &OperationIdMap<'_>) -> Self { + // For each `ChangePath`, `old.iter()` iterates over the reference stack + // starting at the leaf (the directly-affected schema), through any + // `$ref` chains, and terminating at the originating endpoint. For + // example, a change at `SubType` might really be: + // + // [0] #/components/schemas/SubType <- leaf + // [1] #/components/schemas/Wrapper/.../$ref <- ref source + // [2] #/paths/~1hello/get/.../$ref <- endpoint + // + // The first entry [0] is the changed schema itself. This is identical + // across every path in this `Change` and already shown in the issue + // header above the path tree, so we skip over that. We do need to show + // the remaining entries, though. + // + // We read the old (blessed) side rather than the new (generated) side + // because in case of renames, both sides have the same chain shape, but + // only the blessed names match the `blessed_base` in the header. + let mut tree = PathTree::default(); + for path in paths { + let ref_chain = path + .old + .iter() + .skip(1) + .map(|entry| PathTreeKey::parse(entry, op_ids)); + tree.insert(ref_chain); + } + tree + } +} + +/// Walk `doc.paths..` and collect each operation's +/// `operationId` into a map keyed by its `paths//` base. +/// +/// Endpoints without an `operationId` (or with a non-string value) are simply +/// omitted. We consider that to be okay because the operation ID isn't +/// load-bearing internally and is just used for user-friendly output. +fn extract_operation_ids(doc: &serde_json::Value) -> OperationIdMap<'_> { + let mut out = OperationIdMap::new(); + let Some(paths) = doc.pointer("/paths").and_then(|v| v.as_object()) else { + return out; + }; + for (route, item) in paths { + let Some(item) = item.as_object() else { continue }; + for (method, op) in item { + let Some(op) = op.as_object() else { continue }; + let Some(op_id) = op.get("operationId").and_then(|v| v.as_str()) + else { + continue; + }; + let base = DocumentPath { + segments: vec![ + "paths".to_string(), + route.clone(), + method.clone(), + ], + }; + out.insert(base, op_id); + } } + out } fn get_json_value( @@ -156,78 +230,6 @@ fn surround_with_map( serde_json::Value::Object(map) } -fn to_json_pretty(value: Option<&serde_json::Value>) -> String { - match value { - Some(value) => serde_json::to_string_pretty(value) - .expect("serializing serde_json::Value should always succeed"), - None => String::new(), - } -} - -#[derive(Debug, Eq, PartialEq)] -enum ApiCompatPointer<'a> { - Same(&'a str), - Blessed(&'a str), - Generated(&'a str), - Rename { blessed_pointer: &'a str, generated_pointer: &'a str }, -} - -impl<'a> ApiCompatPointer<'a> { - fn best_pointer( - blessed_pointer: &'a str, - generated_pointer: &'a str, - ) -> Self { - if blessed_pointer == generated_pointer { - return ApiCompatPointer::Same(blessed_pointer); - } - - // If one of the pointers (transformed into a prefix-free string by - // adding a trailing `/`) is a parent of the other, return the child. - if let Some(suffix) = blessed_pointer.strip_prefix(generated_pointer) - && suffix.starts_with('/') - { - return ApiCompatPointer::Blessed(blessed_pointer); - } - if let Some(suffix) = generated_pointer.strip_prefix(blessed_pointer) - && suffix.starts_with('/') - { - return ApiCompatPointer::Generated(generated_pointer); - } - - // Neither pointer is a parent of the other, so we need to treat this as - // a rename. - ApiCompatPointer::Rename { blessed_pointer, generated_pointer } - } -} - -fn json_pointer_to_jq(pointer: &str) -> String { - let mut out = String::new(); - // Strip the leading `#` and/or `/`. - let pointer = pointer.trim_matches('#').trim_matches('/'); - - // For each component (split by slash): - for component in pointer.split('/') { - // Produce a leading `.`. - out.push('.'); - - // If there are any escapes (~s), then unescape and quote the string. - if component.contains('~') { - let component = unescape_pointer_component(component); - out.push('"'); - out.push_str(&component); - out.push('"'); - } else { - out.push_str(component); - } - } - - out -} - -fn unescape_pointer_component(component: &str) -> String { - component.replace("~1", "/").replace("~0", "~") -} - /// Escape a string for use as a JSON Pointer component (RFC 6901). fn escape_json_pointer(s: &str) -> String { s.replace('~', "~0").replace('/', "~1") @@ -340,199 +342,59 @@ pub fn api_compatible( // not a wire-format change. normalize_old_websocket_responses(&mut blessed, generated); - let changes = drift::compare(&blessed, generated)?; + // Build the per-spec op-id maps once. Each issue consults them through + // `DocumentBasePath::classify` to populate the `operation_id` field on + // its endpoint variants. + let blessed_op_ids = extract_operation_ids(&blessed); + let generated_op_ids = extract_operation_ids(generated); - // drift 0.2.0 groups multiple paths and multiple inner changes under a - // single `Change`. Flatten back to (path, info) pairs and aggregate by - // (blessed_pointer, generated_pointer) to preserve the existing - // grouping and rendering. - let aggregated = changes - .into_iter() - .flat_map(|Change { paths, changes: change_infos }| { - // Filter trivial here so an entry whose only inner changes are - // trivial doesn't create empty groups. - let non_trivial: Vec = change_infos - .into_iter() - .filter(|info| match info.class { - ChangeClass::BackwardIncompatible - | ChangeClass::ForwardIncompatible - | ChangeClass::Incompatible - | ChangeClass::Unhandled => true, - ChangeClass::Trivial => false, - }) - .collect(); - paths - .into_iter() - .flat_map(move |path| { - let blessed_pointer = - path.old.iter().next().unwrap_or("").to_owned(); - let generated_pointer = - path.new.iter().next().unwrap_or("").to_owned(); - non_trivial.clone().into_iter().map(move |info| { - ( - blessed_pointer.clone(), - generated_pointer.clone(), - info, - ) - }) - }) - .collect::>() - }) - .fold( - BTreeMap::<(String, String), CompatIssueData>::new(), - |mut acc, (blessed_pointer, generated_pointer, info)| { - acc.entry((blessed_pointer.clone(), generated_pointer.clone())) - .or_insert_with(|| { - CompatIssueData::new( - &blessed, - &blessed_pointer, - generated, - &generated_pointer, - ) - }) - .changes - .push(info); - acc - }, - ); - Ok(aggregated - .into_iter() - .map(|((blessed_pointer, generated_pointer), data)| ApiCompatIssue { - blessed_pointer, - generated_pointer, - data, - }) - .collect()) -} - -pub fn change_class_str(class: &ChangeClass) -> &'static str { - match class { - // Add spaces to the end of everything so "unhandled" can return an - // empty string. - ChangeClass::BackwardIncompatible => "backward-incompatible ", - ChangeClass::ForwardIncompatible => "forward-incompatible ", - ChangeClass::Incompatible => "incompatible ", - // For unhandled changes, just say "change" in the error message (so - // nothing here). - ChangeClass::Unhandled => "", - ChangeClass::Trivial => "trivial ", + let changes = drift::compare(&blessed, generated)?; + let mut issues = Vec::new(); + for Change { paths, changes: change_infos } in changes { + // Filter out trivial changes; if nothing non-trivial remains, skip + // the issue entirely. + let non_trivial: Vec<_> = change_infos + .into_iter() + .filter(|c| match c.class { + ChangeClass::BackwardIncompatible + | ChangeClass::ForwardIncompatible + | ChangeClass::Incompatible + | ChangeClass::Unhandled => true, + ChangeClass::Trivial => false, + }) + .collect(); + if non_trivial.is_empty() { + continue; + } + issues.push(ApiCompatIssue::new( + &blessed, + generated, + paths, + non_trivial, + &blessed_op_ids, + &generated_op_ids, + )); } + // Sort by base location so iteration order is independent of whatever + // traversal order drift returns. (We expect drift to return changes in a + // stable order, but things like hash ordering can potentially cause + // variations.) + // + // We don't compare blessed_value and generated_value here, because (a) they + // are derived from the base, and (b) serde_json::Value doesn't implement + // Ord. + issues.sort_by(|a, b| { + (&a.blessed_base, &a.generated_base) + .cmp(&(&b.blessed_base, &b.generated_base)) + }); + Ok(issues) } #[cfg(test)] mod tests { use super::*; - - #[test] - fn test_best_pointer() { - let cases = vec![ - // Same pointers. - ( - "#/paths/~1users/get", - "#/paths/~1users/get", - ApiCompatPointer::Same("#/paths/~1users/get"), - ), - // Blessed pointer is child of generated pointer. - ( - "#/paths/~1users/get/responses", - "#/paths/~1users/get", - ApiCompatPointer::Blessed("#/paths/~1users/get/responses"), - ), - ( - "#/paths/~1users/get/responses/200", - "#/paths/~1users/get", - ApiCompatPointer::Blessed("#/paths/~1users/get/responses/200"), - ), - // Generated pointer is child of blessed pointer. - ( - "#/paths/~1users/get", - "#/paths/~1users/get/responses", - ApiCompatPointer::Generated("#/paths/~1users/get/responses"), - ), - ( - "#/paths/~1users/get", - "#/paths/~1users/get/responses/200/content", - ApiCompatPointer::Generated( - "#/paths/~1users/get/responses/200/content", - ), - ), - // Neither is parent of the other (rename case). - ( - "#/paths/~1users/get", - "#/paths/~1accounts/get", - ApiCompatPointer::Rename { - blessed_pointer: "#/paths/~1users/get", - generated_pointer: "#/paths/~1accounts/get", - }, - ), - ( - "#/paths/~1users/post/requestBody", - "#/paths/~1users/put/requestBody", - ApiCompatPointer::Rename { - blessed_pointer: "#/paths/~1users/post/requestBody", - generated_pointer: "#/paths/~1users/put/requestBody", - }, - ), - // Edge case: similar paths but not parent-child. - ( - "#/paths/~1user", - "#/paths/~1users", - ApiCompatPointer::Rename { - blessed_pointer: "#/paths/~1user", - generated_pointer: "#/paths/~1users", - }, - ), - ]; - - for (blessed_pointer, generated_pointer, expected) in cases { - eprintln!("testing {blessed_pointer} -> {generated_pointer}"); - let actual = ApiCompatPointer::best_pointer( - blessed_pointer, - generated_pointer, - ); - assert_eq!(actual, expected); - } - } - - #[test] - fn test_json_pointer_to_jq() { - let cases = vec![ - // Basic path with no escapes. - ("#/paths/users", ".paths.users"), - // Path with tilde escape. - ("#/paths/~0users", r#".paths."~users""#), - // Path with slash escape. - ("#/paths/~1users", r#".paths."/users""#), - // Path with both escapes. - ("#/paths/~0users~1get", r#".paths."~users/get""#), - // Complex path with multiple segments. - ( - "#/paths/~1users/get/responses/200", - r#".paths."/users".get.responses.200"#, - ), - // Path without leading #. - ("/paths/users", ".paths.users"), - // Empty path. - ("", "."), - // Just #. - ("#", "."), - // Path with multiple slashes to escape. - ("#/paths/~1api~1v1~1users", r#".paths."/api/v1/users""#), - // Path with mixed escapes. - ( - "#/components/schemas/User~0Name~1Field", - r#".components.schemas."User~Name/Field""#, - ), - ]; - - for (input, expected) in cases { - assert_eq!( - json_pointer_to_jq(input), - expected, - "for input: {input}", - ); - } - } + use dropshot_api_manager_types::ApiIdent; + use std::collections::BTreeSet; #[test] fn test_normalize_old_websocket_responses() { @@ -913,4 +775,218 @@ mod tests { "websocket-to-HTTP change should be detected as incompatible", ); } + + /// Shorthand for a component-shaped [`DocumentBasePath`] from a JSON + /// Pointer string. + fn component_base(p: &str) -> DocumentBasePath { + DocumentBasePath::Component(DocumentPath::parse(p)) + } + + /// Build an `ApiCompatIssue` directly. The `tree` is left empty since the + /// dedupe key intentionally ignores it — different APIs reach the same + /// component via different `$ref` chains and we want them to dedupe. + fn synthetic_issue( + base: &str, + message: &str, + blessed_value: serde_json::Value, + generated_value: serde_json::Value, + ) -> ApiCompatIssue { + ApiCompatIssue { + blessed_base: component_base(base), + generated_base: component_base(base), + changes: BTreeSet::from([SubpathChange { + class: ChangeClass::Incompatible, + message: message.into(), + old_subpath: DocumentPath::parse("properties/value"), + new_subpath: DocumentPath::parse("properties/value"), + }]), + tree: PathTree::default(), + blessed_value: Some(blessed_value), + generated_value: Some(generated_value), + } + } + + /// Owns the data a [`CompatIssueLocation`] borrows from, so a test can + /// hold it alive while passing the location around. + struct OwnedLoc { + api: ApiIdent, + version: semver::Version, + } + + impl OwnedLoc { + fn new(api: &str, version: &str) -> Self { + Self { + api: ApiIdent::from(api.to_string()), + version: version.parse().unwrap(), + } + } + + fn as_loc(&self) -> CompatIssueLocation<'_> { + CompatIssueLocation { api: &self.api, version: &self.version } + } + } + + /// An identical issue reported by two APIs is "first occurrence" for the + /// first API and "duplicate" (pointing at the first) for the second. + #[test] + fn test_dedupe_basic() { + let issue_a = synthetic_issue( + "#/components/schemas/Error", + "schema types changed", + serde_json::json!({"Error": {"type": "string"}}), + serde_json::json!({"Error": {"type": "integer"}}), + ); + let issue_b = synthetic_issue( + "#/components/schemas/Error", + "schema types changed", + serde_json::json!({"Error": {"type": "string"}}), + serde_json::json!({"Error": {"type": "integer"}}), + ); + + let foo = OwnedLoc::new("foo", "1.0.0"); + let bar = OwnedLoc::new("bar", "1.0.0"); + let mut dedupe = CompatDedupeMap::default(); + dedupe.insert(foo.as_loc(), &issue_a); + dedupe.insert(bar.as_loc(), &issue_b); + + assert!(matches!( + dedupe.lookup(&issue_a, foo.as_loc()), + DedupeStatus::FirstOccurrence, + )); + match dedupe.lookup(&issue_b, bar.as_loc()) { + DedupeStatus::Duplicate { first_occurrence } => { + assert_eq!(first_occurrence.api, foo.as_loc().api); + assert_eq!(first_occurrence.version, foo.as_loc().version); + } + DedupeStatus::FirstOccurrence => { + panic!("expected duplicate, got first occurrence"); + } + } + } + + /// Two issues with the same name and message but different underlying + /// values are not duplicates: an `Error` schema in one API may be a + /// completely different type from `Error` in another. + #[test] + fn test_dedupe_distinguishes_by_value() { + let issue_a = synthetic_issue( + "#/components/schemas/Error", + "schema types changed", + serde_json::json!({"Error": {"type": "string"}}), + serde_json::json!({"Error": {"type": "integer"}}), + ); + // Same name, same change message, different concrete schema. + let issue_b = synthetic_issue( + "#/components/schemas/Error", + "schema types changed", + serde_json::json!({"Error": {"type": "object"}}), + serde_json::json!({"Error": {"type": "array"}}), + ); + + let foo = OwnedLoc::new("foo", "1.0.0"); + let bar = OwnedLoc::new("bar", "1.0.0"); + let mut dedupe = CompatDedupeMap::default(); + dedupe.insert(foo.as_loc(), &issue_a); + dedupe.insert(bar.as_loc(), &issue_b); + + assert!(matches!( + dedupe.lookup(&issue_a, foo.as_loc()), + DedupeStatus::FirstOccurrence, + )); + assert!(matches!( + dedupe.lookup(&issue_b, bar.as_loc()), + DedupeStatus::FirstOccurrence, + )); + } + + /// The same issue reported under multiple versions of the same API + /// dedupes the second version, not the first. + #[test] + fn test_dedupe_across_versions_of_same_api() { + let issue = synthetic_issue( + "#/components/schemas/Error", + "schema types changed", + serde_json::json!({"Error": {"type": "string"}}), + serde_json::json!({"Error": {"type": "integer"}}), + ); + + let v1 = OwnedLoc::new("foo", "1.0.0"); + let v2 = OwnedLoc::new("foo", "2.0.0"); + let mut dedupe = CompatDedupeMap::default(); + dedupe.insert(v1.as_loc(), &issue); + dedupe.insert(v2.as_loc(), &issue); + + assert!(matches!( + dedupe.lookup(&issue, v1.as_loc()), + DedupeStatus::FirstOccurrence, + )); + match dedupe.lookup(&issue, v2.as_loc()) { + DedupeStatus::Duplicate { first_occurrence } => { + assert_eq!(first_occurrence.version, v1.as_loc().version); + } + DedupeStatus::FirstOccurrence => { + panic!("expected duplicate at v2"); + } + } + } + + /// Two issues at the same base with the same change set but reported in + /// different orders should dedupe. The `changes` field is a `BTreeSet`, + /// so the comparison is order-independent regardless of what order + /// drift happened to emit the inner changes in. + #[test] + fn test_dedupe_change_order_independent() { + fn make_change(message: &str, subpath: &str) -> SubpathChange { + SubpathChange { + class: ChangeClass::Incompatible, + message: message.into(), + old_subpath: DocumentPath::parse(subpath), + new_subpath: DocumentPath::parse(subpath), + } + } + + fn issue_with_changes( + changes: impl IntoIterator, + ) -> ApiCompatIssue { + ApiCompatIssue { + blessed_base: component_base("#/components/schemas/User"), + generated_base: component_base("#/components/schemas/User"), + changes: changes.into_iter().collect(), + tree: PathTree::default(), + blessed_value: Some(serde_json::json!({"User": {}})), + generated_value: Some(serde_json::json!({"User": {}})), + } + } + + let a_changes = [ + make_change("a changed", "properties/a"), + make_change("b changed", "properties/b"), + ]; + let b_changes = [ + make_change("b changed", "properties/b"), + make_change("a changed", "properties/a"), + ]; + + let issue_a = issue_with_changes(a_changes); + let issue_b = issue_with_changes(b_changes); + + assert!( + issue_a.is_same_change_as(&issue_b), + "issues with same change set in different order should dedupe", + ); + + let foo = OwnedLoc::new("foo", "1.0.0"); + let bar = OwnedLoc::new("bar", "1.0.0"); + let mut dedupe = CompatDedupeMap::default(); + dedupe.insert(foo.as_loc(), &issue_a); + dedupe.insert(bar.as_loc(), &issue_b); + match dedupe.lookup(&issue_b, bar.as_loc()) { + DedupeStatus::Duplicate { first_occurrence } => { + assert_eq!(first_occurrence.api, foo.as_loc().api); + } + DedupeStatus::FirstOccurrence => { + panic!("expected order-reversed issue to dedupe"); + } + } + } } diff --git a/crates/dropshot-api-manager/src/compatibility/display.rs b/crates/dropshot-api-manager/src/compatibility/display.rs new file mode 100644 index 0000000..a7e595b --- /dev/null +++ b/crates/dropshot-api-manager/src/compatibility/display.rs @@ -0,0 +1,1339 @@ +// Copyright 2026 Oxide Computer Company + +//! Render [`ApiCompatIssue`]s as styled text for the CLI. +//! +//! The data side of compatibility checking lives in [`super::detect`]; this +//! module turns one of those issues into the labeled, tree-drawn output the +//! user sees on `cargo openapi check`. + +use super::types::{ + ApiCompatIssue, CompatIssueLocation, DocumentBasePath, DocumentPath, + PathTree, PathTreeKey, SubpathChange, +}; +use crate::output::Styles; +use drift::ChangeClass; +use owo_colors::{OwoColorize, Style}; +use std::{ + collections::{BTreeMap, HashSet}, + fmt::{self, Write as _}, +}; + +/// `Display` adapter for [`ApiCompatIssue`] that applies styling. +/// +/// Returned by [`ApiCompatIssue::display`] and +/// [`ApiCompatIssue::display_abbreviated`]. +/// +/// Each issue is rendered as a single block, with the per-change severity / +/// `at:` at the top, then all the `used by:` at the bottom. Multiple changes +/// within the same issue are all used by the same set of endpoints (by the +/// structure of how OpenAPI works), so we combine those changes. +/// +/// Separately, `output.rs` emits a single JSON diff per issue which is treated +/// as part of the block. +pub(crate) struct ApiCompatIssueDisplay<'a> { + pub(super) issue: &'a ApiCompatIssue, + pub(super) styles: &'a Styles, + pub(super) prev: Option>, +} + +impl fmt::Display for ApiCompatIssueDisplay<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let groups = collect_endpoint_groups(&self.issue.tree); + let label_width = self.block_label_width(&groups); + + // Newlines separate sections rather than terminate them, so the + // rendered block always ends mid-line regardless of whether a + // `used by:` section follows. The caller supplies the trailing + // newline (typically via `eprintln!` or `format!("{}\n", ...)`). + // + // The `(see )` back-reference for cross-API duplicates goes + // on the first severity line only. + let mut prev_marker = self.prev; + for (i, change) in self.issue.changes.iter().enumerate() { + if i > 0 { + writeln!(f)?; + } + self.write_change_header(f, change, label_width, prev_marker)?; + prev_marker = None; + } + if !groups.is_empty() { + writeln!(f)?; + self.write_used_by_section(f, &groups, label_width)?; + } + Ok(()) + } +} + +impl ApiCompatIssueDisplay<'_> { + /// Compute the label column width for this block. + /// + /// The width is the longest label that will appear, across: + /// + /// * every change's severity word + /// * `at`, which is always present + /// * `used by`, if there's at least one endpoint group + fn block_label_width(&self, groups: &[EndpointGroup<'_>]) -> usize { + let mut max = "at".len(); + for change in &self.issue.changes { + max = max.max(class_label(change.class).len()); + } + if !groups.is_empty() { + max = max.max("used by".len()); + } + max + } + + /// Render one change's severity and `at:` lines (without a trailing newline + /// after `at:`). + fn write_change_header( + &self, + f: &mut fmt::Formatter<'_>, + change: &SubpathChange, + label_width: usize, + prev_marker: Option>, + ) -> fmt::Result { + let styles = self.styles; + + // Severity line: `: `, with an optional + // `(see )` suffix when this is the first severity line of + // an abbreviated duplicate. The `(see ...)` doubles as the + // version anchor — for the first-occurrence rendering, the + // surrounding cargo Failure header already names the version, so + // we don't repeat it here. + let severity = class_label(change.class); + write_label( + f, + severity, + class_style(change.class, styles), + label_width, + )?; + f.write_str(&change.message)?; + if let Some(prev) = prev_marker { + write!( + f, + " {}", + format_args!("(see {prev})").style(styles.warning), + )?; + } + writeln!(f)?; + + write_label(f, "at", styles.dimmed, label_width)?; + self.write_at_content(f, change) + } + + /// Render `used by:` lines and their intermediate trees, without a leading + /// or trailing newline. + fn write_used_by_section( + &self, + f: &mut fmt::Formatter<'_>, + groups: &[EndpointGroup<'_>], + label_width: usize, + ) -> fmt::Result { + let styles = self.styles; + // This map is used to elide shared subtrees with `(*)` back-references, + // similar to `cargo tree`. + // + // Note that the same `base` with a different `subpath` is not deduped + // at that level, though it's likely to be deduped at the level above. + let mut seen = HashSet::new(); + for (i, group) in groups.iter().enumerate() { + if i > 0 { + writeln!(f)?; + } + write_label(f, "used by", styles.dimmed, label_width)?; + write_humanized_base(f, group.endpoint_base, styles)?; + if !group.intermediates.children.is_empty() { + let mut prefix = String::new(); + for _ in 0..(label_width + 2) { + prefix.push(' '); + } + self.write_intermediate_tree( + f, + &group.intermediates, + &mut prefix, + &mut seen, + )?; + } + } + Ok(()) + } + + /// Recursively render an [`IntermediateTree`] using `cargo tree`-style + /// `├─` / `└─` connectors with `│ ` continuations down the levels. + /// + /// `prefix` carries the indentation from ancestor levels (the spaces + /// to skip past the `used by:` label, plus a `│ ` for each unfinished + /// ancestor branch). It's mutated in place for convenience but is + /// always restored to its incoming value before this function returns. + /// + /// `seen` records [`PathTreeKey`]s already expanded earlier in the + /// rendering. A repeated key is rendered with a trailing `(*)` and its + /// subtree is suppressed — the reader can find the full chain at the + /// first occurrence. + fn write_intermediate_tree<'a>( + &self, + f: &mut fmt::Formatter<'_>, + tree: &'a IntermediateTree<'a>, + prefix: &mut String, + seen: &mut HashSet<&'a PathTreeKey>, + ) -> fmt::Result { + let styles = self.styles; + let last = tree.children.len().saturating_sub(1); + for (i, (&key, child)) in tree.children.iter().enumerate() { + let is_last = i == last; + let connector = if is_last { "└─ " } else { "├─ " }; + let child_indent = if is_last { " " } else { "│ " }; + + writeln!(f)?; + // Dim the whole tree-drawing portion (prefix bars + connector) + // so the chain reads as quieter scaffolding around the bolded + // type names. Plain spaces in the prefix render the same dim + // or not, so styling the whole string is fine. + write!( + f, + "{}{}", + prefix.as_str().style(styles.dimmed), + connector.style(styles.dimmed), + )?; + // Type name stays bolded as the row's anchor; field path + // renders at default weight so the tree stays quieter than + // the `at:` line above. + write_humanized_at_pair( + f, + &key.base, + &key.subpath, + styles, + Style::default(), + )?; + + // If this exact key was already expanded earlier in the + // block, mark `(*)` and skip the subtree. + if !seen.insert(key) { + write!(f, " {}", "(*)".style(styles.dimmed))?; + continue; + } + + let saved = prefix.len(); + prefix.push_str(child_indent); + self.write_intermediate_tree(f, child, prefix, seen)?; + prefix.truncate(saved); + } + Ok(()) + } + + /// Write the content of the `at:` line for one [`SubpathChange`]. + /// + /// * If the blessed and generated locations match exactly, render once. + /// * For pure additions (old subpath is root, base unchanged) or + /// removals (new subpath is root, base unchanged), render only the + /// side that names a real location — the message text already + /// conveys whether something was added or removed. + /// * If exactly one side classifies as + /// [`DocumentBasePath::PathsRoot`] (which is drift's view of the + /// `.paths` container when the endpoint itself doesn't exist on + /// that side), render only the recognized side. Reporting it as a + /// rename from `.paths` to `GET /system/info` is misleading: it's + /// really an add or remove of the endpoint, with the parent + /// container being a noisy artifact of drift's path stack. + /// * Otherwise (rename, type change with rename, etc.) render both + /// forms separated by ` -> ` so the rename is explicit. + fn write_at_content( + &self, + f: &mut fmt::Formatter<'_>, + change: &SubpathChange, + ) -> fmt::Result { + let issue = self.issue; + let styles = self.styles; + let same_base = issue.blessed_base == issue.generated_base; + let same_subpath = change.old_subpath == change.new_subpath; + + // The `at:` line is the primary location of the change, so we + // bold both the type name and the field path within it. The + // tree-drawn continuation lines below render the field path with + // a quieter field style; see `write_intermediate_tree`. + let field_style = styles.bold; + if same_base && same_subpath { + return write_humanized_at_pair( + f, + &issue.blessed_base, + &change.old_subpath, + styles, + field_style, + ); + } + if same_base && change.old_subpath.is_root() { + return write_humanized_at_pair( + f, + &issue.generated_base, + &change.new_subpath, + styles, + field_style, + ); + } + if same_base && change.new_subpath.is_root() { + return write_humanized_at_pair( + f, + &issue.blessed_base, + &change.old_subpath, + styles, + field_style, + ); + } + + // Pair on `(blessed_is_paths_root, generated_is_paths_root)` so the + // four cases are explicit. + match ( + issue.blessed_base.is_paths_root(), + issue.generated_base.is_paths_root(), + ) { + // Endpoint added: drop the `.paths` side and render only the + // new endpoint. + (true, false) => write_humanized_at_pair( + f, + &issue.generated_base, + &change.new_subpath, + styles, + field_style, + ), + // Endpoint removed: drop the `.paths` side and render only the old + // endpoint. + (false, true) => write_humanized_at_pair( + f, + &issue.blessed_base, + &change.old_subpath, + styles, + field_style, + ), + // `(false, false)` is a real rename. `(true, true)` should not + // ordinarily happen, but handle it in case it does anyway. In both + // cases, render both sides with ` -> `. + (true, true) | (false, false) => { + write_humanized_at_pair( + f, + &issue.blessed_base, + &change.old_subpath, + styles, + field_style, + )?; + f.write_str(" -> ")?; + write_humanized_at_pair( + f, + &issue.generated_base, + &change.new_subpath, + styles, + field_style, + ) + } + } + } +} + +/// Write a right-aligned label followed by `: `. +/// +/// Uses manual whitespace padding (rather than the `{:>n}` width specifier) +/// so that ANSI escape sequences in the styled label don't throw the +/// alignment off. +fn write_label( + f: &mut fmt::Formatter<'_>, + label: &str, + label_style: Style, + max_width: usize, +) -> fmt::Result { + let pad = max_width.saturating_sub(label.len()); + for _ in 0..pad { + f.write_char(' ')?; + } + write!(f, "{}: ", label.style(label_style)) +} + +fn class_label(class: ChangeClass) -> &'static str { + match class { + ChangeClass::BackwardIncompatible => "backward-incompatible", + ChangeClass::ForwardIncompatible => "forward-incompatible", + ChangeClass::Incompatible => "incompatible", + // Trivial changes are filtered out in `api_compatible`, so this + // branch is only reachable if a future drift release classifies + // something here that we then forget to filter; show it neutrally + // rather than panic. + ChangeClass::Trivial => "trivial", + ChangeClass::Unhandled => "change", + } +} + +/// Pick the severity color for a [`ChangeClass`]. +/// +/// Hard incompatibilities are red (the surface either is or is about to be +/// broken). Forward-incompatible is amber: newer clients can't talk to +/// older servers, but the deployed surface still works. Other changes get +/// a neutral warning style. +fn class_style(class: ChangeClass, styles: &Styles) -> Style { + match class { + ChangeClass::BackwardIncompatible | ChangeClass::Incompatible => { + styles.failure_header + } + ChangeClass::ForwardIncompatible => styles.warning_header, + ChangeClass::Unhandled | ChangeClass::Trivial => styles.warning, + } +} + +/// One originating endpoint, plus the tree of `$ref` chains that link it back +/// to the changed component. +#[derive(Debug)] +struct EndpointGroup<'a> { + /// Base of the originating endpoint. + endpoint_base: &'a DocumentBasePath, + /// The tree of `$ref` chains leading from `endpoint_base` back toward the + /// changed component. + intermediates: IntermediateTree<'a>, +} + +/// Tree of `$ref` intermediate sources, oriented endpoint-first. +/// +/// Structurally identical to [`PathTree`], but a different type to keep +/// the orientation explicit: in [`PathTree`] the root is the changed +/// component and the leaves are endpoints; here the (implicit) root is +/// an endpoint and the deepest nodes are the components closest to the +/// changed type. +/// +/// Borrows [`PathTreeKey`]s from a [`PathTree`] rather than owning them +/// so the rebuild at display time is cheap. +#[derive(Debug, Default)] +struct IntermediateTree<'a> { + children: BTreeMap<&'a PathTreeKey, IntermediateTree<'a>>, +} + +/// Enumerate distinct endpoints through `tree`, grouping all chains originating +/// from the same endpoint into an [`EndpointGroup`]. Within each group, chains +/// that share a prefix are also merged into an `IntermediateTree`. (This +/// effectively inverts the `PathTree`, as desired by display output.) +fn collect_endpoint_groups(tree: &PathTree) -> Vec> { + let mut groups: BTreeMap<_, IntermediateTree<'_>> = BTreeMap::new(); + let mut ancestors = Vec::new(); + for (key, child) in &tree.children { + walk_and_collect(key, child, &mut ancestors, &mut groups); + } + groups + .into_iter() + .map(|(endpoint_base, intermediates)| EndpointGroup { + endpoint_base, + intermediates, + }) + .collect() +} + +/// Walk the `node` rooted at `key`, and at each endpoint leaf, insert the +/// accumulated `ancestors` into that endpoint's [`IntermediateTree`]. +fn walk_and_collect<'a>( + key: &'a PathTreeKey, + node: &'a PathTree, + ancestors: &mut Vec<&'a PathTreeKey>, + groups: &mut BTreeMap<&'a DocumentBasePath, IntermediateTree<'a>>, +) { + if node.children.is_empty() { + // This is a leaf node in the `PathTree`. This means that `key` is the + // originating endpoint. + // + // `ancestors` covers the rest of the tree. Walk it in reverse, + // descending through `IntermediateTree` children, so the resulting + // subtree is endpoint-first. + let mut curr = groups.entry(&key.base).or_default(); + for &ancestor in ancestors.iter().rev() { + curr = curr.children.entry(ancestor).or_default(); + } + return; + } + ancestors.push(key); + for (child_key, child) in &node.children { + walk_and_collect(child_key, child, ancestors, groups); + } + ancestors.pop(); +} + +/// Write a base and subpath pair in the form used by `at:` and the tree-drawn +/// continuation lines. +/// +/// `field_style` is forwarded to [`write_humanized_subpath`] for components +/// and controls how the subpath portion is styled. +fn write_humanized_at_pair( + f: &mut fmt::Formatter<'_>, + base: &DocumentBasePath, + subpath: &DocumentPath, + styles: &Styles, + field_style: Style, +) -> fmt::Result { + write_humanized_base(f, base, styles)?; + if subpath.is_root() { + return Ok(()); + } + match base { + DocumentBasePath::Component(_) => { + write_humanized_subpath(f, subpath, field_style) + } + DocumentBasePath::Endpoint { .. } => { + // We call `write_jq_path` and not `write_humanized_subpath` + // here. This is because segments like `properties` are not a + // structural part of endpoints (only of components). + f.write_str(" (")?; + write_jq_path(f, subpath, Style::default())?; + f.write_str(")") + } + DocumentBasePath::PathsRoot | DocumentBasePath::Other(_) => { + write_jq_path(f, subpath, Style::default()) + } + } +} + +/// Write `base` in humanized form by dispatching on its variant: +/// +/// * For [`DocumentBasePath::Component`], the bolded schema name. +/// * For [`DocumentBasePath::Endpoint`], a green-bold `` followed by +/// ``. If an `operation_id` is provided, it is appended in +/// parentheses. +/// * For [`DocumentBasePath::PathsRoot`], the literal `.paths`. In practice +/// this is dropped in [`Self::write_at_content`] before reaching here, so +/// this is purely defensive. +/// * For [`DocumentBasePath::Other`], falls back to full jq notation. +fn write_humanized_base( + f: &mut fmt::Formatter<'_>, + base: &DocumentBasePath, + styles: &Styles, +) -> fmt::Result { + match base { + DocumentBasePath::Component(path) => { + write!(f, "{}", component_name(path).style(styles.bold)) + } + DocumentBasePath::Endpoint { name, operation_id } => { + write_endpoint_method_route(f, name, styles)?; + write_operation_id(f, operation_id.as_deref(), styles) + } + DocumentBasePath::PathsRoot => { + write!(f, "{}", ".paths".style(styles.bold)) + } + DocumentBasePath::Other(path) => write_jq_path(f, path, styles.bold), + } +} + +/// Write `METHOD /route` for an endpoint base path. +/// +/// `name` is assumed to match the `paths//` shape that +/// [`DocumentBasePath::classify`] enforces on `Endpoint` variants. +fn write_endpoint_method_route( + f: &mut fmt::Formatter<'_>, + name: &DocumentPath, + styles: &Styles, +) -> fmt::Result { + // We expect this to be `paths//`: + // + // * segments[1] is the unescaped route, + // * segments[2] is the HTTP method. + let route = &name.segments[1]; + let method = &name.segments[2]; + write!( + f, + "{} {}", + method.to_uppercase().style(styles.success_header), + route.style(styles.filename), + ) +} + +/// Append `" ()"`, or nothing if `operation_id` is `None`. +fn write_operation_id( + f: &mut fmt::Formatter<'_>, + operation_id: Option<&str>, + styles: &Styles, +) -> fmt::Result { + let Some(op_id) = operation_id else { return Ok(()) }; + write!(f, " ({})", op_id.style(styles.operation_id)) +} + +/// Write a component's subpath in OpenAPI-aware form. +/// +/// With OpenAPI, paths can contain both structural keywords (`properties`, +/// `items`, etc.) and field names. The structural pieces do not add much value, +/// so we skip over them: +/// +/// * `.properties.` just becomes `.`. The `.properties` marker +/// just says that the next segment is a field name. +/// * `.items` (the schema `items` keyword, marking the element type of +/// an array) becomes `[]`. (This makes `.foo.items` render as `.foo[]`. +/// +/// Other keywords (`allOf.0`, `additionalProperties`, etc.) are +/// rendered literally. +fn write_humanized_subpath( + f: &mut fmt::Formatter<'_>, + subpath: &DocumentPath, + style: Style, +) -> fmt::Result { + let mut next_is_field_name = false; + for s in &subpath.segments { + if next_is_field_name { + write_jq_segment(f, s, style)?; + next_is_field_name = false; + continue; + } + match s.as_str() { + "properties" => next_is_field_name = true, + "items" => write!(f, "{}", "[]".style(style))?, + other => write_jq_segment(f, other, style)?, + } + } + Ok(()) +} + +/// Write `path` in plain jq notation, applying `style` to every segment. +/// +/// The root (empty) path renders as `.`, while non-empty paths render as +/// `.seg1.seg2…`. +fn write_jq_path( + f: &mut fmt::Formatter<'_>, + path: &DocumentPath, + style: Style, +) -> fmt::Result { + if path.is_root() { + return write!(f, "{}", ".".style(style)); + } + for s in &path.segments { + write_jq_segment(f, s, style)?; + } + Ok(()) +} + +/// Write one jq segment starting with a leading `.`. +/// +/// The segment text is quoted if it contains characters ambiguous as a bare jq +/// identifier. +fn write_jq_segment( + f: &mut fmt::Formatter<'_>, + segment: &str, + style: Style, +) -> fmt::Result { + write!(f, "{}", ".".style(style))?; + if segment.contains(['/', '~']) { + write!(f, "{}", format_args!("\"{segment}\"").style(style)) + } else { + write!(f, "{}", segment.style(style)) + } +} + +/// Extract the schema/component name from a known-component path +/// (`components//`). The shape is enforced by +/// [`DocumentBasePath::classify`] on the `Component` variant. +fn component_name(base: &DocumentPath) -> &str { + &base.segments[2] +} + +#[cfg(test)] +mod tests { + use super::{ + super::types::{ + ApiCompatIssue, CompatIssueLocation, DocumentBasePath, + DocumentPath, OperationIdMap, PathTree, PathTreeKey, SubpathChange, + }, + *, + }; + use crate::output::Styles; + use camino::Utf8PathBuf; + use drift::ChangeClass; + use dropshot_api_manager_types::ApiIdent; + use owo_colors::OwoColorize; + use std::collections::BTreeSet; + + /// Build a small [`OperationIdMap`] from `(endpoint_base, op_id)` pairs. + /// Used by synthetic-issue tests that bypass `api_compatible` and so + /// don't have a spec to extract op ids from. + fn op_ids<'a>(entries: &[(&str, &'a str)]) -> OperationIdMap<'a> { + entries + .iter() + .map(|(base, op_id)| (DocumentPath::parse(base), *op_id)) + .collect() + } + + /// Shorthand for `DocumentBasePath::Component(DocumentPath::parse(p))`. + fn component(p: &str) -> DocumentBasePath { + DocumentBasePath::Component(DocumentPath::parse(p)) + } + + fn output_path(name: &str) -> camino::Utf8PathBuf { + Utf8PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("tests/output/compatibility") + .join(name) + } + + /// Render `issues` with `styles`, joining multiple issues with a blank + /// line and ensuring a trailing newline. + fn render_with_styles( + issues: &[ApiCompatIssue], + styles: &Styles, + ) -> String { + if issues.is_empty() { + return "(no issues)\n".to_string(); + } + let mut out = issues + .iter() + .map(|i| i.display(styles).to_string()) + .collect::>() + .join("\n\n"); + out.push('\n'); + out + } + + /// Run `api_compatible` on `(blessed, generated)` and snapshot both the + /// plain (`.txt`) and colorized (`.ansi`) renderings of the + /// resulting issues. + fn assert_render_snapshots( + name: &str, + blessed: &serde_json::Value, + generated: &serde_json::Value, + ) { + let issues = super::super::detect::api_compatible(blessed, generated) + .expect("api_compatible should not fail on well-formed input"); + let mut colorized = Styles::default(); + colorized.colorize(); + expectorate::assert_contents( + output_path(&format!("{name}.txt")), + &render_with_styles(&issues, &Styles::default()), + ); + expectorate::assert_contents( + output_path(&format!("{name}.ansi")), + &render_with_styles(&issues, &colorized), + ); + } + + /// Snapshot the plain (`.txt`) and colorized (`.ansi`) + /// renderings of a synthetic [`ApiCompatIssue`]. + fn assert_issue_snapshots(name: &str, issue: &ApiCompatIssue) { + let mut colorized = Styles::default(); + colorized.colorize(); + expectorate::assert_contents( + output_path(&format!("{name}.txt")), + &format!("{}\n", issue.display(&Styles::default())), + ); + expectorate::assert_contents( + output_path(&format!("{name}.ansi")), + &format!("{}\n", issue.display(&colorized)), + ); + } + + /// Render `path` unstyled into a `String` using [`write_jq_path`]. + fn jq_string(path: &DocumentPath) -> String { + struct Render<'a>(&'a DocumentPath); + impl fmt::Display for Render<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write_jq_path(f, self.0, Style::default()) + } + } + Render(path).to_string() + } + + /// Owns the data a [`CompatIssueLocation`] borrows from, so a test can + /// hold it alive while passing the location around. + struct OwnedLoc { + api: ApiIdent, + version: semver::Version, + } + + impl OwnedLoc { + fn new(api: &str, version: &str) -> Self { + Self { + api: ApiIdent::from(api.to_string()), + version: version.parse().unwrap(), + } + } + + fn as_loc(&self) -> CompatIssueLocation<'_> { + CompatIssueLocation { api: &self.api, version: &self.version } + } + } + + #[test] + fn test_jq_path_display() { + let cases = vec![ + // Basic path with no escapes. + ("#/paths/users", ".paths.users"), + // Path with tilde escape. + ("#/paths/~0users", r#".paths."~users""#), + // Path with slash escape. + ("#/paths/~1users", r#".paths."/users""#), + // Path with both escapes. + ("#/paths/~0users~1get", r#".paths."~users/get""#), + // Complex path with multiple segments. + ( + "#/paths/~1users/get/responses/200", + r#".paths."/users".get.responses.200"#, + ), + // Path without leading #. + ("/paths/users", ".paths.users"), + // Empty path (root). + ("", "."), + // Just #. + ("#", "."), + // Path with multiple slashes to escape. + ("#/paths/~1api~1v1~1users", r#".paths."/api/v1/users""#), + // Path with mixed escapes. + ( + "#/components/schemas/User~0Name~1Field", + r#".components.schemas."User~Name/Field""#, + ), + // Relative subpath (no `#`/leading `/`). + ("properties/value", ".properties.value"), + ( + "responses/200/content/application~1json", + r#".responses.200.content."application/json""#, + ), + ]; + + for (input, expected) in cases { + assert_eq!( + jq_string(&DocumentPath::parse(input)), + expected, + "for input: {input}", + ); + } + } + + /// Build a minimal OpenAPI 3.0 document with a single GET endpoint + /// whose 200 response schema is a `$ref` to the named schema, and + /// the supplied `components.schemas` map. + fn doc_with_ref_schema( + target_ref: &str, + schemas: serde_json::Value, + ) -> serde_json::Value { + serde_json::json!({ + "openapi": "3.0.3", + "info": { "title": "T", "version": "1.0.0" }, + "paths": { + "/hello/{name}": { + "get": { + "operationId": "hello", + "parameters": [{ + "name": "name", "in": "path", + "required": true, + "schema": { "type": "string" } + }], + "responses": { + "200": { + "description": "ok", + "content": { + "application/json": { + "schema": { "$ref": target_ref } + } + } + } + } + } + } + }, + "components": { "schemas": schemas } + }) + } + + /// An endpoint-level change (a new required parameter) reaches no schema + /// component, so the rendered output should not include a reference tree. + #[test] + fn test_render_endpoint_only_change() { + let blessed = serde_json::json!({ + "openapi": "3.0.3", + "info": { "title": "T", "version": "1.0.0" }, + "paths": { + "/users/{id}": { + "get": { + "operationId": "get_user", + "parameters": [{ + "name": "id", "in": "path", + "required": true, + "schema": { "type": "string" } + }], + "responses": { + "200": { "description": "ok" } + } + } + } + }, + "components": { "schemas": {} } + }); + + let mut generated = blessed.clone(); + + // This is a new required query parameter, which means it is + // backward-incompatible. + let params = generated + .pointer_mut("/paths/~1users~1{id}/get/parameters") + .and_then(|v| v.as_array_mut()) + .unwrap(); + params.push(serde_json::json!({ + "name": "token", "in": "query", + "required": true, + "schema": { "type": "string" } + })); + + assert_render_snapshots("endpoint_only_change", &blessed, &generated); + } + + /// Tree with 2 siblings at depth 1, each with 2 siblings at depth 2. + #[test] + fn test_render_multi_sibling_tree() { + let ops = op_ids(&[ + ("#/paths/~1a/get", "list_a"), + ("#/paths/~1b/get", "list_b"), + ]); + let make_chain = |wrapper_field: &str, route: &str| { + vec![ + PathTreeKey::parse( + &format!( + "#/components/schemas/Wrapper/properties/{wrapper_field}/$ref" + ), + &ops, + ), + PathTreeKey::parse( + &format!( + "#/paths/~1{route}/get/responses/200/content/application~1json/schema/$ref" + ), + &ops, + ), + ] + }; + let mut tree = PathTree::default(); + tree.insert(make_chain("via_a", "a")); + tree.insert(make_chain("via_a", "b")); + tree.insert(make_chain("via_b", "a")); + tree.insert(make_chain("via_b", "b")); + + let issue = ApiCompatIssue { + blessed_base: component("#/components/schemas/SubType"), + generated_base: component("#/components/schemas/SubType"), + changes: BTreeSet::from([SubpathChange { + class: ChangeClass::Incompatible, + message: "schema types changed".into(), + old_subpath: DocumentPath::parse("properties/value"), + new_subpath: DocumentPath::parse("properties/value"), + }]), + tree, + blessed_value: None, + generated_value: None, + }; + assert_issue_snapshots("multi_sibling_tree", &issue); + } + + /// Tree under a single endpoint with shared prefixes that branch at + /// deeper levels. + /// + /// In this test: + /// + /// * `/a`'s response schema refers to `Wrapper` + /// * `Wrapper` has two fields that each refer to an `Inner` + /// * `Inner` has three fields (`x`, `y`, `z`) that all refer to `SubType` + /// + /// So three chains reach the change site: + /// + /// 1. endpoint → Wrapper.via_a → Inner.x → SubType + /// 2. endpoint → Wrapper.via_a → Inner.y → SubType + /// 3. endpoint → Wrapper.via_b → Inner.z → SubType + /// + /// The first two share `Wrapper.via_a` and fan out at `Inner`; the + /// third is its own branch. + #[test] + fn test_render_deep_branching_tree() { + // `tree.insert` expects entries in `JsonPathStack::iter().skip(1)` + // order: closest to the changed component first (drift's + // most-recent-ref is at the head), origin endpoint last. So + // `Inner` precedes `Wrapper` even though the endpoint visits + // `Wrapper` first when following refs forward. + let ops = op_ids(&[("#/paths/~1a/get", "list_a")]); + let make_chain = |wrapper_field: &str, inner_field: &str| { + vec![ + PathTreeKey::parse( + &format!( + "#/components/schemas/Inner/properties/{inner_field}/$ref" + ), + &ops, + ), + PathTreeKey::parse( + &format!( + "#/components/schemas/Wrapper/properties/{wrapper_field}/$ref" + ), + &ops, + ), + PathTreeKey::parse( + "#/paths/~1a/get/responses/200/content/application~1json/schema/$ref", + &ops, + ), + ] + }; + let mut tree = PathTree::default(); + tree.insert(make_chain("via_a", "x")); + tree.insert(make_chain("via_a", "y")); + tree.insert(make_chain("via_b", "z")); + + let issue = ApiCompatIssue { + blessed_base: component("#/components/schemas/SubType"), + generated_base: component("#/components/schemas/SubType"), + changes: BTreeSet::from([SubpathChange { + class: ChangeClass::Incompatible, + message: "schema types changed".into(), + old_subpath: DocumentPath::parse("properties/value"), + new_subpath: DocumentPath::parse("properties/value"), + }]), + tree, + blessed_value: None, + generated_value: None, + }; + assert_issue_snapshots("deep_branching_tree", &issue); + } + + #[test] + fn test_render_renamed_base() { + let issue = ApiCompatIssue { + blessed_base: component("#/components/schemas/OldName"), + generated_base: component("#/components/schemas/NewName"), + changes: BTreeSet::from([SubpathChange { + class: ChangeClass::Incompatible, + message: "schema types changed".into(), + old_subpath: DocumentPath::parse("properties/value"), + new_subpath: DocumentPath::parse("properties/value"), + }]), + tree: PathTree::default(), + blessed_value: None, + generated_value: None, + }; + assert_issue_snapshots("renamed_base", &issue); + } + + /// An added endpoint: drift reports the change with `Other(.paths)` + /// on the blessed side and the full `Endpoint(...)` on the generated + /// side. The `at:` line should show only the recognized side rather + /// than render the asymmetric pair as a rename from `.paths`. + #[test] + fn test_render_endpoint_added() { + let ops = op_ids(&[("#/paths/~1system~1info/get", "get_system_info")]); + let issue = ApiCompatIssue { + blessed_base: DocumentBasePath::classify( + DocumentPath::parse("/paths"), + &OperationIdMap::new(), + ), + generated_base: DocumentBasePath::classify( + DocumentPath::parse("/paths/~1system~1info/get"), + &ops, + ), + changes: BTreeSet::from([SubpathChange { + class: ChangeClass::ForwardIncompatible, + message: "The operation get_system_info was added".into(), + old_subpath: DocumentPath::root(), + new_subpath: DocumentPath::root(), + }]), + tree: PathTree::default(), + blessed_value: None, + generated_value: None, + }; + assert_issue_snapshots("endpoint_added", &issue); + } + + /// A subpath rename within a fixed base — the field name differs + /// between blessed and generated. The `at:` line surfaces the rename + /// with the `->` arrow. + #[test] + fn test_render_subpath_renamed() { + let issue = ApiCompatIssue { + blessed_base: component("#/components/schemas/Foo"), + generated_base: component("#/components/schemas/Foo"), + changes: BTreeSet::from([SubpathChange { + class: ChangeClass::Incompatible, + message: "field renamed".into(), + old_subpath: DocumentPath::parse("properties/old_name"), + new_subpath: DocumentPath::parse("properties/new_name"), + }]), + tree: PathTree::default(), + blessed_value: None, + generated_value: None, + }; + assert_issue_snapshots("subpath_renamed", &issue); + } + + /// An [`ChangeClass::Unhandled`] change renders with the neutral + /// `change:` keyword rather than a severity color. + #[test] + fn test_render_unhandled_class() { + let issue = ApiCompatIssue { + blessed_base: component("#/components/schemas/Foo"), + generated_base: component("#/components/schemas/Foo"), + changes: BTreeSet::from([SubpathChange { + class: ChangeClass::Unhandled, + message: "array maxItems changed".into(), + old_subpath: DocumentPath::parse("items"), + new_subpath: DocumentPath::parse("items"), + }]), + tree: PathTree::default(), + blessed_value: None, + generated_value: None, + }; + assert_issue_snapshots("unhandled_class", &issue); + } + + /// Multiple non-trivial changes within the same component render as + /// separate self-contained blocks separated by a blank line. + #[test] + fn test_render_multiple_changes_in_one_component() { + let blessed = doc_with_ref_schema( + "#/components/schemas/Wrapper", + serde_json::json!({ + "Wrapper": { + "type": "object", + "properties": { + "a": { "type": "string" }, + "b": { "type": "integer" } + }, + "required": ["a", "b"] + } + }), + ); + + let mut generated = blessed.clone(); + // Flip both field types — two backward-incompatible changes at + // the same schema base. + *generated + .pointer_mut("/components/schemas/Wrapper/properties/a/type") + .unwrap() = serde_json::Value::String("integer".into()); + *generated + .pointer_mut("/components/schemas/Wrapper/properties/b/type") + .unwrap() = serde_json::Value::String("string".into()); + + assert_render_snapshots( + "multiple_changes_in_one_component", + &blessed, + &generated, + ); + } + + /// Abbreviated rendering of an issue with multiple [`SubpathChange`]s + /// — the `(see foo v1.0.0)` back-reference must appear *only* on the + /// first severity line. The grouping logic stacks both severities at + /// the top of one block, sharing the `exposed:` tree below; the + /// pointer to the first occurrence is a property of the issue, not of + /// any one change within it, so repeating it on every line would just + /// be noise. + #[test] + fn test_render_abbreviated_multi_change() { + let ops = op_ids(&[("#/paths/~1bar1/get", "get_bar1")]); + let make_chain = |route: &str| { + vec![ + PathTreeKey::parse( + "#/components/schemas/Wrapper/properties/value/$ref", + &ops, + ), + PathTreeKey::parse( + &format!( + "#/paths/~1{route}/get/responses/200/content/application~1json/schema/$ref" + ), + &ops, + ), + ] + }; + let mut tree = PathTree::default(); + tree.insert(make_chain("bar1")); + + let issue = ApiCompatIssue { + blessed_base: component("#/components/schemas/SubType"), + generated_base: component("#/components/schemas/SubType"), + changes: BTreeSet::from([ + SubpathChange { + class: ChangeClass::Incompatible, + message: "schema kind changed".into(), + old_subpath: DocumentPath::parse("properties/value"), + new_subpath: DocumentPath::parse("properties/value"), + }, + SubpathChange { + class: ChangeClass::Unhandled, + message: "object properties changed".into(), + old_subpath: DocumentPath::root(), + new_subpath: DocumentPath::root(), + }, + ]), + tree, + blessed_value: None, + generated_value: None, + }; + + let prev = OwnedLoc::new("foo", "1.0.0"); + let mut colorized = Styles::default(); + colorized.colorize(); + expectorate::assert_contents( + output_path("abbreviated_multi_change.txt"), + &format!( + "{}\n", + issue.display_abbreviated(&Styles::default(), prev.as_loc()), + ), + ); + expectorate::assert_contents( + output_path("abbreviated_multi_change.ansi"), + &format!( + "{}\n", + issue.display_abbreviated(&colorized, prev.as_loc()), + ), + ); + } + + /// Snapshot the abbreviated rendering used for duplicate issues. The + /// header and the per-API tree should both appear; the back-reference + /// `(see foo v1.0.0)` should be on the header line. + #[test] + fn test_render_abbreviated() { + let ops = op_ids(&[ + ("#/paths/~1bar1/get", "get_bar1"), + ("#/paths/~1bar2/get", "get_bar2"), + ]); + let make_chain = |route: &str| { + vec![ + PathTreeKey::parse( + "#/components/schemas/Wrapper/properties/value/$ref", + &ops, + ), + PathTreeKey::parse( + &format!( + "#/paths/~1{route}/get/responses/200/content/application~1json/schema/$ref" + ), + &ops, + ), + ] + }; + let mut tree = PathTree::default(); + tree.insert(make_chain("bar1")); + tree.insert(make_chain("bar2")); + + let issue = ApiCompatIssue { + blessed_base: component("#/components/schemas/SubType"), + generated_base: component("#/components/schemas/SubType"), + changes: BTreeSet::from([SubpathChange { + class: ChangeClass::Incompatible, + message: "schema types changed".into(), + old_subpath: DocumentPath::parse("properties/value"), + new_subpath: DocumentPath::parse("properties/value"), + }]), + tree, + blessed_value: None, + generated_value: None, + }; + + let prev = OwnedLoc::new("foo", "1.0.0"); + let mut colorized = Styles::default(); + colorized.colorize(); + expectorate::assert_contents( + output_path("abbreviated.txt"), + &format!( + "{}\n", + issue.display_abbreviated(&Styles::default(), prev.as_loc()), + ), + ); + expectorate::assert_contents( + output_path("abbreviated.ansi"), + &format!( + "{}\n", + issue.display_abbreviated(&colorized, prev.as_loc()), + ), + ); + } + + /// Snapshot the issue in its surrounding render context: a cargo-style + /// `Failure` header, the second-tier `error:` keyword, and the issue + /// body indented so its leftmost label aligns with where `error` + /// begins. This validates the design intent that the verb column + /// "extends downward" from the cargo headers through `error:` into + /// the per-issue labels. + #[test] + fn test_render_in_context() { + // Mirror the indents picked by `display_resolution_problems` / + // `display_compat_issue` in `output.rs` so this test exercises the + // same alignment math. + const HEADER_WIDTH: usize = 12; + let issue_indent = " ".repeat(HEADER_WIDTH - "error".len()); + + let ops = op_ids(&[( + "#/paths/~1v1~1system~1audit-log/get", + "audit_log_list", + )]); + let make_chain = |route: &str| { + vec![ + PathTreeKey::parse( + "#/components/schemas/AuditLogEntryResultsPage\ + /properties/items/items/$ref", + &ops, + ), + PathTreeKey::parse( + &format!( + "#/paths/~1v1~1system~1{route}/get/responses/200/content\ + /application~1json/schema/$ref" + ), + &ops, + ), + ] + }; + let mut tree = PathTree::default(); + tree.insert(make_chain("audit-log")); + + let issue = ApiCompatIssue { + blessed_base: component("#/components/schemas/AuditLogEntry"), + generated_base: component("#/components/schemas/AuditLogEntry"), + changes: BTreeSet::from([ + SubpathChange { + class: ChangeClass::Incompatible, + message: "schema kind changed".into(), + old_subpath: DocumentPath::parse("properties/auth_method"), + new_subpath: DocumentPath::parse("properties/auth_method"), + }, + SubpathChange { + class: ChangeClass::Unhandled, + message: "object properties changed".into(), + old_subpath: DocumentPath::root(), + new_subpath: DocumentPath::root(), + }, + ]), + tree, + blessed_value: None, + generated_value: None, + }; + + let render = |styles: &Styles| -> String { + let mut out = String::new(); + // The full preceding cargo lines aren't relevant to alignment — + // just the Failure header that immediately precedes `error:`. + writeln!( + &mut out, + "{:>HEADER_WIDTH$} nexus (versioned v2025112000.0.0 \ + (blessed)): Oxide Region API", + "Failure".style(styles.failure_header), + ) + .unwrap(); + writeln!( + &mut out, + "{:>HEADER_WIDTH$}: OpenAPI document is incompatible with \ + the blessed upstream", + "error".style(styles.failure_header), + ) + .unwrap(); + writeln!(&mut out).unwrap(); + // Indent the issue body. + let mut indented = String::new(); + write!( + indent_write::fmt::IndentWriter::new( + &issue_indent, + &mut indented, + ), + "{}", + issue.display(styles), + ) + .unwrap(); + out.push_str(&indented); + out.push('\n'); + out + }; + + let mut colorized = Styles::default(); + colorized.colorize(); + expectorate::assert_contents( + output_path("in_context.txt"), + &render(&Styles::default()), + ); + expectorate::assert_contents( + output_path("in_context.ansi"), + &render(&colorized), + ); + } +} diff --git a/crates/dropshot-api-manager/src/compatibility/mod.rs b/crates/dropshot-api-manager/src/compatibility/mod.rs index efe9e25..bd4d919 100644 --- a/crates/dropshot-api-manager/src/compatibility/mod.rs +++ b/crates/dropshot-api-manager/src/compatibility/mod.rs @@ -3,5 +3,10 @@ //! Determine if one OpenAPI document is a subset of another. mod detect; +mod display; +mod types; -pub use detect::{ApiCompatIssue, api_compatible}; +pub use detect::api_compatible; +pub(crate) use detect::{CompatDedupeMap, DedupeStatus}; +pub use types::ApiCompatIssue; +pub(crate) use types::CompatIssueLocation; diff --git a/crates/dropshot-api-manager/src/compatibility/types.rs b/crates/dropshot-api-manager/src/compatibility/types.rs new file mode 100644 index 0000000..737905d --- /dev/null +++ b/crates/dropshot-api-manager/src/compatibility/types.rs @@ -0,0 +1,464 @@ +// Copyright 2026 Oxide Computer Company + +//! Data model for compatibility issues. +//! +//! These types are shared between [`super::detect`] (which builds them from +//! drift output) and [`super::display`] (which renders them for the CLI). The +//! detection algorithm lives in `detect`; this module is purely the shape of +//! what gets passed between the two. + +use super::display::ApiCompatIssueDisplay; +use crate::output::Styles; +use drift::ChangeClass; +use dropshot_api_manager_types::ApiIdent; +use std::{ + collections::{BTreeMap, BTreeSet, HashMap}, + fmt, +}; + +/// A compatibility error between two OpenAPI documents. +/// +/// Each issue corresponds to one component or endpoint that has non-trivial +/// changes. The same component may be reachable from multiple endpoints; the +/// inverted reference tree records every such chain so we can show all affected +/// endpoints. +#[derive(Debug)] +pub struct ApiCompatIssue { + /// Base location in the blessed (old) document, e.g. a schema component + /// or an endpoint. + pub(super) blessed_base: DocumentBasePath, + /// Base location in the generated (new) document. + /// + /// Differs from `blessed_base` only when the component or endpoint was + /// renamed. + pub(super) generated_base: DocumentBasePath, + /// Non-trivial changes detected within this base location. + /// + /// A `BTreeSet` rather than a `Vec` so that: + /// * iteration (and therefore display) order is deterministic regardless + /// of the order drift emits changes in; + /// * dedupe comparison via `is_same_change_as` is order-independent — two + /// issues that name the same set of changes in different orders are + /// correctly recognized as duplicates. + pub(super) changes: BTreeSet, + /// Inverted reference tree. + /// + /// Empty when the change is directly at an endpoint with no `$ref` + /// indirection. + pub(super) tree: PathTree, + /// Cached blessed JSON value at the base, for diff display. + pub(super) blessed_value: Option, + /// Cached generated JSON value at the base, for diff display. + pub(super) generated_value: Option, +} + +impl ApiCompatIssue { + pub(crate) fn blessed_json(&self) -> String { + to_json_pretty(self.blessed_value.as_ref()) + } + + pub(crate) fn generated_json(&self) -> String { + to_json_pretty(self.generated_value.as_ref()) + } + + pub(crate) fn display<'a>( + &'a self, + styles: &'a Styles, + ) -> ApiCompatIssueDisplay<'a> { + ApiCompatIssueDisplay { issue: self, styles, prev: None } + } + + /// Returns a `Display` adapter that renders this issue with a + /// `(see v)` back-reference to the first occurrence of + /// the same issue. + /// + /// Used for cross-API duplicates, to suppress the bulky JSON diff and emit + /// only the header and the per-API "used by" list. + pub(crate) fn display_abbreviated<'a>( + &'a self, + styles: &'a Styles, + prev: CompatIssueLocation<'a>, + ) -> ApiCompatIssueDisplay<'a> { + ApiCompatIssueDisplay { issue: self, styles, prev: Some(prev) } + } + + /// Returns true if `self` and `other` represent the same underlying + /// compatibility change for cross-API dedup purposes. + /// + /// `Self::tree` is deliberately excluded for this comparison. That's + /// because if for two versions of an API, a change is the same along all + /// dimensions other than the tree, we still want to treat it as a + /// duplicate. (The abbreviated display renders the tree for duplicates, so + /// there's no loss of information this way.) + pub(super) fn is_same_change_as(&self, other: &Self) -> bool { + let Self { + blessed_base, + generated_base, + changes, + // See doc comment for why tree is excluded. + tree: _, + // We do include the blessed and generated value fields so that the + // same schema name with different values doesn't get deduplicated. + blessed_value, + generated_value, + } = self; + *blessed_base == other.blessed_base + && *generated_base == other.generated_base + && *changes == other.changes + && *blessed_value == other.blessed_value + && *generated_value == other.generated_value + } +} + +/// A label identifying which API and version first reported a given +/// compatibility issue. +/// +/// Used by [`super::detect::CompatDedupeMap`] to point readers from a +/// deduplicated issue back to its first occurrence. The fields borrow from +/// the [`super::super::resolved::Resolved`] that the dedupe map was built +/// over, sharing the map's lifetime. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) struct CompatIssueLocation<'a> { + pub(crate) api: &'a ApiIdent, + pub(crate) version: &'a semver::Version, +} + +impl fmt::Display for CompatIssueLocation<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{} v{}", self.api, self.version) + } +} + +/// A non-trivial change at a particular subpath within a base location. +/// +/// One [`ApiCompatIssue`] may carry several `SubpathChange`s when multiple +/// non-trivial changes were detected within the same component or +/// endpoint. +/// +/// The derived `Ord` follows struct field order — `(class, message, +/// old_subpath, new_subpath)` — and is used by the surrounding +/// [`BTreeSet`] for deterministic iteration. The ordering itself is +/// arbitrary; only its totality matters. +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] +pub(super) struct SubpathChange { + pub(super) class: ChangeClass, + pub(super) message: String, + /// Subpath of the change within the base location, on the blessed side. + pub(super) old_subpath: DocumentPath, + /// Subpath on the generated side. Same as `old_subpath` unless a field + /// within the base was renamed. + pub(super) new_subpath: DocumentPath, +} + +/// A base location within an OpenAPI document. +/// +/// This enum classifies base document paths so the rendering layer can pattern +/// match on the variant rather than asking "is this a component? is this an +/// endpoint?" each time it inspects a base path. For endpoint variants, the +/// spec's `operationId` is carried alongside the path so that display code can +/// annotate it on the rendered endpoint. +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub(super) enum DocumentBasePath { + /// A reusable component, e.g., `#/components/schemas/Foo`. + Component(DocumentPath), + /// An endpoint, e.g., `#/paths/~1users~1{id}/get`. + Endpoint { + /// The path (`paths//`). + name: DocumentPath, + /// The endpoint's `operationId` from the spec, if found. + operation_id: Option, + }, + /// The `.paths` container itself — not a meaningful change location. + /// + /// Drift emits `.paths` on whichever side is *missing* an endpoint when + /// one is added or removed: the `paths` map exists on both sides, even + /// though the leaf doesn't. Classifying this explicitly (rather than + /// letting it fall into [`Self::Other`]) lets the renderer treat it as a + /// "non-location" — see how `display::write_at_content` drops a + /// `PathsRoot` paired with a real `Component`/`Endpoint`. + /// + /// Drift 0.2's `compare.rs` only diffs operations, not top-level + /// component additions/removals, so `.components.` containers + /// never need their own variant here. If drift later adds + /// component-level comparison, expect a similar variant per container. + PathsRoot, + /// Anything that doesn't match a known base shape. + /// + /// This shouldn't happen in practice, but we preserve these paths anyway + /// so the renderer can still fall back to something reasonable (and not + /// just drop the path). + Other(DocumentPath), +} + +impl DocumentBasePath { + /// Classify a raw `path`: + /// + /// * `components//` is [`Self::Component`]. + /// * `paths//` is [`Self::Endpoint`], with the operation id + /// looked up in `op_ids` (absent or non-string `operationId` + /// becomes `None`). + /// * `paths` (1 segment) is [`Self::PathsRoot`] — the parent container + /// of endpoints, which drift uses when an endpoint is added or + /// removed on one side. + /// * Anything else is [`Self::Other`]. + pub(super) fn classify( + path: DocumentPath, + op_ids: &OperationIdMap<'_>, + ) -> Self { + match path.segments.as_slice() { + [a, _, _] if a == "components" => Self::Component(path), + [a, _, _] if a == "paths" => { + let operation_id = op_ids.get(&path).map(|s| s.to_string()); + Self::Endpoint { name: path, operation_id } + } + [a] if a == "paths" => Self::PathsRoot, + _ => Self::Other(path), + } + } + + /// Returns true if this base is the `.paths` container root rather than + /// a real endpoint or component location. Used by display code to drop + /// the missing side of an add/remove pair. + pub(super) fn is_paths_root(&self) -> bool { + matches!(self, Self::PathsRoot) + } +} + +/// Composite key for [`PathTree`]: a base location together with the +/// subpath of the `$ref` source within that base. Each tree edge +/// corresponds to one such `(base, subpath)` pair. +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub(super) struct PathTreeKey { + pub(super) base: DocumentBasePath, + pub(super) subpath: DocumentPath, +} + +impl PathTreeKey { + /// Parse a `JsonPathStack` ref entry from drift into a typed + /// `PathTreeKey`. + /// + /// Drift returns refs as `//$ref`. For Dropshot (which is + /// what we're targeting), the base is either + /// `#/paths//` or `#/components//` — + /// three segments once the leading `#/` is stripped. + /// + /// Anything not ending in `/$ref` is unexpected, so we fall back to + /// treating the whole entry as the base with no subpath. (This happens + /// naturally because a ≤ 3-segment path leaves nothing for the subpath + /// half after [`DocumentPath::split_at`].) + /// + /// `op_ids` is consulted by [`DocumentBasePath::classify`] when the + /// parsed base shapes up as an endpoint. + pub(super) fn parse(entry: &str, op_ids: &OperationIdMap<'_>) -> Self { + /// Number of segments in a Dropshot ref base + /// (`components//` or `paths//`), + /// matching the length check in [`DocumentBasePath::classify`]. + const BASE_SEGMENTS: usize = 3; + + let without_ref = entry.strip_suffix("/$ref").unwrap_or(entry); + let (base, subpath) = + DocumentPath::parse(without_ref).split_at(BASE_SEGMENTS); + Self { base: DocumentBasePath::classify(base, op_ids), subpath } + } +} + +/// An inverted reference tree, rooted at the directly-changed component or +/// endpoint. +/// +/// Each entry in `children` is one immediate `$ref` source pointing at the +/// root: the key is the location of the ref, and the value is the subtree of +/// further refs pointing at *that* node, walking back along the `$ref` chain +/// all the way out to originating endpoints. +/// +/// `BTreeMap` is used so rendering order is sorted rather than the order drift +/// happens to yield paths in. +/// +/// For example, the chain "SubType ← Wrapper(.properties.via_a) ← +/// /a(.responses.200…/schema)" produces: +/// +/// ```text +/// PathTree { +/// children: { +/// PathTreeKey { +/// base: Component(".components.schemas.Wrapper"), +/// subpath: ".properties.via_a", +/// } => PathTree { +/// children: { +/// PathTreeKey { +/// base: Endpoint { +/// name: ".paths.\"/a\".get", +/// operation_id: Some("get_a"), +/// }, +/// subpath: ".responses.200…schema", +/// } => PathTree { children: {} }, +/// }, +/// }, +/// }, +/// } +/// ``` +#[derive(Debug, Default)] +pub(super) struct PathTree { + pub(super) children: BTreeMap, +} + +impl PathTree { + /// Insert one `$ref` chain of [`PathTreeKey`]s, in leaf-first order. + /// + /// Each entry walks one level deeper into the tree: if a sibling at + /// the current level already has the same key, reuse it and merge the + /// rest of the chain into its children. Otherwise, append a new + /// sibling. + /// + /// Two chains sharing only `base` (e.g., the same type referenced + /// from multiple fields) produce separate sibling entries: each + /// represents a distinct route from endpoint to changed component. + pub(super) fn insert( + &mut self, + chain: impl IntoIterator, + ) { + let mut curr = self; + for key in chain { + curr = curr.children.entry(key).or_default(); + } + } +} + +/// A map from endpoint base (`paths//`) to its +/// `operationId`. Built once per document in [`super::detect::api_compatible`] +/// and consulted during issue construction to populate +/// [`DocumentBasePath::Endpoint::operation_id`]. +/// +/// The map values are borrowed from the `serde_json::Value` representing the +/// OpenAPI document so that we don't allocate for the (typically large) set of op +/// ids that are never referenced by an issue; the few that are looked up get +/// cloned at the lookup site when constructing the owned `Endpoint` variant. +pub(super) type OperationIdMap<'a> = HashMap; + +/// A location within an OpenAPI document, parsed from a JSON Pointer into +/// its component segments. +/// +/// For base locations, code that needs to know whether a base names a component +/// or an endpoint should use [`DocumentBasePath`] instead -- it makes that +/// distinction part of the type, and for endpoints the `operationId` is also +/// stored. +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub(super) struct DocumentPath { + /// The list of segments in this path. + /// + /// * For the root path (`.`), this is empty. + /// * Otherwise, there's one entry per `/`-separated component. + /// + /// Note that the segment is stored in unescaped form. This means: + /// + /// * JSON Pointer escapes have been decoded. + /// * Whether to add quotes is determined in `write`. + pub(super) segments: Vec, +} + +impl DocumentPath { + /// Parse a JSON Pointer (or relative subpath) into segments. + /// + /// Accepts a leading `#` or `/`. + /// + /// An empty input is treated as the root path. + pub(super) fn parse(pointer: &str) -> Self { + let trimmed = pointer.trim_matches('#').trim_matches('/'); + if trimmed.is_empty() { + return Self::root(); + } + let segments = + trimmed.split('/').map(unescape_pointer_component).collect(); + Self { segments } + } + + pub(super) fn root() -> Self { + Self { segments: Vec::new() } + } + + pub(super) fn is_root(&self) -> bool { + self.segments.is_empty() + } + + /// Split into the first `n` segments and the remainder. + /// + /// If this path has `n` or fewer segments, the head is the whole path and + /// the tail is [`Self::root`]. + pub(super) fn split_at(mut self, n: usize) -> (Self, Self) { + if n >= self.segments.len() { + return (self, Self::root()); + } + let tail = Self { segments: self.segments.split_off(n) }; + (self, tail) + } +} + +/// Decode a JSON Pointer component (RFC 6901): `~1` → `/`, `~0` → `~`. +pub(super) fn unescape_pointer_component(component: &str) -> String { + component.replace("~1", "/").replace("~0", "~") +} + +fn to_json_pretty(value: Option<&serde_json::Value>) -> String { + match value { + Some(value) => serde_json::to_string_pretty(value) + .expect("serializing serde_json::Value should always succeed"), + None => String::new(), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Shorthand for `DocumentPath { segments: [...] }`. + fn path(segments: &[&str]) -> DocumentPath { + DocumentPath { + segments: segments.iter().map(|&s| s.to_owned()).collect(), + } + } + + #[test] + fn test_path_tree_key_parse() { + // Each case is (input ref entry, expected base, expected subpath + // segments). + let ops = OperationIdMap::new(); + let cases: &[(&str, DocumentBasePath, &[&str])] = &[ + ( + "#/components/schemas/Foo/properties/x/$ref", + DocumentBasePath::Component(path(&[ + "components", + "schemas", + "Foo", + ])), + &["properties", "x"], + ), + ( + "#/paths/~1users/get/responses/200/content/application~1json/schema/$ref", + DocumentBasePath::Endpoint { + name: path(&["paths", "/users", "get"]), + operation_id: None, + }, + &["responses", "200", "content", "application/json", "schema"], + ), + // Without a trailing /$ref (e.g., the leaf of a path stack), the + // whole entry is the base, and the subpath is the root. + ( + "#/components/schemas/Foo", + DocumentBasePath::Component(path(&[ + "components", + "schemas", + "Foo", + ])), + &[], + ), + ]; + for (entry, want_base, want_subpath) in cases { + let key = PathTreeKey::parse(entry, &ops); + assert_eq!(&key.base, want_base, "base for entry {entry}"); + assert_eq!( + key.subpath.segments.as_slice(), + *want_subpath, + "subpath for entry {entry}", + ); + } + } +} diff --git a/crates/dropshot-api-manager/src/output.rs b/crates/dropshot-api-manager/src/output.rs index 1aa656c..e621847 100644 --- a/crates/dropshot-api-manager/src/output.rs +++ b/crates/dropshot-api-manager/src/output.rs @@ -3,6 +3,9 @@ use crate::{ FAILURE_EXIT_CODE, NEEDS_UPDATE_EXIT_CODE, apis::{ManagedApi, ManagedApis}, + compatibility::{ + ApiCompatIssue, CompatDedupeMap, CompatIssueLocation, DedupeStatus, + }, environment::{ErrorAccumulator, ResolvedEnv}, resolved::{ Fix, NonVersionProblem, Resolution, ResolutionKind, Resolved, @@ -55,13 +58,16 @@ impl OutputOpts { #[derive(Clone, Debug, Default)] pub(crate) struct Styles { pub(crate) bold: Style, + pub(crate) dimmed: Style, pub(crate) header: Style, pub(crate) success_header: Style, pub(crate) failure: Style, pub(crate) failure_header: Style, + pub(crate) warning: Style, pub(crate) warning_header: Style, pub(crate) unchanged_header: Style, pub(crate) filename: Style, + pub(crate) operation_id: Style, pub(crate) diff_before: Style, pub(crate) diff_after: Style, } @@ -69,13 +75,16 @@ pub(crate) struct Styles { impl Styles { pub(crate) fn colorize(&mut self) { self.bold = Style::new().bold(); + self.dimmed = Style::new().dimmed(); self.header = Style::new().purple(); self.success_header = Style::new().green().bold(); self.failure = Style::new().red(); self.failure_header = Style::new().red().bold(); - self.unchanged_header = Style::new().blue().bold(); + self.warning = Style::new().yellow(); self.warning_header = Style::new().yellow().bold(); + self.unchanged_header = Style::new().blue().bold(); self.filename = Style::new().cyan(); + self.operation_id = Style::new().purple(); self.diff_before = Style::new().red(); self.diff_after = Style::new().green(); } @@ -299,6 +308,10 @@ pub fn display_resolution( let mut num_failed = 0; let mut num_non_version_problems = 0; + // Built up front so per-API rendering can elide changes that were + // already shown in full for another API or version. + let dedupe = resolved.build_compat_dedupe_map(); + // Print problems associated with a supported API version // (i.e., one of the expected OpenAPI documents). for api in apis.iter_apis() { @@ -315,7 +328,9 @@ pub fn display_resolution( } else { num_fresh += 1; } - summarize_one(writer, env, api, version, resolution, styles)?; + summarize_one( + writer, env, api, version, resolution, styles, &dedupe, + )?; } if !api.is_versioned() { @@ -476,6 +491,7 @@ fn summarize_one( version: &semver::Version, resolution: &Resolution<'_>, styles: &Styles, + dedupe: &CompatDedupeMap, ) -> io::Result<()> { let problems: Vec<_> = resolution.problems().collect(); if problems.is_empty() { @@ -500,18 +516,34 @@ fn summarize_one( display_api_spec_version(api, version, styles, resolution), )?; - display_version_problems(writer, env, problems, styles)?; + let compat_ctx = CompatDisplayContext { + dedupe, + current: CompatIssueLocation { api: api.ident(), version }, + }; + display_version_problems(writer, env, problems, styles, compat_ctx)?; } Ok(()) } +/// Bundles a [`CompatDedupeMap`] with the API/version location currently +/// being rendered, so [`display_version_problems`] can identify duplicate +/// compatibility issues and abbreviate them. +pub(crate) struct CompatDisplayContext<'a> { + pub(crate) dedupe: &'a CompatDedupeMap<'a>, + pub(crate) current: CompatIssueLocation<'a>, +} + /// Print a formatted list of per-(api, version) [`VersionProblem`]s to /// `writer`, including any compatibility-issue diffs and fix descriptions. +/// +/// `compat_ctx` enables [`VersionProblem::BlessedVersionBroken`] rendering to +/// abbreviate compatibility issues that have already been shown elsewhere. pub fn display_version_problems<'a, T>( writer: &mut dyn io::Write, env: &ResolvedEnv, problems: T, styles: &Styles, + compat_ctx: CompatDisplayContext<'_>, ) -> io::Result<()> where T: IntoIterator>, @@ -519,54 +551,39 @@ where for p in problems.into_iter() { write_problem_header(writer, p, p.is_fixable(), styles)?; - // Body indent used by the nested BlessedVersionBroken issue list. - // Mirrors the continuation indent inside the problem header so - // wrapped issue lines line up under the header text. - let more_indent = " ".repeat(HEADER_WIDTH + 4 + 2); + // Indent for compat-issue bodies. The issue's longest label sits + // at this column so its leftmost edge lines up with where `error` + // begins (HEADER_WIDTH minus the 5 chars of "error"), preserving + // the verb column the eye is already tracking down from the + // cargo headers. + let issue_indent = " ".repeat(HEADER_WIDTH - "error".len()); // For BlessedVersionBroken, print each item separately, along with a - // diff between blessed and generated versions. + // diff between blessed and generated versions. If a compat dedupe + // context is provided, an issue that was already reported for a + // different API/version is rendered in abbreviated form. + // + // Each issue is surrounded by blank lines: the leading blank in + // `display_compat_issue` separates it from the problem header (or + // the previous issue's diff), and a single trailing blank here + // separates the last issue from the next problem. if let VersionProblem::BlessedVersionBroken { compatibility_issues } = &p { for issue in compatibility_issues { - // Print each compatibility issue on a new line, prefixed with - // "- ". - let nested_first_indent = format!("{}- ", more_indent); - let nested_more_indent = format!("{} ", more_indent); - writeln!( - writer, - "{}", - textwrap::fill( - &issue.to_string(), - textwrap::Options::new(term_width()) - .initial_indent(&nested_first_indent) - .subsequent_indent(&nested_more_indent) - ) - )?; - - // Now print a textual diff between the blessed and generated - // versions. - let blessed_json = issue.blessed_json(); - let generated_json = issue.generated_json(); - - let diff = TextDiff::from_lines(&blessed_json, &generated_json); - write_diff( - &diff, - "blessed".as_ref(), - "generated".as_ref(), + let dedupe_status = + compat_ctx.dedupe.lookup(issue, compat_ctx.current); + display_compat_issue( + &mut *writer, + issue, + &issue_indent, styles, - // context_radius: use a large radius to ensure that most of - // the schema is printed out. - 8, - /* missing_newline_hint */ false, - // Add an indent to align diff with the status message. - &mut indent_write::io::IndentWriter::new( - &nested_more_indent, - &mut *writer, - ), + &dedupe_status, )?; } + if !compatibility_issues.is_empty() { + writeln!(writer)?; + } } // For BlessedLatestVersionBytewiseMismatch, show a diff between blessed @@ -684,22 +701,27 @@ where /// Write the `problem:` / `error:` header line for a single problem, wrapping /// the error chain to the terminal width. +/// +/// The keyword is the second-tier verb: it continues the cargo-style verb +/// column from the surrounding `Fresh` / `Failure` headers. It's right-aligned +/// to the same width so the colons stack vertically as the eye scans down. fn write_problem_header( writer: &mut dyn io::Write, error: &dyn std::error::Error, is_fixable: bool, styles: &Styles, ) -> io::Result<()> { - let subheader_width = HEADER_WIDTH + 4; let first_indent = format!( - "{:>subheader_width$}: ", + "{:>HEADER_WIDTH$}: ", if is_fixable { "problem".style(styles.warning_header) } else { "error".style(styles.failure_header) } ); - let more_indent = " ".repeat(subheader_width + 2); + // Continuation indent for wrapped error text. Aligns with the + // post-keyword content (HEADER_WIDTH + ": ".len()). + let more_indent = " ".repeat(HEADER_WIDTH + 2); writeln!( writer, "{}", @@ -720,10 +742,9 @@ fn write_fix_summary( fix: &Fix<'_>, styles: &Styles, ) -> io::Result<()> { - let subheader_width = HEADER_WIDTH + 4; let first_indent = - format!("{:>subheader_width$}: ", "fix".style(styles.warning_header)); - let more_indent = " ".repeat(subheader_width + 2); + format!("{:>HEADER_WIDTH$}: ", "fix".style(styles.warning_header)); + let more_indent = " ".repeat(HEADER_WIDTH + 2); let fix_str = fix.to_string(); for s in fix_str.trim_end().split("\n") { writeln!( @@ -740,6 +761,74 @@ fn write_fix_summary( Ok(()) } +/// Render one compatibility issue under a problem to `writer`. +/// +/// `body_indent` is the column the issue body starts at (each rendered +/// line gets this prefix). The labels right-align within an issue's own +/// colon column, so a single `body_indent` is sufficient — no separate +/// initial/continuation indents are needed. +/// +/// `dedupe_status` controls the rendering form: [`FirstOccurrence`] renders +/// the issue in full, while [`Duplicate`] renders an abbreviated form that +/// drops the JSON diff and just emits the labeled block with a `(see ...)` +/// back-reference to the first occurrence. +/// +/// [`FirstOccurrence`]: DedupeStatus::FirstOccurrence +/// [`Duplicate`]: DedupeStatus::Duplicate +fn display_compat_issue( + writer: &mut dyn io::Write, + issue: &ApiCompatIssue, + body_indent: &str, + styles: &Styles, + dedupe_status: &DedupeStatus<'_>, +) -> io::Result<()> { + // A blank line separates this issue from the previous problem header + // (or, in the full form, from the previous issue's JSON diff which + // already ends in a newline). + writeln!(writer)?; + + let display = match dedupe_status { + DedupeStatus::FirstOccurrence => issue.display(styles), + DedupeStatus::Duplicate { first_occurrence } => { + issue.display_abbreviated(styles, *first_occurrence) + } + }; + + // Indent every line of the rendered block. `IndentWriter` prefixes the + // first line as well, so we don't need a separate initial-indent string. + let mut buf = String::new(); + write!(IndentWriter::new(body_indent, &mut buf), "{display}") + .expect("writing to a String never fails"); + writeln!(writer, "{buf}")?; + + match dedupe_status { + DedupeStatus::FirstOccurrence => { + // Full form: print the textual diff between the blessed and + // generated values for this base. + let blessed_json = issue.blessed_json(); + let generated_json = issue.generated_json(); + + let diff = TextDiff::from_lines(&blessed_json, &generated_json); + write_diff( + &diff, + "blessed".as_ref(), + "generated".as_ref(), + styles, + // context_radius: use a large radius to ensure that most + // of the schema is printed out. + 8, + /* missing_newline_hint */ false, + // Align diff with the issue body. + &mut indent_write::io::IndentWriter::new(body_indent, writer), + ) + } + DedupeStatus::Duplicate { .. } => { + // Abbreviated form: no JSON diff, since it was already shown + // at the first occurrence. + Ok(()) + } + } +} /// Adapter for [`Error`]s that provides a [`std::fmt::Display`] implementation /// that print the full chain of error sources, separated by `: `. pub struct InlineErrorChain<'a>(&'a dyn std::error::Error); diff --git a/crates/dropshot-api-manager/src/resolved.rs b/crates/dropshot-api-manager/src/resolved.rs index 6039f5d..fb9c893 100644 --- a/crates/dropshot-api-manager/src/resolved.rs +++ b/crates/dropshot-api-manager/src/resolved.rs @@ -4,7 +4,9 @@ use crate::{ apis::{ManagedApi, ManagedApis}, - compatibility::{ApiCompatIssue, api_compatible}, + compatibility::{ + ApiCompatIssue, CompatDedupeMap, CompatIssueLocation, api_compatible, + }, environment::ResolvedEnv, iter_only::iter_only, output::{InlineErrorChain, plural}, @@ -1295,6 +1297,36 @@ impl<'a> Resolved<'a> { self.api_results.get(ident).and_then(|v| v.by_version.get(version)) } + /// Build a [`CompatDedupeMap`] over every `BlessedVersionBroken` problem + /// across every API and version, in iteration order (sorted by API ident, + /// then by semver). + /// + /// The first occurrence of each unique compatibility issue will be rendered + /// in full (with a diff of the JSON). Subsequent occurrences will be + /// rendered as back-references to the first occurrence. + pub fn build_compat_dedupe_map(&self) -> CompatDedupeMap<'_> { + let mut dedupe = CompatDedupeMap::default(); + for (api, api_resolved) in &self.api_results { + for (version, resolution) in &api_resolved.by_version { + for problem in resolution.problems() { + let VersionProblem::BlessedVersionBroken { + compatibility_issues, + } = problem + else { + continue; + }; + for issue in compatibility_issues { + dedupe.insert( + CompatIssueLocation { api, version }, + issue, + ); + } + } + } + } + dedupe + } + /// Returns the "latest" symlink problem for an API, if any. /// /// Symlink problems are [`NonVersionProblem`]s but live separately from diff --git a/crates/dropshot-api-manager/tests/output/compatibility/abbreviated.ansi b/crates/dropshot-api-manager/tests/output/compatibility/abbreviated.ansi new file mode 100644 index 0000000..93e3b8b --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/abbreviated.ansi @@ -0,0 +1,6 @@ +incompatible: schema types changed (see foo v1.0.0) + at: SubType.value + used by: GET /bar1 (get_bar1) + └─ Wrapper.value + used by: GET /bar2 (get_bar2) + └─ Wrapper.value (*) diff --git a/crates/dropshot-api-manager/tests/output/compatibility/abbreviated.txt b/crates/dropshot-api-manager/tests/output/compatibility/abbreviated.txt new file mode 100644 index 0000000..f92ab76 --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/abbreviated.txt @@ -0,0 +1,6 @@ +incompatible: schema types changed (see foo v1.0.0) + at: SubType.value + used by: GET /bar1 (get_bar1) + └─ Wrapper.value + used by: GET /bar2 (get_bar2) + └─ Wrapper.value (*) diff --git a/crates/dropshot-api-manager/tests/output/compatibility/abbreviated_multi_change.ansi b/crates/dropshot-api-manager/tests/output/compatibility/abbreviated_multi_change.ansi new file mode 100644 index 0000000..48e07f6 --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/abbreviated_multi_change.ansi @@ -0,0 +1,6 @@ +incompatible: schema kind changed (see foo v1.0.0) + at: SubType.value + change: object properties changed + at: SubType + used by: GET /bar1 (get_bar1) + └─ Wrapper.value diff --git a/crates/dropshot-api-manager/tests/output/compatibility/abbreviated_multi_change.txt b/crates/dropshot-api-manager/tests/output/compatibility/abbreviated_multi_change.txt new file mode 100644 index 0000000..5ea281e --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/abbreviated_multi_change.txt @@ -0,0 +1,6 @@ +incompatible: schema kind changed (see foo v1.0.0) + at: SubType.value + change: object properties changed + at: SubType + used by: GET /bar1 (get_bar1) + └─ Wrapper.value diff --git a/crates/dropshot-api-manager/tests/output/compatibility/deep_branching_tree.ansi b/crates/dropshot-api-manager/tests/output/compatibility/deep_branching_tree.ansi new file mode 100644 index 0000000..f29b082 --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/deep_branching_tree.ansi @@ -0,0 +1,8 @@ +incompatible: schema types changed + at: SubType.value + used by: GET /a (list_a) + ├─ Wrapper.via_a + │ ├─ Inner.x + │ └─ Inner.y + └─ Wrapper.via_b + └─ Inner.z diff --git a/crates/dropshot-api-manager/tests/output/compatibility/deep_branching_tree.txt b/crates/dropshot-api-manager/tests/output/compatibility/deep_branching_tree.txt new file mode 100644 index 0000000..c982e3b --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/deep_branching_tree.txt @@ -0,0 +1,8 @@ +incompatible: schema types changed + at: SubType.value + used by: GET /a (list_a) + ├─ Wrapper.via_a + │ ├─ Inner.x + │ └─ Inner.y + └─ Wrapper.via_b + └─ Inner.z diff --git a/crates/dropshot-api-manager/tests/output/compatibility/endpoint_added.ansi b/crates/dropshot-api-manager/tests/output/compatibility/endpoint_added.ansi new file mode 100644 index 0000000..3fd6c32 --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/endpoint_added.ansi @@ -0,0 +1,2 @@ +forward-incompatible: The operation get_system_info was added + at: GET /system/info (get_system_info) diff --git a/crates/dropshot-api-manager/tests/output/compatibility/endpoint_added.txt b/crates/dropshot-api-manager/tests/output/compatibility/endpoint_added.txt new file mode 100644 index 0000000..3d745da --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/endpoint_added.txt @@ -0,0 +1,2 @@ +forward-incompatible: The operation get_system_info was added + at: GET /system/info (get_system_info) diff --git a/crates/dropshot-api-manager/tests/output/compatibility/endpoint_only_change.ansi b/crates/dropshot-api-manager/tests/output/compatibility/endpoint_only_change.ansi new file mode 100644 index 0000000..dcf835f --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/endpoint_only_change.ansi @@ -0,0 +1,2 @@ +backward-incompatible: A new, required parameter 'token' was added + at: GET /users/{id} (get_user) (.parameters.1) diff --git a/crates/dropshot-api-manager/tests/output/compatibility/endpoint_only_change.txt b/crates/dropshot-api-manager/tests/output/compatibility/endpoint_only_change.txt new file mode 100644 index 0000000..d94cbc9 --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/endpoint_only_change.txt @@ -0,0 +1,2 @@ +backward-incompatible: A new, required parameter 'token' was added + at: GET /users/{id} (get_user) (.parameters.1) diff --git a/crates/dropshot-api-manager/tests/output/compatibility/in_context.ansi b/crates/dropshot-api-manager/tests/output/compatibility/in_context.ansi new file mode 100644 index 0000000..729aca3 --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/in_context.ansi @@ -0,0 +1,9 @@ + Failure nexus (versioned v2025112000.0.0 (blessed)): Oxide Region API + error: OpenAPI document is incompatible with the blessed upstream + + incompatible: schema kind changed + at: AuditLogEntry.auth_method + change: object properties changed + at: AuditLogEntry + used by: GET /v1/system/audit-log (audit_log_list) +  └─ AuditLogEntryResultsPage.items[] diff --git a/crates/dropshot-api-manager/tests/output/compatibility/in_context.txt b/crates/dropshot-api-manager/tests/output/compatibility/in_context.txt new file mode 100644 index 0000000..40072ab --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/in_context.txt @@ -0,0 +1,9 @@ + Failure nexus (versioned v2025112000.0.0 (blessed)): Oxide Region API + error: OpenAPI document is incompatible with the blessed upstream + + incompatible: schema kind changed + at: AuditLogEntry.auth_method + change: object properties changed + at: AuditLogEntry + used by: GET /v1/system/audit-log (audit_log_list) + └─ AuditLogEntryResultsPage.items[] diff --git a/crates/dropshot-api-manager/tests/output/compatibility/multi_sibling_tree.ansi b/crates/dropshot-api-manager/tests/output/compatibility/multi_sibling_tree.ansi new file mode 100644 index 0000000..b3035fe --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/multi_sibling_tree.ansi @@ -0,0 +1,8 @@ +incompatible: schema types changed + at: SubType.value + used by: GET /a (list_a) + ├─ Wrapper.via_a + └─ Wrapper.via_b + used by: GET /b (list_b) + ├─ Wrapper.via_a (*) + └─ Wrapper.via_b (*) diff --git a/crates/dropshot-api-manager/tests/output/compatibility/multi_sibling_tree.txt b/crates/dropshot-api-manager/tests/output/compatibility/multi_sibling_tree.txt new file mode 100644 index 0000000..e7f9c7a --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/multi_sibling_tree.txt @@ -0,0 +1,8 @@ +incompatible: schema types changed + at: SubType.value + used by: GET /a (list_a) + ├─ Wrapper.via_a + └─ Wrapper.via_b + used by: GET /b (list_b) + ├─ Wrapper.via_a (*) + └─ Wrapper.via_b (*) diff --git a/crates/dropshot-api-manager/tests/output/compatibility/multiple_changes_in_one_component.ansi b/crates/dropshot-api-manager/tests/output/compatibility/multiple_changes_in_one_component.ansi new file mode 100644 index 0000000..7175d6e --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/multiple_changes_in_one_component.ansi @@ -0,0 +1,5 @@ +incompatible: schema types changed + at: Wrapper.a +incompatible: schema types changed + at: Wrapper.b + used by: GET /hello/{name} (hello) diff --git a/crates/dropshot-api-manager/tests/output/compatibility/multiple_changes_in_one_component.txt b/crates/dropshot-api-manager/tests/output/compatibility/multiple_changes_in_one_component.txt new file mode 100644 index 0000000..6f082da --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/multiple_changes_in_one_component.txt @@ -0,0 +1,5 @@ +incompatible: schema types changed + at: Wrapper.a +incompatible: schema types changed + at: Wrapper.b + used by: GET /hello/{name} (hello) diff --git a/crates/dropshot-api-manager/tests/output/compatibility/renamed_base.ansi b/crates/dropshot-api-manager/tests/output/compatibility/renamed_base.ansi new file mode 100644 index 0000000..f20cfbb --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/renamed_base.ansi @@ -0,0 +1,2 @@ +incompatible: schema types changed + at: OldName.value -> NewName.value diff --git a/crates/dropshot-api-manager/tests/output/compatibility/renamed_base.txt b/crates/dropshot-api-manager/tests/output/compatibility/renamed_base.txt new file mode 100644 index 0000000..e96f971 --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/renamed_base.txt @@ -0,0 +1,2 @@ +incompatible: schema types changed + at: OldName.value -> NewName.value diff --git a/crates/dropshot-api-manager/tests/output/compatibility/subpath_renamed.ansi b/crates/dropshot-api-manager/tests/output/compatibility/subpath_renamed.ansi new file mode 100644 index 0000000..9f72738 --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/subpath_renamed.ansi @@ -0,0 +1,2 @@ +incompatible: field renamed + at: Foo.old_name -> Foo.new_name diff --git a/crates/dropshot-api-manager/tests/output/compatibility/subpath_renamed.txt b/crates/dropshot-api-manager/tests/output/compatibility/subpath_renamed.txt new file mode 100644 index 0000000..dc44143 --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/subpath_renamed.txt @@ -0,0 +1,2 @@ +incompatible: field renamed + at: Foo.old_name -> Foo.new_name diff --git a/crates/dropshot-api-manager/tests/output/compatibility/unhandled_class.ansi b/crates/dropshot-api-manager/tests/output/compatibility/unhandled_class.ansi new file mode 100644 index 0000000..c03ee55 --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/unhandled_class.ansi @@ -0,0 +1,2 @@ +change: array maxItems changed + at: Foo[] diff --git a/crates/dropshot-api-manager/tests/output/compatibility/unhandled_class.txt b/crates/dropshot-api-manager/tests/output/compatibility/unhandled_class.txt new file mode 100644 index 0000000..a1c7bab --- /dev/null +++ b/crates/dropshot-api-manager/tests/output/compatibility/unhandled_class.txt @@ -0,0 +1,2 @@ +change: array maxItems changed + at: Foo[] diff --git a/crates/integration-tests/tests/output/integration/blessed_version_broken.txt b/crates/integration-tests/tests/output/integration/blessed_version_broken.txt index 4c0320d..3888e69 100644 --- a/crates/integration-tests/tests/output/integration/blessed_version_broken.txt +++ b/crates/integration-tests/tests/output/integration/blessed_version_broken.txt @@ -6,95 +6,75 @@ Checking 3 OpenAPI documents... Fresh versioned-health (versioned v1.0.0 (blessed)): Versioned Health API Failure versioned-health (versioned v2.0.0 (blessed)): Versioned Health API - error: OpenAPI document generated from the current code is not - compatible with the blessed document (from upstream) - - at .paths."/health/detailed".get: backward-incompatible - change: A new, required parameter 'verbose' was added - --- a/blessed - +++ b/generated - @@ -1,11 +1,22 @@ - { - "get": { - + "description": "Adds a required query parameter `verbose` relative to the blessed shape. See `DetailedFilter` below.", - "operationId": "detailed_health_check", - + "parameters": [ - + { - + "in": "query", - + "name": "verbose", - + "required": true, - + "schema": { - + "type": "boolean" - + } - + } - + ], - "responses": { - "200": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/DetailedHealthStatus" - } - } + error: OpenAPI document generated from the current code is not compatible + with the blessed document (from upstream) + + backward-incompatible: A new, required parameter 'verbose' was added + at: GET /health/detailed (detailed_health_check) (.parameters.0) + --- a/blessed + +++ b/generated + @@ -1,11 +1,22 @@ + { + "get": { + + "description": "Adds a required query parameter `verbose` relative to the blessed shape. See `DetailedFilter` below.", + "operationId": "detailed_health_check", + + "parameters": [ + + { + + "in": "query", + + "name": "verbose", + + "required": true, + + "schema": { + + "type": "boolean" + + } + + } + + ], + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DetailedHealthStatus" + } + } + Failure versioned-health (versioned v3.0.0 (blessed)): Versioned Health API - error: OpenAPI document generated from the current code is not - compatible with the blessed document (from upstream) - - at .paths."/system/info".get: forward-incompatible change: - The operation get_system_info was added - --- a/blessed - +++ b/generated - @@ -0,0 +1,25 @@ - +{ - + "get": { - + "description": "This breaks backward compatibility by adding a new endpoint to v3.0.0.", - + "operationId": "get_system_info", - + "responses": { - + "200": { - + "content": { - + "application/json": { - + "schema": { - + "$ref": "#/components/schemas/SystemInfo" - + } - + } - + }, - + "description": "successful operation" - + }, - + "4XX": { - + "$ref": "#/components/responses/Error" - + }, - + "5XX": { - + "$ref": "#/components/responses/Error" - + } - + }, - + "summary": "Get system info (v3+): new endpoint added to existing version." - + } - +} - - at .paths."/health/detailed".get: backward-incompatible - change: A new, required parameter 'verbose' was added - --- a/blessed - +++ b/generated - @@ -1,11 +1,22 @@ - { - "get": { - + "description": "Adds a required query parameter `verbose` relative to the blessed shape. See `DetailedFilter` below.", - "operationId": "detailed_health_check", - + "parameters": [ - + { - + "in": "query", - + "name": "verbose", - + "required": true, - + "schema": { - + "type": "boolean" - + } - + } - + ], - "responses": { - "200": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/DetailedHealthStatus" - } - } + error: OpenAPI document generated from the current code is not compatible + with the blessed document (from upstream) + + backward-incompatible: A new, required parameter 'verbose' was added (see versioned-health v2.0.0) + at: GET /health/detailed (detailed_health_check) (.parameters.0) + + forward-incompatible: The operation get_system_info was added + at: GET /system/info (get_system_info) + --- a/blessed + +++ b/generated + @@ -0,0 +1,25 @@ + +{ + + "get": { + + "description": "This breaks backward compatibility by adding a new endpoint to v3.0.0.", + + "operationId": "get_system_info", + + "responses": { + + "200": { + + "content": { + + "application/json": { + + "schema": { + + "$ref": "#/components/schemas/SystemInfo" + + } + + } + + }, + + "description": "successful operation" + + }, + + "4XX": { + + "$ref": "#/components/responses/Error" + + }, + + "5XX": { + + "$ref": "#/components/responses/Error" + + } + + }, + + "summary": "Get system info (v3+): new endpoint added to existing version." + + } + +} + Fresh versioned-health "latest" symlink ------- Failure 3 documents checked: 2 fresh, 0 stale, 2 failed, 0 other problems diff --git a/crates/integration-tests/tests/output/integration/blessed_version_endpoint_removed.txt b/crates/integration-tests/tests/output/integration/blessed_version_endpoint_removed.txt index 045f99e..0d869b2 100644 --- a/crates/integration-tests/tests/output/integration/blessed_version_endpoint_removed.txt +++ b/crates/integration-tests/tests/output/integration/blessed_version_endpoint_removed.txt @@ -6,95 +6,75 @@ Checking 3 OpenAPI documents... Fresh versioned-health (versioned v1.0.0 (blessed)): Versioned Health API Failure versioned-health (versioned v2.0.0 (blessed)): Versioned Health API - error: OpenAPI document generated from the current code is not - compatible with the blessed document (from upstream) - - at .paths."/health/detailed".get: backward-incompatible - change: The parameter 'verbose' was removed - --- a/blessed - +++ b/generated - @@ -1,22 +1,11 @@ - { - "get": { - - "description": "Adds a required query parameter `verbose` relative to the blessed shape. See `DetailedFilter` below.", - "operationId": "detailed_health_check", - - "parameters": [ - - { - - "in": "query", - - "name": "verbose", - - "required": true, - - "schema": { - - "type": "boolean" - - } - - } - - ], - "responses": { - "200": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/DetailedHealthStatus" - } - } + error: OpenAPI document generated from the current code is not compatible + with the blessed document (from upstream) + + backward-incompatible: The parameter 'verbose' was removed + at: GET /health/detailed (detailed_health_check) (.parameters.0) + --- a/blessed + +++ b/generated + @@ -1,22 +1,11 @@ + { + "get": { + - "description": "Adds a required query parameter `verbose` relative to the blessed shape. See `DetailedFilter` below.", + "operationId": "detailed_health_check", + - "parameters": [ + - { + - "in": "query", + - "name": "verbose", + - "required": true, + - "schema": { + - "type": "boolean" + - } + - } + - ], + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DetailedHealthStatus" + } + } + Failure versioned-health (versioned v3.0.0 (blessed)): Versioned Health API - error: OpenAPI document generated from the current code is not - compatible with the blessed document (from upstream) - - at .paths."/health/detailed".get: backward-incompatible - change: The parameter 'verbose' was removed - --- a/blessed - +++ b/generated - @@ -1,22 +1,11 @@ - { - "get": { - - "description": "Adds a required query parameter `verbose` relative to the blessed shape. See `DetailedFilter` below.", - "operationId": "detailed_health_check", - - "parameters": [ - - { - - "in": "query", - - "name": "verbose", - - "required": true, - - "schema": { - - "type": "boolean" - - } - - } - - ], - "responses": { - "200": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/DetailedHealthStatus" - } - } - - at .paths."/system/info".get: backward-incompatible change: - The operation get_system_info was removed - --- a/blessed - +++ b/generated - @@ -1,25 +0,0 @@ - -{ - - "get": { - - "description": "This breaks backward compatibility by adding a new endpoint to v3.0.0.", - - "operationId": "get_system_info", - - "responses": { - - "200": { - - "content": { - - "application/json": { - - "schema": { - - "$ref": "#/components/schemas/SystemInfo" - - } - - } - - }, - - "description": "successful operation" - - }, - - "4XX": { - - "$ref": "#/components/responses/Error" - - }, - - "5XX": { - - "$ref": "#/components/responses/Error" - - } - - }, - - "summary": "Get system info (v3+): new endpoint added to existing version." - - } - -} + error: OpenAPI document generated from the current code is not compatible + with the blessed document (from upstream) + + backward-incompatible: The parameter 'verbose' was removed (see versioned-health v2.0.0) + at: GET /health/detailed (detailed_health_check) (.parameters.0) + + backward-incompatible: The operation get_system_info was removed + at: GET /system/info (get_system_info) + --- a/blessed + +++ b/generated + @@ -1,25 +0,0 @@ + -{ + - "get": { + - "description": "This breaks backward compatibility by adding a new endpoint to v3.0.0.", + - "operationId": "get_system_info", + - "responses": { + - "200": { + - "content": { + - "application/json": { + - "schema": { + - "$ref": "#/components/schemas/SystemInfo" + - } + - } + - }, + - "description": "successful operation" + - }, + - "4XX": { + - "$ref": "#/components/responses/Error" + - }, + - "5XX": { + - "$ref": "#/components/responses/Error" + - } + - }, + - "summary": "Get system info (v3+): new endpoint added to existing version." + - } + -} + Fresh versioned-health "latest" symlink ------- Failure 3 documents checked: 2 fresh, 0 stale, 2 failed, 0 other problems diff --git a/crates/integration-tests/tests/output/integration/cross_api_dedup.txt b/crates/integration-tests/tests/output/integration/cross_api_dedup.txt index 433e915..9c9a818 100644 --- a/crates/integration-tests/tests/output/integration/cross_api_dedup.txt +++ b/crates/integration-tests/tests/output/integration/cross_api_dedup.txt @@ -6,187 +6,94 @@ Checking 6 OpenAPI documents... Fresh versioned-health (versioned v1.0.0 (blessed)): Versioned Health API Failure versioned-health (versioned v2.0.0 (blessed)): Versioned Health API - error: OpenAPI document generated from the current code is not - compatible with the blessed document (from upstream) - - at .paths."/health/detailed".get: backward-incompatible - change: A new, required parameter 'verbose' was added - --- a/blessed - +++ b/generated - @@ -1,11 +1,22 @@ - { - "get": { - + "description": "Adds a required query parameter `verbose` relative to the blessed shape. See `DetailedFilter` below.", - "operationId": "detailed_health_check", - + "parameters": [ - + { - + "in": "query", - + "name": "verbose", - + "required": true, - + "schema": { - + "type": "boolean" - + } - + } - + ], - "responses": { - "200": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/DetailedHealthStatus" - } - } + error: OpenAPI document generated from the current code is not compatible + with the blessed document (from upstream) + + backward-incompatible: A new, required parameter 'verbose' was added + at: GET /health/detailed (detailed_health_check) (.parameters.0) + --- a/blessed + +++ b/generated + @@ -1,11 +1,22 @@ + { + "get": { + + "description": "Adds a required query parameter `verbose` relative to the blessed shape. See `DetailedFilter` below.", + "operationId": "detailed_health_check", + + "parameters": [ + + { + + "in": "query", + + "name": "verbose", + + "required": true, + + "schema": { + + "type": "boolean" + + } + + } + + ], + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DetailedHealthStatus" + } + } + Failure versioned-health (versioned v3.0.0 (blessed)): Versioned Health API - error: OpenAPI document generated from the current code is not - compatible with the blessed document (from upstream) - - at .paths."/system/info".get: forward-incompatible change: - The operation get_system_info was added - --- a/blessed - +++ b/generated - @@ -0,0 +1,25 @@ - +{ - + "get": { - + "description": "This breaks backward compatibility by adding a new endpoint to v3.0.0.", - + "operationId": "get_system_info", - + "responses": { - + "200": { - + "content": { - + "application/json": { - + "schema": { - + "$ref": "#/components/schemas/SystemInfo" - + } - + } - + }, - + "description": "successful operation" - + }, - + "4XX": { - + "$ref": "#/components/responses/Error" - + }, - + "5XX": { - + "$ref": "#/components/responses/Error" - + } - + }, - + "summary": "Get system info (v3+): new endpoint added to existing version." - + } - +} - - at .paths."/health/detailed".get: backward-incompatible - change: A new, required parameter 'verbose' was added - --- a/blessed - +++ b/generated - @@ -1,11 +1,22 @@ - { - "get": { - + "description": "Adds a required query parameter `verbose` relative to the blessed shape. See `DetailedFilter` below.", - "operationId": "detailed_health_check", - + "parameters": [ - + { - + "in": "query", - + "name": "verbose", - + "required": true, - + "schema": { - + "type": "boolean" - + } - + } - + ], - "responses": { - "200": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/DetailedHealthStatus" - } - } + error: OpenAPI document generated from the current code is not compatible + with the blessed document (from upstream) + + backward-incompatible: A new, required parameter 'verbose' was added (see versioned-health v2.0.0) + at: GET /health/detailed (detailed_health_check) (.parameters.0) + + forward-incompatible: The operation get_system_info was added + at: GET /system/info (get_system_info) + --- a/blessed + +++ b/generated + @@ -0,0 +1,25 @@ + +{ + + "get": { + + "description": "This breaks backward compatibility by adding a new endpoint to v3.0.0.", + + "operationId": "get_system_info", + + "responses": { + + "200": { + + "content": { + + "application/json": { + + "schema": { + + "$ref": "#/components/schemas/SystemInfo" + + } + + } + + }, + + "description": "successful operation" + + }, + + "4XX": { + + "$ref": "#/components/responses/Error" + + }, + + "5XX": { + + "$ref": "#/components/responses/Error" + + } + + }, + + "summary": "Get system info (v3+): new endpoint added to existing version." + + } + +} + Fresh versioned-health "latest" symlink Fresh versioned-monitor (versioned v1.0.0 (blessed)): Versioned Monitor API Failure versioned-monitor (versioned v2.0.0 (blessed)): Versioned Monitor API - error: OpenAPI document generated from the current code is not - compatible with the blessed document (from upstream) - - at .paths."/health/detailed".get: backward-incompatible - change: A new, required parameter 'verbose' was added - --- a/blessed - +++ b/generated - @@ -1,11 +1,22 @@ - { - "get": { - + "description": "Adds a required query parameter `verbose` relative to the blessed shape. See `DetailedFilter` below.", - "operationId": "detailed_health_check", - + "parameters": [ - + { - + "in": "query", - + "name": "verbose", - + "required": true, - + "schema": { - + "type": "boolean" - + } - + } - + ], - "responses": { - "200": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/DetailedHealthStatus" - } - } + error: OpenAPI document generated from the current code is not compatible + with the blessed document (from upstream) + + backward-incompatible: A new, required parameter 'verbose' was added (see versioned-health v2.0.0) + at: GET /health/detailed (detailed_health_check) (.parameters.0) + Failure versioned-monitor (versioned v3.0.0 (blessed)): Versioned Monitor API - error: OpenAPI document generated from the current code is not - compatible with the blessed document (from upstream) - - at .paths."/system/info".get: forward-incompatible change: - The operation get_system_info was added - --- a/blessed - +++ b/generated - @@ -0,0 +1,25 @@ - +{ - + "get": { - + "description": "This breaks backward compatibility by adding a new endpoint to v3.0.0.", - + "operationId": "get_system_info", - + "responses": { - + "200": { - + "content": { - + "application/json": { - + "schema": { - + "$ref": "#/components/schemas/SystemInfo" - + } - + } - + }, - + "description": "successful operation" - + }, - + "4XX": { - + "$ref": "#/components/responses/Error" - + }, - + "5XX": { - + "$ref": "#/components/responses/Error" - + } - + }, - + "summary": "Get system info (v3+): new endpoint added to existing version." - + } - +} - - at .paths."/health/detailed".get: backward-incompatible - change: A new, required parameter 'verbose' was added - --- a/blessed - +++ b/generated - @@ -1,11 +1,22 @@ - { - "get": { - + "description": "Adds a required query parameter `verbose` relative to the blessed shape. See `DetailedFilter` below.", - "operationId": "detailed_health_check", - + "parameters": [ - + { - + "in": "query", - + "name": "verbose", - + "required": true, - + "schema": { - + "type": "boolean" - + } - + } - + ], - "responses": { - "200": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/DetailedHealthStatus" - } - } + error: OpenAPI document generated from the current code is not compatible + with the blessed document (from upstream) + + backward-incompatible: A new, required parameter 'verbose' was added (see versioned-health v2.0.0) + at: GET /health/detailed (detailed_health_check) (.parameters.0) + + forward-incompatible: The operation get_system_info was added (see versioned-health v3.0.0) + at: GET /system/info (get_system_info) + Fresh versioned-monitor "latest" symlink ------- Failure 6 documents checked: 4 fresh, 0 stale, 4 failed, 0 other problems From 956815ebb027e4568f9a042cae1d8a64aa2722f9 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 19 May 2026 21:41:53 -0700 Subject: [PATCH 2/2] clean up some comments Created using spr 1.3.6-beta.1 --- .../dropshot-api-manager/src/compatibility/wrap.rs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/crates/dropshot-api-manager/src/compatibility/wrap.rs b/crates/dropshot-api-manager/src/compatibility/wrap.rs index 2998d85..533f3ad 100644 --- a/crates/dropshot-api-manager/src/compatibility/wrap.rs +++ b/crates/dropshot-api-manager/src/compatibility/wrap.rs @@ -267,7 +267,6 @@ mod tests { Adapter { line, width, indent: Indent::spaces(indent) }.to_string() } - /// A line that fits the width prints unchanged. #[test] fn test_fit_on_one_line() { let mut line = Line::new(); @@ -275,9 +274,6 @@ mod tests { assert_eq!(render(&line, 80, " "), "GET /short"); } - /// Two ordinary (non-overflowing) words past the limit split at the - /// whitespace between them; the trailing space at the end of the - /// first line is dropped. #[test] fn test_break_at_whitespace() { let mut line = Line::new(); @@ -288,9 +284,6 @@ mod tests { assert_eq!(render(&line, 7, " "), "alpha\n beta"); } - /// Adjacent spans without a space between them belong to the same - /// word and don't introduce a break — the styled split between - /// `/route` and `(op_id)` must stay glued. #[test] fn test_adjacent_spans_form_one_word() { let mut line = Line::new(); @@ -301,9 +294,6 @@ mod tests { assert_eq!(render(&line, 3, " "), "ABCD"); } - /// A word wider than the line width prints on its own line rather - /// than being broken mid-word — copy-from-terminal stays intact at - /// the cost of overflowing the margin. #[test] fn test_long_word_overflows() { let mut line = Line::new(); @@ -313,9 +303,6 @@ mod tests { assert_eq!(render(&line, 5, ""), "a\nloooooooooong\nz"); } - /// Style transitions inside a word survive wrapping: when a word that - /// mixes styles gets pushed to a new line, every styled slice still - /// renders with its original style. #[test] fn test_preserve_styles_across_wrap() { let bold = Style::new().bold();