diff --git a/src/compare.rs b/src/compare.rs index f32fff3..c674f2c 100644 --- a/src/compare.rs +++ b/src/compare.rs @@ -10,7 +10,7 @@ use openapiv3::{ use serde_json::Value; use crate::{ - Change, ChangeClass, ChangeComparison, ChangeDetails, + Change, ChangeClass, ChangeComparison, ChangeDetails, JsonPathStack, context::{Context, Contextual, ToContext}, operations::{all_params, operations}, resolve::ReferenceOrResolver, @@ -25,23 +25,72 @@ pub fn compare(old: &Value, new: &Value) -> anyhow::Result> { Ok(comp.changes) } +/// The full current location in a document, including appended segments. +/// +/// This is the path returned by `JsonPathStack::current_pointer()`, wrapped +/// in a newtype to prevent confusion with other path-like strings (e.g. a +/// future base path type that excludes appended segments). +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] +pub(crate) struct CurrentPointer(String); + +impl CurrentPointer { + pub(crate) fn new(stack: &JsonPathStack) -> Self { + Self(stack.current_pointer().to_string()) + } +} + +/// Key for memoizing schema comparison results. +/// +/// This uses the full schema path (including internal paths like +/// `SubType/properties/value`) for accurate memoization. The comparison +/// direction (Input vs Output) is included because compatibility semantics +/// differ based on direction. +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] +pub(crate) struct VisitedKey { + comparison: SchemaComparison, + old_path: CurrentPointer, + new_path: CurrentPointer, +} + +impl VisitedKey { + pub(crate) fn new( + comparison: SchemaComparison, + old_stack: &JsonPathStack, + new_stack: &JsonPathStack, + ) -> Self { + Self { + comparison, + old_path: CurrentPointer::new(old_stack), + new_path: CurrentPointer::new(new_stack), + } + } +} + +/// Lifecycle of a schema comparison, keyed by `VisitedKey` in +/// [`Compare::visit_state`]. +#[derive(Clone, Debug)] +pub(crate) enum VisitState { + /// Comparison is in progress somewhere up the call stack. Re-entering a + /// key in this state means the schema graph has a cycle. + Visiting, + /// Comparison has completed. + Completed { + /// Whether the two schemas were determined to be equal. + equal: bool, + }, +} + #[derive(Default)] pub(crate) struct Compare { pub changes: Vec, - /// Cache comparisons (type of comparison, old and new path to the schema) - /// and the boolean result (are the schemas backward-compatible?). - pub visited: BTreeMap<(SchemaComparison, String, String), bool>, + /// State of every schema comparison we've started. + pub visit_state: BTreeMap, } impl Compare { pub fn compare(&mut self, old: &Value, new: &Value) -> anyhow::Result<()> { - let old_context = Context::new(old); - let old_operations = - operations(&old_context).context("error deserializing old OpenAPI document")?; - - let new_context = Context::new(new); - let new_operations = - operations(&new_context).context("error deserializing new OpenAPI document")?; + let old_operations = operations(old).context("error deserializing old OpenAPI document")?; + let new_operations = operations(new).context("error deserializing new OpenAPI document")?; let SetCompare { a_unique, @@ -55,10 +104,11 @@ impl Compare { Some(name) => name.as_str(), None => "", }; + let new_paths_root = Context::for_paths_root(new); self.push_change( format!("The operation {op_name} was removed"), &op_info.operation, - &new_context.append("paths"), + &new_paths_root, ChangeComparison::Structural, ChangeClass::BackwardIncompatible, ChangeDetails::Removed, @@ -71,10 +121,10 @@ impl Compare { Some(name) => name.as_str(), None => "", }; - + let old_paths_root = Context::for_paths_root(old); self.push_change( format!("The operation {op_name} was added"), - &old_context.append("paths"), + &old_paths_root, &op_info.operation, ChangeComparison::Structural, ChangeClass::ForwardIncompatible, diff --git a/src/context.rs b/src/context.rs index 04c6c7e..80b6830 100644 --- a/src/context.rs +++ b/src/context.rs @@ -1,4 +1,4 @@ -// Copyright 2025 Oxide Computer Company +// Copyright 2026 Oxide Computer Company use std::{borrow::Cow, ops::Deref}; @@ -6,7 +6,10 @@ use openapiv3::ReferenceOr; use serde::de::DeserializeOwned; use serde_json::Value; -use crate::{JsonPathStack, resolve::ReferenceOrResolver}; +use crate::{ + path::{EndpointPath, InvalidComponentRef, JsonPathStack}, + resolve::ReferenceOrResolver, +}; #[derive(Clone, Debug)] pub struct Context<'a> { @@ -15,33 +18,42 @@ pub struct Context<'a> { } impl<'a> Context<'a> { - pub fn new(raw_openapi: &'a Value) -> Self { + /// Create a context at a specific endpoint. + /// + /// This is the normal way to create a context for schema comparison. + pub fn for_endpoint(raw_openapi: &'a Value, endpoint: EndpointPath) -> Self { Self { raw_openapi, - stack: JsonPathStack::new(), + stack: JsonPathStack::for_endpoint(endpoint), } } - pub fn append(&self, segment: &str) -> Context<'a> { - let stack = self - .stack - .append(&segment.replace("~", "~0").replace("/", "~1")); - + /// Create a context at `#/paths`. + /// + /// This is used only when reporting that an operation was added or removed, + /// where one side doesn't have the operation. In normal use, prefer + /// `for_endpoint()`. + pub fn for_paths_root(raw_openapi: &'a Value) -> Self { Self { - raw_openapi: self.raw_openapi, - stack, + raw_openapi, + stack: JsonPathStack::paths_root(), } } - pub(crate) fn push(&self, path: &str) -> Context<'a> { - let stack = self.stack.push(path); - + pub fn append(&self, segment: &str) -> Context<'a> { Self { raw_openapi: self.raw_openapi, - stack, + stack: self.stack.append(segment), } } + pub(crate) fn push(&self, path: &str) -> Result, InvalidComponentRef> { + Ok(Self { + raw_openapi: self.raw_openapi, + stack: self.stack.push(path)?, + }) + } + pub fn stack(&self) -> &JsonPathStack { &self.stack } diff --git a/src/lib.rs b/src/lib.rs index b79bea8..ac548b6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,4 +1,4 @@ -// Copyright 2025 Oxide Computer Company +// Copyright 2026 Oxide Computer Company //! Drift //! @@ -8,152 +8,11 @@ mod change; mod compare; mod context; mod operations; +mod path; mod resolve; mod schema; mod setops; -use std::fmt::Debug; - pub use change::*; pub use compare::compare; - -/// Represents a location in an OpenAPI document. -/// -/// This takes the the form of a stack of JSON paths where each element of the -/// stack starts at the document root and terminates in either a reference -/// (i.e. to the subsequent element in the stack) or the item being identified. -#[derive(Clone)] -pub struct JsonPathStack { - top: String, - stack: Vec, -} - -impl Debug for JsonPathStack { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let mut out = f.debug_list(); - out.entry(&self.top); - self.stack.iter().rev().for_each(|path| { - let _ = out.entry(path); - }); - out.finish() - } -} - -impl JsonPathStack { - fn new() -> Self { - Self { - top: "#".to_string(), - stack: Vec::new(), - } - } - - fn append(&self, segment: &str) -> JsonPathStack { - let Self { top, stack } = self; - - Self { - top: format!("{top}/{segment}"), - stack: stack.clone(), - } - } - - fn push(&self, path: &str) -> JsonPathStack { - let Self { top, stack } = self; - let mut stack = stack.clone(); - stack.push(format!("{top}/$ref")); - - Self { - top: path.to_string(), - stack, - } - } - - pub fn contains_cycle(&self) -> bool { - self.stack - .iter() - .any(|item| is_path_ancestor_of(&self.top, item)) - } - - pub fn iter(&self) -> impl Iterator { - std::iter::once(&self.top).chain(self.stack.iter().rev()) - } -} - -impl Default for JsonPathStack { - fn default() -> Self { - Self::new() - } -} - -/// Check if `ancestor` is a path-segment-aligned prefix of `path`. -/// -/// Returns `true` if either of the following conditions are true: -/// -/// 1. `ancestor` is the same as `path`. -/// 2. `ancestor` is a prefix of `path`, and the character immediately -/// following the prefix is `/`. -/// -/// The second condition prevents false matches where schema names share a -/// common string prefix (e.g., `Item` matching `ItemPage`). -fn is_path_ancestor_of(ancestor: &str, path: &str) -> bool { - path.starts_with(ancestor) - && path - .as_bytes() - .get(ancestor.len()) - .is_none_or(|&b| b == b'/') -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_contains_cycle() { - // A genuine cycle: pushing back to a schema we've already visited. - let stack = JsonPathStack::new() - .push("#/components/schemas/Tree") - .append("properties") - .append("children") - .push("#/components/schemas/Tree"); - - assert!(stack.contains_cycle()); - - // Schemas whose names share a common string prefix must not be treated - // as cycles. e.g. the string "Item" is a prefix of "ItemPage", but they - // are different schemas. - let stack = JsonPathStack::new() - .push("#/components/schemas/ItemPage") - .append("properties") - .append("items") - .append("items") - .push("#/components/schemas/Item"); - - assert!(!stack.contains_cycle()); - } - - #[test] - fn test_is_path_ancestor_of_basics() { - // Exact match. - assert!(is_path_ancestor_of( - "#/components/schemas/Item", - "#/components/schemas/Item" - )); - - // Proper ancestor. - assert!(is_path_ancestor_of( - "#/components/schemas/Item", - "#/components/schemas/Item/properties/value" - )); - - // Shared string prefix but not a path ancestor. - assert!(!is_path_ancestor_of( - "#/components/schemas/Item", - "#/components/schemas/ItemPage" - )); - - // Sibling, not ancestor. - assert!(!is_path_ancestor_of( - "#/components/schemas/Item", - "#/components/schemas/Other" - )); - } -} +pub use path::JsonPathStack; diff --git a/src/operations.rs b/src/operations.rs index d21af01..3874cb6 100644 --- a/src/operations.rs +++ b/src/operations.rs @@ -1,4 +1,4 @@ -// Copyright 2025 Oxide Computer Company +// Copyright 2026 Oxide Computer Company use std::{collections::BTreeMap, sync::LazyLock}; @@ -7,8 +7,11 @@ use openapiv3::{OpenAPI, Operation, Parameter, ParameterData, ReferenceOr}; use regex::Regex; use serde::Deserialize; +use serde_json::Value; + use crate::{ context::{Context, Contextual}, + path::EndpointPath, resolve::ReferenceOrResolver, }; @@ -38,24 +41,27 @@ pub struct OperationInfo<'a> { pub shared_parameters: Contextual<'a, Vec>>, } -pub fn operations<'a>( - context: &Context<'a>, -) -> anyhow::Result)>> { - let api = OpenAPI::deserialize(context.raw_openapi)?; +pub fn operations(raw_openapi: &Value) -> anyhow::Result)>> { + let api = OpenAPI::deserialize(raw_openapi)?; let mut out = Vec::new(); - let context = context.append("paths"); - for (path, ref_or_operation) in api.paths.paths.iter() { - let context = context.append(path); - let (path_item, context) = ref_or_operation.resolve(&context)?; + // Build a context at #/paths/ for resolving path item refs. + let path_endpoint = EndpointPath::for_path(path); + let path_context = Context::for_endpoint(raw_openapi, path_endpoint); + let (path_item, path_context) = ref_or_operation.resolve(&path_context)?; - let shared_parameters = - Contextual::new(context.append("parameters"), path_item.parameters.clone()); + let shared_parameters = Contextual::new( + path_context.append("parameters"), + path_item.parameters.clone(), + ); for (method, operation) in path_item.iter() { - let context = context.append(method); + // Build endpoint path for this operation: #/paths// + let endpoint = EndpointPath::for_path(path).append(method); + let context = Context::for_endpoint(raw_openapi, endpoint); + let op_key = OperationKey::new(path, method); let op_info = OperationInfo { path: path.clone(), diff --git a/src/path.rs b/src/path.rs new file mode 100644 index 0000000..99631f2 --- /dev/null +++ b/src/path.rs @@ -0,0 +1,480 @@ +// Copyright 2026 Oxide Computer Company + +//! Strongly-typed JSON path stack for tracking location in OpenAPI documents. +//! +//! This module provides types for tracking location while traversing an OpenAPI +//! document, particularly when following `$ref` references. +//! +//! ## State machine +//! +//! The [`JsonPathStack`] uses an internal state machine ([`PathState`]) that +//! enforces these invariants at the type level: +//! +//! - **`PathsRoot`**: At `#/paths`, used only for reporting operation add/remove. +//! Cannot follow refs or append segments from this state. +//! - **`AtEndpoint`**: At an endpoint path (`#/paths//...`), no refs +//! followed yet. Can append segments or push a ref. +//! - **`AtComponent`**: At a ref target (any JSON pointer), with a reference +//! chain. The chain always has exactly one endpoint origin (the first ref), +//! followed by zero or more intermediates. +//! +//! This makes illegal states unrepresentable: you cannot have endpoint refs +//! mixed into the intermediate chain, or push refs from `PathsRoot`. +//! +//! ## What this module deliberately does *not* do +//! +//! Cycle detection is not the path stack's responsibility. It belongs at the +//! comparison layer, keyed by the (old, new) schema pair currently being +//! expanded -- see `compare::Compare::visit_state`. + +use std::fmt; + +/// Error returned when `JsonPathStack::push()` receives an invalid reference. +/// +/// A valid reference must be a JSON pointer starting with `#/`. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct InvalidComponentRef { + /// The invalid reference that was provided. + pub(crate) reference: String, +} + +impl fmt::Display for InvalidComponentRef { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "invalid reference {:?}: expected JSON pointer starting with #/", + self.reference + ) + } +} + +impl std::error::Error for InvalidComponentRef {} + +/// An endpoint path, guaranteed to start with `#/paths/`. +/// +/// This represents a location within the paths section of an OpenAPI document. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct EndpointPath(String); + +impl EndpointPath { + /// Create an endpoint path from a raw API path (like `/users/{id}`). + /// + /// This escapes the path according to JSON pointer rules (RFC 6901). + pub(crate) fn for_path(api_path: &str) -> Self { + let escaped = escape_json_pointer_segment(api_path); + Self(format!("#/paths/{}", escaped)) + } + + /// Append a path segment, escaping special characters per RFC 6901. + pub(crate) fn append(&self, segment: &str) -> Self { + let escaped = escape_json_pointer_segment(segment); + Self(format!("{}/{}", self.0, escaped)) + } + + /// Get the JSON pointer string. + fn as_str(&self) -> &str { + &self.0 + } +} + +/// A JSON pointer reference (starting with `#/`): guaranteed by construction. +/// +/// This represents a location after following at least one `$ref`. It can point +/// to any location in the document, not only components. +#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +struct RefTargetPath(String); + +impl RefTargetPath { + /// Parse from a JSON pointer. Returns `None` if not a valid local ref. + fn parse(pointer: &str) -> Option { + pointer.starts_with("#/").then(|| Self(pointer.to_string())) + } + + /// Append a path segment, escaping special characters per RFC 6901. + fn append(&self, segment: &str) -> Self { + let escaped = escape_json_pointer_segment(segment); + Self(format!("{}/{}", self.0, escaped)) + } + + /// Get the JSON pointer string. + fn as_str(&self) -> &str { + &self.0 + } +} + +/// The state of the path stack, encoding the invariants: +/// +/// - If refs have been followed, the first was always from an endpoint. +/// - All subsequent refs form a chain of intermediate locations. +/// - Transitions: endpoint → ref target, or ref target → ref target. +#[derive(Clone, Debug, PartialEq, Eq)] +enum PathState { + /// At the paths root (`#/paths`), used only for operation add/remove reporting. + /// Cannot follow refs from this state. + PathsRoot, + + /// At an endpoint, no refs followed yet. + AtEndpoint(EndpointPath), + + /// At a component, with reference chain. + AtComponent { + /// The current component location. + current: RefTargetPath, + /// Where the first ref was followed from (always an endpoint, with + /// `/$ref` appended). + origin_ref: EndpointPath, + /// Intermediate refs (all from components, each with `/$ref` appended). + intermediate_refs: Vec, + }, +} + +/// Strongly-typed path stack for tracking location in OpenAPI documents. +/// +/// The stack tracks the location while traversing an OpenAPI document, +/// particularly when following `$ref` references. +#[derive(Clone)] +pub struct JsonPathStack { + state: PathState, +} + +impl JsonPathStack { + /// Create a path stack starting at the given endpoint. + /// + /// The endpoint must be a valid endpoint path (starting with `#/paths/`). + pub(crate) fn for_endpoint(endpoint: EndpointPath) -> Self { + Self { + state: PathState::AtEndpoint(endpoint), + } + } + + /// Create a path stack at `#/paths` (no specific endpoint). + /// + /// This is used only when reporting that an operation was added or removed, + /// where one side doesn't have the operation at all. In normal use, prefer + /// `for_endpoint()`. + pub(crate) fn paths_root() -> Self { + Self { + state: PathState::PathsRoot, + } + } + + /// Return `true` if this stack has a root endpoint (not created via + /// `paths_root()`). + #[cfg(test)] + fn has_root(&self) -> bool { + !matches!(self.state, PathState::PathsRoot) + } + + /// Return `true` if the current location is an endpoint (not a schema). + #[cfg(test)] + fn is_at_endpoint(&self) -> bool { + matches!(self.state, PathState::AtEndpoint(_)) + } + + /// Return `true` if the current location is a schema. + #[cfg(test)] + fn is_at_component(&self) -> bool { + matches!(self.state, PathState::AtComponent { .. }) + } + + /// Get the JSON pointer string for the current location. + pub fn current_pointer(&self) -> &str { + match &self.state { + PathState::PathsRoot => "#/paths", + PathState::AtEndpoint(path) => path.as_str(), + PathState::AtComponent { current, .. } => current.as_str(), + } + } + + /// Append a path segment to the current location. + /// + /// This does not push a new reference; it extends the current path. + /// + /// # Panics + /// + /// Panics if called on a `paths_root()` stack (use `for_endpoint()` for traversal). + pub(crate) fn append(&self, segment: &str) -> JsonPathStack { + let state = match &self.state { + PathState::PathsRoot => { + panic!("cannot append to paths_root (use for_endpoint for traversal)") + } + PathState::AtEndpoint(path) => PathState::AtEndpoint(path.append(segment)), + PathState::AtComponent { + current, + origin_ref, + intermediate_refs, + } => PathState::AtComponent { + current: current.append(segment), + origin_ref: origin_ref.clone(), + intermediate_refs: intermediate_refs.clone(), + }, + }; + Self { state } + } + + /// Push a schema reference onto the stack. + /// + /// This records the current location (with `/$ref` appended) in the + /// reference chain and sets the new current location to the schema path. + /// + /// Returns an error if `reference` is not a valid local schema path + /// (`#/...`). + /// + /// # Panics + /// + /// Panics if called on a `paths_root()` stack (a programming error). + pub(crate) fn push(&self, reference: &str) -> Result { + let schema = RefTargetPath::parse(reference).ok_or_else(|| InvalidComponentRef { + reference: reference.to_string(), + })?; + + let state = match &self.state { + PathState::PathsRoot => { + panic!("cannot push from paths_root (no endpoint context)") + } + PathState::AtEndpoint(path) => PathState::AtComponent { + current: schema, + origin_ref: path.append("$ref"), + intermediate_refs: Vec::new(), + }, + PathState::AtComponent { + current, + origin_ref, + intermediate_refs, + } => { + let mut new_intermediates = intermediate_refs.clone(); + new_intermediates.push(current.append("$ref")); + PathState::AtComponent { + current: schema, + origin_ref: origin_ref.clone(), + intermediate_refs: new_intermediates, + } + } + }; + Ok(Self { state }) + } + + /// Iterate over the path reference stack from top (current) to bottom + /// (origin). + /// + /// This yields the current path first, then the reference chain in reverse + /// order (most recent reference first, origin endpoint last). + pub fn iter(&self) -> impl Iterator { + match &self.state { + PathState::PathsRoot => { + Box::new(std::iter::once("#/paths")) as Box> + } + PathState::AtEndpoint(path) => { + Box::new(std::iter::once(path.as_str())) as Box> + } + PathState::AtComponent { + current, + origin_ref, + intermediate_refs, + } => Box::new( + std::iter::once(current.as_str()) + .chain(intermediate_refs.iter().rev().map(RefTargetPath::as_str)) + .chain(std::iter::once(origin_ref.as_str())), + ), + } + } +} + +impl fmt::Debug for JsonPathStack { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + // Format as a list of strings for compatibility with existing tests. + let mut out = f.debug_list(); + for path in self.iter() { + out.entry(&path); + } + out.finish() + } +} + +impl fmt::Display for JsonPathStack { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let mut first = true; + for path in self.iter() { + if !first { + write!(f, " -> ")?; + } + write!(f, "{}", path)?; + first = false; + } + Ok(()) + } +} + +/// Escape a segment for use in a JSON pointer per RFC 6901. +fn escape_json_pointer_segment(segment: &str) -> String { + segment.replace('~', "~0").replace('/', "~1") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn endpoint_path_for_path() { + let path = EndpointPath::for_path("/users"); + assert_eq!(path.as_str(), "#/paths/~1users"); + } + + #[test] + fn endpoint_path_for_path_escapes() { + // Paths with special characters get escaped. + let path = EndpointPath::for_path("/users/{id}/posts"); + assert_eq!(path.as_str(), "#/paths/~1users~1{id}~1posts"); + } + + #[test] + fn endpoint_path_append() { + let path = EndpointPath::for_path("/users") + .append("get") + .append("responses"); + assert_eq!(path.as_str(), "#/paths/~1users/get/responses"); + } + + #[test] + fn endpoint_path_append_escapes() { + let path = EndpointPath::for_path("/users") + .append("foo/bar") + .append("a~b"); + assert_eq!(path.as_str(), "#/paths/~1users/foo~1bar/a~0b"); + } + + #[test] + fn component_path_parse_valid() { + let path = RefTargetPath::parse("#/components/schemas/User"); + assert!(path.is_some()); + assert_eq!(path.unwrap().as_str(), "#/components/schemas/User"); + } + + #[test] + fn component_path_parse_valid_various() { + // Any JSON pointer is valid. + assert!(RefTargetPath::parse("#/components/responses/NotFound").is_some()); + assert!(RefTargetPath::parse("#/components/parameters/PageSize").is_some()); + assert!(RefTargetPath::parse("#/paths/~1users/get").is_some()); + assert!(RefTargetPath::parse("#/definitions/User").is_some()); + } + + #[test] + fn component_path_parse_invalid() { + // Must start with #/. + assert!(RefTargetPath::parse("components/schemas/User").is_none()); + assert!(RefTargetPath::parse("/components/schemas/User").is_none()); + assert!(RefTargetPath::parse("https://example.com/schema.json").is_none()); + } + + #[test] + fn component_path_append() { + let path = RefTargetPath::parse("#/components/schemas/User") + .unwrap() + .append("properties") + .append("name"); + assert_eq!(path.as_str(), "#/components/schemas/User/properties/name"); + } + + #[test] + fn json_path_stack_for_endpoint() { + let endpoint = EndpointPath::for_path("/users").append("get"); + let stack = JsonPathStack::for_endpoint(endpoint); + + assert_eq!(stack.current_pointer(), "#/paths/~1users/get"); + assert!(stack.is_at_endpoint()); + assert!(stack.has_root()); + } + + #[test] + fn json_path_stack_paths_root() { + let stack = JsonPathStack::paths_root(); + + assert_eq!(stack.current_pointer(), "#/paths"); + assert!(!stack.has_root()); + } + + #[test] + fn json_path_stack_append() { + let endpoint = EndpointPath::for_path("/users").append("get"); + let stack = JsonPathStack::for_endpoint(endpoint) + .append("responses") + .append("200") + .append("schema"); + + assert_eq!( + stack.current_pointer(), + "#/paths/~1users/get/responses/200/schema" + ); + assert!(stack.is_at_endpoint()); + } + + #[test] + fn json_path_stack_push() { + let endpoint = EndpointPath::for_path("/users").append("get"); + let stack = JsonPathStack::for_endpoint(endpoint) + .append("responses") + .append("200") + .append("schema") + .push("#/components/schemas/User") + .unwrap(); + + assert_eq!(stack.current_pointer(), "#/components/schemas/User"); + assert!(stack.is_at_component()); + + let entries: Vec<_> = stack.iter().collect(); + assert_eq!(entries.len(), 2); + assert_eq!(entries[0], "#/components/schemas/User"); + assert_eq!(entries[1], "#/paths/~1users/get/responses/200/schema/$ref"); + } + + #[test] + fn json_path_stack_iter_order() { + let endpoint = EndpointPath::for_path("/users").append("get"); + let stack = JsonPathStack::for_endpoint(endpoint) + .push("#/components/schemas/A") + .unwrap() + .push("#/components/schemas/B") + .unwrap(); + + let entries: Vec<_> = stack.iter().collect(); + assert_eq!(entries.len(), 3); + // Current is first. + assert_eq!(entries[0], "#/components/schemas/B"); + // Then refs in reverse order (most recent first). + assert_eq!(entries[1], "#/components/schemas/A/$ref"); + // Origin is last. + assert_eq!(entries[2], "#/paths/~1users/get/$ref"); + } + + #[test] + fn json_path_stack_display() { + let endpoint = EndpointPath::for_path("/users").append("get"); + let stack = JsonPathStack::for_endpoint(endpoint) + .push("#/components/schemas/User") + .unwrap(); + + let displayed = format!("{}", stack); + assert_eq!( + displayed, + "#/components/schemas/User -> #/paths/~1users/get/$ref" + ); + } + + #[test] + fn json_path_stack_push_invalid_ref_returns_error() { + let endpoint = EndpointPath::for_path("/users").append("get"); + let stack = JsonPathStack::for_endpoint(endpoint); + + let err = stack + .push("not/a/json/pointer") + .expect_err("expected error for invalid reference"); + + assert_eq!(err.reference, "not/a/json/pointer"); + assert_eq!( + err.to_string(), + "invalid reference \"not/a/json/pointer\": \ + expected JSON pointer starting with #/" + ); + } +} diff --git a/src/resolve.rs b/src/resolve.rs index e828842..187191e 100644 --- a/src/resolve.rs +++ b/src/resolve.rs @@ -1,4 +1,4 @@ -// Copyright 2025 Oxide Computer Company +// Copyright 2026 Oxide Computer Company use std::borrow::Cow; @@ -33,8 +33,7 @@ where }; loop { - assert!(target.starts_with("#/")); - context = context.push(target.as_ref()); + context = context.push(target.as_ref())?; let subtree = context .raw_openapi diff --git a/src/schema.rs b/src/schema.rs index 699ffca..234687e 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -8,7 +8,7 @@ use openapiv3::{ use crate::{ ChangeClass, ChangeComparison, ChangeDetails, - compare::Compare, + compare::{Compare, VisitState, VisitedKey}, context::{Context, Contextual, ToContext}, resolve::ReferenceOrResolver, setops::SetCompare, @@ -189,20 +189,44 @@ impl Compare { old_schema: Contextual<'_, &Schema>, new_schema: Contextual<'_, &Schema>, ) -> anyhow::Result { - if old_schema.context().stack().contains_cycle() - && new_schema.context().stack().contains_cycle() - { - return Ok(true); - } - - // Return the cached compatibility of these schemas so that we don't - // generate redundant notes. - if let Some(equal) = self.visited.get(&( + let key = VisitedKey::new( comparison, - old_schema.context().stack().top.clone(), - new_schema.context().stack().top.clone(), - )) { - return Ok(*equal); + old_schema.context().stack(), + new_schema.context().stack(), + ); + + // Check if there are cycles or if this is a visit state cache hit: + // + // - `Visiting`: the same key is being visited up the call stack, which + // means we've encountered a cycle. Over here, we treat this as + // compatible; if there are differences, they will be reported by the + // in-flight comparison wherever the cycle originates. + // + // Returning `true` on `Visiting` is sound only because the compare + // methods call `schema_push_change` eagerly on detecting a change + // rather than making decisions based on the value returned from this + // function. If that weren't the case -- for example, if there were + // code which did something like: + // + // let inner_eq = self.compare_schema(...)?; + // if !inner_eq { + // self.schema_push_change(...); + // } + // + // Then, relying on `true` here would result in a false negative. + // This invariant must be upheld by all compare methods. + // + // We don't write to `visit_state` when we hit this branch. + // + // - `Completed`: we've already compared this pair; reuse the result + // so we don't generate redundant notes. + // + // - Not present: we haven't seen this key before, so we need to + // proceed with comparisons. + match self.visit_state.get(&key) { + Some(VisitState::Visiting) => return Ok(true), + Some(VisitState::Completed { equal }) => return Ok(*equal), + None => {} } // We expand structures to ensure we don't accidentally fail to examine @@ -267,24 +291,28 @@ impl Compare { } let nullable_equal = old_nullable == new_nullable; + + // Mark the key as in flight while we recurse. + // + // If `compare_schema_kind` returns an error, the `?` below leaves the + // `Visiting` marker in place. This is benign today because `compare` + // propagates the error, and the entire `Compare` is dropped along with + // the marker. If we need to recover from this state in the future, we + // would most likely want to clear the stale `Visiting` entry -- if we + // don't do that, a re-entry for the same key would short-circuit to + // `Ok(true)`. + self.visit_state.insert(key.clone(), VisitState::Visiting); let schema_equal = self.compare_schema_kind( comparison, dry_run, Contextual::new(old_schema.context().clone(), old_schema_kind), Contextual::new(new_schema.context().clone(), new_schema_kind), )?; + let equal = nullable_equal && schema_equal; + self.visit_state + .insert(key, VisitState::Completed { equal }); - // Cache the result. - self.visited.insert( - ( - comparison, - old_schema.context().stack().top.clone(), - new_schema.context().stack().top.clone(), - ), - nullable_equal && schema_equal, - ); - - Ok(nullable_equal && schema_equal) + Ok(equal) } pub(crate) fn compare_schema_kind(