Merge branch 'wt/review-002-oai11-ref-memoization'

src/adapters/openapi_spec.rs

@@ -9,12 +9,32 @@ use std::collections::{BTreeMap, HashMap, HashSet};
 use alkcall::client::AdapterError;
 use serde_json::Value;
 
-/// Maximum structural depth budget for recursive `$ref` resolution
-/// (review 001 OAI-01). Bounds both `$ref` hop chains and schema nesting
-/// height so a self-referential or pathologically deep component fails
-/// import with a clean [`AdapterError::SchemaParse`] instead of
-/// exhausting the stack.
-pub(crate) const MAX_REF_RESOLUTION_DEPTH: usize = 64;
+/// Maximum structural nesting depth for recursive `$ref` resolution
+/// (review 001 OAI-01). Bounds schema object/array height so a
+/// pathologically deep component fails import with a clean
+/// [`AdapterError::SchemaParse`] instead of exhausting the stack.
+/// `$ref` hop chains are bounded separately by [`MAX_REF_HOP_DEPTH`] —
+/// one hop per schema level is legitimate above this height (a 40-level
+/// chain nests ~2 objects per level).
+pub(crate) const MAX_REF_RESOLUTION_DEPTH: usize = 128;
+
+/// Maximum `$ref` hop-chain length in one resolution (review 002 OAI-11).
+/// Depth of *expansion* and length of *pointer chasing* are different
+/// axes: the deepest structure reached and the number of distinct refs
+/// dereferenced along the way. A linear schema chain of any realistic
+/// size stays well under this; runaway chains fail with a clean
+/// [`AdapterError::SchemaParse`].
+pub(crate) const MAX_REF_HOP_DEPTH: usize = 64;
+
+/// Maximum number of `Value` nodes materialized by one `resolve_refs_recursive`
+/// call (review 002 OAI-11). Memoization makes repeated `$ref` hops cheap, but
+/// the fully-inlined output of an acyclic shared-ref chain still grows
+/// exponentially in the chain length — a doubly-linked-list spec
+/// (`S_i` referenced by both `a` and `b` of `S_{i-1}`) inlines to ~2^N nodes.
+/// Neither the visited set nor the depth budget fires for that shape; this
+/// budget does, failing import with a clean [`AdapterError::SchemaParse`]
+/// instead of wedging with no error (or exhausting memory).
+pub(crate) const MAX_REF_EXPANSION_NODES: usize = 1_000_000;
 
 /// The `paths`-level HTTP methods the adapter models. `trace` is
 /// deliberately absent (OAI-06): a path carrying only unsupported
