Skip to content

[4/n] add stricter typing in JsonPathStack #15

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sunshowers wants to merge 8 commits into
base: sunshowers/spr/main.4n-add-stricter-typing-in-jsonpathstack
Choose a base branch
from
Open

[4/n] add stricter typing in JsonPathStack #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b72f148
[spr] initial version
sunshowers Feb 6, 2026
23982f9
[spr] changes to main this commit is based on
sunshowers Feb 6, 2026
2c96ca4
clippy
sunshowers Feb 6, 2026
96dab89
[spr] changes introduced through rebase
sunshowers Feb 6, 2026
7045223
copyright year
sunshowers Feb 6, 2026
1c765ad
rebase on main, include test for omicron failure
sunshowers May 5, 2026
4addd2c
rebase on #23, switch to cycle detection via postorder DFS
sunshowers May 8, 2026
8a960bf
more comments explaining soundness
sunshowers May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 64 additions & 14 deletions src/compare.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -25,23 +25,72 @@ pub fn compare(old: &Value, new: &Value) -> anyhow::Result<Vec<Change>> {
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,
},
}
Comment on lines +69 to +81
Copy link
Copy Markdown
Contributor Author

@sunshowers sunshowers May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this PR I replaced the existing cycle detection check, which relied on looking at the stack of schemas, with a more explicit DFS postorder check.

This is equivalent to the old code:

        if old_schema.context().stack().contains_cycle()
            && new_schema.context().stack().contains_cycle()
        {
            return Ok(true);
        }

Why? Basically because having a prefix in the stack is equivalent to being in the visiting state while performing the DFS. The tests added in #23 don't have any output differences, which is a reasonable sign that there aren't any behavior changes here.

I like how explicit the postorder check is, and it also fits quite nicely into visit_state.


#[derive(Default)]
pub(crate) struct Compare {
pub changes: Vec<Change>,
/// 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<VisitedKey, VisitState>,
}

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,
Expand All @@ -55,10 +104,11 @@ impl Compare {
Some(name) => name.as_str(),
None => "<unnamed>",
};
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,
Expand All @@ -71,10 +121,10 @@ impl Compare {
Some(name) => name.as_str(),
None => "<unnamed>",
};

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,
Expand Down
42 changes: 27 additions & 15 deletions src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
// Copyright 2025 Oxide Computer Company
// Copyright 2026 Oxide Computer Company

use std::{borrow::Cow, ops::Deref};

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> {
Expand All @@ -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<Context<'a>, InvalidComponentRef> {
Ok(Self {
raw_openapi: self.raw_openapi,
stack: self.stack.push(path)?,
})
}

pub fn stack(&self) -> &JsonPathStack {
&self.stack
}
Expand Down
147 changes: 3 additions & 144 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2025 Oxide Computer Company
// Copyright 2026 Oxide Computer Company

//! Drift
//!
Expand All @@ -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<String>,
}

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<Item = &String> {
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;
Loading
Loading