diff --git a/scripts/breaking_changes_checker/breaking_changes_tracker.py b/scripts/breaking_changes_checker/breaking_changes_tracker.py index d5c5aab352c6..33ab33a3dbf9 100644 --- a/scripts/breaking_changes_checker/breaking_changes_tracker.py +++ b/scripts/breaking_changes_checker/breaking_changes_tracker.py @@ -31,6 +31,7 @@ class BreakingChangeType(str, Enum): REMOVED_OR_RENAMED_MODULE = "RemovedOrRenamedModule" REMOVED_FUNCTION_KWARGS = "RemovedFunctionKwargs" REMOVED_OR_RENAMED_OPERATION_GROUP = "RemovedOrRenamedOperationGroup" + REQUIRED_PROPERTY = "RequiredProperty" class BreakingChangesTracker: @@ -90,6 +91,7 @@ class BreakingChangesTracker: "Function `{}` changed from accepting keyword arguments to not accepting them" REMOVED_OR_RENAMED_OPERATION_GROUP_MSG = \ "Deleted or renamed client operation group `{}.{}`" + REQUIRED_PROPERTY_MSG = "`{}.{}` is now required." def __init__(self, stable: Dict, current: Dict, package_name: str, **kwargs: Any) -> None: self.stable = stable @@ -259,6 +261,7 @@ def run_class_level_diff_checks(self, module: Dict) -> None: if class_deleted: continue # class was deleted, abort other checks self.check_class_instance_attribute_removed_or_renamed(class_components) + self.check_property_required(class_components) for method_name, method_components in class_components.get("methods", {}).items(): self._function_name = method_name @@ -659,6 +662,28 @@ def check_class_instance_attribute_removed_or_renamed(self, components: Dict) -> if bc: self.breaking_changes.append(bc) + def check_property_required(self, components: Dict) -> None: + for key, value in components.get("properties", {}).items(): + if not isinstance(value, dict): + continue + stable_type = self.stable[self._module_name]["class_nodes"][self._class_name]["properties"].get(key, {}).get("attr_type") + current_type = value.get("attr_type") + + if ( + isinstance(stable_type, str) + and stable_type.startswith("Optional[") + and isinstance(current_type, str) + and not current_type.startswith("Optional[") + ): + bc = ( + self.REQUIRED_PROPERTY_MSG, + BreakingChangeType.REQUIRED_PROPERTY, + self._module_name, + self._class_name, + key, + ) + self.breaking_changes.append(bc) + def check_class_removed_or_renamed(self, class_components: Dict) -> Union[bool, None]: if isinstance(self._class_name, jsondiff.Symbol): deleted_classes = [] @@ -767,6 +792,11 @@ def get_reportable_changes(self, ignore_changes: Dict, changes_list: List) -> Li should_keep = True for suppression in suppressions: + if ( + bc_type == "AddedClassMethod" + and self.is_operation_group(module_name, class_name) + ): + continue if suppression.parameter_or_property_name is not None: # If the ignore rule is for a property or parameter, we should check up to that level on the original change if self.match((bc_type, module_name, class_name, function_name, parameter_name), suppression): diff --git a/scripts/breaking_changes_checker/tests/test_changelog.py b/scripts/breaking_changes_checker/tests/test_changelog.py index 46fcc32e4c4c..de6e5203136a 100644 --- a/scripts/breaking_changes_checker/tests/test_changelog.py +++ b/scripts/breaking_changes_checker/tests/test_changelog.py @@ -1137,3 +1137,106 @@ def test_added_keyword_only_param_to_model_method_still_reported_as_class(): assert msg == ChangelogTracker.ADDED_CLASS_METHOD_PARAMETER_MSG assert args == ["azure.contoso", "ContosoModel", "extra", "do_something"] +def test_added_update_method_for_operation_group(): + stable = { + "azure.mgmt.contoso.operations": { + "class_nodes": { + "ContosoOperations": { + "type": None, + "methods": { + "list": { + "parameters": { + "self": { + "default": None, + "param_type": "positional_or_keyword" + } + }, + "is_async": False + } + }, + "properties": {} + } + } + } + } + current = { + "azure.mgmt.contoso.operations": { + "class_nodes": { + "ContosoOperations": { + "type": None, + "methods": { + "list": { + "parameters": { + "self": { + "default": None, + "param_type": "positional_or_keyword" + } + }, + "is_async": False + }, + "update": { + "parameters": { + "self": { + "default": None, + "param_type": "positional_or_keyword" + } + }, + "is_async": False + } + }, + "properties": {} + } + } + } + } + IGNORE = { + "azure-mgmt-contoso": [ + ("AddedClassMethod", "*", "*", "update") + ] + } + bc = ChangelogTracker(stable, current, "azure-mgmt-contoso", ignore=IGNORE) + bc.run_checks() + + assert len(bc.features_added) == 1 + msg, _, *args = bc.features_added[0] + assert msg == ChangelogTracker.ADDED_CLASS_METHOD_MSG + assert args == ["azure.mgmt.contoso.operations", "ContosoOperations", "update"] + +def test_class_property_is_required(): + stable = { + "azure.contoso.models": { + "class_nodes": { + "ContosoModel": { + "type": None, + "methods": {}, + "properties": { + "foo": { + "attr_type": "Optional[Foo]" + } + } + } + } + } + } + current = { + "azure.contoso.models": { + "class_nodes": { + "ContosoModel": { + "type": None, + "methods": {}, + "properties": { + "foo": { + "attr_type": "Foo" + } + } + } + } + } + } + bc = ChangelogTracker(stable, current, "azure-contoso") + bc.run_checks() + + assert len(bc.breaking_changes) == 1 + msg, _, *args = bc.breaking_changes[0] + assert msg == BreakingChangesTracker.REQUIRED_PROPERTY_MSG + assert args == ["azure.contoso.models", "ContosoModel", "foo"]