diff --git a/scripts/breaking_changes_checker/breaking_changes_tracker.py b/scripts/breaking_changes_checker/breaking_changes_tracker.py index a683f5f3f977..61a5e3d1d9b6 100644 --- a/scripts/breaking_changes_checker/breaking_changes_tracker.py +++ b/scripts/breaking_changes_checker/breaking_changes_tracker.py @@ -35,6 +35,9 @@ class ChangeType(str, Enum): ADDED_CLIENT_METHOD = "AddedClientMethod" ADDED_CLASS = "AddedClass" ADDED_CLASS_METHOD = "AddedClassMethod" + ADDED_CLASS_METHOD_PARAMETER = "AddedClassMethodParameter" + ADDED_CLASS_PROPERTY = "AddedClassProperty" + ADDED_FUNCTION_PARAMETER = "AddedFunctionParameter" class BreakingChangesTracker: REMOVED_OR_RENAMED_CLIENT_MSG = \ @@ -103,6 +106,12 @@ class BreakingChangesTracker: "The model or publicly exposed class '{}.{}' was added in the current version" ADDED_CLASS_METHOD_MSG = \ "The '{}.{}' method '{}' was added in the current version" + ADDED_CLASS_METHOD_PARAMETER_MSG = \ + "The model or publicly exposed class '{}.{}' had property '{}' added in the {} method in the current version" + ADDED_FUNCTION_PARAMETER_MSG = \ + "The function '{}.{}' had parameter '{}' added in the current version" + ADDED_CLASS_PROPERTY_MSG = \ + "The model or publicly exposed class '{}.{}' had property '{}' added in the current version" def __init__(self, stable: Dict, current: Dict, diff: Dict, package_name: str, **kwargs: Any) -> None: @@ -183,24 +192,51 @@ def run_non_breaking_class_level_diff_checks(self, module: Dict) -> None: stable_methods_node = stable_class_nodes[self.class_name]["methods"] for method_name, method_components in class_components.get("methods", {}).items(): self.function_name = method_name - if self.function_name not in stable_methods_node and \ - not isinstance(self.function_name, jsondiff.Symbol): - if self.class_name.endswith("Client"): - # This is a new client method - fa = ( - self.ADDED_CLIENT_METHOD_MSG, - ChangeType.ADDED_CLIENT_METHOD, - self.module_name, self.class_name, method_name - ) - self.features_added.append(fa) + if not isinstance(self.function_name, jsondiff.Symbol): + if self.function_name not in stable_methods_node: + if self.class_name.endswith("Client"): + # This is a new client method + fa = ( + self.ADDED_CLIENT_METHOD_MSG, + ChangeType.ADDED_CLIENT_METHOD, + self.module_name, self.class_name, method_name + ) + self.features_added.append(fa) + else: + # This is a new class method + fa = ( + self.ADDED_CLASS_METHOD_MSG, + ChangeType.ADDED_CLASS_METHOD, + self.module_name, class_name, method_name + ) + self.features_added.append(fa) else: - # This is a new class method - fa = ( - self.ADDED_CLASS_METHOD_MSG, - ChangeType.ADDED_CLASS_METHOD, - self.module_name, class_name, method_name - ) - self.features_added.append(fa) + # Check existing methods for new parameters + stable_parameters_node = stable_methods_node[self.function_name]["parameters"] + current_parameters_node = self.current[self.module_name]["class_nodes"][self.class_name]["methods"][self.function_name]["parameters"] + for param_name, param_components in method_components.get("parameters", {}).items(): + self.parameter_name = param_name + if self.parameter_name not in stable_parameters_node and \ + not isinstance(self.parameter_name, jsondiff.Symbol): + if self.function_name == "__init__": + # If this is a new class property skip reporting it here and let the class properties check handle it. + # This is because we'll get multiple reports for the same new property if it's a parameter in __init__ + # and a class level attribute. + if self.parameter_name in class_components.get("properties", {}).keys(): + continue + self.check_non_positional_parameter_added( + current_parameters_node[param_name] + ) + stable_property_node = stable_class_nodes[self.class_name]["properties"] + for property_name, property_components in class_components.get("properties", {}).items(): + if property_name not in stable_property_node and \ + not isinstance(property_name, jsondiff.Symbol): + fa = ( + self.ADDED_CLASS_PROPERTY_MSG, + ChangeType.ADDED_CLASS_PROPERTY, + self.module_name, class_name, property_name + ) + self.features_added.append(fa) def run_class_level_diff_checks(self, module: Dict) -> None: @@ -504,6 +540,23 @@ def check_positional_parameter_added(self, current_parameters_node: Dict) -> Non ) ) + def check_non_positional_parameter_added(self, current_parameters_node: Dict) -> None: + if current_parameters_node["param_type"] != "positional_or_keyword": + if self.class_name: + self.features_added.append( + ( + self.ADDED_CLASS_METHOD_PARAMETER_MSG, ChangeType.ADDED_CLASS_METHOD_PARAMETER, + self.module_name, self.class_name, self.parameter_name, self.function_name + ) + ) + else: + self.features_added.append( + ( + self.ADDED_FUNCTION_PARAMETER_MSG, ChangeType.ADDED_FUNCTION_PARAMETER, + self.module_name, self.function_name, self.parameter_name + ) + ) + def check_positional_parameter_removed_or_renamed( self, param_type: str, deleted: str, diff --git a/scripts/breaking_changes_checker/detect_breaking_changes.py b/scripts/breaking_changes_checker/detect_breaking_changes.py index 9f81ffe02c41..e1de09b6ad74 100644 --- a/scripts/breaking_changes_checker/detect_breaking_changes.py +++ b/scripts/breaking_changes_checker/detect_breaking_changes.py @@ -100,6 +100,23 @@ def get_parameter_default(param: inspect.Parameter) -> None: def get_property_names(node: ast.AST, attribute_names: Dict) -> None: + assign_nodes = [node for node in node.body if isinstance(node, ast.AnnAssign)] + # Check for class level attributes that follow the pattern: foo: List["_models.FooItem"] = rest_field(name="foo") + for assign in assign_nodes: + if hasattr(assign, "target"): + if hasattr(assign.target, "id") and not assign.target.id.startswith("_"): + attr = assign.target.id + attr_type = None + # FIXME: This can get the type hint for a limited set attributes. We need to address more complex + # type hints in the future. + # Build type hint for the attribute + if hasattr(assign.annotation, "value") and isinstance(assign.annotation.value, ast.Name): + attr_type = assign.annotation.value.id + if attr_type == "List" and hasattr(assign.annotation, "slice"): + if isinstance(assign.annotation.slice, ast.Constant): + attr_type = f"{attr_type}[{assign.annotation.slice.value}]" + attribute_names.update({attr: attr_type}) + func_nodes = [node for node in node.body if isinstance(node, ast.FunctionDef)] if func_nodes: assigns = [node for node in func_nodes[0].body if isinstance(node, (ast.Assign, ast.AnnAssign))] diff --git a/scripts/breaking_changes_checker/tests/test_changelog.py b/scripts/breaking_changes_checker/tests/test_changelog.py index 5cf15cda5930..2197e789d368 100644 --- a/scripts/breaking_changes_checker/tests/test_changelog.py +++ b/scripts/breaking_changes_checker/tests/test_changelog.py @@ -25,3 +25,281 @@ def test_changelog_flag(): msg, _, *args = bc.features_added[0] assert msg == BreakingChangesTracker.ADDED_CLIENT_METHOD_MSG assert args == ['azure.ai.contentsafety', 'BlocklistClient', 'new_blocklist_client_method'] + +def test_new_class_property_added(): + # Testing reporting on class level property added + current = { + "azure.ai.contentsafety": { + "class_nodes": { + "AnalyzeTextResult": { + "methods": {}, + "properties": { + "blocklists_match": "Optional", + "categories_analysis": "List[_models.TextCategoriesAnalysis]", + "new_class_att": "str" + } + }, + } + } + } + + stable = { + "azure.ai.contentsafety": { + "class_nodes": { + "AnalyzeTextResult": { + "methods": {}, + "properties": { + "blocklists_match": "Optional", + "categories_analysis": "List[_models.TextCategoriesAnalysis]", + } + }, + } + } + } + + diff = jsondiff.diff(stable, current) + bc = BreakingChangesTracker(stable, current, diff, "azure-ai-contentsafety", changelog=True) + bc.run_checks() + + assert len(bc.features_added) == 1 + msg, _, *args = bc.features_added[0] + assert msg == BreakingChangesTracker.ADDED_CLASS_PROPERTY_MSG + assert args == ['azure.ai.contentsafety', 'AnalyzeTextResult', 'new_class_att'] + + +def test_new_class_property_added_init(): + # Testing if a property is added both in the init and at the class level that we only get 1 report for it + current = { + "azure.ai.contentsafety": { + "class_nodes": { + "AnalyzeTextResult": { + "methods": { + "__init__": { + "parameters": { + "self": { + "default": None, + "param_type": "positional_or_keyword" + }, + "new_class_att": { + "default": None, + "param_type": "keyword_only" + }, + "kwargs": { + "default": None, + "param_type": "var_keyword" + } + }, + "is_async": False + }, + }, + "properties": { + "blocklists_match": "Optional", + "categories_analysis": "List[_models.TextCategoriesAnalysis]", + "new_class_att": "str" + } + }, + } + } + } + + stable = { + "azure.ai.contentsafety": { + "class_nodes": { + "AnalyzeTextResult": { + "methods": { + "__init__": { + "parameters": { + "self": { + "default": None, + "param_type": "positional_or_keyword" + }, + "new_class_att": { + "default": None, + "param_type": "keyword_only" + }, + "kwargs": { + "default": None, + "param_type": "var_keyword" + } + }, + "is_async": False + }, + }, + "properties": { + "blocklists_match": "Optional", + "categories_analysis": "List[_models.TextCategoriesAnalysis]", + } + }, + } + } + } + + diff = jsondiff.diff(stable, current) + bc = BreakingChangesTracker(stable, current, diff, "azure-ai-contentsafety", changelog=True) + bc.run_checks() + + assert len(bc.features_added) == 1 + msg, _, *args = bc.features_added[0] + assert msg == BreakingChangesTracker.ADDED_CLASS_PROPERTY_MSG + assert args == ['azure.ai.contentsafety', 'AnalyzeTextResult', 'new_class_att'] + + +def test_new_class_property_added_init_only(): + # Testing if we get a report on a new class property added only in the init + current = { + "azure.ai.contentsafety": { + "class_nodes": { + "AnalyzeTextResult": { + "methods": { + "__init__": { + "parameters": { + "self": { + "default": None, + "param_type": "positional_or_keyword" + }, + "new_class_att": { + "default": None, + "param_type": "keyword_only" + }, + "kwargs": { + "default": None, + "param_type": "var_keyword" + } + }, + "is_async": False + }, + }, + "properties": { + "blocklists_match": "Optional", + "categories_analysis": "List[_models.TextCategoriesAnalysis]", + } + }, + } + } + } + + stable = { + "azure.ai.contentsafety": { + "class_nodes": { + "AnalyzeTextResult": { + "methods": { + "__init__": { + "parameters": { + "self": { + "default": None, + "param_type": "positional_or_keyword" + }, + "kwargs": { + "default": None, + "param_type": "var_keyword" + } + }, + "is_async": False + }, + }, + "properties": { + "blocklists_match": "Optional", + "categories_analysis": "List[_models.TextCategoriesAnalysis]", + } + }, + } + } + } + + diff = jsondiff.diff(stable, current) + bc = BreakingChangesTracker(stable, current, diff, "azure-ai-contentsafety", changelog=True) + bc.run_checks() + + assert len(bc.features_added) == 1 + msg, _, *args = bc.features_added[0] + assert msg == BreakingChangesTracker.ADDED_CLASS_METHOD_PARAMETER_MSG + assert args == ['azure.ai.contentsafety', 'AnalyzeTextResult', 'new_class_att', '__init__'] + + +def test_new_class_method_parameter_added(): + # Testing if we get a report on a new class method parameter added + current = { + "azure.ai.contentsafety": { + "class_nodes": { + "AnalyzeTextResult": { + "methods": { + "__init__": { + "parameters": { + "self": { + "default": None, + "param_type": "positional_or_keyword" + }, + "kwargs": { + "default": None, + "param_type": "var_keyword" + } + }, + "is_async": False + }, + "foo": { + "parameters": { + "self": { + "default": None, + "param_type": "positional_or_keyword" + }, + "bar": { + "default": None, + "param_type": "var_keyword" + } + } + } + }, + "properties": { + "blocklists_match": "Optional", + "categories_analysis": "List[_models.TextCategoriesAnalysis]", + } + }, + } + } + } + + stable = { + "azure.ai.contentsafety": { + "class_nodes": { + "AnalyzeTextResult": { + "methods": { + "__init__": { + "parameters": { + "self": { + "default": None, + "param_type": "positional_or_keyword" + }, + "kwargs": { + "default": None, + "param_type": "var_keyword" + } + }, + "is_async": False + }, + "foo": { + "parameters": { + "self": { + "default": None, + "param_type": "positional_or_keyword" + }, + }, + "is_async": False + } + }, + "properties": { + "blocklists_match": "Optional", + "categories_analysis": "List[_models.TextCategoriesAnalysis]", + } + }, + } + } + } + + diff = jsondiff.diff(stable, current) + bc = BreakingChangesTracker(stable, current, diff, "azure-ai-contentsafety", changelog=True) + bc.run_checks() + + assert len(bc.features_added) == 1 + msg, _, *args = bc.features_added[0] + assert msg == BreakingChangesTracker.ADDED_CLASS_METHOD_PARAMETER_MSG + assert args == ['azure.ai.contentsafety', 'AnalyzeTextResult', 'bar', 'foo']