diff --git a/README.md b/README.md index 5840318..6107bc7 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ Monitor OpenAPI schema diffs between git branches, detect breaking changes, gene |[![License: MIT](https://img.shields.io/badge/License-MIT-yellow.svg)](https://github.com/Coding-Dev-Tools/api-contract-guardian/blob/main/LICENSE) |[![Open Source Alternative](https://img.shields.io/badge/Open_Source_Alternative-%E2%87%92-blue?logo=opensourceinitiative)](https://www.opensourcealternative.to/project/api-contract-guardian) |[![LibHunt](https://img.shields.io/badge/LibHunt-%E2%87%92-blue?logo=codeigniter)](https://www.libhunt.com/r/Coding-Dev-Tools/api-contract-guardian) -|[![PyPI](https://img.shields.io/pypi/v/api-contract-guardian)](https://pypi.org/project/api-contract-guardian/) +| **Why API Contract Guardian?** @@ -24,11 +24,9 @@ Real-world scenarios: ## Installation -```bash -pip install api-contract-guardian -``` +> **Note:** `api-contract-guardian` is not yet on public PyPI. Use one of the methods below. -Or install the latest version directly from GitHub: +Install the latest version directly from GitHub: ```bash pip install git+https://github.com/Coding-Dev-Tools/api-contract-guardian.git diff --git a/src/api_contract_guardian/diff.py b/src/api_contract_guardian/diff.py index 8a68ef4..180131d 100644 --- a/src/api_contract_guardian/diff.py +++ b/src/api_contract_guardian/diff.py @@ -985,6 +985,110 @@ def _diff_schema_details( ) ) + # Deep structural diff of NESTED object / array-of-object properties. + # Previously _diff_schema_details only compared a component's DIRECT + # properties (type/format/enum); a nested object property whose sub-fields + # changed (e.g. a required field added deep inside) was reported as "no + # change" -- the silent-green failure class this tool exists to catch. + # Reuse the proven recursive inline-schema walker (conservative: treats all + # required/presence changes as BREAKING, since a component may back a request + # body). Only the nested sub-schemas are walked here, so this never + # double-reports the component's own top-level properties already checked above. + for prop_name in old_props: + if prop_name not in new_props: + continue + old_sub = old_props[prop_name] or {} + new_sub = new_props[prop_name] or {} + if not isinstance(old_sub, dict) or not isinstance(new_sub, dict): + continue + is_nested_object = ( + (old_sub.get("type") == "object" and new_sub.get("type") == "object") + or ("properties" in old_sub and "properties" in new_sub) + ) + if is_nested_object: + _diff_inline_schema( + f"{schema_path}.properties.{prop_name}", + old_sub, + new_sub, + result, + is_request=True, + depth=1, + ) + continue + if old_sub.get("type") == "array" and new_sub.get("type") == "array": + old_items = old_sub.get("items") or {} + new_items = new_sub.get("items") or {} + if isinstance(old_items, dict) and isinstance(new_items, dict) and ( + (old_items.get("type") == "object" and new_items.get("type") == "object") + or ("properties" in old_items and "properties" in new_items) + ): + _diff_inline_schema( + f"{schema_path}.properties.{prop_name}.items", + old_items, + new_items, + result, + is_request=True, + depth=1, + ) + + # A property that is a $ref (or array of $ref) into another component was + # previously ignored entirely: retargeting or dropping the ref produced no + # change record. Flag ref-target changes at the property's OWN path. The + # referenced target's internals are already diffed by _diff_schemas at + # components.schemas., so this never double-reports them -- it only + # surfaces that THIS property's reference changed. + for prop_name in set(old_props) | set(new_props): + _flag_schema_ref_prop( + schema_path, prop_name, old_props.get(prop_name), new_props.get(prop_name), result + ) + + +def _flag_schema_ref_prop( + schema_path: str, + prop_name: str, + old_prop: Any, + new_prop: Any, + result: DiffResult, +) -> None: + """Flag a component property whose ``$ref`` target changed or was dropped. + + Refs into ``components.schemas`` are intentionally NOT followed into the + target's internals (``_diff_schemas`` already diffs the target at its own + path); we only report that the reference here changed, which is independent + information not otherwise surfaced. + """ + + def ref_of(sub: Any) -> Any: + if isinstance(sub, dict): + return sub.get("$ref") + return None + + old_ref = ref_of(old_prop) + new_ref = ref_of(new_prop) + # Array whose items are a $ref. + if old_ref is None and isinstance(old_prop, dict) and old_prop.get("type") == "array": + old_ref = ref_of(old_prop.get("items")) + if new_ref is None and isinstance(new_prop, dict) and new_prop.get("type") == "array": + new_ref = ref_of(new_prop.get("items")) + + if old_ref is None and new_ref is None: + return + + prop_path = f"{schema_path}.properties.{prop_name}" + if old_ref != new_ref: + result.changes.append( + Change( + kind="schema_ref_changed", + severity=Severity.DANGEROUS, + path=prop_path, + description=( + f"$ref property '{prop_name}' target changed from '{old_ref}' to '{new_ref}'" + ), + old_value=old_ref, + new_value=new_ref, + ) + ) + def _diff_security_schemes( old: dict[str, Any], diff --git a/tests/test_diff.py b/tests/test_diff.py index f7b6106..628e109 100644 --- a/tests/test_diff.py +++ b/tests/test_diff.py @@ -663,6 +663,159 @@ def test_property_no_longer_required_non_breaking(self): result = diff_specs(old, new) assert any(c.kind == "property_no_longer_required" for c in result.changes) + def test_nested_object_property_required_change_is_breaking(self): + # A required field added DEEP inside a nested object property was + # previously reported as "no change" (silent-green gap at the + # component level). It must now surface as a breaking change. + old = _make_spec( + schemas={ + "User": { + "type": "object", + "properties": { + "id": {"type": "string"}, + "address": { + "type": "object", + "properties": { + "street": {"type": "string"}, + "zip": {"type": "string"}, + }, + }, + }, + } + } + ) + new = _make_spec( + schemas={ + "User": { + "type": "object", + "properties": { + "id": {"type": "string"}, + "address": { + "type": "object", + "required": ["zip"], + "properties": { + "street": {"type": "string"}, + "zip": {"type": "string"}, + }, + }, + }, + } + } + ) + result = diff_specs(old, new) + became_required = [ + c for c in result.changes if c.kind == "request_property_became_required" + ] + assert became_required, "nested required change must be detected" + assert any( + "address" in c.path and "zip" in c.path for c in became_required + ), "change must be reported at the nested path" + assert all(c.severity.value == "breaking" for c in became_required), ( + "nested required change is breaking" + ) + + def test_nested_array_item_property_required_change_is_breaking(self): + old = _make_spec( + schemas={ + "Cart": { + "type": "object", + "properties": { + "items": { + "type": "array", + "items": { + "type": "object", + "properties": {"sku": {"type": "string"}}, + }, + }, + }, + } + } + ) + new = _make_spec( + schemas={ + "Cart": { + "type": "object", + "properties": { + "items": { + "type": "array", + "items": { + "type": "object", + "required": ["sku"], + "properties": {"sku": {"type": "string"}}, + }, + }, + }, + } + } + ) + result = diff_specs(old, new) + became_required = [ + c for c in result.changes if c.kind == "request_property_became_required" + ] + assert became_required, "nested array-item required change must be detected" + assert any("items" in c.path and "sku" in c.path for c in became_required) + + def test_component_ref_property_target_change_is_dangerous(self): + # A component property whose $ref target was retargeted must be + # flagged as DANGEROUS (it was previously silently ignored at the + # component level). + old = _make_spec( + schemas={ + "Order": { + "type": "object", + "properties": { + "customer": {"$ref": "#/components/schemas/CustomerV1"}, + }, + }, + "CustomerV1": {"type": "object"}, + "CustomerV2": {"type": "object"}, + } + ) + new = _make_spec( + schemas={ + "Order": { + "type": "object", + "properties": { + "customer": {"$ref": "#/components/schemas/CustomerV2"}, + }, + }, + "CustomerV1": {"type": "object"}, + "CustomerV2": {"type": "object"}, + } + ) + result = diff_specs(old, new) + ref_changed = [c for c in result.changes if c.kind == "schema_ref_changed"] + assert ref_changed, "ref-target change must be detected" + assert any(c.severity.value == "dangerous" for c in ref_changed) + assert any("Order" in c.path and "customer" in c.path for c in ref_changed) + + def test_component_ref_property_removed_is_dangerous(self): + old = _make_spec( + schemas={ + "Order": { + "type": "object", + "properties": { + "customer": {"$ref": "#/components/schemas/Customer"}, + }, + }, + "Customer": {"type": "object"}, + } + ) + new = _make_spec( + schemas={ + "Order": { + "type": "object", + "properties": { + "customer": {"type": "object"}, + }, + }, + "Customer": {"type": "object"}, + } + ) + result = diff_specs(old, new) + ref_changed = [c for c in result.changes if c.kind == "schema_ref_changed"] + assert ref_changed, "dropped $ref must be detected" + def test_property_removed_breaking_if_required(self): old = _make_spec( schemas={