From ff427ab095f79eae660ba6e8b12f314e85b2e1d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20Sch=C3=B6dl?= Date: Fri, 16 Oct 2026 16:31:34 +0200 Subject: [PATCH 1/3] Grade breaking changes from the consumer or the producer perspective --- CHANGELOG.md | 7 + README.md | 2 +- datacontract/api.py | 18 +- datacontract/breaking/detector.py | 76 ++- datacontract/breaking/rules.py | 418 +++++++++--- datacontract/command_breaking.py | 7 +- datacontract/data_contract.py | 35 +- datacontract/model/breaking.py | 20 + datacontract/output/text_breaking_results.py | 3 + docs/docs/api.md | 4 +- docs/docs/compare-contract-versions.md | 47 +- docs/docs/python-library.md | 6 +- .../changelog/golden_breaking_text.txt | 187 +++-- tests/test_api.py | 13 + tests/test_breaking.py | 640 +++++++++--------- tests/test_breaking_odcs_3_2.py | 51 +- 16 files changed, 924 insertions(+), 610 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index efb0d3d5d..b038eba1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added +- `datacontract breaking --perspective producer` (and `perspective` on `POST /breaking` and `DataContract.breaking()`) flags changes that data which passed v1 may fail +- `datacontract breaking` grades relationships, `physicalName`, `minItems`, `maxItems`, `uniqueItems`, `mustBeBetween`, quality rule `severity`, and the `freshness` service level, and shows each change's direction + +### Changed +- `datacontract breaking` grades from the consumer's perspective by default + ## [1.2.6] - 2026-10-13 ### Changed diff --git a/README.md b/README.md index fc2abe198..0047fe162 100644 --- a/README.md +++ b/README.md @@ -147,7 +147,7 @@ $ datacontract lint odcs.yaml # show a changelog between two data contracts $ datacontract changelog v1.odcs.yaml v2.odcs.yaml -# fail when a contract change is backward-incompatible +# fail when a contract change breaks a guarantee consumers rely on $ datacontract breaking v1.odcs.yaml v2.odcs.yaml # execute schema and quality checks (define credentials as environment variables) diff --git a/datacontract/api.py b/datacontract/api.py index 40945b186..6626010d8 100644 --- a/datacontract/api.py +++ b/datacontract/api.py @@ -14,7 +14,7 @@ from datacontract.config import Config, known_env_names from datacontract.config.variables import CONTRACT_VARIABLES_ENV, allowed_environment, resolve_variables from datacontract.data_contract import DataContract, ExportFormat -from datacontract.model.breaking import BreakingChangeEntry +from datacontract.model.breaking import BreakingChangeEntry, Perspective from datacontract.model.changelog import ChangelogEntry from datacontract.model.exceptions import DataContractException, DefinitionResolutionError from datacontract.model.run import Check, ResultEnum, Run @@ -943,6 +943,9 @@ class BreakingChangesResponse(BaseModel): entries: list[BreakingChangeEntry] = Field( description="Every single change with its grade and the old and new value.", ) + perspective: Perspective = Field( + description="Whose expectations the changes were graded against: `consumer` or `producer`.", + ) is_breaking: bool = Field( description="Whether any detected change is classified as an error-level breaking change.", ) @@ -1006,13 +1009,22 @@ def changelog_endpoint( def breaking_endpoint( body: ChangelogRequest, api_key: Annotated[str | None, Depends(api_key_header)] = None, + perspective: Annotated[ + Perspective, + Query( + description="Whose expectations a change must not break: `consumer` flags changes that let v2 deliver " + "data v1 ruled out, `producer` flags changes that data which passed v1 may fail.", + ), + ] = Perspective.CONSUMER, ) -> BreakingChangesResponse: check_api_key(api_key) result = DataContract(data_contract_str=body.v1, untrusted_contract=True).breaking( - DataContract(data_contract_str=body.v2, untrusted_contract=True) + DataContract(data_contract_str=body.v2, untrusted_contract=True), perspective + ) + return BreakingChangesResponse( + summary=result.summary, entries=result.entries, perspective=result.perspective, is_breaking=result.is_breaking ) - return BreakingChangesResponse(summary=result.summary, entries=result.entries, is_breaking=result.is_breaking) @app.post( diff --git a/datacontract/breaking/detector.py b/datacontract/breaking/detector.py index 0bc0c4dc3..ce4facc07 100644 --- a/datacontract/breaking/detector.py +++ b/datacontract/breaking/detector.py @@ -1,8 +1,14 @@ from collections import defaultdict from collections.abc import Iterable -from datacontract.breaking.rules import DEFAULT_RULES, BreakingChangeRule -from datacontract.model.breaking import BreakingChangeEntry, BreakingChangeLevel, BreakingChangeResult +from datacontract.breaking.rules import DEFAULT_RULES, BreakingChangeRule, MetadataFallbackRule +from datacontract.model.breaking import ( + BreakingChangeEntry, + BreakingChangeLevel, + BreakingChangeResult, + Direction, + Perspective, +) from datacontract.model.changelog import ChangelogEntry, ChangelogResult, ChangelogType _LEVEL_ORDER = { @@ -20,43 +26,22 @@ def __init__(self, rules: Iterable[BreakingChangeRule] = DEFAULT_RULES): if not self._rules: raise ValueError("At least one breaking-change rule is required") - def detect(self, changelog: ChangelogResult) -> BreakingChangeResult: - entries = [self._classify(entry) for entry in changelog.entries] - # Nothing inside an added element can break a consumer: it did not exist before. - # Inside a removed one, the element's own entry carries the grade; its former - # constraints are gone, which is a relaxation. - added_or_removed = { + def detect( + self, changelog: ChangelogResult, perspective: Perspective, before: dict, after: dict + ) -> BreakingChangeResult: + """Grade ``changelog``, built from the normalized contracts ``before`` and ``after``, for ``perspective``.""" + entries = [self._classify(entry, perspective, before, after) for entry in changelog.entries] + # An added or removed element carries the grade for everything inside it. + elements = { entry.path for entry in entries if entry.change_type in (ChangelogType.added, ChangelogType.removed) - and ( - entry.path.startswith("schema.") - and (len(entry.path.split(".")) == 2 or entry.path.split(".")[-2] in ("properties", "quality")) - ) - } - removed_enums = { - entry.path - for entry in entries - if entry.change_type == ChangelogType.removed and entry.path.endswith(".enum") + and entry.rule_id != MetadataFallbackRule.rule_id } - # Stable IDs may change or swap without changing the set of allowed - # values. Match removed/updated values against additions in the same enum. - added_values = defaultdict(set) for entry in entries: - if entry.rule_id == "enum-constraint-changed" and entry.path.endswith(".value"): - if entry.change_type in (ChangelogType.added, ChangelogType.updated): - added_values[entry.path.rsplit(".enum.", 1)[0]].add(entry.new_value) - for entry in entries: - segments = entry.path.split(".") - if any(".".join(segments[:i]) in added_or_removed for i in range(1, len(segments))): + if any(entry.path.startswith(f"{path}.") for path in elements): + entry.direction = None entry.level = BreakingChangeLevel.INFO - if entry.rule_id == "enum-constraint-changed": - if any(entry.path.startswith(path + ".") for path in removed_enums): - entry.level = BreakingChangeLevel.INFO - elif entry.path.endswith(".value") and entry.old_value in added_values.get( - entry.path.rsplit(".enum.", 1)[0], set() - ): - entry.level = BreakingChangeLevel.INFO entries_by_prefix = defaultdict(list) for entry in entries: prefix = "" @@ -64,16 +49,33 @@ def detect(self, changelog: ChangelogResult) -> BreakingChangeResult: prefix = f"{prefix}.{segment}" if prefix else segment entries_by_prefix[prefix].append(entry) summary = [self._summarize(entry, entries_by_prefix.get(entry.path, [])) for entry in changelog.summary] - return BreakingChangeResult(v1=changelog.v1, v2=changelog.v2, summary=summary, entries=entries) + return BreakingChangeResult( + v1=changelog.v1, v2=changelog.v2, perspective=perspective, summary=summary, entries=entries + ) - def _classify(self, entry: ChangelogEntry) -> BreakingChangeEntry: + def _classify( + self, entry: ChangelogEntry, perspective: Perspective, before: dict, after: dict + ) -> BreakingChangeEntry: for rule in self._rules: - evaluation = rule.evaluate(entry) + evaluation = rule.evaluate(entry, before, after) if evaluation is not None: + direction = evaluation.direction + if direction == Direction.BOTH: + level = BreakingChangeLevel.ERROR + elif direction == Direction.UNDETERMINED: + level = BreakingChangeLevel.WARNING + elif (direction, perspective) in ( + (Direction.TIGHTENS, Perspective.PRODUCER), + (Direction.LOOSENS, Perspective.CONSUMER), + ): + level = BreakingChangeLevel.ERROR + else: + level = BreakingChangeLevel.INFO return BreakingChangeEntry( path=entry.path, change_type=entry.type, - level=evaluation.level, + level=level, + direction=direction, message=evaluation.message, rule_id=evaluation.rule_id, old_value=entry.old_value, diff --git a/datacontract/breaking/rules.py b/datacontract/breaking/rules.py index 5271e0061..08170ef01 100644 --- a/datacontract/breaking/rules.py +++ b/datacontract/breaking/rules.py @@ -2,14 +2,17 @@ from dataclasses import dataclass from datetime import date -from datacontract.model.breaking import BreakingChangeLevel +from datacontract.engines.checks.create_checks import _freshness_value_to_seconds +from datacontract.engines.checks.severity import failure_result +from datacontract.model.breaking import Direction from datacontract.model.changelog import ChangelogEntry, ChangelogType +from datacontract.model.run import ResultEnum @dataclass(frozen=True) class RuleEvaluation: rule_id: str - level: BreakingChangeLevel + direction: Direction message: str @@ -17,94 +20,116 @@ class BreakingChangeRule(ABC): rule_id: str @abstractmethod - def evaluate(self, entry: ChangelogEntry) -> RuleEvaluation | None: - """Return a classification when this rule applies to ``entry``.""" + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: + """Return the direction of ``entry`` when this rule applies to it. + ``before`` and ``after`` are the two normalized contracts the changelog was built from. + """ -class SchemaRemovedRule(BreakingChangeRule): - rule_id = "schema-removed" - def evaluate(self, entry: ChangelogEntry) -> RuleEvaluation | None: +class SchemaAddedOrRemovedRule(BreakingChangeRule): + rule_id = "schema-added-or-removed" + + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: segments = entry.path.split(".") - if entry.type == ChangelogType.removed and len(segments) == 2 and segments[0] == "schema": - return RuleEvaluation(self.rule_id, BreakingChangeLevel.ERROR, f"Removed schema {segments[1]}") + if len(segments) != 2 or segments[0] != "schema": + return None + if entry.type == ChangelogType.added: + return RuleEvaluation(self.rule_id, Direction.TIGHTENS, f"Added schema {segments[1]}") + if entry.type == ChangelogType.removed: + return RuleEvaluation(self.rule_id, Direction.LOOSENS, f"Removed schema {segments[1]}") return None -class FieldRemovedRule(BreakingChangeRule): - rule_id = "field-removed" +class PropertyAddedOrRemovedRule(BreakingChangeRule): + rule_id = "property-added-or-removed" - def evaluate(self, entry: ChangelogEntry) -> RuleEvaluation | None: + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: segments = entry.path.split(".") - if entry.type != ChangelogType.removed or not _is_schema_property_path(segments): + if not _is_schema_property_path(segments): + return None + if entry.type == ChangelogType.added: + return RuleEvaluation(self.rule_id, Direction.TIGHTENS, f"Added property {segments[-1]}") + if entry.type == ChangelogType.removed: + return RuleEvaluation(self.rule_id, Direction.LOOSENS, f"Removed property {segments[-1]}") + return None + + +class PhysicalNameChangedRule(BreakingChangeRule): + rule_id = "physical-name-changed" + + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: + if not entry.path.startswith("schema.") or not entry.path.endswith(".physicalName"): return None - property_name = segments[-1] - return RuleEvaluation(self.rule_id, BreakingChangeLevel.ERROR, f"Removed property {property_name}") + name = entry.path.removesuffix(".physicalName").rsplit(".", 1)[1] + direction = Direction.UNTESTED if (entry.old_value or name) == (entry.new_value or name) else Direction.BOTH + return RuleEvaluation(self.rule_id, direction, _change_message("physical name", entry)) class RequiredChangedRule(BreakingChangeRule): rule_id = "required-changed" - def evaluate(self, entry: ChangelogEntry) -> RuleEvaluation | None: - if not entry.path.endswith(".required"): + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: + if not entry.path.startswith("schema.") or not entry.path.endswith(".required"): return None - old = _parse_bool(entry.old_value) - new = _parse_bool(entry.new_value) - if entry.type == ChangelogType.added and new is True: - level = BreakingChangeLevel.ERROR - elif entry.type == ChangelogType.removed or new is False: - # Dropping the requirement, or writing out the optional default, only loosens the contract. - level = BreakingChangeLevel.INFO - elif entry.type == ChangelogType.updated and old is False and new is True: - level = BreakingChangeLevel.ERROR - elif entry.type in (ChangelogType.added, ChangelogType.updated): - level = BreakingChangeLevel.WARNING - else: - return None - return RuleEvaluation(self.rule_id, level, _change_message("requiredness", entry)) + return RuleEvaluation(self.rule_id, _flag_direction(entry), _change_message("requiredness", entry)) class TypeChangedRule(BreakingChangeRule): rule_id = "type-changed" - def evaluate(self, entry: ChangelogEntry) -> RuleEvaluation | None: - if not entry.path.endswith((".logicalType", ".physicalType")): + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: + # schema..physicalType is the kind of table (table, view), which no check reads. + if ( + not entry.path.startswith("schema.") + or len(entry.path.split(".")) < 5 + or not entry.path.endswith((".logicalType", ".physicalType")) + ): return None if entry.type == ChangelogType.updated: - level = BreakingChangeLevel.ERROR + direction = Direction.BOTH elif entry.type == ChangelogType.added: - level = BreakingChangeLevel.INFO - elif entry.type == ChangelogType.removed: - level = BreakingChangeLevel.WARNING + direction = Direction.TIGHTENS else: - return None - return RuleEvaluation(self.rule_id, level, _change_message("type", entry)) + direction = Direction.LOOSENS + return RuleEvaluation(self.rule_id, direction, _change_message("type", entry)) class UniqueConstraintRule(BreakingChangeRule): rule_id = "unique-constraint-changed" - def evaluate(self, entry: ChangelogEntry) -> RuleEvaluation | None: - if not entry.path.endswith(".unique"): + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: + if not entry.path.startswith("schema.") or not entry.path.endswith((".unique", ".uniqueItems")): return None - old = _parse_bool(entry.old_value) - new = _parse_bool(entry.new_value) - if new is True and old is not True: - level = BreakingChangeLevel.ERROR - elif old is True and new is not True: - level = BreakingChangeLevel.INFO - else: - level = BreakingChangeLevel.WARNING - return RuleEvaluation(self.rule_id, level, _change_message("uniqueness", entry)) + return RuleEvaluation(self.rule_id, _flag_direction(entry), _change_message("uniqueness", entry)) class KeyConstraintRule(BreakingChangeRule): rule_id = "key-constraint-changed" - def evaluate(self, entry: ChangelogEntry) -> RuleEvaluation | None: + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: + if not entry.path.startswith("schema."): + return None + if entry.path.endswith(".primaryKeyPosition"): + # A composite key is unique as a set of columns, whatever their order. + return RuleEvaluation(self.rule_id, Direction.UNTESTED, _change_message("key constraint", entry)) if not entry.path.endswith((".primaryKey", ".primary_key")): return None - return RuleEvaluation(self.rule_id, BreakingChangeLevel.WARNING, _change_message("key constraint", entry)) + direction = _flag_direction(entry) + schema_path = entry.path.split(".properties.", 1)[0] + is_composite = any( + sum( + 1 + for prop in ((_lookup(tree, schema_path) or {}).get("properties") or {}).values() + if prop.get("primaryKey") + ) + > 1 + for tree in (before, after) + ) + if is_composite and direction in (Direction.TIGHTENS, Direction.LOOSENS): + # A column joining or leaving a composite key changes both its not-null and the key's uniqueness. + direction = Direction.BOTH + return RuleEvaluation(self.rule_id, direction, _change_message("key constraint", entry)) class ValidationConstraintRule(BreakingChangeRule): @@ -117,47 +142,128 @@ class ValidationConstraintRule(BreakingChangeRule): ".maximum", ".exclusiveMinimum", ".exclusiveMaximum", + ".minItems", + ".maxItems", ) - def evaluate(self, entry: ChangelogEntry) -> RuleEvaluation | None: - if not entry.path.endswith(self._suffixes): + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: + if not entry.path.startswith("schema.") or not entry.path.endswith(self._suffixes): return None - old = _parse_number(entry.old_value) - new = _parse_number(entry.new_value) - if old is None or new is None: - level = BreakingChangeLevel.WARNING - elif _is_tightening(entry.path, old, new): - level = BreakingChangeLevel.ERROR + if entry.type == ChangelogType.added: + direction = Direction.TIGHTENS + elif entry.type == ChangelogType.removed: + direction = Direction.LOOSENS + elif entry.path.endswith(".pattern"): + direction = Direction.UNDETERMINED else: - level = BreakingChangeLevel.INFO - return RuleEvaluation(self.rule_id, level, _change_message("validation constraint", entry)) - - -class MetadataFallbackRule(BreakingChangeRule): - rule_id = "metadata-or-unknown-change" - - def evaluate(self, entry: ChangelogEntry) -> RuleEvaluation: - return RuleEvaluation(self.rule_id, BreakingChangeLevel.INFO, _change_message("contract", entry)) + direction = _ordered_direction(entry.path, _parse_number(entry.old_value), _parse_number(entry.new_value)) + return RuleEvaluation(self.rule_id, direction, _change_message("validation constraint", entry)) class EnumConstraintRule(BreakingChangeRule): rule_id = "enum-constraint-changed" - def evaluate(self, entry: ChangelogEntry) -> RuleEvaluation | None: + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: if not entry.path.startswith("schema.") or ".properties." not in entry.path: return None if entry.path.endswith(".enum"): - level = BreakingChangeLevel.ERROR if entry.type == ChangelogType.added else BreakingChangeLevel.INFO + direction = Direction.TIGHTENS if entry.type == ChangelogType.added else Direction.LOOSENS elif ( ".enum." in entry.path and entry.path.endswith(".value") and ".customProperties." not in entry.path.rsplit(".enum.", 1)[1] and ".authoritativeDefinitions." not in entry.path.rsplit(".enum.", 1)[1] ): - level = BreakingChangeLevel.INFO if entry.type == ChangelogType.added else BreakingChangeLevel.ERROR + # Stable IDs may change or swap without changing the set of allowed values. + enum_path = entry.path.rsplit(".enum.", 1)[0] + ".enum" + old_values, new_values = ( + {str(item.get("value")) for item in (_lookup(tree, enum_path) or {}).values() if isinstance(item, dict)} + for tree in (before, after) + ) + removes = entry.old_value is not None and entry.old_value not in new_values + adds = entry.new_value is not None and entry.new_value not in old_values + if removes and adds: + direction = Direction.BOTH + elif removes: + direction = Direction.TIGHTENS + elif adds: + direction = Direction.LOOSENS + else: + direction = Direction.UNTESTED + else: + return None + return RuleEvaluation(self.rule_id, direction, _change_message("allowed values", entry)) + + +class RelationshipChangedRule(BreakingChangeRule): + rule_id = "relationship-changed" + + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: + if not entry.path.startswith("schema.") or ".relationships." not in entry.path: + return None + owner, _, rest = entry.path.rpartition(".relationships.") + container = f"{owner}.relationships" + relationships = {**(_lookup(before, container) or {}), **(_lookup(after, container) or {})} + # A relationship key may contain dots (customers.id), so match it against the known keys. + key = max((k for k in relationships if rest == k or rest.startswith(f"{k}.")), key=len, default=None) + if key is None: + return None + field = rest[len(key) + 1 :] + if field == "type": + was_checked = (entry.old_value or "foreignKey") == "foreignKey" + is_checked = (entry.new_value or "foreignKey") == "foreignKey" + if was_checked == is_checked: + direction = Direction.UNTESTED + else: + direction = Direction.TIGHTENS if is_checked else Direction.LOOSENS + elif (relationships[key].get("type") or "foreignKey") != "foreignKey": + direction = Direction.UNTESTED + elif not field: + direction = Direction.TIGHTENS if entry.type == ChangelogType.added else Direction.LOOSENS + elif field.split(".")[0] in ("from", "to"): + direction = Direction.BOTH else: + direction = Direction.UNTESTED + return RuleEvaluation(self.rule_id, direction, _change_message("relationship", entry)) + + +class FreshnessChangedRule(BreakingChangeRule): + rule_id = "freshness-changed" + + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: + segments = entry.path.split(".") + if len(segments) < 2 or segments[0] != "slaProperties" or segments[1] != "freshness": return None - return RuleEvaluation(self.rule_id, level, _change_message("allowed values", entry)) + sla_path = ".".join(segments[:2]) + old_sla, new_sla = _lookup(before, sla_path) or {}, _lookup(after, sla_path) or {} + field = segments[2] if len(segments) > 2 else None + if field is None: + direction = Direction.TIGHTENS if entry.type == ChangelogType.added else Direction.LOOSENS + elif field == "element": + direction = { + ChangelogType.added: Direction.TIGHTENS, + ChangelogType.removed: Direction.LOOSENS, + }.get(entry.type, Direction.BOTH) + elif field in ("value", "unit"): + old_value, new_value = old_sla.get("value"), new_sla.get("value") + if old_value is None and new_value is None: + direction = Direction.UNTESTED + elif old_value is None: + direction = Direction.TIGHTENS + elif new_value is None: + direction = Direction.LOOSENS + else: + old_seconds = _freshness_value_to_seconds(old_value, old_sla.get("unit")) + new_seconds = _freshness_value_to_seconds(new_value, new_sla.get("unit")) + if old_seconds is None or new_seconds is None: + direction = Direction.UNDETERMINED + elif old_seconds == new_seconds: + direction = Direction.UNTESTED + else: + direction = Direction.TIGHTENS if new_seconds < old_seconds else Direction.LOOSENS + else: + direction = Direction.UNTESTED + return RuleEvaluation(self.rule_id, direction, _change_message("freshness", entry)) class QualityConstraintRule(BreakingChangeRule): @@ -169,45 +275,95 @@ class QualityConstraintRule(BreakingChangeRule): ".mustBeGreaterOrEqualTo", ) - def evaluate(self, entry: ChangelogEntry) -> RuleEvaluation | None: + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: if not entry.path.startswith("schema.") or ".quality." not in entry.path: return None rule_path = entry.path.rsplit(".quality.", 1)[1] if rule_path.endswith(".arguments.validValues"): # The diff emits one entry per list element. - level = BreakingChangeLevel.INFO if entry.type == ChangelogType.added else BreakingChangeLevel.ERROR + direction = { + ChangelogType.added: Direction.LOOSENS, + ChangelogType.removed: Direction.TIGHTENS, + }.get(entry.type, Direction.BOTH) elif rule_path.endswith(".arguments.invalidValues"): - level = BreakingChangeLevel.INFO if entry.type == ChangelogType.removed else BreakingChangeLevel.ERROR - elif rule_path.endswith(self._thresholds): - old = _parse_number(entry.old_value) - new = _parse_number(entry.new_value) - if entry.type == ChangelogType.removed: - level = BreakingChangeLevel.INFO - elif old is None or new is None: - level = BreakingChangeLevel.WARNING - elif _is_tightening(entry.path, old, new): - level = BreakingChangeLevel.ERROR + direction = { + ChangelogType.added: Direction.TIGHTENS, + ChangelogType.removed: Direction.LOOSENS, + }.get(entry.type, Direction.BOTH) + elif rule_path.endswith(self._thresholds + (".mustBe", ".mustNotBe", ".mustBeBetween", ".mustNotBeBetween")): + rule_path_in_contract, threshold = entry.path.rsplit(".", 1) + old_rule = _lookup(before, rule_path_in_contract) or {} + new_rule = _lookup(after, rule_path_in_contract) or {} + if entry.type == ChangelogType.added and threshold not in old_rule: + # Replacing one kind of threshold with another cannot be ordered. + direction = ( + Direction.UNDETERMINED if any(k.startswith("must") for k in old_rule) else Direction.TIGHTENS + ) + elif entry.type == ChangelogType.removed and threshold not in new_rule: + direction = Direction.UNDETERMINED if any(k.startswith("must") for k in new_rule) else Direction.LOOSENS + elif rule_path.endswith(self._thresholds): + direction = _ordered_direction( + entry.path, _parse_number(entry.old_value), _parse_number(entry.new_value) + ) + elif rule_path.endswith((".mustBe", ".mustNotBe")): + direction = Direction.UNDETERMINED + else: + # The diff reports the bounds as unordered list elements, so compare the whole ranges. + old_range, new_range = ( + sorted(bounds) if None not in bounds else None + for bounds in ( + [_parse_number(str(v)) for v in value] + if isinstance(value, list) and len(value) == 2 + else [None] + for value in (old_rule.get(threshold), new_rule.get(threshold)) + ) + ) + if old_range is None or new_range is None: + direction = Direction.UNDETERMINED + elif old_range == new_range: + direction = Direction.UNTESTED + else: + narrower = new_range[0] >= old_range[0] and new_range[1] <= old_range[1] + wider = new_range[0] <= old_range[0] and new_range[1] >= old_range[1] + # mustBeBetween names the allowed range, mustNotBeBetween the forbidden one. + if threshold == "mustNotBeBetween": + narrower, wider = wider, narrower + if narrower: + direction = Direction.TIGHTENS + elif wider: + direction = Direction.LOOSENS + else: + direction = Direction.BOTH + elif rule_path.endswith(".severity"): + was_blocking = failure_result(entry.old_value) == ResultEnum.failed + is_blocking = failure_result(entry.new_value) == ResultEnum.failed + if was_blocking == is_blocking: + direction = Direction.UNTESTED else: - level = BreakingChangeLevel.INFO + direction = Direction.TIGHTENS if is_blocking else Direction.LOOSENS elif "." not in rule_path: - # A new rule is a constraint existing data may not satisfy; a removed one is a relaxation. - level = BreakingChangeLevel.ERROR if entry.type == ChangelogType.added else BreakingChangeLevel.INFO - elif rule_path.endswith((".description", ".dimension", ".businessImpact", ".severity")): - level = BreakingChangeLevel.INFO + rule = _lookup(after if entry.type == ChangelogType.added else before, entry.path) + # A rule that can only warn guarantees nothing. + if failure_result(rule.get("severity") if isinstance(rule, dict) else None) != ResultEnum.failed: + direction = Direction.UNTESTED + else: + direction = Direction.TIGHTENS if entry.type == ChangelogType.added else Direction.LOOSENS + elif rule_path.endswith((".description", ".dimension", ".businessImpact")): + direction = Direction.UNTESTED elif ".customProperties." in rule_path or ".authoritativeDefinitions." in rule_path or ".tags" in rule_path: - level = BreakingChangeLevel.INFO + direction = Direction.UNTESTED else: # What the rule checks changed (type, metric, query, other arguments, thresholds we cannot order). - level = BreakingChangeLevel.WARNING + direction = Direction.UNDETERMINED name, _, field = rule_path.partition(".") subject = f"{field} of quality rule '{name}'" if field else "quality rule" - return RuleEvaluation(self.rule_id, level, _change_message(subject, entry)) + return RuleEvaluation(self.rule_id, direction, _change_message(subject, entry)) class VectorShapeRule(BreakingChangeRule): rule_id = "vector-shape-changed" - def evaluate(self, entry: ChangelogEntry) -> RuleEvaluation | None: + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: if not entry.path.startswith("schema.") or not entry.path.endswith( (".logicalTypeOptions.dimensions", ".logicalTypeOptions.elementType") ): @@ -215,19 +371,66 @@ def evaluate(self, entry: ChangelogEntry) -> RuleEvaluation | None: if entry.path.endswith(".elementType"): old = entry.old_value or "float32" new = entry.new_value or "float32" - level = BreakingChangeLevel.INFO if old == new else BreakingChangeLevel.ERROR + direction = Direction.UNTESTED if old == new else Direction.BOTH else: - level = BreakingChangeLevel.WARNING if entry.type == ChangelogType.removed else BreakingChangeLevel.ERROR - return RuleEvaluation(self.rule_id, level, _change_message("vector shape", entry)) + direction = { + ChangelogType.added: Direction.TIGHTENS, + ChangelogType.removed: Direction.LOOSENS, + }.get(entry.type, Direction.BOTH) + return RuleEvaluation(self.rule_id, direction, _change_message("vector shape", entry)) + + +class MetadataFallbackRule(BreakingChangeRule): + rule_id = "metadata-or-unknown-change" + + def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation: + return RuleEvaluation(self.rule_id, Direction.UNTESTED, _change_message("contract", entry)) + + +def _lookup(tree, path: str): + """The value at a dotted changelog path; a key may itself contain dots.""" + if not path: + return tree + if not isinstance(tree, dict): + return None + for key in sorted(tree, key=lambda k: -len(str(k))): + if path == str(key): + return tree[key] + if path.startswith(f"{key}."): + found = _lookup(tree[key], path[len(str(key)) + 1 :]) + if found is not None: + return found + return None def _is_schema_property_path(segments: list[str]) -> bool: return len(segments) >= 3 and segments[0] == "schema" and segments[-2] == "properties" -def _parse_bool(value: str | None) -> bool | None: - if value is None: - return None +def _flag_direction(entry: ChangelogEntry) -> Direction: + """Direction of a boolean constraint, which is off when absent.""" + old = False if entry.old_value is None else _parse_bool(entry.old_value) + new = False if entry.new_value is None else _parse_bool(entry.new_value) + if old is None or new is None: + return Direction.UNDETERMINED + if old == new: + return Direction.UNTESTED + return Direction.TIGHTENS if new else Direction.LOOSENS + + +def _ordered_direction(path: str, old: float | None, new: float | None) -> Direction: + if old is None or new is None: + return Direction.UNDETERMINED + if old == new: + return Direction.UNTESTED + if path.endswith( + (".minLength", ".minimum", ".exclusiveMinimum", ".minItems", ".mustBeGreaterThan", ".mustBeGreaterOrEqualTo") + ): + return Direction.TIGHTENS if new > old else Direction.LOOSENS + return Direction.TIGHTENS if new < old else Direction.LOOSENS + + +def _parse_bool(value: str) -> bool | None: normalized = value.strip().lower() if normalized in {"true", "1"}: return True @@ -249,14 +452,6 @@ def _parse_number(value: str | None) -> float | None: return None -def _is_tightening(path: str, old: float, new: float) -> bool: - if path.endswith((".minLength", ".minimum", ".exclusiveMinimum", ".mustBeGreaterThan", ".mustBeGreaterOrEqualTo")): - return new > old - if path.endswith((".maxLength", ".maximum", ".exclusiveMaximum", ".mustBeLessThan", ".mustBeLessOrEqualTo")): - return new < old - return True - - def _change_message(subject: str, entry: ChangelogEntry) -> str: if entry.type == ChangelogType.added: return f"Added {subject} at {entry.path}" @@ -267,9 +462,12 @@ def _change_message(subject: str, entry: ChangelogEntry) -> str: DEFAULT_RULES: tuple[BreakingChangeRule, ...] = ( QualityConstraintRule(), + RelationshipChangedRule(), + FreshnessChangedRule(), + PhysicalNameChangedRule(), RequiredChangedRule(), - SchemaRemovedRule(), - FieldRemovedRule(), + SchemaAddedOrRemovedRule(), + PropertyAddedOrRemovedRule(), TypeChangedRule(), UniqueConstraintRule(), KeyConstraintRule(), diff --git a/datacontract/command_breaking.py b/datacontract/command_breaking.py index 29dd631dc..0e1fd1a39 100644 --- a/datacontract/command_breaking.py +++ b/datacontract/command_breaking.py @@ -4,6 +4,7 @@ from datacontract.cli import app, console, debug_option, enable_debug_logging, inline_references_option from datacontract.config import cli_config from datacontract.data_contract import DataContract +from datacontract.model.breaking import Perspective from datacontract.output.text_breaking_results import write_text_breaking_results @@ -20,13 +21,17 @@ def breaking( str, typer.Argument(help="The location (url, s3 url, or local path) of the target (after) data contract YAML."), ], + perspective: Annotated[ + Perspective, + typer.Option(help="Grade for consumers relying on v1 (default) or for producer data that passed v1."), + ] = Perspective.CONSUMER, inline_references: inline_references_option = True, debug: debug_option = None, ): """Show compatibility impact between two data contracts.""" enable_debug_logging(debug) result = DataContract(config=cli_config(), data_contract_file=v1, inline_references=inline_references).breaking( - DataContract(config=cli_config(), data_contract_file=v2, inline_references=inline_references) + DataContract(config=cli_config(), data_contract_file=v2, inline_references=inline_references), perspective ) write_text_breaking_results(result, console) if result.is_breaking: diff --git a/datacontract/data_contract.py b/datacontract/data_contract.py index 6b1fc6b3d..08b0dbc74 100644 --- a/datacontract/data_contract.py +++ b/datacontract/data_contract.py @@ -20,7 +20,7 @@ from datacontract.init.init_template import get_init_template from datacontract.integration.entropy_data import publish_test_results_to_entropy_data from datacontract.lint import resolve -from datacontract.model.breaking import BreakingChangeResult +from datacontract.model.breaking import BreakingChangeResult, Perspective from datacontract.model.changelog import ChangelogEntry, ChangelogResult, ChangelogType from datacontract.model.exceptions import ( DataContractException, @@ -340,12 +340,17 @@ def export( def changelog(self, other: "DataContract") -> ChangelogResult: """Generate a changelog between this data contract and another, returning a ChangelogResult.""" + return self._changelog(self.get_data_contract(), other, other.get_data_contract()) + + def _changelog( + self, source: OpenDataContractStandard, other: "DataContract", target: OpenDataContractStandard + ) -> ChangelogResult: from datacontract.changelog.changelog import build_changelog changelog = build_changelog( - self.get_data_contract(), + source, self.get_data_contract_file(), - other.get_data_contract(), + target, other.get_data_contract_file(), ) @@ -370,10 +375,26 @@ def changelog(self, other: "DataContract") -> ChangelogResult: ) return result - def breaking(self, other: "DataContract", detector: BreakingChangeDetector | None = None) -> BreakingChangeResult: - """Classify the changelog between this contract and another for compatibility impact.""" - changelog = self.changelog(other) - return (detector or BreakingChangeDetector()).detect(changelog) + def breaking( + self, + other: "DataContract", + perspective: Perspective = Perspective.CONSUMER, + detector: BreakingChangeDetector | None = None, + ) -> BreakingChangeResult: + """Classify the changelog between this contract and another for compatibility impact. + + The consumer perspective flags changes that break a guarantee v1 gave its consumers; the producer + perspective flags changes that data which passed v1 may fail. + """ + from datacontract.changelog.normalize import normalize + + source, target = self.get_data_contract(), other.get_data_contract() + return (detector or BreakingChangeDetector()).detect( + self._changelog(source, other, target), + Perspective(perspective), + normalize(source.model_dump(exclude_none=True, by_alias=True)), + normalize(target.model_dump(exclude_none=True, by_alias=True)), + ) @classmethod def import_from_source( diff --git a/datacontract/model/breaking.py b/datacontract/model/breaking.py index 157f1f1b6..9cf7ac712 100644 --- a/datacontract/model/breaking.py +++ b/datacontract/model/breaking.py @@ -11,10 +11,29 @@ class BreakingChangeLevel(str, Enum): ERROR = "error" +class Perspective(str, Enum): + """Who a change breaks: consumers relying on v1's guarantees, or the producer whose data passed v1.""" + + CONSUMER = "consumer" + PRODUCER = "producer" + + +class Direction(str, Enum): + """How a change moves the set of data the contract permits.""" + + TIGHTENS = "tightens" + LOOSENS = "loosens" + BOTH = "both" + UNDETERMINED = "undetermined" + UNTESTED = "untested" + + class BreakingChangeEntry(BaseModel): path: str change_type: ChangelogType level: BreakingChangeLevel + # None inside an added or removed element, which carries the grade, and on summary entries. + direction: Direction | None = None message: str rule_id: str old_value: str | None = None @@ -24,6 +43,7 @@ class BreakingChangeEntry(BaseModel): class BreakingChangeResult(BaseModel): v1: str v2: str + perspective: Perspective summary: list[BreakingChangeEntry] = Field(default_factory=list) entries: list[BreakingChangeEntry] = Field(default_factory=list) diff --git a/datacontract/output/text_breaking_results.py b/datacontract/output/text_breaking_results.py index 2b396d52d..226acc22e 100644 --- a/datacontract/output/text_breaking_results.py +++ b/datacontract/output/text_breaking_results.py @@ -22,6 +22,7 @@ def write_text_breaking_results(result: BreakingChangeResult, console: Console): if not result.entries: console.print("No changes.") return + console.print(f"Perspective: {result.perspective.value}") _print_summary(result, console) _print_table(result, console) @@ -56,6 +57,7 @@ def _print_table(result: BreakingChangeResult, console: Console): table = Table(box=box.ROUNDED) table.add_column("Severity", no_wrap=True) table.add_column("Change", no_wrap=True) + table.add_column("Direction", no_wrap=True) table.add_column("Path", no_wrap=True) table.add_column("Old Value", max_width=_VAL_W, no_wrap=True) table.add_column("New Value", max_width=_VAL_W, no_wrap=True) @@ -64,6 +66,7 @@ def _print_table(result: BreakingChangeResult, console: Console): table.add_row( _severity_markup(entry.level), entry.change_type.value.capitalize(), + entry.direction.value if entry.direction else "", Text(entry.path), Text(_wrap(entry.old_value or "", _VAL_W)), Text(_wrap(entry.new_value or "", _VAL_W)), diff --git a/docs/docs/api.md b/docs/docs/api.md index 528e2d81d..5d62eca6f 100644 --- a/docs/docs/api.md +++ b/docs/docs/api.md @@ -81,7 +81,7 @@ curl -X POST "http://localhost:4242/changelog" \ }' ``` -`POST /breaking` grades those same changes for compatibility impact. It adds a `level` (`info`, `warning` or `error`) and a `rule_id` to every entry, plus a top-level `is_breaking` flag that is `true` when any entry is an `error`: +`POST /breaking` grades those same changes for compatibility impact. It adds a `direction` (`tightens`, `loosens`, `both`, `undetermined` or `untested`), a `level` (`info`, `warning` or `error`) and a `rule_id` to every entry, plus the `perspective` it graded from and a top-level `is_breaking` flag that is `true` when any entry is an `error`. Pass `?perspective=producer` to grade whether data that passed v1 may fail v2 instead of whether v2 still keeps v1's guarantees to consumers: ```bash curl -X POST "http://localhost:4242/breaking" \ @@ -100,12 +100,14 @@ curl -X POST "http://localhost:4242/breaking" \ "path": "schema.orders.properties.order_id.logicalTypeOptions.pattern", "change_type": "updated", "level": "warning", + "direction": "undetermined", "message": "Changed validation constraint at schema.orders.properties.order_id.logicalTypeOptions.pattern from '^ORD-[0-9]+$' to '^ORD-[0-9]{4}$'", "rule_id": "validation-constraint-changed", "old_value": "^ORD-[0-9]+$", "new_value": "^ORD-[0-9]{4}$" } ], + "perspective": "consumer", "is_breaking": false } ``` diff --git a/docs/docs/compare-contract-versions.md b/docs/docs/compare-contract-versions.md index 9aeb94260..392b8b914 100644 --- a/docs/docs/compare-contract-versions.md +++ b/docs/docs/compare-contract-versions.md @@ -1,12 +1,12 @@ --- sidebar_position: 8 title: "Compare contract versions" -description: "Describe differences and classify backward-compatibility impact between two ODCS data contracts." +description: "Describe differences and classify compatibility impact between two ODCS data contracts." --- # Compare contract versions -Use `datacontract changelog` to describe the differences between two versions of a contract, or use `datacontract breaking` to classify their backward-compatibility impact. +Use `datacontract changelog` to describe the differences between two versions of a contract, or use `datacontract breaking` to classify their compatibility impact. ## Changelog @@ -20,7 +20,7 @@ See the generated [changelog command reference](./commands/changelog.md) for all ## Breaking changes -Use `datacontract breaking` when a contract change must be checked for backward compatibility. The command compares the source contract (`v1`) with the target contract (`v2`) and preserves the detailed changelog while adding a severity classification. +Use `datacontract breaking` when a contract change must be checked for compatibility. The command compares the source contract (`v1`) with the target contract (`v2`) and preserves the detailed changelog while adding a severity classification. ```bash datacontract breaking v1.odcs.yaml v2.odcs.yaml @@ -28,17 +28,48 @@ datacontract breaking v1.odcs.yaml v2.odcs.yaml See the generated [breaking command reference](./commands/breaking.md) for all options. +### Perspectives + +Whether a change breaks depends on whose expectations it must keep. `--perspective` selects one of two: + +- **`consumer`** (default) - a change is breaking when v2 permits data that v1 ruled out, so a guarantee consumers relied on no longer holds. Removing a property and loosening a constraint break; adding a property and tightening a constraint do not. +- **`producer`** - a change is breaking when v2 rules out data that v1 permitted, so data that passed [`datacontract test`](./commands/test.md) against v1 may fail against v2. Adding a property and tightening a constraint break; removing a property and loosening a constraint do not. + +```bash +datacontract breaking --perspective producer v1.odcs.yaml v2.odcs.yaml +``` + +### Directions + +Every change has a direction, independent of the perspective, which the perspective turns into a severity: + +| Direction | Meaning | `consumer` | `producer` | +|---|---|---|---| +| `tightens` | v2 rules out more data | INFO | ERROR | +| `loosens` | v2 permits more data | ERROR | INFO | +| `both` | v2 rules out some data v1 permitted and permits some v1 ruled out | ERROR | ERROR | +| `undetermined` | depends on content the CLI cannot compare | WARNING | WARNING | +| `untested` | changes nothing `datacontract test` checks | INFO | INFO | + +- **Tightens** - adding a schema or property, a type where there was none, `required`, `unique`, `uniqueItems`, a primary key, an enum, a foreign key relationship, a blocking quality rule, or a `freshness` service level; raising a minimum or lowering a maximum (`minLength`, `maxLength`, `minimum`, `maximum`, `minItems`, `maxItems`, quality thresholds); narrowing `mustBeBetween` or widening `mustNotBeBetween`; removing an enum value or a `validValues` value; adding an `invalidValues` value; shortening a `freshness` duration; making a quality rule blocking. +- **Loosens** - the reverse of each of these. +- **Both** - changing a `logicalType` or `physicalType`, a `physicalName`, a relationship's target, the column of a `freshness` service level, a vector's `dimensions` or `elementType`, or a column joining or leaving a composite primary key. +- **Undetermined** - changing a `pattern`, a quality rule's query, metric or implementation, or a value the CLI cannot parse. +- **Untested** - descriptions, tags, servers, team, and other metadata; quality rules whose `severity` only warns (`info`, `warning`, `warn`, `low`, `minor`, `trivial`); service levels other than `freshness`, including `retention`. + +A change inside an added or removed element has no direction of its own: the element carries the grade. + ### Severity levels -- **ERROR** - a backward-incompatible change, i.e. testing the contract against existing data can fail. The command exits with status `1`. -- **WARNING** - a potentially incompatible change that requires review. The command exits with status `0`. -- **INFO** - a metadata change, a relaxed constraint, or an unclassified change. The command exits with status `0`. +- **ERROR** - a breaking change from the selected perspective. The command exits with status `1`. +- **WARNING** - a change that may break and requires review. The command exits with status `0`. +- **INFO** - a change that does not break from the selected perspective. The command exits with status `0`. The result is breaking only when at least one detailed entry has severity `ERROR`. ### API -The same result is available from `POST /breaking`, using the same JSON request shape as `POST /changelog`: +The same result is available from `POST /breaking`, using the same JSON request shape as `POST /changelog` and an optional `perspective` query parameter: ```json { @@ -47,4 +78,4 @@ The same result is available from `POST /breaking`, using the same JSON request } ``` -The response includes `summary`, `entries`, and an `is_breaking` boolean. The endpoint returns HTTP `200` for a valid comparison even when `is_breaking` is `true`; invalid YAML or contracts return HTTP `422`. \ No newline at end of file +The response includes `summary`, `entries`, the `perspective` it was graded from, and an `is_breaking` boolean. The endpoint returns HTTP `200` for a valid comparison even when `is_breaking` is `true`; invalid YAML or contracts return HTTP `422`. \ No newline at end of file diff --git a/docs/docs/python-library.md b/docs/docs/python-library.md index 936d0fd4c..25138afd3 100644 --- a/docs/docs/python-library.md +++ b/docs/docs/python-library.md @@ -135,16 +135,16 @@ result = v1.changelog(v2) print(result) ``` -`breaking()` grades those same changes for compatibility impact. Every entry carries a `level` (`info`, `warning` or `error`), and `is_breaking` is `True` when any entry is an error: +`breaking()` grades those same changes for compatibility impact. Every entry carries a `direction` (`tightens`, `loosens`, `both`, `undetermined` or `untested`) and a `level` (`info`, `warning` or `error`), and `is_breaking` is `True` when any entry is an error. By default it grades from the consumer's perspective; pass `perspective="producer"` to grade whether data that passed v1 may fail v2: ```python -result = v1.breaking(v2) +result = v1.breaking(v2, perspective="producer") for entry in result.entries: print(entry.level.value, entry.path, entry.message) ``` -See [Compare contract versions](./compare-contract-versions.md) for the severity levels and the rules behind them. +See [Compare contract versions](./compare-contract-versions.md) for the perspectives, directions and severity levels. ## Spark DataFrames and Databricks diff --git a/tests/fixtures/changelog/golden_breaking_text.txt b/tests/fixtures/changelog/golden_breaking_text.txt index 8ab02e722..eb71c8c21 100644 --- a/tests/fixtures/changelog/golden_breaking_text.txt +++ b/tests/fixtures/changelog/golden_breaking_text.txt @@ -1,5 +1,6 @@ +Perspective: consumer Summary -[ 3 Error ] [ 5 Info ] +[ 2 Error ] [ 6 Info ] ╭──────────┬─────────┬───────────────────────────────────────╮ │ Severity │ Change │ Field │ ├──────────┼─────────┼───────────────────────────────────────┤ @@ -8,105 +9,97 @@ Summary │ ERROR │ Updated │ schema.orders.properties.order_date │ │ INFO │ Updated │ schema.orders.properties.order_id │ │ INFO │ Added │ schema.orders.properties.region │ -│ ERROR │ Updated │ schema.orders.properties.total_amount │ +│ INFO │ Updated │ schema.orders.properties.total_amount │ │ INFO │ Updated │ slaProperties.availability │ │ INFO │ Updated │ version │ ╰──────────┴─────────┴───────────────────────────────────────╯ Details -╭──────────┬─────────┬──────────────────────────────────────────────────────────┬────────────────────────────────┬───────────────────────────────┬────────────────────────────────╮ -│ Severity │ Change │ Path │ Old Value │ New Value │ Message │ -├──────────┼─────────┼──────────────────────────────────────────────────────────┼────────────────────────────────┼───────────────────────────────┼────────────────────────────────┤ -│ INFO │ Added │ schema.customers │ │ │ Added contract at │ -│ │ │ │ │ │ schema.customers │ -│ INFO │ Added │ schema.customers.physicalName │ │ customers_tbl │ Added contract at │ -│ │ │ │ │ │ schema.customers.physicalName │ -│ INFO │ Added │ schema.customers.properties │ │ │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties │ -│ INFO │ Added │ schema.customers.properties.country │ │ │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.country.logicalType │ │ string │ Added type at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.country.partitionKeyPosition │ │ 1 │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.country.partitioned │ │ True │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.country.required │ │ False │ Added requiredness at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.created_at │ │ │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.created_at.description │ │ Record creation timestamp │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.created_at.logicalType │ │ timestamp │ Added type at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.created_at.required │ │ True │ Added requiredness at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.customer_id │ │ │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.customer_id.description │ │ Unique order ID │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.customer_id.logicalType │ │ string │ Added type at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.customer_id.primaryKey │ │ True │ Added key constraint at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.customer_id.required │ │ True │ Added requiredness at │ -│ │ │ │ │ │ schema.customers.properties.c… │ -│ INFO │ Added │ schema.customers.properties.date_of_birth │ │ │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.d… │ -│ INFO │ Added │ schema.customers.properties.date_of_birth.classification │ │ restricted │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.d… │ -│ INFO │ Added │ schema.customers.properties.date_of_birth.logicalType │ │ date │ Added type at │ -│ │ │ │ │ │ schema.customers.properties.d… │ -│ INFO │ Added │ schema.customers.properties.date_of_birth.required │ │ False │ Added requiredness at │ -│ │ │ │ │ │ schema.customers.properties.d… │ -│ INFO │ Added │ schema.customers.properties.email │ │ │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.e… │ -│ INFO │ Added │ schema.customers.properties.email.classification │ │ confidential │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.e… │ -│ INFO │ Added │ schema.customers.properties.email.encryptedName │ │ email_encrypt │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.e… │ -│ INFO │ Added │ schema.customers.properties.email.logicalType │ │ string │ Added type at │ -│ │ │ │ │ │ schema.customers.properties.e… │ -│ INFO │ Added │ schema.customers.properties.email.required │ │ True │ Added requiredness at │ -│ │ │ │ │ │ schema.customers.properties.e… │ -│ INFO │ Added │ schema.customers.properties.full_name │ │ │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.f… │ -│ INFO │ Added │ schema.customers.properties.full_name.businessName │ │ Customer Full Name │ Added contract at │ -│ │ │ │ │ │ schema.customers.properties.f… │ -│ INFO │ Added │ schema.customers.properties.full_name.logicalType │ │ string │ Added type at │ -│ │ │ │ │ │ schema.customers.properties.f… │ -│ INFO │ Added │ schema.customers.properties.full_name.required │ │ True │ Added requiredness at │ -│ │ │ │ │ │ schema.customers.properties.f… │ -│ ERROR │ Removed │ schema.orders.properties.customer_id │ │ │ Removed property customer_id │ -│ INFO │ Removed │ schema.orders.properties.customer_id.logicalType │ string │ │ Removed type at │ -│ │ │ │ │ │ schema.orders.properties.cust… │ -│ INFO │ Removed │ schema.orders.properties.customer_id.required │ True │ │ Removed requiredness at │ -│ │ │ │ │ │ schema.orders.properties.cust… │ -│ ERROR │ Updated │ schema.orders.properties.order_date.logicalType │ string │ date │ Changed type at │ -│ │ │ │ │ │ schema.orders.properties.orde… │ -│ │ │ │ │ │ from 'string' to 'date' │ -│ INFO │ Updated │ schema.orders.properties.order_id.description │ Unique order ID and a rather │ Unique order ID and another │ Changed contract at │ -│ │ │ │ lenghty description that │ rather lenghty description │ schema.orders.properties.orde… │ -│ │ │ │ should be wrapped in the table │ that should be wrapped in the │ from 'Unique order ID and a │ -│ │ │ │ │ table │ rather lenghty description │ -│ │ │ │ │ │ that should be wrapped in the │ -│ │ │ │ │ │ table' to 'Unique order ID and │ -│ │ │ │ │ │ another rather lenghty │ -│ │ │ │ │ │ description that should be │ -│ │ │ │ │ │ wrapped in the table' │ -│ INFO │ Added │ schema.orders.properties.region │ │ │ Added contract at │ -│ │ │ │ │ │ schema.orders.properties.regi… │ -│ INFO │ Added │ schema.orders.properties.region.logicalType │ │ string │ Added type at │ -│ │ │ │ │ │ schema.orders.properties.regi… │ -│ INFO │ Added │ schema.orders.properties.region.required │ │ False │ Added requiredness at │ -│ │ │ │ │ │ schema.orders.properties.regi… │ -│ ERROR │ Updated │ schema.orders.properties.total_amount.required │ False │ True │ Changed requiredness at │ -│ │ │ │ │ │ schema.orders.properties.tota… │ -│ │ │ │ │ │ from 'False' to 'True' │ -│ INFO │ Updated │ slaProperties.availability.value │ 99.9% │ 99.5% │ Changed contract at │ -│ │ │ │ │ │ slaProperties.availability.va… │ -│ │ │ │ │ │ from '99.9%' to '99.5%' │ -│ INFO │ Updated │ version │ 1.0.0 │ 2.0.0 │ Changed contract at version │ -│ │ │ │ │ │ from '1.0.0' to '2.0.0' │ -╰──────────┴─────────┴──────────────────────────────────────────────────────────┴────────────────────────────────┴───────────────────────────────┴────────────────────────────────╯ +╭──────────┬─────────┬───────────┬──────────────────────────────────────────────────────────┬────────────────────────────────┬───────────────────────────────┬────────────────────────────────╮ +│ Severity │ Change │ Direction │ Path │ Old Value │ New Value │ Message │ +├──────────┼─────────┼───────────┼──────────────────────────────────────────────────────────┼────────────────────────────────┼───────────────────────────────┼────────────────────────────────┤ +│ INFO │ Added │ tightens │ schema.customers │ │ │ Added schema customers │ +│ INFO │ Added │ │ schema.customers.physicalName │ │ customers_tbl │ Added physical name at │ +│ │ │ │ │ │ │ schema.customers.physicalName │ +│ INFO │ Added │ │ schema.customers.properties │ │ │ Added contract at │ +│ │ │ │ │ │ │ schema.customers.properties │ +│ INFO │ Added │ │ schema.customers.properties.country │ │ │ Added property country │ +│ INFO │ Added │ │ schema.customers.properties.country.logicalType │ │ string │ Added type at │ +│ │ │ │ │ │ │ schema.customers.properties.c… │ +│ INFO │ Added │ │ schema.customers.properties.country.partitionKeyPosition │ │ 1 │ Added contract at │ +│ │ │ │ │ │ │ schema.customers.properties.c… │ +│ INFO │ Added │ │ schema.customers.properties.country.partitioned │ │ True │ Added contract at │ +│ │ │ │ │ │ │ schema.customers.properties.c… │ +│ INFO │ Added │ │ schema.customers.properties.country.required │ │ False │ Added requiredness at │ +│ │ │ │ │ │ │ schema.customers.properties.c… │ +│ INFO │ Added │ │ schema.customers.properties.created_at │ │ │ Added property created_at │ +│ INFO │ Added │ │ schema.customers.properties.created_at.description │ │ Record creation timestamp │ Added contract at │ +│ │ │ │ │ │ │ schema.customers.properties.c… │ +│ INFO │ Added │ │ schema.customers.properties.created_at.logicalType │ │ timestamp │ Added type at │ +│ │ │ │ │ │ │ schema.customers.properties.c… │ +│ INFO │ Added │ │ schema.customers.properties.created_at.required │ │ True │ Added requiredness at │ +│ │ │ │ │ │ │ schema.customers.properties.c… │ +│ INFO │ Added │ │ schema.customers.properties.customer_id │ │ │ Added property customer_id │ +│ INFO │ Added │ │ schema.customers.properties.customer_id.description │ │ Unique order ID │ Added contract at │ +│ │ │ │ │ │ │ schema.customers.properties.c… │ +│ INFO │ Added │ │ schema.customers.properties.customer_id.logicalType │ │ string │ Added type at │ +│ │ │ │ │ │ │ schema.customers.properties.c… │ +│ INFO │ Added │ │ schema.customers.properties.customer_id.primaryKey │ │ True │ Added key constraint at │ +│ │ │ │ │ │ │ schema.customers.properties.c… │ +│ INFO │ Added │ │ schema.customers.properties.customer_id.required │ │ True │ Added requiredness at │ +│ │ │ │ │ │ │ schema.customers.properties.c… │ +│ INFO │ Added │ │ schema.customers.properties.date_of_birth │ │ │ Added property date_of_birth │ +│ INFO │ Added │ │ schema.customers.properties.date_of_birth.classification │ │ restricted │ Added contract at │ +│ │ │ │ │ │ │ schema.customers.properties.d… │ +│ INFO │ Added │ │ schema.customers.properties.date_of_birth.logicalType │ │ date │ Added type at │ +│ │ │ │ │ │ │ schema.customers.properties.d… │ +│ INFO │ Added │ │ schema.customers.properties.date_of_birth.required │ │ False │ Added requiredness at │ +│ │ │ │ │ │ │ schema.customers.properties.d… │ +│ INFO │ Added │ │ schema.customers.properties.email │ │ │ Added property email │ +│ INFO │ Added │ │ schema.customers.properties.email.classification │ │ confidential │ Added contract at │ +│ │ │ │ │ │ │ schema.customers.properties.e… │ +│ INFO │ Added │ │ schema.customers.properties.email.encryptedName │ │ email_encrypt │ Added contract at │ +│ │ │ │ │ │ │ schema.customers.properties.e… │ +│ INFO │ Added │ │ schema.customers.properties.email.logicalType │ │ string │ Added type at │ +│ │ │ │ │ │ │ schema.customers.properties.e… │ +│ INFO │ Added │ │ schema.customers.properties.email.required │ │ True │ Added requiredness at │ +│ │ │ │ │ │ │ schema.customers.properties.e… │ +│ INFO │ Added │ │ schema.customers.properties.full_name │ │ │ Added property full_name │ +│ INFO │ Added │ │ schema.customers.properties.full_name.businessName │ │ Customer Full Name │ Added contract at │ +│ │ │ │ │ │ │ schema.customers.properties.f… │ +│ INFO │ Added │ │ schema.customers.properties.full_name.logicalType │ │ string │ Added type at │ +│ │ │ │ │ │ │ schema.customers.properties.f… │ +│ INFO │ Added │ │ schema.customers.properties.full_name.required │ │ True │ Added requiredness at │ +│ │ │ │ │ │ │ schema.customers.properties.f… │ +│ ERROR │ Removed │ loosens │ schema.orders.properties.customer_id │ │ │ Removed property customer_id │ +│ INFO │ Removed │ │ schema.orders.properties.customer_id.logicalType │ string │ │ Removed type at │ +│ │ │ │ │ │ │ schema.orders.properties.cust… │ +│ INFO │ Removed │ │ schema.orders.properties.customer_id.required │ True │ │ Removed requiredness at │ +│ │ │ │ │ │ │ schema.orders.properties.cust… │ +│ ERROR │ Updated │ both │ schema.orders.properties.order_date.logicalType │ string │ date │ Changed type at │ +│ │ │ │ │ │ │ schema.orders.properties.orde… │ +│ │ │ │ │ │ │ from 'string' to 'date' │ +│ INFO │ Updated │ untested │ schema.orders.properties.order_id.description │ Unique order ID and a rather │ Unique order ID and another │ Changed contract at │ +│ │ │ │ │ lenghty description that │ rather lenghty description │ schema.orders.properties.orde… │ +│ │ │ │ │ should be wrapped in the table │ that should be wrapped in the │ from 'Unique order ID and a │ +│ │ │ │ │ │ table │ rather lenghty description │ +│ │ │ │ │ │ │ that should be wrapped in the │ +│ │ │ │ │ │ │ table' to 'Unique order ID and │ +│ │ │ │ │ │ │ another rather lenghty │ +│ │ │ │ │ │ │ description that should be │ +│ │ │ │ │ │ │ wrapped in the table' │ +│ INFO │ Added │ tightens │ schema.orders.properties.region │ │ │ Added property region │ +│ INFO │ Added │ │ schema.orders.properties.region.logicalType │ │ string │ Added type at │ +│ │ │ │ │ │ │ schema.orders.properties.regi… │ +│ INFO │ Added │ │ schema.orders.properties.region.required │ │ False │ Added requiredness at │ +│ │ │ │ │ │ │ schema.orders.properties.regi… │ +│ INFO │ Updated │ tightens │ schema.orders.properties.total_amount.required │ False │ True │ Changed requiredness at │ +│ │ │ │ │ │ │ schema.orders.properties.tota… │ +│ │ │ │ │ │ │ from 'False' to 'True' │ +│ INFO │ Updated │ untested │ slaProperties.availability.value │ 99.9% │ 99.5% │ Changed contract at │ +│ │ │ │ │ │ │ slaProperties.availability.va… │ +│ │ │ │ │ │ │ from '99.9%' to '99.5%' │ +│ INFO │ Updated │ untested │ version │ 1.0.0 │ 2.0.0 │ Changed contract at version │ +│ │ │ │ │ │ │ from '1.0.0' to '2.0.0' │ +╰──────────┴─────────┴───────────┴──────────────────────────────────────────────────────────┴────────────────────────────────┴───────────────────────────────┴────────────────────────────────╯ diff --git a/tests/test_api.py b/tests/test_api.py index 1262473db..360a2fc59 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -93,12 +93,25 @@ def test_breaking(): assert "v1" not in data assert "v2" not in data assert data["is_breaking"] is True + assert data["perspective"] == "consumer" assert "summary" in data assert "entries" in data assert data["entries"][0]["level"] in ("info", "warning", "error") assert "message" in data["entries"][0] +def test_breaking_from_the_producer_perspective(): + with open("fixtures/changelog/breaking/warning_only_v1.yaml", "r") as f: + v1 = f.read() + with open("fixtures/changelog/breaking/warning_only_v2.yaml", "r") as f: + v2 = f.read() + assert client.post(url="/breaking", json={"v1": v1, "v2": v2}).json()["is_breaking"] is False + response = client.post(url="/breaking", params={"perspective": "producer"}, json={"v1": v1, "v2": v2}) + assert response.status_code == 200 + assert response.json()["perspective"] == "producer" + assert response.json()["is_breaking"] is True + + def test_breaking_invalid_yaml(): response = client.post(url="/breaking", json={"v1": "invalid: yaml: [", "v2": "valid: yaml"}) assert response.status_code == 422 diff --git a/tests/test_breaking.py b/tests/test_breaking.py index de3b3971c..be86ee8a3 100644 --- a/tests/test_breaking.py +++ b/tests/test_breaking.py @@ -2,26 +2,28 @@ import sys from pathlib import Path +import pytest +import yaml from rich.console import Console from typer.testing import CliRunner from datacontract.breaking.detector import BreakingChangeDetector from datacontract.breaking.rules import ( BreakingChangeRule, - FieldRemovedRule, KeyConstraintRule, MetadataFallbackRule, + PropertyAddedOrRemovedRule, QualityConstraintRule, RequiredChangedRule, RuleEvaluation, - SchemaRemovedRule, + SchemaAddedOrRemovedRule, TypeChangedRule, UniqueConstraintRule, ValidationConstraintRule, ) from datacontract.cli import app from datacontract.data_contract import DataContract -from datacontract.model.breaking import BreakingChangeLevel +from datacontract.model.breaking import BreakingChangeLevel, Direction, Perspective from datacontract.model.changelog import ChangelogEntry, ChangelogResult, ChangelogType from datacontract.output.text_breaking_results import write_text_breaking_results @@ -39,358 +41,372 @@ def _entry(path, change_type, old_value=None, new_value=None): return ChangelogEntry(path=path, type=change_type, old_value=old_value, new_value=new_value) -def test_required_change_to_true_is_error(): - result = RequiredChangedRule().evaluate( - _entry("schema.orders.properties.customer_id.required", ChangelogType.updated, "False", "True") - ) - assert result is not None - assert result.level == BreakingChangeLevel.ERROR - - -def test_required_change_to_false_is_info(): - result = RequiredChangedRule().evaluate( - _entry("schema.orders.properties.customer_id.required", ChangelogType.updated, "true", "false") - ) - assert result is not None - assert result.level == BreakingChangeLevel.INFO - - -def test_adding_required_false_is_info(): - result = RequiredChangedRule().evaluate( - _entry("schema.orders.properties.region.required", ChangelogType.added, None, "False") - ) - assert result is not None - assert result.level == BreakingChangeLevel.INFO - - -def test_adding_a_type_is_info(): - result = TypeChangedRule().evaluate( - _entry("schema.orders.properties.region.logicalType", ChangelogType.added, None, "string") - ) - assert result is not None - assert result.level == BreakingChangeLevel.INFO - - -def test_removing_a_type_is_warning(): - result = TypeChangedRule().evaluate( - _entry("schema.orders.properties.region.logicalType", ChangelogType.removed, "string") - ) - assert result is not None - assert result.level == BreakingChangeLevel.WARNING - - -def test_type_change_is_error(): - result = TypeChangedRule().evaluate( - _entry("schema.orders.properties.order_id.logicalType", ChangelogType.updated, "string", "integer") - ) - assert result is not None - assert result.level == BreakingChangeLevel.ERROR +def _direction(rule, entry): + return rule.evaluate(entry, {}, {}).direction -def test_removing_schema_is_breaking(): - result = SchemaRemovedRule().evaluate(_entry("schema.orders", ChangelogType.removed)) - assert result is not None - assert result.level == BreakingChangeLevel.ERROR - assert result.message == "Removed schema orders" +def _detect(*entries, perspective=Perspective.CONSUMER, summary=()): + changelog = ChangelogResult(v1="v1", v2="v2", summary=list(summary), entries=list(entries)) + return BreakingChangeDetector().detect(changelog, perspective, {}, {}) -def test_removing_property_metadata_is_not_property_removal(): - result = FieldRemovedRule().evaluate( - _entry("schema.orders.properties.customer_id.description", ChangelogType.removed, "old description") - ) - assert result is None +@pytest.mark.parametrize( + "direction,consumer,producer", + [ + (Direction.TIGHTENS, BreakingChangeLevel.INFO, BreakingChangeLevel.ERROR), + (Direction.LOOSENS, BreakingChangeLevel.ERROR, BreakingChangeLevel.INFO), + (Direction.BOTH, BreakingChangeLevel.ERROR, BreakingChangeLevel.ERROR), + (Direction.UNDETERMINED, BreakingChangeLevel.WARNING, BreakingChangeLevel.WARNING), + (Direction.UNTESTED, BreakingChangeLevel.INFO, BreakingChangeLevel.INFO), + ], +) +def test_perspective_grades_direction(direction, consumer, producer): + class FixedRule(BreakingChangeRule): + rule_id = "fixed" + + def evaluate(self, entry, before, after): + return RuleEvaluation(self.rule_id, direction, "fixed") + + changelog = ChangelogResult(v1="v1", v2="v2", entries=[_entry("field", ChangelogType.updated)]) + detector = BreakingChangeDetector((FixedRule(),)) + assert detector.detect(changelog, Perspective.CONSUMER, {}, {}).entries[0].level == consumer + assert detector.detect(changelog, Perspective.PRODUCER, {}, {}).entries[0].level == producer + + +@pytest.mark.parametrize( + "change_type,old,new,direction", + [ + (ChangelogType.updated, "False", "True", Direction.TIGHTENS), + (ChangelogType.updated, "true", "false", Direction.LOOSENS), + (ChangelogType.added, None, "True", Direction.TIGHTENS), + (ChangelogType.removed, "True", None, Direction.LOOSENS), + (ChangelogType.added, None, "False", Direction.UNTESTED), + (ChangelogType.updated, "True", "maybe", Direction.UNDETERMINED), + ], +) +def test_required_direction(change_type, old, new, direction): + entry = _entry("schema.orders.properties.customer_id.required", change_type, old, new) + assert _direction(RequiredChangedRule(), entry) == direction + + +@pytest.mark.parametrize( + "change_type,direction", + [ + (ChangelogType.added, Direction.TIGHTENS), + (ChangelogType.removed, Direction.LOOSENS), + (ChangelogType.updated, Direction.BOTH), + ], +) +def test_type_direction(change_type, direction): + entry = _entry("schema.orders.properties.region.logicalType", change_type, "string", "integer") + assert _direction(TypeChangedRule(), entry) == direction -def test_removing_property_is_breaking(): - result = FieldRemovedRule().evaluate( - _entry("schema.orders.properties.customer_id", ChangelogType.removed, "string") - ) - assert result is not None - assert result.level == BreakingChangeLevel.ERROR - assert result.message == "Removed property customer_id" +def test_schema_physical_type_is_untested(): + entry = _entry("schema.orders.physicalType", ChangelogType.updated, "table", "view") + assert TypeChangedRule().evaluate(entry, {}, {}) is None -def test_removing_nested_property_is_breaking(): - result = FieldRemovedRule().evaluate( - _entry("schema.orders.properties.customer.properties.email", ChangelogType.removed, "string") - ) - assert result is not None - assert result.level == BreakingChangeLevel.ERROR - assert result.message == "Removed property email" +def test_schema_added_or_removed(): + assert _direction(SchemaAddedOrRemovedRule(), _entry("schema.orders", ChangelogType.added)) == Direction.TIGHTENS + removed = SchemaAddedOrRemovedRule().evaluate(_entry("schema.orders", ChangelogType.removed), {}, {}) + assert removed.direction == Direction.LOOSENS + assert removed.message == "Removed schema orders" -def test_removing_nested_property_metadata_is_not_property_removal(): - result = FieldRemovedRule().evaluate( - _entry("schema.orders.properties.customer.properties.email.description", ChangelogType.removed, "old") - ) - assert result is None +@pytest.mark.parametrize( + "path", + ["schema.orders.properties.customer_id", "schema.orders.properties.customer.properties.email"], +) +def test_property_added_or_removed(path): + assert _direction(PropertyAddedOrRemovedRule(), _entry(path, ChangelogType.added)) == Direction.TIGHTENS + removed = PropertyAddedOrRemovedRule().evaluate(_entry(path, ChangelogType.removed, "string"), {}, {}) + assert removed.direction == Direction.LOOSENS + assert removed.message == f"Removed property {path.rsplit('.', 1)[1]}" -def test_tightening_uniqueness_is_breaking(): - result = UniqueConstraintRule().evaluate( - _entry("schema.orders.properties.order_id.unique", ChangelogType.updated, "False", "True") - ) - assert result is not None - assert result.level == BreakingChangeLevel.ERROR +def test_property_metadata_is_not_a_property(): + entry = _entry("schema.orders.properties.customer.properties.email.description", ChangelogType.removed, "old") + assert PropertyAddedOrRemovedRule().evaluate(entry, {}, {}) is None -def test_changing_primary_key_is_warning(): - result = KeyConstraintRule().evaluate( - _entry("schema.orders.properties.order_id.primaryKey", ChangelogType.updated, "True", "False") - ) - assert result is not None - assert result.level == BreakingChangeLevel.WARNING +def test_uniqueness_direction(): + entry = _entry("schema.orders.properties.order_id.unique", ChangelogType.updated, "False", "True") + assert _direction(UniqueConstraintRule(), entry) == Direction.TIGHTENS + entry = _entry("schema.orders.properties.tags.logicalTypeOptions.uniqueItems", ChangelogType.removed, "True") + assert _direction(UniqueConstraintRule(), entry) == Direction.LOOSENS -def test_tightening_validation_constraint_is_breaking(): - result = ValidationConstraintRule().evaluate( - _entry("schema.orders.properties.customer_id.minLength", ChangelogType.updated, "5", "10") - ) - assert result is not None - assert result.level == BreakingChangeLevel.ERROR +def test_single_primary_key_direction(): + entry = _entry("schema.orders.properties.order_id.primaryKey", ChangelogType.updated, "True", "False") + assert _direction(KeyConstraintRule(), entry) == Direction.LOOSENS -def test_tightening_date_validation_constraint_is_breaking(): - result = ValidationConstraintRule().evaluate( - _entry( +@pytest.mark.parametrize( + "path,old,new,direction", + [ + ("schema.orders.properties.customer_id.minLength", "5", "10", Direction.TIGHTENS), + ("schema.orders.properties.customer_id.maxLength", "5", "10", Direction.LOOSENS), + ("schema.orders.properties.tags.logicalTypeOptions.minItems", "1", "2", Direction.TIGHTENS), + ("schema.orders.properties.tags.logicalTypeOptions.maxItems", "1", "2", Direction.LOOSENS), + ( "schema.orders.properties.order_date.logicalTypeOptions.minimum", - ChangelogType.updated, "2024-01-01", "2024-02-01", - ) - ) - assert result is not None - assert result.level == BreakingChangeLevel.ERROR - - -def test_relaxing_date_minimum_constraint_is_info(): - result = ValidationConstraintRule().evaluate( - _entry( + Direction.TIGHTENS, + ), + ( "schema.orders.properties.order_date.logicalTypeOptions.minimum", - ChangelogType.updated, "2024-02-01", "2024-01-01", - ) - ) - assert result is not None - assert result.level == BreakingChangeLevel.INFO - - -def test_tightening_date_maximum_constraint_is_breaking(): - result = ValidationConstraintRule().evaluate( - _entry( + Direction.LOOSENS, + ), + ( "schema.orders.properties.order_date.logicalTypeOptions.maximum", - ChangelogType.updated, "2024-02-01", "2024-01-01", - ) - ) - assert result is not None - assert result.level == BreakingChangeLevel.ERROR - - -def test_relaxing_date_validation_constraint_is_info(): - result = ValidationConstraintRule().evaluate( - _entry( + Direction.TIGHTENS, + ), + ( "schema.orders.properties.order_date.logicalTypeOptions.maximum", - ChangelogType.updated, "2024-01-01", "2024-02-01", - ) - ) - assert result is not None - assert result.level == BreakingChangeLevel.INFO - - -def test_invalid_date_validation_constraint_is_warning(): - result = ValidationConstraintRule().evaluate( - _entry( + Direction.LOOSENS, + ), + ( "schema.orders.properties.order_date.logicalTypeOptions.minimum", - ChangelogType.updated, "not-a-date", "2024-01-01", - ) - ) - assert result is not None - assert result.level == BreakingChangeLevel.WARNING - - -QUALITY = "schema.orders.properties.status.quality.allowed" - - -def test_removing_a_valid_value_is_breaking(): - result = QualityConstraintRule().evaluate(_entry(f"{QUALITY}.arguments.validValues", ChangelogType.removed, "3")) - assert result.level == BreakingChangeLevel.ERROR - - -def test_adding_a_valid_value_is_info(): - result = QualityConstraintRule().evaluate( - _entry(f"{QUALITY}.arguments.validValues", ChangelogType.added, None, "4") - ) - assert result.level == BreakingChangeLevel.INFO - - -def test_adding_an_invalid_value_is_breaking(): - result = QualityConstraintRule().evaluate( - _entry(f"{QUALITY}.arguments.invalidValues", ChangelogType.added, None, "x") - ) - assert result.level == BreakingChangeLevel.ERROR - - -def test_tightening_a_quality_threshold_is_breaking(): - result = QualityConstraintRule().evaluate(_entry(f"{QUALITY}.mustBeLessThan", ChangelogType.updated, "10", "5")) - assert result.level == BreakingChangeLevel.ERROR - - -def test_relaxing_a_quality_threshold_is_info(): - result = QualityConstraintRule().evaluate(_entry(f"{QUALITY}.mustBeGreaterThan", ChangelogType.updated, "10", "5")) - assert result.level == BreakingChangeLevel.INFO + Direction.UNDETERMINED, + ), + ("schema.orders.properties.order_id.logicalTypeOptions.pattern", "^[0-9]+$", "^\\d+$", Direction.UNDETERMINED), + ], +) +def test_validation_constraint_direction(path, old, new, direction): + assert _direction(ValidationConstraintRule(), _entry(path, ChangelogType.updated, old, new)) == direction -def test_changing_a_quality_query_is_warning(): - result = QualityConstraintRule().evaluate(_entry(f"{QUALITY}.query", ChangelogType.updated, "SELECT 1", "SELECT 2")) - assert result.level == BreakingChangeLevel.WARNING +def test_adding_a_validation_constraint_tightens(): + entry = _entry("schema.orders.properties.code.logicalTypeOptions.pattern", ChangelogType.added, None, "^A") + assert _direction(ValidationConstraintRule(), entry) == Direction.TIGHTENS -def test_adding_a_quality_rule_is_breaking(): - result = QualityConstraintRule().evaluate(_entry(QUALITY, ChangelogType.added)) - assert result.level == BreakingChangeLevel.ERROR +QUALITY = "schema.orders.properties.status.quality.allowed" -def test_removing_a_quality_rule_is_info(): - result = QualityConstraintRule().evaluate(_entry(QUALITY, ChangelogType.removed)) - assert result.level == BreakingChangeLevel.INFO +@pytest.mark.parametrize( + "path,change_type,old,new,direction", + [ + (f"{QUALITY}.arguments.validValues", ChangelogType.removed, "3", None, Direction.TIGHTENS), + (f"{QUALITY}.arguments.validValues", ChangelogType.added, None, "4", Direction.LOOSENS), + (f"{QUALITY}.arguments.invalidValues", ChangelogType.added, None, "x", Direction.TIGHTENS), + (f"{QUALITY}.mustBeLessThan", ChangelogType.updated, "10", "5", Direction.TIGHTENS), + (f"{QUALITY}.mustBeGreaterThan", ChangelogType.updated, "10", "5", Direction.LOOSENS), + (f"{QUALITY}.query", ChangelogType.updated, "SELECT 1", "SELECT 2", Direction.UNDETERMINED), + (QUALITY, ChangelogType.added, None, None, Direction.TIGHTENS), + (QUALITY, ChangelogType.removed, None, None, Direction.LOOSENS), + (f"{QUALITY}.description", ChangelogType.updated, "a", "b", Direction.UNTESTED), + (f"{QUALITY}.severity", ChangelogType.updated, "error", "warning", Direction.LOOSENS), + (f"{QUALITY}.severity", ChangelogType.removed, "warning", None, Direction.TIGHTENS), + (f"{QUALITY}.severity", ChangelogType.updated, "warning", "info", Direction.UNTESTED), + ], +) +def test_quality_direction(path, change_type, old, new, direction): + assert _direction(QualityConstraintRule(), _entry(path, change_type, old, new)) == direction -def test_quality_rule_description_is_info(): - result = QualityConstraintRule().evaluate(_entry(f"{QUALITY}.description", ChangelogType.updated, "a", "b")) - assert result.level == BreakingChangeLevel.INFO +def test_unknown_change_is_untested(): + result = MetadataFallbackRule().evaluate(_entry("description.purpose", ChangelogType.updated, "old", "new"), {}, {}) + assert result.direction == Direction.UNTESTED + assert result.message == "Changed contract at description.purpose from 'old' to 'new'" -def test_only_the_added_quality_rule_itself_is_graded(): - changelog = ChangelogResult( - v1="v1", - v2="v2", - summary=[], - entries=[ - _entry(QUALITY, ChangelogType.added), - _entry(f"{QUALITY}.mustBeLessThan", ChangelogType.added, None, "5"), - ], +def test_only_the_added_element_itself_is_graded(): + result = _detect( + _entry("schema.customers", ChangelogType.added), + _entry("schema.customers.properties.customer_id", ChangelogType.added), + _entry("schema.customers.properties.customer_id.required", ChangelogType.added, None, "True"), + perspective=Perspective.PRODUCER, ) - result = BreakingChangeDetector().detect(changelog) - assert [entry.level for entry in result.entries] == [BreakingChangeLevel.ERROR, BreakingChangeLevel.INFO] + assert [(entry.direction, entry.level) for entry in result.entries] == [ + (Direction.TIGHTENS, BreakingChangeLevel.ERROR), + (None, BreakingChangeLevel.INFO), + (None, BreakingChangeLevel.INFO), + ] -def test_removing_a_whole_quality_rule_is_not_breaking(): - with_rule = """ -apiVersion: v3.0.2 -kind: DataContract -id: orders -version: 1.0.0 -status: active -schema: - - name: orders - properties: - - name: status - logicalType: string - quality: - - name: {name} - type: library - metric: invalidValues - arguments: - validValues: [1, 2, 3] - mustBeLessThan: 10 -""" - without = with_rule[: with_rule.index(" quality:")] - result = DataContract(data_contract_str=with_rule.format(name="allowed")).breaking( - DataContract(data_contract_str=without) +def test_summary_uses_highest_detail_severity(): + result = _detect( + _entry("schema.orders.description", ChangelogType.updated, "old", "new"), + _entry("schema.orders.properties.id", ChangelogType.removed, "string"), + summary=[_entry("schema.orders", ChangelogType.updated)], ) - assert not result.is_breaking - assert {entry.level for entry in result.entries} == {BreakingChangeLevel.INFO} + assert result.summary[0].level == BreakingChangeLevel.ERROR -def test_narrowing_valid_values_is_breaking_end_to_end(): - contract = """ -apiVersion: v3.0.2 -kind: DataContract -id: orders -version: 1.0.0 -status: active -schema: - - name: orders - properties: - - name: status - logicalType: string - quality: - - name: allowed - type: library - metric: invalidValues - arguments: - validValues: {values} - mustBe: 0 -""" - v1 = DataContract(data_contract_str=contract.format(values="[1, 2, 3]")) - v2 = DataContract(data_contract_str=contract.format(values="[1, 2]")) - result = v1.breaking(v2) - assert result.is_breaking - assert [e.rule_id for e in result.entries] == ["quality-constraint-changed"] +def test_detector_uses_first_matching_rule(): + class FirstRule(BreakingChangeRule): + rule_id = "first" + def evaluate(self, entry, before, after): + return RuleEvaluation(self.rule_id, Direction.UNTESTED, "first") -def test_unknown_change_is_info(): - result = MetadataFallbackRule().evaluate(_entry("description.purpose", ChangelogType.updated, "old", "new")) - assert result.level == BreakingChangeLevel.INFO - assert result.message == "Changed contract at description.purpose from 'old' to 'new'" + class SecondRule(BreakingChangeRule): + rule_id = "second" + def evaluate(self, entry, before, after): + return RuleEvaluation(self.rule_id, Direction.UNDETERMINED, "second") -def test_unmatched_entry_uses_info_fallback(): - changelog = ChangelogResult( - v1="v1", - v2="v2", - entries=[_entry("description.purpose", ChangelogType.updated, "old", "new")], - ) - result = BreakingChangeDetector().detect(changelog) - assert result.entries[0].level == BreakingChangeLevel.INFO - assert result.entries[0].rule_id == "metadata-or-unknown-change" - assert not result.is_breaking - - -def test_required_inside_an_added_schema_is_not_breaking(): - changelog = ChangelogResult( - v1="v1", - v2="v2", - entries=[ - _entry("schema.customers", ChangelogType.added), - _entry("schema.customers.properties.customer_id", ChangelogType.added), - _entry("schema.customers.properties.customer_id.required", ChangelogType.added, None, "True"), - ], + result = BreakingChangeDetector((FirstRule(), SecondRule())).detect( + ChangelogResult(v1="v1", v2="v2", entries=[_entry("field", ChangelogType.updated)]), + Perspective.CONSUMER, + {}, + {}, ) - result = BreakingChangeDetector().detect(changelog) - assert all(entry.level == BreakingChangeLevel.INFO for entry in result.entries) - assert not result.is_breaking + assert result.entries[0].rule_id == "first" -def test_required_added_to_an_existing_property_is_breaking(): - changelog = ChangelogResult( - v1="v1", - v2="v2", - entries=[_entry("schema.orders.properties.order_id.required", ChangelogType.added, None, "True")], - ) - result = BreakingChangeDetector().detect(changelog) - assert result.is_breaking +def _contract(schema=None, sla=None): + document = { + "apiVersion": "v3.2.0", + "kind": "DataContract", + "id": "orders", + "version": "1.0.0", + "status": "active", + "schema": schema or [{"name": "orders", "properties": [{"name": "id", "logicalType": "integer"}]}], + } + if sla is not None: + document["slaProperties"] = sla + return document -def test_summary_uses_highest_detail_severity(): - changelog = ChangelogResult( - v1="v1", - v2="v2", - summary=[_entry("schema.orders", ChangelogType.updated)], - entries=[ - _entry("schema.orders.description", ChangelogType.updated, "old", "new"), - _entry("schema.orders.properties.id", ChangelogType.removed, "string"), - ], +def _directions(v1, v2): + result = DataContract(data_contract_str=yaml.safe_dump(v1)).breaking( + DataContract(data_contract_str=yaml.safe_dump(v2)) ) - result = BreakingChangeDetector().detect(changelog) - assert result.summary[0].level == BreakingChangeLevel.ERROR + return {entry.direction for entry in result.entries if entry.direction not in (None, Direction.UNTESTED)} + + +def _orders(**changes): + prop = {"name": "id", "logicalType": "integer", **changes} + return [{"name": "orders", "properties": [prop]}] + + +def _quality(**rule): + return _orders(quality=[{"name": "q", "type": "library", "metric": "rowCount", **rule}]) + + +def _freshness(value, unit="h", element="orders.created_at"): + return [{"property": "freshness", "value": value, "unit": unit, "element": element}] + + +def _retention(value): + return [{"property": "retention", "value": value, "element": "orders.created_at"}] + + +def _with_customers(*relationship): + schema = _orders(relationships=list(relationship)) if relationship else _orders() + return schema + [{"name": "customers", "properties": [{"name": "id"}, {"name": "account_id"}]}] + + +def _keyed(*names): + return [{"name": "orders", "properties": [{"name": name, "primaryKey": name in names} for name in ("a", "b", "c")]}] + + +@pytest.mark.parametrize( + "v1,v2,directions", + [ + pytest.param(_contract(), _contract(_orders(physicalName="order_id")), {Direction.BOTH}, id="rename-column"), + pytest.param(_contract(), _contract(_orders(physicalName="id")), set(), id="spell-out-name"), + pytest.param( + _contract( + _orders(logicalType="array", items={"logicalType": "string"}, logicalTypeOptions={"minItems": 1}) + ), + _contract( + _orders(logicalType="array", items={"logicalType": "string"}, logicalTypeOptions={"minItems": 2}) + ), + {Direction.TIGHTENS}, + id="min-items", + ), + pytest.param( + _contract(_with_customers()), + _contract(_with_customers({"to": "customers.id"})), + {Direction.TIGHTENS}, + id="relationship-added", + ), + pytest.param( + _contract(_with_customers({"to": "customers.id"})), + _contract(_with_customers()), + {Direction.LOOSENS}, + id="relationship-removed", + ), + pytest.param( + _contract(_with_customers({"id": "fk", "to": "customers.id"})), + _contract(_with_customers({"id": "fk", "to": "customers.account_id"})), + {Direction.BOTH}, + id="relationship-retargeted", + ), + pytest.param( + _contract(sla=_freshness(24)), _contract(sla=_freshness(1)), {Direction.TIGHTENS}, id="freshness-shorter" + ), + pytest.param( + _contract(sla=_freshness(24, "h")), + _contract(sla=_freshness(1, "d")), + set(), + id="freshness-same-duration", + ), + pytest.param( + _contract(sla=_freshness(1, "d")), + _contract(sla=_freshness(48, "h")), + {Direction.LOOSENS}, + id="freshness-longer", + ), + pytest.param( + _contract(sla=_freshness(1)), + _contract(sla=_freshness(1, element="orders.updated_at")), + {Direction.BOTH}, + id="freshness-other-column", + ), + pytest.param(_contract(sla=_retention("P2Y")), _contract(sla=_retention("P1Y")), set(), id="retention"), + pytest.param(_contract(), _contract(sla=_freshness(1)), {Direction.TIGHTENS}, id="freshness-added"), + pytest.param( + _contract(sla=[{"property": "latency", "value": "1s"}]), + _contract(sla=[{"property": "latency", "value": "5s"}]), + set(), + id="untested-service-level", + ), + pytest.param( + _contract(_quality(mustBeBetween=[0, 10])), + _contract(_quality(mustBeBetween=[0, 20])), + {Direction.LOOSENS}, + id="between-wider", + ), + pytest.param( + _contract(_quality(mustBeBetween=[0, 10])), + _contract(_quality(mustBeBetween=[5, 20])), + {Direction.BOTH}, + id="between-shifted", + ), + pytest.param( + _contract(_quality(mustNotBeBetween=[0, 10])), + _contract(_quality(mustNotBeBetween=[0, 20])), + {Direction.TIGHTENS}, + id="not-between-wider", + ), + pytest.param( + _contract(_quality(mustBeBetween=[0, 10])), + _contract(_quality(mustBeLessThan=10)), + {Direction.UNDETERMINED}, + id="threshold-replaced", + ), + pytest.param(_contract(), _contract(_quality(mustBe=5, severity="warning")), set(), id="warning-rule"), + pytest.param(_contract(_keyed()), _contract(_keyed("a")), {Direction.TIGHTENS}, id="key-added"), + pytest.param(_contract(_keyed("a", "b")), _contract(_keyed("a", "b", "c")), {Direction.BOTH}, id="key-widened"), + pytest.param(_contract(_keyed("a")), _contract(_keyed("a", "b")), {Direction.BOTH}, id="key-made-composite"), + ], +) +def test_change_direction(v1, v2, directions): + assert _directions(v1, v2) == directions def test_golden_output(): @@ -416,15 +432,23 @@ def test_data_contract_breaking_reuses_changelog(): result = DataContract(data_contract_file=V1).breaking(DataContract(data_contract_file=V2)) assert result.v1 == V1 assert result.v2 == V2 + assert result.perspective == Perspective.CONSUMER assert result.is_breaking def test_cli_warning_only_change_exits_zero_but_shows_warning(): result = runner.invoke(app, ["breaking", WARNING_ONLY_V1, WARNING_ONLY_V2]) assert result.exit_code == 0 + assert "Perspective: consumer" in result.output assert "[ 1 Warning ] [ 2 Info ]" in result.output +def test_cli_producer_perspective_breaks_on_an_added_property(): + result = runner.invoke(app, ["breaking", "--perspective", "producer", WARNING_ONLY_V1, WARNING_ONLY_V2]) + assert result.exit_code == 1 + assert "Perspective: producer" in result.output + + def test_cli_missing_file_exits_nonzero(): result = runner.invoke(app, ["breaking", "unknown.yaml", "unknown.yaml"]) assert result.exit_code == 1 @@ -432,31 +456,9 @@ def test_cli_missing_file_exits_nonzero(): assert "The file 'unknown.yaml' does not exist." in str(result.exception) -def test_cli_exits_nonzero_and_badges_errors_for_mixed_changes(): +def test_cli_exits_nonzero_for_mixed_changes(): result = runner.invoke(app, ["breaking", V1, V2]) assert result.exit_code == 1 - assert "[ 3 Error ] [ 5 Info ]" in result.output - - -def test_detector_uses_first_matching_rule(): - class FirstRule(BreakingChangeRule): - rule_id = "first" - - def evaluate(self, entry): - return RuleEvaluation(self.rule_id, BreakingChangeLevel.INFO, "first") - - class SecondRule(BreakingChangeRule): - rule_id = "second" - - def evaluate(self, entry): - return RuleEvaluation(self.rule_id, BreakingChangeLevel.WARNING, "second") - - result = BreakingChangeDetector((FirstRule(), SecondRule())).detect( - ChangelogResult(v1="v1", v2="v2", entries=[_entry("field", ChangelogType.updated)]) - ) - - assert result.entries[0].rule_id == "first" - assert result.entries[0].level == BreakingChangeLevel.INFO def test_contract_strings_are_not_parsed_as_markup(capsys): diff --git a/tests/test_breaking_odcs_3_2.py b/tests/test_breaking_odcs_3_2.py index 47aaa8865..e36688682 100644 --- a/tests/test_breaking_odcs_3_2.py +++ b/tests/test_breaking_odcs_3_2.py @@ -4,6 +4,7 @@ import yaml from datacontract.data_contract import DataContract +from datacontract.model.breaking import Direction def contract(properties): @@ -21,20 +22,25 @@ def contract(properties): ) +def directions(old, new): + result = old.breaking(new) + return {entry.direction for entry in result.entries if entry.direction not in (None, Direction.UNTESTED)} + + @pytest.mark.parametrize( - "old,new,breaking", + "old,new,expected", [ - ([{"value": "a"}, {"value": "b"}], [{"value": "a"}], True), - ([{"value": "a"}], [{"value": "a"}, {"value": "b"}], False), - (None, [{"value": "a"}], True), - ([{"value": "a"}], None, False), - ([{"id": "one", "value": "a"}], [{"id": "one", "value": "b"}], True), - ([{"id": "one", "value": "a"}], [{"id": "renamed", "value": "a"}], False), - ([{"value": "a", "label": "Old"}], [{"value": "a", "label": "New"}], False), + ([{"value": "a"}, {"value": "b"}], [{"value": "a"}], {Direction.TIGHTENS}), + ([{"value": "a"}], [{"value": "a"}, {"value": "b"}], {Direction.LOOSENS}), + (None, [{"value": "a"}], {Direction.TIGHTENS}), + ([{"value": "a"}], None, {Direction.LOOSENS}), + ([{"id": "one", "value": "a"}], [{"id": "one", "value": "b"}], {Direction.BOTH}), + ([{"id": "one", "value": "a"}], [{"id": "renamed", "value": "a"}], set()), + ([{"value": "a", "label": "Old"}], [{"value": "a", "label": "New"}], set()), ], ) @pytest.mark.parametrize("nested", [False, True]) -def test_enum_compatibility(old, new, breaking, nested): +def test_enum_compatibility(old, new, expected, nested): def properties(entries): prop = {"name": "status", "logicalType": "string"} if entries is not None: @@ -50,35 +56,34 @@ def properties(entries): } return [prop] - result = contract(properties(old)).breaking(contract(properties(new))) - assert result.is_breaking is breaking, result.model_dump() + assert directions(contract(properties(old)), contract(properties(new))) == expected @pytest.mark.parametrize( - "old,new,breaking", + "old,new,expected", [ - ({"dimensions": 3}, {"dimensions": 4}, True), - ({"dimensions": 3}, {"dimensions": 3, "elementType": "float32"}, False), - ({"dimensions": 3}, {"dimensions": 3, "elementType": "float64"}, True), - ({"dimensions": 3, "elementType": "float64"}, {"dimensions": 3}, True), - ({"dimensions": 3, "elementType": "float32"}, {"dimensions": 3}, False), - ({"dimensions": 3}, {"dimensions": 3, "embeddingModel": "updated"}, False), + ({"dimensions": 3}, {"dimensions": 4}, {Direction.BOTH}), + ({"dimensions": 3}, {"dimensions": 3, "elementType": "float32"}, set()), + ({"dimensions": 3}, {"dimensions": 3, "elementType": "float64"}, {Direction.BOTH}), + ({"dimensions": 3, "elementType": "float64"}, {"dimensions": 3}, {Direction.BOTH}), + ({"dimensions": 3, "elementType": "float32"}, {"dimensions": 3}, set()), + ({"dimensions": 3}, {"dimensions": 3, "embeddingModel": "updated"}, set()), ], ) -def test_vector_shape_compatibility(old, new, breaking): +def test_vector_shape_compatibility(old, new, expected): def properties(options): return [{"name": "embedding", "logicalType": "vector", "logicalTypeOptions": options}] - assert contract(properties(old)).breaking(contract(properties(new))).is_breaking is breaking + assert directions(contract(properties(old)), contract(properties(new))) == expected -def test_adding_properties_with_enum_and_vector_constraints_is_not_breaking(): +def test_only_added_properties_are_graded_not_their_enum_and_vector_constraints(): original = [{"name": "id", "logicalType": "integer"}] added = copy.deepcopy(original) + [ {"name": "status", "logicalType": "string", "enum": [{"value": "a"}]}, {"name": "embedding", "logicalType": "vector", "logicalTypeOptions": {"dimensions": 3}}, ] - assert not contract(original).breaking(contract(added)).is_breaking + assert directions(contract(original), contract(added)) == {Direction.TIGHTENS} def test_enum_custom_property_change_is_metadata(): @@ -91,4 +96,4 @@ def test_enum_custom_property_change_is_metadata(): ] updated = copy.deepcopy(original) updated[0]["enum"][0]["customProperties"][0]["value"] = "green" - assert not contract(original).breaking(contract(updated)).is_breaking + assert directions(contract(original), contract(updated)) == set() From 3310967c4d99f1ff92004772f85bd2fbbf2939df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20Sch=C3=B6dl?= Date: Fri, 16 Oct 2026 17:14:38 +0200 Subject: [PATCH 2/3] Fix breaking grades for warn-only quality rules, freshness without a checkable element, and dotted physical names --- datacontract/breaking/rules.py | 70 +++++++++++--------- datacontract/engines/checks/create_checks.py | 4 +- tests/test_breaking.py | 33 ++++++++- 3 files changed, 73 insertions(+), 34 deletions(-) diff --git a/datacontract/breaking/rules.py b/datacontract/breaking/rules.py index 08170ef01..9566c3137 100644 --- a/datacontract/breaking/rules.py +++ b/datacontract/breaking/rules.py @@ -2,7 +2,7 @@ from dataclasses import dataclass from datetime import date -from datacontract.engines.checks.create_checks import _freshness_value_to_seconds +from datacontract.engines.checks.create_checks import freshness_value_to_seconds from datacontract.engines.checks.severity import failure_result from datacontract.model.breaking import Direction from datacontract.model.changelog import ChangelogEntry, ChangelogType @@ -21,10 +21,7 @@ class BreakingChangeRule(ABC): @abstractmethod def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: - """Return the direction of ``entry`` when this rule applies to it. - - ``before`` and ``after`` are the two normalized contracts the changelog was built from. - """ + """The direction of ``entry`` when this rule applies; ``before`` and ``after`` are the normalized contracts.""" class SchemaAddedOrRemovedRule(BreakingChangeRule): @@ -61,7 +58,9 @@ class PhysicalNameChangedRule(BreakingChangeRule): def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: if not entry.path.startswith("schema.") or not entry.path.endswith(".physicalName"): return None - name = entry.path.removesuffix(".physicalName").rsplit(".", 1)[1] + owner = entry.path.removesuffix(".physicalName") + # Schema and property names may contain dots (sales.orders). + name = owner.rsplit(".properties.", 1)[1] if ".properties." in owner else owner.removeprefix("schema.") direction = Direction.UNTESTED if (entry.old_value or name) == (entry.new_value or name) else Direction.BOTH return RuleEvaluation(self.rule_id, direction, _change_message("physical name", entry)) @@ -236,31 +235,29 @@ def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEval return None sla_path = ".".join(segments[:2]) old_sla, new_sla = _lookup(before, sla_path) or {}, _lookup(after, sla_path) or {} + # datacontract test checks freshness only with a value and an element naming a model.field of the contract. + was_checked, is_checked = ( + sla.get("value") is not None + and str(sla.get("element") or "").count(".") == 1 + and str(sla["element"]).split(".")[0] in (tree.get("schema") or {}) + for sla, tree in ((old_sla, before), (new_sla, after)) + ) field = segments[2] if len(segments) > 2 else None - if field is None: - direction = Direction.TIGHTENS if entry.type == ChangelogType.added else Direction.LOOSENS + if not was_checked and not is_checked: + direction = Direction.UNTESTED + elif was_checked != is_checked: + direction = Direction.TIGHTENS if is_checked else Direction.LOOSENS elif field == "element": - direction = { - ChangelogType.added: Direction.TIGHTENS, - ChangelogType.removed: Direction.LOOSENS, - }.get(entry.type, Direction.BOTH) + direction = Direction.BOTH elif field in ("value", "unit"): - old_value, new_value = old_sla.get("value"), new_sla.get("value") - if old_value is None and new_value is None: + old_seconds = freshness_value_to_seconds(old_sla["value"], old_sla.get("unit")) + new_seconds = freshness_value_to_seconds(new_sla["value"], new_sla.get("unit")) + if old_seconds is None or new_seconds is None: + direction = Direction.UNDETERMINED + elif old_seconds == new_seconds: direction = Direction.UNTESTED - elif old_value is None: - direction = Direction.TIGHTENS - elif new_value is None: - direction = Direction.LOOSENS else: - old_seconds = _freshness_value_to_seconds(old_value, old_sla.get("unit")) - new_seconds = _freshness_value_to_seconds(new_value, new_sla.get("unit")) - if old_seconds is None or new_seconds is None: - direction = Direction.UNDETERMINED - elif old_seconds == new_seconds: - direction = Direction.UNTESTED - else: - direction = Direction.TIGHTENS if new_seconds < old_seconds else Direction.LOOSENS + direction = Direction.TIGHTENS if new_seconds < old_seconds else Direction.LOOSENS else: direction = Direction.UNTESTED return RuleEvaluation(self.rule_id, direction, _change_message("freshness", entry)) @@ -278,8 +275,21 @@ class QualityConstraintRule(BreakingChangeRule): def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEvaluation | None: if not entry.path.startswith("schema.") or ".quality." not in entry.path: return None - rule_path = entry.path.rsplit(".quality.", 1)[1] - if rule_path.endswith(".arguments.validValues"): + quality_path, rule_path = entry.path.rsplit(".quality.", 1) + name, _, field = rule_path.partition(".") + rule_versions = [ + rule + for rule in (_lookup(tree, f"{quality_path}.quality.{name}") for tree in (before, after)) + if isinstance(rule, dict) + ] + if ( + field + and rule_versions + and all(failure_result(rule.get("severity")) != ResultEnum.failed for rule in rule_versions) + ): + # A rule that can only warn guarantees nothing. + direction = Direction.UNTESTED + elif rule_path.endswith(".arguments.validValues"): # The diff emits one entry per list element. direction = { ChangelogType.added: Direction.LOOSENS, @@ -341,9 +351,8 @@ def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEval direction = Direction.UNTESTED else: direction = Direction.TIGHTENS if is_blocking else Direction.LOOSENS - elif "." not in rule_path: + elif not field: rule = _lookup(after if entry.type == ChangelogType.added else before, entry.path) - # A rule that can only warn guarantees nothing. if failure_result(rule.get("severity") if isinstance(rule, dict) else None) != ResultEnum.failed: direction = Direction.UNTESTED else: @@ -355,7 +364,6 @@ def evaluate(self, entry: ChangelogEntry, before: dict, after: dict) -> RuleEval else: # What the rule checks changed (type, metric, query, other arguments, thresholds we cannot order). direction = Direction.UNDETERMINED - name, _, field = rule_path.partition(".") subject = f"{field} of quality rule '{name}'" if field else "quality rule" return RuleEvaluation(self.rule_id, direction, _change_message(subject, entry)) diff --git a/datacontract/engines/checks/create_checks.py b/datacontract/engines/checks/create_checks.py index ec2788077..7f2da764e 100644 --- a/datacontract/engines/checks/create_checks.py +++ b/datacontract/engines/checks/create_checks.py @@ -1201,7 +1201,7 @@ def _freshness_check( model, field = resolved unit = (sla.unit or "d").lower() or "d" - seconds = _freshness_value_to_seconds(sla.value, unit) + seconds = freshness_value_to_seconds(sla.value, unit) if seconds is None: # A freshness the CLI cannot interpret is a contract error, reported as a failed # check. It must not raise: checks are created for the whole contract before any @@ -1245,7 +1245,7 @@ def _freshness_check( ) -def _freshness_value_to_seconds(value, unit: Optional[str]) -> Optional[int]: +def freshness_value_to_seconds(value, unit: Optional[str]) -> Optional[int]: """The freshness threshold in seconds, or ``None`` when it cannot be interpreted. Accepts the same value forms as retention: a number qualified by ``unit``, or an diff --git a/tests/test_breaking.py b/tests/test_breaking.py index be86ee8a3..3ff6841a5 100644 --- a/tests/test_breaking.py +++ b/tests/test_breaking.py @@ -297,7 +297,7 @@ def _quality(**rule): def _freshness(value, unit="h", element="orders.created_at"): - return [{"property": "freshness", "value": value, "unit": unit, "element": element}] + return [{"property": "freshness", "value": value, "unit": unit, **({"element": element} if element else {})}] def _retention(value): @@ -318,6 +318,12 @@ def _keyed(*names): [ pytest.param(_contract(), _contract(_orders(physicalName="order_id")), {Direction.BOTH}, id="rename-column"), pytest.param(_contract(), _contract(_orders(physicalName="id")), set(), id="spell-out-name"), + pytest.param( + _contract([{"name": "sales.orders"}]), + _contract([{"name": "sales.orders", "physicalName": "orders"}]), + {Direction.BOTH}, + id="rename-dotted-table", + ), pytest.param( _contract( _orders(logicalType="array", items={"logicalType": "string"}, logicalTypeOptions={"minItems": 1}) @@ -369,6 +375,19 @@ def _keyed(*names): ), pytest.param(_contract(sla=_retention("P2Y")), _contract(sla=_retention("P1Y")), set(), id="retention"), pytest.param(_contract(), _contract(sla=_freshness(1)), {Direction.TIGHTENS}, id="freshness-added"), + pytest.param(_contract(), _contract(sla=_freshness(1, element=None)), set(), id="freshness-added-unchecked"), + pytest.param( + _contract(sla=_freshness(24, element=None)), + _contract(sla=_freshness(1, element=None)), + set(), + id="freshness-shorter-unchecked", + ), + pytest.param( + _contract(sla=_freshness(24, element="other.created_at")), + _contract(sla=_freshness(24)), + {Direction.TIGHTENS}, + id="freshness-element-resolved", + ), pytest.param( _contract(sla=[{"property": "latency", "value": "1s"}]), _contract(sla=[{"property": "latency", "value": "5s"}]), @@ -400,6 +419,18 @@ def _keyed(*names): id="threshold-replaced", ), pytest.param(_contract(), _contract(_quality(mustBe=5, severity="warning")), set(), id="warning-rule"), + pytest.param( + _contract(_quality(mustBeLessThan=10, severity="warning")), + _contract(_quality(mustBeLessThan=5, severity="warning")), + set(), + id="warning-rule-threshold", + ), + pytest.param( + _contract(_quality(mustBeLessThan=10, severity="warning")), + _contract(_quality(mustBeLessThan=5)), + {Direction.TIGHTENS}, + id="warning-rule-made-blocking", + ), pytest.param(_contract(_keyed()), _contract(_keyed("a")), {Direction.TIGHTENS}, id="key-added"), pytest.param(_contract(_keyed("a", "b")), _contract(_keyed("a", "b", "c")), {Direction.BOTH}, id="key-widened"), pytest.param(_contract(_keyed("a")), _contract(_keyed("a", "b")), {Direction.BOTH}, id="key-made-composite"), From 3bd6441021a05c4ffbe6ec0925eab1ea3cd5cda1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20Sch=C3=B6dl?= Date: Fri, 16 Oct 2026 17:14:39 +0200 Subject: [PATCH 3/3] Keep detector as the second argument of breaking() and shorten the perspective docs --- CHANGELOG.md | 7 +++---- datacontract/api.py | 5 ++--- datacontract/command_breaking.py | 7 +++++-- datacontract/data_contract.py | 9 +++------ docs/docs/api.md | 2 +- docs/docs/compare-contract-versions.md | 24 ++---------------------- docs/docs/python-library.md | 6 +++--- 7 files changed, 19 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b038eba1d..767263681 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,11 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added -- `datacontract breaking --perspective producer` (and `perspective` on `POST /breaking` and `DataContract.breaking()`) flags changes that data which passed v1 may fail -- `datacontract breaking` grades relationships, `physicalName`, `minItems`, `maxItems`, `uniqueItems`, `mustBeBetween`, quality rule `severity`, and the `freshness` service level, and shows each change's direction +- `datacontract breaking` now allows to select a `--perspective` (either `consumer` or `producer`) -### Changed -- `datacontract breaking` grades from the consumer's perspective by default +### Fixed +- `datacontract breaking` grades from the consumer's perspective by default (previously inconsistent mix of consumer and producer perspective) ## [1.2.6] - 2026-10-13 diff --git a/datacontract/api.py b/datacontract/api.py index 6626010d8..6d69c4949 100644 --- a/datacontract/api.py +++ b/datacontract/api.py @@ -1012,15 +1012,14 @@ def breaking_endpoint( perspective: Annotated[ Perspective, Query( - description="Whose expectations a change must not break: `consumer` flags changes that let v2 deliver " - "data v1 ruled out, `producer` flags changes that data which passed v1 may fail.", + description="`consumer` flags changes that lead to unexpected data for consumers, `producer` flags changes that can make data contract tests fail.", ), ] = Perspective.CONSUMER, ) -> BreakingChangesResponse: check_api_key(api_key) result = DataContract(data_contract_str=body.v1, untrusted_contract=True).breaking( - DataContract(data_contract_str=body.v2, untrusted_contract=True), perspective + DataContract(data_contract_str=body.v2, untrusted_contract=True), perspective=perspective ) return BreakingChangesResponse( summary=result.summary, entries=result.entries, perspective=result.perspective, is_breaking=result.is_breaking diff --git a/datacontract/command_breaking.py b/datacontract/command_breaking.py index 0e1fd1a39..2f940db34 100644 --- a/datacontract/command_breaking.py +++ b/datacontract/command_breaking.py @@ -23,7 +23,9 @@ def breaking( ], perspective: Annotated[ Perspective, - typer.Option(help="Grade for consumers relying on v1 (default) or for producer data that passed v1."), + typer.Option( + help="`consumer` flags changes that lead to unexpected data for consumers, `producer` flags changes that can make data contract tests fail." + ), ] = Perspective.CONSUMER, inline_references: inline_references_option = True, debug: debug_option = None, @@ -31,7 +33,8 @@ def breaking( """Show compatibility impact between two data contracts.""" enable_debug_logging(debug) result = DataContract(config=cli_config(), data_contract_file=v1, inline_references=inline_references).breaking( - DataContract(config=cli_config(), data_contract_file=v2, inline_references=inline_references), perspective + DataContract(config=cli_config(), data_contract_file=v2, inline_references=inline_references), + perspective=perspective, ) write_text_breaking_results(result, console) if result.is_breaking: diff --git a/datacontract/data_contract.py b/datacontract/data_contract.py index 08b0dbc74..680cdb5de 100644 --- a/datacontract/data_contract.py +++ b/datacontract/data_contract.py @@ -378,14 +378,11 @@ def _changelog( def breaking( self, other: "DataContract", - perspective: Perspective = Perspective.CONSUMER, detector: BreakingChangeDetector | None = None, + *, + perspective: Perspective | str = Perspective.CONSUMER, ) -> BreakingChangeResult: - """Classify the changelog between this contract and another for compatibility impact. - - The consumer perspective flags changes that break a guarantee v1 gave its consumers; the producer - perspective flags changes that data which passed v1 may fail. - """ + """Classify the changelog between this contract and another for compatibility impact from ``perspective``.""" from datacontract.changelog.normalize import normalize source, target = self.get_data_contract(), other.get_data_contract() diff --git a/docs/docs/api.md b/docs/docs/api.md index 5d62eca6f..db1311cbf 100644 --- a/docs/docs/api.md +++ b/docs/docs/api.md @@ -81,7 +81,7 @@ curl -X POST "http://localhost:4242/changelog" \ }' ``` -`POST /breaking` grades those same changes for compatibility impact. It adds a `direction` (`tightens`, `loosens`, `both`, `undetermined` or `untested`), a `level` (`info`, `warning` or `error`) and a `rule_id` to every entry, plus the `perspective` it graded from and a top-level `is_breaking` flag that is `true` when any entry is an `error`. Pass `?perspective=producer` to grade whether data that passed v1 may fail v2 instead of whether v2 still keeps v1's guarantees to consumers: +`POST /breaking` grades those same changes for compatibility impact. It adds a `level` (`info`, `warning` or `error`) and a `rule_id` to every entry, plus a top-level `is_breaking` flag that is `true` when any entry is an `error`: ```bash curl -X POST "http://localhost:4242/breaking" \ diff --git a/docs/docs/compare-contract-versions.md b/docs/docs/compare-contract-versions.md index 392b8b914..a9b580a91 100644 --- a/docs/docs/compare-contract-versions.md +++ b/docs/docs/compare-contract-versions.md @@ -32,33 +32,13 @@ See the generated [breaking command reference](./commands/breaking.md) for all o Whether a change breaks depends on whose expectations it must keep. `--perspective` selects one of two: -- **`consumer`** (default) - a change is breaking when v2 permits data that v1 ruled out, so a guarantee consumers relied on no longer holds. Removing a property and loosening a constraint break; adding a property and tightening a constraint do not. -- **`producer`** - a change is breaking when v2 rules out data that v1 permitted, so data that passed [`datacontract test`](./commands/test.md) against v1 may fail against v2. Adding a property and tightening a constraint break; removing a property and loosening a constraint do not. +- **`consumer`** (default) - "If we change the data in this way, will the expectations of data users break?" +- **`producer`** - "If we change the contract in this way, will the next contract test fail?" ```bash datacontract breaking --perspective producer v1.odcs.yaml v2.odcs.yaml ``` -### Directions - -Every change has a direction, independent of the perspective, which the perspective turns into a severity: - -| Direction | Meaning | `consumer` | `producer` | -|---|---|---|---| -| `tightens` | v2 rules out more data | INFO | ERROR | -| `loosens` | v2 permits more data | ERROR | INFO | -| `both` | v2 rules out some data v1 permitted and permits some v1 ruled out | ERROR | ERROR | -| `undetermined` | depends on content the CLI cannot compare | WARNING | WARNING | -| `untested` | changes nothing `datacontract test` checks | INFO | INFO | - -- **Tightens** - adding a schema or property, a type where there was none, `required`, `unique`, `uniqueItems`, a primary key, an enum, a foreign key relationship, a blocking quality rule, or a `freshness` service level; raising a minimum or lowering a maximum (`minLength`, `maxLength`, `minimum`, `maximum`, `minItems`, `maxItems`, quality thresholds); narrowing `mustBeBetween` or widening `mustNotBeBetween`; removing an enum value or a `validValues` value; adding an `invalidValues` value; shortening a `freshness` duration; making a quality rule blocking. -- **Loosens** - the reverse of each of these. -- **Both** - changing a `logicalType` or `physicalType`, a `physicalName`, a relationship's target, the column of a `freshness` service level, a vector's `dimensions` or `elementType`, or a column joining or leaving a composite primary key. -- **Undetermined** - changing a `pattern`, a quality rule's query, metric or implementation, or a value the CLI cannot parse. -- **Untested** - descriptions, tags, servers, team, and other metadata; quality rules whose `severity` only warns (`info`, `warning`, `warn`, `low`, `minor`, `trivial`); service levels other than `freshness`, including `retention`. - -A change inside an added or removed element has no direction of its own: the element carries the grade. - ### Severity levels - **ERROR** - a breaking change from the selected perspective. The command exits with status `1`. diff --git a/docs/docs/python-library.md b/docs/docs/python-library.md index 25138afd3..6e6b1a44a 100644 --- a/docs/docs/python-library.md +++ b/docs/docs/python-library.md @@ -135,16 +135,16 @@ result = v1.changelog(v2) print(result) ``` -`breaking()` grades those same changes for compatibility impact. Every entry carries a `direction` (`tightens`, `loosens`, `both`, `undetermined` or `untested`) and a `level` (`info`, `warning` or `error`), and `is_breaking` is `True` when any entry is an error. By default it grades from the consumer's perspective; pass `perspective="producer"` to grade whether data that passed v1 may fail v2: +`breaking()` grades those same changes for compatibility impact. Every entry carries a `direction` (`tightens`, `loosens`, `both`, `undetermined` or `untested`) and a `level` (`info`, `warning` or `error`), and `is_breaking` is `True` when any entry is an error. ```python -result = v1.breaking(v2, perspective="producer") +result = v1.breaking(v2, perspective="consumer") for entry in result.entries: print(entry.level.value, entry.path, entry.message) ``` -See [Compare contract versions](./compare-contract-versions.md) for the perspectives, directions and severity levels. +See [Compare contract versions](./compare-contract-versions.md) for the perspectives and severity levels. ## Spark DataFrames and Databricks