This is an automated email from the ASF dual-hosted git repository. mobuchowski pushed a commit to branch verify-method-signatures-common-compat in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 6b819e1af6f71d6a7178649143abea7cb740a7c9 Author: Maciej Obuchowski <[email protected]> AuthorDate: Fri Dec 13 14:27:21 2024 +0100 add check to find if common.compat provider is changed in a non-compatible way Signed-off-by: Maciej Obuchowski <[email protected]> --- .pre-commit-config.yaml | 7 + contributing-docs/08_static_code_checks.rst | 2 + dev/breeze/doc/images/output_static-checks.svg | 24 +-- dev/breeze/doc/images/output_static-checks.txt | 2 +- dev/breeze/src/airflow_breeze/pre_commit_ids.py | 1 + .../check_common_compat_compatibility.py | 211 +++++++++++++++++++++ 6 files changed, 234 insertions(+), 13 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 65e3a0895b2..4d78e5af75e 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -419,6 +419,13 @@ repos: exclude: ^airflow/kubernetes/ entry: ./scripts/ci/pre_commit/check_airflow_k8s_not_used.py additional_dependencies: ['rich>=12.4.4'] + - id: check-common-compat-compatibility + name: Check if compat is changed in a non-compatible way + language: python + files: ^providers/src/airflow/providers/common/compat/.*\.py$ + require_serial: true + entry: ./scripts/ci/pre_commit/check_common_compat_compatibility.py + additional_dependencies: ['rich>=12.4.4'] - id: check-common-compat-used-for-openlineage name: Check common.compat is used for OL deprecated classes language: python diff --git a/contributing-docs/08_static_code_checks.rst b/contributing-docs/08_static_code_checks.rst index 0775c83ef06..3b60cfe378a 100644 --- a/contributing-docs/08_static_code_checks.rst +++ b/contributing-docs/08_static_code_checks.rst @@ -150,6 +150,8 @@ require Breeze Docker image to be built locally. +-----------------------------------------------------------+--------------------------------------------------------+---------+ | check-code-deprecations | Check deprecations categories in decorators | | +-----------------------------------------------------------+--------------------------------------------------------+---------+ +| check-common-compat-compatibility | Check if compat is changed in a non-compatible way | | ++-----------------------------------------------------------+--------------------------------------------------------+---------+ | check-common-compat-used-for-openlineage | Check common.compat is used for OL deprecated classes | | +-----------------------------------------------------------+--------------------------------------------------------+---------+ | check-core-deprecation-classes | Verify usage of Airflow deprecation classes in core | | diff --git a/dev/breeze/doc/images/output_static-checks.svg b/dev/breeze/doc/images/output_static-checks.svg index 8d78c5924b4..148c19ae7a6 100644 --- a/dev/breeze/doc/images/output_static-checks.svg +++ b/dev/breeze/doc/images/output_static-checks.svg @@ -341,18 +341,18 @@ </text><text class="breeze-static-checks-r5" x="0" y="288.4" textLength="12.2" clip-path="url(#breeze-static-checks-line-11)">│</text><text class="breeze-static-checks-r7" x="451.4" y="288.4" textLength="988.2" clip-path="url(#breeze-static-checks-line-11)">check-boring-cyborg-configuration | check-breeze-top-dependencies-limited |      </text><text class="breeze-static-checks-r5" x="1451.8" y="288.4" textLength="12.2" clip-path="url(#breeze-s [...] </text><text class="breeze-static-checks-r5" x="0" y="312.8" textLength="12.2" clip-path="url(#breeze-static-checks-line-12)">│</text><text class="breeze-static-checks-r7" x="451.4" y="312.8" textLength="988.2" clip-path="url(#breeze-static-checks-line-12)">check-builtin-literals | check-changelog-format |                            &# [...] </text><text class="breeze-static-checks-r5" x="0" y="337.2" textLength="12.2" clip-path="url(#breeze-static-checks-line-13)">│</text><text class="breeze-static-checks-r7" x="451.4" y="337.2" textLength="988.2" clip-path="url(#breeze-static-checks-line-13)">check-changelog-has-no-duplicates | check-cncf-k8s-only-for-executors |          </text><text class="breeze-static-checks-r5" x="1451.8" y="337.2" textLength="12.2" clip [...] -</text><text class="breeze-static-checks-r5" x="0" y="361.6" textLength="12.2" clip-path="url(#breeze-static-checks-line-14)">│</text><text class="breeze-static-checks-r7" x="451.4" y="361.6" textLength="988.2" clip-path="url(#breeze-static-checks-line-14)">check-code-deprecations | check-common-compat-used-for-openlineage |             </text><text class="breeze-static-checks-r5" x="1451.8" y="361.6" textLen [...] -</text><text class="breeze-static-checks-r5" x="0" y="386" textLength="12.2" clip-path="url(#breeze-static-checks-line-15)">│</text><text class="breeze-static-checks-r7" x="451.4" y="386" textLength="988.2" clip-path="url(#breeze-static-checks-line-15)">check-core-deprecation-classes | check-daysago-import-from-utils |               </text><text class="breeze-static-checks-r5" x="1451.8" y="386" tex [...] -</text><text class="breeze-static-checks-r5" x="0" y="410.4" textLength="12.2" clip-path="url(#breeze-static-checks-line-16)">│</text><text class="breeze-static-checks-r7" x="451.4" y="410.4" textLength="988.2" clip-path="url(#breeze-static-checks-line-16)">check-decorated-operator-implements-custom-name | check-deferrable-default |     </text><text class="breeze-static-checks-r5" x="1451.8" y="410.4" textLength="12.2" clip-path="url(#breeze-static [...] -</text><text class="breeze-static-checks-r5" x="0" y="434.8" textLength="12.2" clip-path="url(#breeze-static-checks-line-17)">│</text><text class="breeze-static-checks-r7" x="451.4" y="434.8" textLength="988.2" clip-path="url(#breeze-static-checks-line-17)">check-docstring-param-types | check-example-dags-urls |                          </text>< [...] -</text><text class="breeze-static-checks-r5" x="0" y="459.2" textLength="12.2" clip-path="url(#breeze-static-checks-line-18)">│</text><text class="breeze-static-checks-r7" x="451.4" y="459.2" textLength="988.2" clip-path="url(#breeze-static-checks-line-18)">check-executables-have-shebangs | check-extra-packages-references |              </text><text class="breeze-static-checks-r5" x="1451.8" y="459.2" te [...] -</text><text class="breeze-static-checks-r5" x="0" y="483.6" textLength="12.2" clip-path="url(#breeze-static-checks-line-19)">│</text><text class="breeze-static-checks-r7" x="451.4" y="483.6" textLength="988.2" clip-path="url(#breeze-static-checks-line-19)">check-extras-order | check-fab-migrations | check-for-inclusive-language |       </text><text class="breeze-static-checks-r5" x="1451.8" y="483.6" textLength="12.2" clip-path [...] -</text><text class="breeze-static-checks-r5" x="0" y="508" textLength="12.2" clip-path="url(#breeze-static-checks-line-20)">│</text><text class="breeze-static-checks-r7" x="451.4" y="508" textLength="988.2" clip-path="url(#breeze-static-checks-line-20)">check-get-lineage-collector-providers | check-google-re2-as-dependency |         </text><text class="breeze-static-checks-r5" x="1451.8" y="508" textLength="12.2" clip-path="url( [...] -</text><text class="breeze-static-checks-r5" x="0" y="532.4" textLength="12.2" clip-path="url(#breeze-static-checks-line-21)">│</text><text class="breeze-static-checks-r7" x="451.4" y="532.4" textLength="988.2" clip-path="url(#breeze-static-checks-line-21)">check-hatch-build-order | check-hooks-apply | check-imports-in-providers |       </text><text class="breeze-static-checks-r5" x="1451.8" y="532.4" textLength="12.2" clip-path [...] -</text><text class="breeze-static-checks-r5" x="0" y="556.8" textLength="12.2" clip-path="url(#breeze-static-checks-line-22)">│</text><text class="breeze-static-checks-r7" x="451.4" y="556.8" textLength="988.2" clip-path="url(#breeze-static-checks-line-22)">check-incorrect-use-of-LoggingMixin | check-init-decorator-arguments |           </text><text class="breeze-static-checks-r5" x="1451.8" y="556.8" textLength="12.2" [...] -</text><text class="breeze-static-checks-r5" x="0" y="581.2" textLength="12.2" clip-path="url(#breeze-static-checks-line-23)">│</text><text class="breeze-static-checks-r7" x="451.4" y="581.2" textLength="988.2" clip-path="url(#breeze-static-checks-line-23)">check-integrations-list-consistent | check-lazy-logging |                        </text><text class [...] -</text><text class="breeze-static-checks-r5" x="0" y="605.6" textLength="12.2" clip-path="url(#breeze-static-checks-line-24)">│</text><text class="breeze-static-checks-r7" x="451.4" y="605.6" textLength="988.2" clip-path="url(#breeze-static-checks-line-24)">check-links-to-example-dags-do-not-use-hardcoded-versions | check-merge-conflict </text><text class="breeze-static-checks-r5" x="1451.8" y="605.6" textLength="12.2" clip-path="url(#breeze-static-checks-line-24)">│</text [...] -</text><text class="breeze-static-checks-r5" x="0" y="630" textLength="12.2" clip-path="url(#breeze-static-checks-line-25)">│</text><text class="breeze-static-checks-r7" x="451.4" y="630" textLength="988.2" clip-path="url(#breeze-static-checks-line-25)">| check-min-python-version | check-newsfragments-are-valid |                     </text><text class="breeze-stati [...] +</text><text class="breeze-static-checks-r5" x="0" y="361.6" textLength="12.2" clip-path="url(#breeze-static-checks-line-14)">│</text><text class="breeze-static-checks-r7" x="451.4" y="361.6" textLength="988.2" clip-path="url(#breeze-static-checks-line-14)">check-code-deprecations | check-common-compat-compatibility |                    </text><text class="breeze-static-chec [...] +</text><text class="breeze-static-checks-r5" x="0" y="386" textLength="12.2" clip-path="url(#breeze-static-checks-line-15)">│</text><text class="breeze-static-checks-r7" x="451.4" y="386" textLength="988.2" clip-path="url(#breeze-static-checks-line-15)">check-common-compat-used-for-openlineage | check-core-deprecation-classes |      </text><text class="breeze-static-checks-r5" x="1451.8" y="386" textLength="12.2" clip-path="url(#breeze-static- [...] +</text><text class="breeze-static-checks-r5" x="0" y="410.4" textLength="12.2" clip-path="url(#breeze-static-checks-line-16)">│</text><text class="breeze-static-checks-r7" x="451.4" y="410.4" textLength="988.2" clip-path="url(#breeze-static-checks-line-16)">check-daysago-import-from-utils | check-decorated-operator-implements-custom-name</text><text class="breeze-static-checks-r5" x="1451.8" y="410.4" textLength="12.2" clip-path="url(#breeze-static-checks-line-16)">│</text><tex [...] +</text><text class="breeze-static-checks-r5" x="0" y="434.8" textLength="12.2" clip-path="url(#breeze-static-checks-line-17)">│</text><text class="breeze-static-checks-r7" x="451.4" y="434.8" textLength="988.2" clip-path="url(#breeze-static-checks-line-17)">| check-deferrable-default | check-docstring-param-types |                       </text><text class [...] +</text><text class="breeze-static-checks-r5" x="0" y="459.2" textLength="12.2" clip-path="url(#breeze-static-checks-line-18)">│</text><text class="breeze-static-checks-r7" x="451.4" y="459.2" textLength="988.2" clip-path="url(#breeze-static-checks-line-18)">check-example-dags-urls | check-executables-have-shebangs |                      </text><text class="breeze-s [...] +</text><text class="breeze-static-checks-r5" x="0" y="483.6" textLength="12.2" clip-path="url(#breeze-static-checks-line-19)">│</text><text class="breeze-static-checks-r7" x="451.4" y="483.6" textLength="988.2" clip-path="url(#breeze-static-checks-line-19)">check-extra-packages-references | check-extras-order | check-fab-migrations |    </text><text class="breeze-static-checks-r5" x="1451.8" y="483.6" textLength="12.2" clip-path="url(#breeze-s [...] +</text><text class="breeze-static-checks-r5" x="0" y="508" textLength="12.2" clip-path="url(#breeze-static-checks-line-20)">│</text><text class="breeze-static-checks-r7" x="451.4" y="508" textLength="988.2" clip-path="url(#breeze-static-checks-line-20)">check-for-inclusive-language | check-get-lineage-collector-providers |           </text><text class="breeze-static-checks-r5" x="1451.8" y="508" textLength="12.2" clip- [...] +</text><text class="breeze-static-checks-r5" x="0" y="532.4" textLength="12.2" clip-path="url(#breeze-static-checks-line-21)">│</text><text class="breeze-static-checks-r7" x="451.4" y="532.4" textLength="988.2" clip-path="url(#breeze-static-checks-line-21)">check-google-re2-as-dependency | check-hatch-build-order | check-hooks-apply |   </text><text class="breeze-static-checks-r5" x="1451.8" y="532.4" textLength="12.2" clip-path="url(#breeze-static [...] +</text><text class="breeze-static-checks-r5" x="0" y="556.8" textLength="12.2" clip-path="url(#breeze-static-checks-line-22)">│</text><text class="breeze-static-checks-r7" x="451.4" y="556.8" textLength="988.2" clip-path="url(#breeze-static-checks-line-22)">check-imports-in-providers | check-incorrect-use-of-LoggingMixin |               </text><text class="breeze-static-checks-r5" x="1451.8" y="556. [...] +</text><text class="breeze-static-checks-r5" x="0" y="581.2" textLength="12.2" clip-path="url(#breeze-static-checks-line-23)">│</text><text class="breeze-static-checks-r7" x="451.4" y="581.2" textLength="988.2" clip-path="url(#breeze-static-checks-line-23)">check-init-decorator-arguments | check-integrations-list-consistent |            </text><text class="breeze-static-checks-r5" x="1451.8" y="581.2" textLength=" [...] +</text><text class="breeze-static-checks-r5" x="0" y="605.6" textLength="12.2" clip-path="url(#breeze-static-checks-line-24)">│</text><text class="breeze-static-checks-r7" x="451.4" y="605.6" textLength="988.2" clip-path="url(#breeze-static-checks-line-24)">check-lazy-logging | check-links-to-example-dags-do-not-use-hardcoded-versions | </text><text class="breeze-static-checks-r5" x="1451.8" y="605.6" textLength="12.2" clip-path="url(#breeze-static-checks-line-24)">│< [...] +</text><text class="breeze-static-checks-r5" x="0" y="630" textLength="12.2" clip-path="url(#breeze-static-checks-line-25)">│</text><text class="breeze-static-checks-r7" x="451.4" y="630" textLength="988.2" clip-path="url(#breeze-static-checks-line-25)">check-merge-conflict | check-min-python-version | check-newsfragments-are-valid |</text><text class="breeze-static-checks-r5" x="1451.8" y="630" textLength="12.2" clip-path="url(#breeze-static-checks-line-25)">│</ [...] </text><text class="breeze-static-checks-r5" x="0" y="654.4" textLength="12.2" clip-path="url(#breeze-static-checks-line-26)">│</text><text class="breeze-static-checks-r7" x="451.4" y="654.4" textLength="988.2" clip-path="url(#breeze-static-checks-line-26)">check-no-airflow-deprecation-in-providers | check-no-providers-in-core-examples |</text><text class="breeze-static-checks-r5" x="1451.8" y="654.4" textLength="12.2" clip-path="url(#breeze-static-checks-line-26)">│</text [...] </text><text class="breeze-static-checks-r5" x="0" y="678.8" textLength="12.2" clip-path="url(#breeze-static-checks-line-27)">│</text><text class="breeze-static-checks-r7" x="451.4" y="678.8" textLength="988.2" clip-path="url(#breeze-static-checks-line-27)">check-only-new-session-with-provide-session |                               [...] </text><text class="breeze-static-checks-r5" x="0" y="703.2" textLength="12.2" clip-path="url(#breeze-static-checks-line-28)">│</text><text class="breeze-static-checks-r7" x="451.4" y="703.2" textLength="988.2" clip-path="url(#breeze-static-checks-line-28)">check-persist-credentials-disabled-in-github-workflows |                         </text><text class="bre [...] diff --git a/dev/breeze/doc/images/output_static-checks.txt b/dev/breeze/doc/images/output_static-checks.txt index 5eafd7bd8a7..03cefb15611 100644 --- a/dev/breeze/doc/images/output_static-checks.txt +++ b/dev/breeze/doc/images/output_static-checks.txt @@ -1 +1 @@ -1fd1ce1703cb27b2fd042f55f5aa6ddd +7007474671f622f69dbccd6d89268188 diff --git a/dev/breeze/src/airflow_breeze/pre_commit_ids.py b/dev/breeze/src/airflow_breeze/pre_commit_ids.py index a3b1ba37266..dd8649eda60 100644 --- a/dev/breeze/src/airflow_breeze/pre_commit_ids.py +++ b/dev/breeze/src/airflow_breeze/pre_commit_ids.py @@ -40,6 +40,7 @@ PRE_COMMIT_LIST = [ "check-changelog-has-no-duplicates", "check-cncf-k8s-only-for-executors", "check-code-deprecations", + "check-common-compat-compatibility", "check-common-compat-used-for-openlineage", "check-core-deprecation-classes", "check-daysago-import-from-utils", diff --git a/scripts/ci/pre_commit/check_common_compat_compatibility.py b/scripts/ci/pre_commit/check_common_compat_compatibility.py new file mode 100755 index 00000000000..1c89ef1fd6d --- /dev/null +++ b/scripts/ci/pre_commit/check_common_compat_compatibility.py @@ -0,0 +1,211 @@ +#!/usr/bin/env python +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import argparse +import ast +import subprocess +from collections.abc import Sequence +from dataclasses import dataclass + + +@dataclass +class Argument: + name: str + has_default: bool + type_annotation: str | None = None + + def __hash__(self): + return hash((self.name, self.has_default, self.type_annotation)) + + +@dataclass +class MethodSignature: + name: str + args: list[Argument] + returns: str | None + is_class_method: bool + parent_class: str | None + + def __hash__(self): + return hash((self.name, tuple(self.args), self.returns, self.is_class_method, self.parent_class)) + + +class MethodVisitor(ast.NodeVisitor): + def __init__(self): + self.methods: dict[str, MethodSignature] = {} + self.current_class = None + + def visit_ClassDef(self, node: ast.ClassDef): + previous_class = self.current_class + self.current_class = node.name + self.generic_visit(node) + self.current_class = previous_class + + def _get_return_annotation(self, node: ast.FunctionDef) -> str | None: + if node.returns: + return ast.unparse(node.returns) + return None + + def _get_args(self, node: ast.FunctionDef) -> list[Argument]: + args = [] + for arg in node.args.args: + type_annotation = ast.unparse(arg.annotation) if arg.annotation else None + args.append(Argument(name=arg.arg, has_default=False, type_annotation=type_annotation)) + + # Handle defaults + defaults_start = len(node.args.args) - len(node.args.defaults) + for i, _ in enumerate(node.args.defaults): + args[defaults_start + i].has_default = True + + return args + + def _is_class_method(self, node: ast.FunctionDef) -> bool: + return any( + isinstance(decorator, ast.Name) and decorator.id == "classmethod" + for decorator in node.decorator_list + ) + + def visit_FunctionDef(self, node: ast.FunctionDef): + qualified_name = f"{self.current_class}.{node.name}" if self.current_class else node.name + self.methods[qualified_name] = MethodSignature( + name=node.name, + args=self._get_args(node), + returns=self._get_return_annotation(node), + is_class_method=self._is_class_method(node), + parent_class=self.current_class, + ) + + def visit_AsyncFuncztionDef(self, node: ast.AsyncFunctionDef): + self.visit_FunctionDef(node) # type: ignore[arg-type] + + +def compare_method_signatures(original: MethodSignature, staged: MethodSignature) -> list[str]: + errors = [] + + # Skip self/cls parameter for class methods + orig_args = original.args[1:] if original.is_class_method else original.args + staged_args = staged.args[1:] if staged.is_class_method else staged.args + + # Create maps of argument names to their properties + orig_arg_map = {arg.name: arg for arg in orig_args} + staged_arg_map = {arg.name: arg for arg in staged_args} + + # Check for removed arguments + for arg_name, _ in orig_arg_map.items(): + if arg_name not in staged_arg_map: + errors.append(f"Removed argument '{arg_name}'") + + # Check for added arguments without defaults + for arg_name, arg in staged_arg_map.items(): + if arg_name not in orig_arg_map and not arg.has_default: + errors.append(f"Added required argument '{arg_name}' without default value") + + return errors + + +def get_methods(content: str) -> dict[str, MethodSignature]: + try: + tree = ast.parse(content) + visitor = MethodVisitor() + visitor.visit(tree) + return visitor.methods + except SyntaxError: + return {} + + +def get_staged_and_original_content(filename: str) -> tuple[str, str]: + # ... existing code from previous example ... + staged_content = subprocess.check_output(["git", "show", ":" + filename], text=True) + + try: + original_content = subprocess.check_output( + ["git", "show", "HEAD:" + filename], text=True, stderr=subprocess.DEVNULL + ) + except subprocess.CalledProcessError: + original_content = "" + + return staged_content, original_content + + +def format_method_signature(method: MethodSignature) -> str: + class_prefix = f"{method.parent_class}." if method.parent_class else "" + decorator = "@classmethod " if method.is_class_method else "" + args_str = ", ".join( + f"{arg.name}: {arg.type_annotation if arg.type_annotation else 'Any'}" + f"{' = ...' if arg.has_default else ''}" + for arg in method.args + ) + returns = f" -> {method.returns}" if method.returns else "" + return f"{decorator}{class_prefix}{method.name}({args_str}){returns}" + + +def main(argv: Sequence[str] | None = None) -> int: + parser = argparse.ArgumentParser() + parser.add_argument("filenames", nargs="*", help="Filenames to check") + args = parser.parse_args(argv) + + retval = 0 + + for filename in args.filenames: + if not filename.endswith(".py"): + continue + + try: + staged_content, original_content = get_staged_and_original_content(filename) + + original_methods = get_methods(original_content) + staged_methods = get_methods(staged_content) + + # Check for removed methods + removed_methods = set(original_methods.keys()) - set(staged_methods.keys()) + + # Check for signature changes in remaining methods + signature_errors = {} + for method_name in set(original_methods.keys()) & set(staged_methods.keys()): + errors = compare_method_signatures(original_methods[method_name], staged_methods[method_name]) + if errors: + signature_errors[method_name] = errors + + if removed_methods or signature_errors: + print(f"\nIssues found in {filename}:") + + if removed_methods: + print("\nRemoved methods:") + for method_name in removed_methods: + print(f" - {format_method_signature(original_methods[method_name])}") + + if signature_errors: + print("\nSignature changes:") + for method_name, errors in signature_errors.items(): + print(f"\n {format_method_signature(staged_methods[method_name])}:") + for error in errors: + print(f" - {error}") + + retval = 1 + + except Exception as e: + print(f"Error processing {filename}: {str(e)}") + retval = 1 + + return retval + + +if __name__ == "__main__": + exit(main())
