[bct] Report operation group changes (Azure#36228)

* add more property information in code report

* report new operation group

* report removed operation group

* add test

* wip

* update tests

* fix type checks

* update test

* update code

* uncomment clean up code

* clean up unused default field

* readme

---------

Co-authored-by: Catalina Peralta <caperal@microsoft.com>

scripts/breaking_changes_checker/README.md

@@ -65,6 +65,7 @@ IGNORE_BREAKING_CHANGES = {
 | RemovedOrRenamedClientMethod        | A client method was removed or renamed in the current version.                                                        | ("RemovedOrRenamedClientMethod", "module-name", "client-name", "function-name")   |
 | RemovedOrRenamedClass               | A model or publicly exposed class was removed or renamed in the current version.                                      | ("RemovedOrRenamedClass", "module-name", "class-name")                            |
 | RemovedOrRenamedClassMethod         | A model or publicly exposed class' method was removed or renamed in the current version.                              | ("RemovedOrRenamedClassMethod", "module-name", "class-name", "function-name")     |
+| RemovedOrRenamedOperationGroup         | An operation group was removed or renamed from the client in the current version.                              | ("RemovedOrRenamedOperationGroup", "module-name", "client-name", "operation-group-name")     |
 | RemovedOrRenamedInstanceAttribute   | An instance attribute was removed or renamed in the current version.                                                  | ("RemovedOrRenamedInstanceAttribute", "module-name", "class-name")                |
 | RemovedOrRenamedEnumValue           | An enum value was removed or renamed in the current version                                                           | ("RemovedOrRenamedEnumValue", "module-name", "class-name")                        |
 | RemovedOrRenamedModuleLevelFunction | A module level function was removed or renamed in the current version.                                                | ("RemovedOrRenamedModuleLevelFunction", "module-name", "function-name")           |

scripts/breaking_changes_checker/breaking_changes_tracker.py

@@ -37,6 +37,7 @@ class BreakingChangeType(str, Enum):
     CHANGED_FUNCTION_KIND = "ChangedFunctionKind"
     REMOVED_OR_RENAMED_MODULE = "RemovedOrRenamedModule"
     REMOVED_FUNCTION_KWARGS = "RemovedFunctionKwargs"
+    REMOVED_OR_RENAMED_OPERATION_GROUP = "RemovedOrRenamedOperationGroup"
 
 
 class BreakingChangesTracker:
@@ -96,6 +97,8 @@ class BreakingChangesTracker:
     REMOVED_FUNCTION_KWARGS_MSG = \
         "The function '{}.{}' changed from accepting keyword arguments to not accepting them in " \
         "the current version"
+    REMOVED_OR_RENAMED_OPERATION_GROUP_MSG = \
+        "The '{}.{}' client had operation group '{}' deleted or renamed in the current version"
 
     def __init__(self, stable: Dict, current: Dict, diff: Dict, package_name: str, **kwargs: Any) -> None:
         self.stable = stable
@@ -476,11 +479,19 @@ def check_class_instance_attribute_removed_or_renamed(self, components: Dict) ->
                 for property in deleted_props:
                     bc = None
                     if self.class_name.endswith("Client"):
-                        bc = (
-                            self.REMOVED_OR_RENAMED_INSTANCE_ATTRIBUTE_FROM_CLIENT_MSG,
-                            BreakingChangeType.REMOVED_OR_RENAMED_INSTANCE_ATTRIBUTE,
-                            self.module_name, self.class_name, property
-                        )
+                        property_type = self.stable[self.module_name]["class_nodes"][self.class_name]["properties"][property]["attr_type"]
+                        if property_type is not None and property_type.lower().endswith("operations"):
+                            bc = (
+                                self.REMOVED_OR_RENAMED_OPERATION_GROUP_MSG,
+                                BreakingChangeType.REMOVED_OR_RENAMED_OPERATION_GROUP,
+                                self.module_name, self.class_name, property
+                            )
+                        else:
+                            bc = (
+                                self.REMOVED_OR_RENAMED_INSTANCE_ATTRIBUTE_FROM_CLIENT_MSG,
+                                BreakingChangeType.REMOVED_OR_RENAMED_INSTANCE_ATTRIBUTE,
+                                self.module_name, self.class_name, property
+                            )
                     elif self.stable[self.module_name]["class_nodes"][self.class_name]["type"] == "Enum":
                         if property.upper() not in self.current[self.module_name]["class_nodes"][self.class_name]["properties"] \
                             and property.lower() not in self.current[self.module_name]["class_nodes"][self.class_name]["properties"]:

scripts/breaking_changes_checker/changelog_tracker.py

@@ -18,6 +18,7 @@ class ChangeType(str, Enum):
     ADDED_CLASS_METHOD_PARAMETER = "AddedClassMethodParameter"
     ADDED_CLASS_PROPERTY = "AddedClassProperty"
     ADDED_FUNCTION_PARAMETER = "AddedFunctionParameter"
+    ADDED_OPERATION_GROUP = "AddedOperationGroup"
 
 class ChangelogTracker(BreakingChangesTracker):
     ADDED_CLIENT_MSG = \
@@ -34,6 +35,8 @@ class ChangelogTracker(BreakingChangesTracker):
         "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"
+    ADDED_OPERATION_GROUP_MSG = \
+        "The '{}.{}' client had operation group '{}' added in the current version"
 
 
     def __init__(self, stable: Dict, current: Dict, diff: Dict, package_name: str, **kwargs: Any) -> None:
@@ -119,12 +122,20 @@ def run_non_breaking_class_level_diff_checks(self, module: Dict) -> None:
                         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.ADDED_CLASS_PROPERTY_MSG,
+                                    ChangeType.ADDED_CLASS_PROPERTY,
+                                    self.module_name, class_name, property_name
+                                )
+                            if self.class_name.endswith("Client"):
+                                if property_components["attr_type"] is not None and property_components["attr_type"].lower().endswith("operations"):
+                                    fa = (
+                                        self.ADDED_OPERATION_GROUP_MSG,
+                                        ChangeType.ADDED_OPERATION_GROUP,
+                                        self.module_name, self.class_name, property_name
+                                    )
                             self.features_added.append(fa)
 
+
     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:

scripts/breaking_changes_checker/detect_breaking_changes.py

@@ -95,13 +95,27 @@ def get_parameter_default(param: inspect.Parameter) -> None:
             default_value = default_value.__name__
         elif inspect.isclass(default_value):
             default_value = default_value.__name__
-        elif hasattr(default_value, "__class__") and default_value.__class__ == object:
+        elif hasattr(default_value, "__class__"):
             # Some default values are objects, e.g. _UNSET = object()
             default_value = default_value.__class__.__name__
 
     return default_value
 
 
+def get_property_type(node: ast.AST) -> str:
+    if hasattr(node, "value"):
+        if isinstance(node.value, ast.Call):
+            if hasattr(node.value.func, "id"):
+                return node.value.func.id
+            elif hasattr(node.value.func, "attr"):
+                return node.value.func.attr
+        elif isinstance(node.value, ast.Name):
+            return node.value.id
+        elif isinstance(node.value, ast.Constant):
+            return node.value.value
+    return 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")
@@ -126,12 +140,17 @@ def get_property_names(node: ast.AST, attribute_names: Dict) -> None:
         if assigns:
             for assign in assigns:
                 if hasattr(assign, "target"):
-                    attr = assign.target
-                    attribute_names.update({attr.attr: attr.attr})
+                    if hasattr(assign.target, "attr") and not assign.target.attr.startswith("_"):
+                        attr = assign.target
+                        attribute_names.update({attr.attr: {
+                                "attr_type": get_property_type(assign)
+                            }})
                 if hasattr(assign, "targets"):
-                    for attr in assign.targets:
-                        if hasattr(attr, "attr") and not attr.attr.startswith("_"):
-                            attribute_names.update({attr.attr: attr.attr})
+                    for target in assign.targets:
+                        if hasattr(target, "attr") and not target.attr.startswith("_"):
+                            attribute_names.update({target.attr: {
+                                "attr_type": get_property_type(assign)
+                            }})
 
 
 def check_base_classes(cls_node: ast.ClassDef) -> bool:

scripts/breaking_changes_checker/tests/test_breaking_changes.py

@@ -395,11 +395,53 @@ def test_pass_custom_reports_breaking(capsys):
     target_report = "test_current.json"
 
     try:
-        main(None, None, None, None, "tests", False, False, source_report, target_report)
+        main(None, None, None, None, "tests", False, False, False, source_report, target_report)
     except SystemExit as e:
         if e.code == 1:
             out, _ = capsys.readouterr()
             # Check if we have some breaking changes reported
             assert out.endswith("See aka.ms/azsdk/breaking-changes-tool to resolve any reported breaking changes or false positives.\n\n")
         else:
             pytest.fail("test_compare_reports failed to report breaking changes when passing custom reports")
+
+
+def test_removed_operation_group():
+    current = {
+        "azure.contoso": {
+            "class_nodes": {
+                "ContosoClient": {
+                    "methods": {},
+                    "properties": {}
+                }
+            },
+        }
+    }
+
+    stable = {
+        "azure.contoso": {
+            "class_nodes": {
+                "ContosoClient": {
+                    "methods": {},
+                    "properties": {
+                        "foo": {
+                            "attr_type": "FooOperations",
+                        }
+                    }
+                }
+            },
+        }
+    }
+
+    EXPECTED = [
+        "(RemovedOrRenamedOperationGroup): The 'azure.contoso.ContosoClient' client had operation group 'foo' deleted or renamed in the current version"
+    ]
+
+    diff = jsondiff.diff(stable, current)
+    bc = BreakingChangesTracker(stable, current, diff, "azure-storage-queue")
+    bc.run_checks()
+
+    changes = bc.report_changes()
+
+    expected_msg = format_breaking_changes(EXPECTED)
+    assert len(bc.breaking_changes) == len(EXPECTED)
+    assert changes == expected_msg

scripts/breaking_changes_checker/tests/test_changelog.py

@@ -312,8 +312,57 @@ def test_pass_custom_reports_changelog(capsys):
     target_report = "test_current.json"
 
     try:
-        main(None, None, None, None, "tests", True, False, source_report, target_report)
+        main(None, None, None, None, "tests", True, False, False, source_report, target_report)
         out, _ = capsys.readouterr()
         assert "### Breaking Changes" in out
     except SystemExit as e:
         pytest.fail("test_compare_reports failed to report changelog when passing custom reports")
+
+
+def test_added_operation_group():
+    stable = {
+        "azure.contoso": {
+            "class_nodes": {
+                "ContosoClient": {
+                    "methods": {},
+                    "properties": {
+                        "bar": {
+                            "attr_type": "str"
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+    current = {
+        "azure.contoso": {
+            "class_nodes": {
+                "ContosoClient": {
+                    "methods": {},
+                    "properties": {
+                        "bar": {
+                            "attr_type": "str"
+                        },
+                        "foo": {
+                            "attr_type": "DeviceGroupsOperations"
+                        },
+                        "zip": {
+                            "attr_type": "bool"
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+    diff = jsondiff.diff(stable, current)
+    bc = ChangelogTracker(stable, current, diff, "azure-contoso", changelog=True)
+    bc.run_checks()
+
+    assert len(bc.features_added) == 2
+    msg, _, *args = bc.features_added[0]
+    assert msg == ChangelogTracker.ADDED_OPERATION_GROUP_MSG
+    assert args == ['azure.contoso', 'ContosoClient', 'foo']
+    msg, _, *args = bc.features_added[1]
+    assert msg == ChangelogTracker.ADDED_CLASS_PROPERTY_MSG

scripts/breaking_changes_checker/tests/test_current.json

@@ -1169,9 +1169,15 @@
           }
         },
         "properties": {
-          "message_encode_policy": "message_encode_policy",
-          "message_decode_policy": "message_decode_policy",
-          "version": "version"
+          "message_encode_policy": {
+            "attr_type": null
+          },
+          "message_decode_policy": {
+            "attr_type": null
+          },
+          "version": {
+            "attr_type": null
+          }
         }
       },
       "QueueProperties": {
@@ -1473,7 +1479,9 @@
           }
         },
         "properties": {
-          "version": "version"
+          "version": {
+            "attr_type": null
+          }
         }
       },
       "ResourceTypes": {
@@ -2228,7 +2236,9 @@
           }
         },
         "properties": {
-          "version": "version"
+          "version": {
+            "attr_type": null
+          }
         }
       },
       "QueueServiceClient": {
@@ -2414,7 +2424,9 @@
           }
         },
         "properties": {
-          "version": "version"
+          "version": {
+            "attr_type": null
+          }
         }
       }
     },

scripts/breaking_changes_checker/tests/test_stable.json

@@ -1170,10 +1170,18 @@
           }
         },
         "properties": {
-          "queue_name": "queue_name",
-          "message_encode_policy": "message_encode_policy",
-          "message_decode_policy": "message_decode_policy",
-          "version": "version"
+          "queue_name": {
+            "attr_type": null
+          },
+          "message_encode_policy": {
+            "attr_type": null
+          },
+          "message_decode_policy": {
+            "attr_type": null
+          },
+          "version": {
+            "attr_type": null
+          }
         }
       },
       "QueueMessage": {
@@ -1602,7 +1610,9 @@
           }
         },
         "properties": {
-          "version": "version"
+          "version": {
+            "attr_type": null
+          }
         }
       },
       "ResourceTypes": {
@@ -2391,7 +2401,9 @@
           }
         },
         "properties": {
-          "version": "version"
+          "version": {
+            "attr_type": null
+          }
         }
       },
       "QueueServiceClient": {
@@ -2577,7 +2589,9 @@
           }
         },
         "properties": {
-          "version": "version"
+          "version": {
+            "attr_type": null
+          }
         }
       }
     },