@@ -367,23 +387,37 @@ impl OpenAPISpec {
     /// Resolve `$ref` pointers within `schema` against the raw document,
     /// returning a fully expanded copy.
     ///
-    /// Recursion is bounded: a visited set keyed on the JSON-pointer path
-    /// rejects a `$ref` that is already being expanded on the current
-    /// branch (a self/circular reference — trees, linked lists, cursor
-    /// pagination), and a depth budget rejects pathologically deep
-    /// nesting. Both surface as [`AdapterError::SchemaParse`] at import
-    /// instead of recursing to stack exhaustion. Fully-expanded inlining
-    /// of recursive schemas is deliberately not supported; specs that
-    /// rely on it fail loudly here (review 001 OAI-01).
+    /// Recursion is bounded in both stack and total work: a visited set
+    /// keyed on the JSON-pointer path rejects a `$ref` that is already
+    /// being expanded on the current branch (a self/circular reference —
+    /// trees, linked lists, cursor pagination), a depth budget rejects
+    /// pathologically deep nesting, and a per-document memo of completed
+    /// cycle-free `$ref` expansions ensures each distinct ref target is
+    /// expanded once and reused by clone on every later hop (review 002
+    /// OAI-11) — an acyclic shared-ref chain grows linearly instead of
+    /// as an exponential tree. All four surface as
+    /// [`AdapterError::SchemaParse`] at import instead of recursing to
+    /// stack exhaustion or wedging without an error. Fully-expanded
+    /// inlining of recursive schemas is deliberately not supported; specs
+    /// that rely on it fail loudly here (review 001 OAI-01, review 002
+    /// OAI-11). Memo entries are written only after a successful
+    /// expansion, so a partial expansion under a cyclic branch is never
+    /// cached and cycle detection semantics are unchanged.
+    ///
+    /// As the final bound (a memo alone cannot shrink the inlined output of
+    /// an acyclic exponential shared-ref chain), a per-call node budget
+    /// counts every materialized `Value` node; exceeding it fails import
+    /// with a clean error naming the budget (OAI-11).
     pub(crate) fn resolve_refs_recursive(&self, schema: &Value) -> Result<Value, AdapterError> {
-        self.resolve_refs_bounded(schema, &mut HashSet::new(), 0)
+        self.resolve_refs_bounded(schema, &mut RefResolution::default(), 0, 0)
     }
 
     fn resolve_refs_bounded(
         &self,
         schema: &Value,
-        resolving: &mut HashSet<String>,
+        state: &mut RefResolution,
         depth: usize,
+        hops: usize,
     ) -> Result<Value, AdapterError> {
         if depth > MAX_REF_RESOLUTION_DEPTH {
             return Err(AdapterError::SchemaParse {
@@ -393,10 +427,36 @@ impl OpenAPISpec {
                 ),
             });
         }
+        state.nodes = state.nodes.saturating_add(1);
+        if state.nodes > MAX_REF_EXPANSION_NODES {
+            return Err(AdapterError::SchemaParse {
+                message: format!(
+                    "$ref expansion exceeded node budget of {MAX_REF_EXPANSION_NODES} \
+                     (acyclic shared-$ref chain expanding exponentially; the schema is \
+                     valid but its full inline expansion is too large to materialize)"
+                ),
+            });
+        }
         match schema {
             Value::Object(obj) => {
                 if let Some(Value::String(reference)) = obj.get("$ref") {
-                    if !resolving.insert(reference.clone()) {
+                    if let Some(resolved) = state.memo.get(reference) {
+                        let nodes = count_nodes(resolved);
+                        state.nodes = state.nodes.saturating_add(nodes);
+                        if state.nodes > MAX_REF_EXPANSION_NODES {
+                            return Err(AdapterError::SchemaParse {
+                                message: format!(
+                                    "$ref expansion exceeded node budget of \
+                                     {MAX_REF_EXPANSION_NODES} \
+                                     (acyclic shared-$ref chain expanding exponentially; \
+                                     the schema is valid but its full inline expansion is \
+                                     too large to materialize)"
+                                ),
+                            });
+                        }
+                        return Ok(resolved.clone());
+                    }
+                    if !state.resolving.insert(reference.clone()) {
                         return Err(AdapterError::SchemaParse {
                             message: format!(
                                 "circular $ref detected at depth {depth}: {reference}"
@@ -404,9 +464,20 @@ impl OpenAPISpec {
                         });
                     }
                     let resolved = self.resolve_ref(reference)?;
-                    let out = self.resolve_refs_bounded(&resolved, resolving, depth + 1);
-                    resolving.remove(reference);
-                    return out;
+                    let hops = hops + 1;
+                    if hops > MAX_REF_HOP_DEPTH {
+                        return Err(AdapterError::SchemaParse {
+                            message: format!(
+                                "$ref resolution exceeded hop budget of {MAX_REF_HOP_DEPTH} \
+                                 (runaway $ref chain)"
+                            ),
+                        });
+                    }
+                    let out = self.resolve_refs_bounded(&resolved, state, depth + 1, hops);
+                    state.resolving.remove(reference);
+                    let value = out?;
+                    state.memo.insert(reference.clone(), value.clone());
+                    return Ok(value);
                 }
                 if depth + 1 > MAX_REF_RESOLUTION_DEPTH {
                     return Err(AdapterError::SchemaParse {
@@ -420,7 +491,7 @@ impl OpenAPISpec {
                 for (k, v) in obj {
                     out.insert(
                         k.clone(),
-                        self.resolve_refs_bounded(v, resolving, depth + 1)?,
+                        self.resolve_refs_bounded(v, state, depth + 1, hops)?,
                     );
                 }
                 Ok(Value::Object(out))
@@ -428,7 +499,7 @@ impl OpenAPISpec {
             Value::Array(arr) => {
                 let mut out = Vec::with_capacity(arr.len());
                 for v in arr {
-                    out.push(self.resolve_refs_bounded(v, resolving, depth + 1)?);
+                    out.push(self.resolve_refs_bounded(v, state, depth + 1, hops)?);
                 }
                 Ok(Value::Array(out))
             }
@@ -437,6 +508,25 @@ impl OpenAPISpec {
     }
 }
 
+/// Cycle-scoped state for [`OpenAPISpec::resolve_refs_recursive`]: the
+/// in-flight `$ref` chain (cycle detection), the memo of completed
+/// cycle-free expansions (total-work bounding, review 002 OAI-11), and the
+/// per-call node counter (the last-resort bound on inlined output size).
+#[derive(Default)]
+struct RefResolution {
+    resolving: HashSet<String>,
+    memo: HashMap<String, Value>,
+    nodes: usize,
+}
+
+fn count_nodes(value: &Value) -> usize {
+    match value {
+        Value::Object(map) => 1 + map.values().map(count_nodes).sum::<usize>(),
+        Value::Array(items) => 1 + items.iter().map(count_nodes).sum::<usize>(),
+        _ => 1,
+    }
+}
+
 fn parse_operation(
     raw: &Value,
     spec: &OpenAPISpec,
@@ -825,6 +915,272 @@ mod tests {
         assert_eq!(resolved["properties"]["a"]["type"], "string");
     }
 
+    #[test]
+    fn thirty_level_shared_chain_returns_bounded_with_clean_budget_error() {
+        let levels = 32usize;
+        let mut components = serde_json::Map::new();
+        for i in 0..levels {
+            let next_ref = if i + 1 < levels {
+                json!({"$ref": format!("#/components/schemas/S{}", i + 1)})
+            } else {
+                json!({"type": "string"})
+            };
+            components.insert(
+                format!("S{i}"),
+                json!({"a": next_ref.clone(), "b": next_ref}),
+            );
+        }
+        let spec = schema_test_spec(Value::Object(components));
+        let schema = spec
+            .components
+            .as_ref()
+            .expect("test spec has schemas")
+            .schemas
+            .get("S0")
+            .expect("test schema present")
+            .clone();
+        let result = spec.resolve_refs_recursive(&schema);
+        match result {
+            Err(AdapterError::SchemaParse { message }) => {
+                assert!(message.contains("node budget"), "message was: {message}");
+            }
+            other => panic!("expected node-budget SchemaParse error, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn forty_level_single_chain_imports_fast_under_memoization() {
+        let levels = 40usize;
+        let mut components = serde_json::Map::new();
+        for i in 0..levels {
+            let next = if i + 1 < levels {
+                json!({"$ref": format!("#/components/schemas/S{}", i + 1)})
+            } else {
+                json!({"type": "string"})
+            };
+            components.insert(format!("S{i}"), json!({"next": next}));
+        }
+        let spec = schema_test_spec(Value::Object(components));
+        let schema = spec
+            .components
+            .as_ref()
+            .expect("test spec has schemas")
+            .schemas
+            .get("S0")
+            .expect("test schema present")
+            .clone();
+        let resolved = spec
+            .resolve_refs_recursive(&schema)
+            .expect("acyclic chain resolves");
+        let mut cursor = &resolved;
+        for _ in 1..=levels {
+            cursor = &cursor["next"];
+        }
+        assert_eq!(cursor["type"], "string", "innermost level fully expanded");
+    }
+
+    #[test]
+    fn memoized_expansion_matches_pre_memoization_golden() {
+        let spec = schema_test_spec(json!({
+            "Id": {"type": "string", "maxLength": 4},
+            "Stamp": {"type": "object", "required": ["at"]},
+            "Chain": {
+                "type": "object",
+                "properties": {
+                    "id": {"$ref": "#/components/schemas/Id"},
+                    "next": {
+                        "type": "object",
+                        "properties": {
+                            "id": {"$ref": "#/components/schemas/Id"},
+                            "stamp": {"$ref": "#/components/schemas/Stamp"},
+                            "again": {"$ref": "#/components/schemas/Id"}
+                        }
+                    },
+                    "stamp": {"$ref": "#/components/schemas/Stamp"}
+                }
+            }
+        }));
+        let schema = spec
+            .components
+            .as_ref()
+            .expect("test spec has schemas")
+            .schemas
+            .get("Chain")
+            .expect("test schema present")
+            .clone();
+        let resolved = spec
+            .resolve_refs_recursive(&schema)
+            .expect("expansion succeeds");
+        let expected = json!({
+            "type": "object",
+            "properties": {
+                "id": {"type": "string", "maxLength": 4},
+                "next": {
+                    "type": "object",
+                    "properties": {
+                        "id": {"type": "string", "maxLength": 4},
+                        "stamp": {"type": "object", "required": ["at"]},
+                        "again": {"type": "string", "maxLength": 4}
+                    }
+                },
+                "stamp": {"type": "object", "required": ["at"]}
+            }
+        });
+        assert_eq!(resolved, expected);
+    }
+
+    #[test]
+    fn cycle_through_shared_node_still_errors() {
+        let spec = schema_test_spec(json!({
+            "Shared": {"properties": {
+                "back": {"$ref": "#/components/schemas/Loop"}
+            }},
+            "Loop": {"properties": {
+                "shared": {"$ref": "#/components/schemas/Shared"}
+            }}
+        }));
+        let schema = spec
+            .components
+            .as_ref()
+            .expect("test spec has schemas")
+            .schemas
+            .get("Loop")
+            .expect("test schema present")
+            .clone();
+        let result = spec.resolve_refs_recursive(&schema);
+        match result {
+            Err(AdapterError::SchemaParse { message }) => {
+                assert!(message.contains("circular $ref"), "message was: {message}");
+            }
+            other => panic!("expected circular-$ref SchemaParse error, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn cycle_via_array_items_errors() {
+        let spec = schema_test_spec(json!({
+            "Node": {"type": "array", "items": {"$ref": "#/components/schemas/Node"}}
+        }));
+        let schema = spec
+            .components
+            .as_ref()
+            .expect("test spec has schemas")
+            .schemas
+            .get("Node")
+            .expect("test schema present")
+            .clone();
+        let result = spec.resolve_refs_recursive(&schema);
+        match result {
+            Err(AdapterError::SchemaParse { message }) => {
+                assert!(message.contains("circular $ref"), "message was: {message}");
+                assert!(message.contains("Node"), "message was: {message}");
+            }
+            other => panic!("expected circular-$ref SchemaParse error, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn cycle_via_all_of_errors() {
+        let spec = schema_test_spec(json!({
+            "Node": {"allOf": [{"$ref": "#/components/schemas/Node"}]}
+        }));
+        let schema = spec
+            .components
+            .as_ref()
+            .expect("test spec has schemas")
+            .schemas
+            .get("Node")
+            .expect("test schema present")
+            .clone();
+        let result = spec.resolve_refs_recursive(&schema);
+        match result {
+            Err(AdapterError::SchemaParse { message }) => {
+                assert!(message.contains("circular $ref"), "message was: {message}");
+                assert!(message.contains("Node"), "message was: {message}");
+            }
+            other => panic!("expected circular-$ref SchemaParse error, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn cycle_via_additional_properties_errors() {
+        let spec = schema_test_spec(json!({
+            "Node": {"type": "object", "additionalProperties": {
+                "$ref": "#/components/schemas/Node"
+            }}
+        }));
+        let schema = spec
+            .components
+            .as_ref()
+            .expect("test spec has schemas")
+            .schemas
+            .get("Node")
+            .expect("test schema present")
+            .clone();
+        let result = spec.resolve_refs_recursive(&schema);
+        match result {
+            Err(AdapterError::SchemaParse { message }) => {
+                assert!(message.contains("circular $ref"), "message was: {message}");
+                assert!(message.contains("Node"), "message was: {message}");
+            }
+            other => panic!("expected circular-$ref SchemaParse error, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn cycle_via_ref_sibling_errors() {
+        let spec = schema_test_spec(json!({
+            "Node": {
+                "$ref": "#/components/schemas/Node",
+                "minLength": 3
+            }
+        }));
+        let schema = spec
+            .components
+            .as_ref()
+            .expect("test spec has schemas")
+            .schemas
+            .get("Node")
+            .expect("test schema present")
+            .clone();
+        let result = spec.resolve_refs_recursive(&schema);
+        match result {
+            Err(AdapterError::SchemaParse { message }) => {
+                assert!(message.contains("circular $ref"), "message was: {message}");
+                assert!(message.contains("Node"), "message was: {message}");
+            }
+            other => panic!("expected circular-$ref SchemaParse error, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn memo_hit_matches_fresh_expansion_for_acyclic_shared_refs() {
+        let spec = schema_test_spec(json!({
+            "Id": {"type": "string", "maxLength": 4},
+            "Wrapper": {
+                "type": "object",
+                "properties": {
+                    "a": {"$ref": "#/components/schemas/Id"},
+                    "b": {"$ref": "#/components/schemas/Id"}
+                }
+            }
+        }));
+        let schema = spec
+            .components
+            .as_ref()
+            .expect("test spec has schemas")
+            .schemas
+            .get("Wrapper")
+            .expect("test schema present")
+            .clone();
+        let resolved = spec
+            .resolve_refs_recursive(&schema)
+            .expect("no false cycle");
+        assert_eq!(resolved["properties"]["a"], resolved["properties"]["b"]);
+        assert_eq!(resolved["properties"]["a"]["type"], "string");
+        assert_eq!(resolved["properties"]["a"]["maxLength"], 4);
+    }
+
     #[test]
     fn import_of_self_referential_spec_returns_error_not_abort() {
         let doc = r##"{