ref(open-pr-comments): use parser classes instead of standalone regexes (#63294)

Author: cathteng

src/sentry/tasks/integrations/github/open_pr_comment.py

@@ -2,7 +2,6 @@
 
 import itertools
 import logging
-import re
 from dataclasses import dataclass
 from datetime import datetime, timedelta
 from typing import Any, Dict, List, Set, Tuple
@@ -37,6 +36,7 @@
 from sentry.snuba.dataset import Dataset
 from sentry.snuba.referrer import Referrer
 from sentry.tasks.base import instrumented_task
+from sentry.tasks.integrations.github.patch_parsers import PATCH_PARSERS
 from sentry.tasks.integrations.github.pr_comment import (
     ISSUE_LOCKED_ERROR_MESSAGE,
     RATE_LIMITED_MESSAGE,
@@ -189,7 +189,7 @@ def safe_for_comment(
     for file in pr_files:
         filename = file["filename"]
         # don't count the file if it was added or is not a Python file
-        if file["status"] == "added" or not filename.endswith(".py"):
+        if file["status"] == "added" or filename.split(".")[-1] not in PATCH_PARSERS:
             continue
 
         changed_file_count += 1
@@ -224,27 +224,6 @@ def get_pr_files(pr_files: List[Dict[str, str]]) -> List[PullRequestFile]:
     return pullrequest_files
 
 
-# currently Python only
-def get_file_functions(patch: str) -> Set[str]:
-    r"""
-    Function header regex pattern
-    ^           - Asserts the start of a line.
-    @@.*@@      - Matches a string that starts with two "@" characters, followed by any characters
-                (except newline), and ends with two "@" characters.
-    \s+         - Matches one or more whitespace characters (spaces, tabs, etc.).
-    def         - Matches the literal characters "def".
-    \\s+         - Matches one or more whitespace characters.
-    (?P<fnc>.*) - This is a named capturing group that captures any characters (except newline)
-                and assigns them to the named group "fnc".
-    \(          - Matches an opening parenthesis "(".
-    .*          - Matches any characters (except newline).
-    $           - Asserts the end of a line.
-    """
-
-    python_function_regex = r"^@@.*@@\s+def\s+(?P<fnc>.*)\(.*$"
-    return set(re.findall(python_function_regex, patch, flags=re.M))
-
-
 def get_projects_and_filenames_from_source_file(
     org_id: int,
     pr_filename: str,
@@ -448,7 +427,11 @@ def open_pr_comment_workflow(pr_id: int) -> None:
         if not len(projects) or not len(sentry_filenames):
             continue
 
-        function_names = get_file_functions(file.patch)
+        language_parser = PATCH_PARSERS.get(file.filename.split(".")[-1], None)
+        if not language_parser:
+            continue
+
+        function_names = language_parser.extract_functions_from_patch(file.patch)
 
         if not len(function_names):
             continue

src/sentry/tasks/integrations/github/patch_parsers.py

@@ -0,0 +1,34 @@
+import re
+from abc import abstractmethod
+from typing import Set
+
+
+class LanguageParser:
+    @staticmethod
+    @abstractmethod
+    def extract_functions_from_patch(patch: str) -> Set[str]:
+        pass
+
+
+class PythonParser(LanguageParser):
+    @staticmethod
+    def extract_functions_from_patch(patch: str) -> Set[str]:
+        r"""
+        Function header regex pattern
+        ^           - Asserts the start of a line.
+        @@.*@@      - Matches a string that starts with two "@" characters, followed by any characters
+                    (except newline), and ends with two "@" characters.
+        \s+         - Matches one or more whitespace characters (spaces, tabs, etc.).
+        def         - Matches the literal characters "def".
+        \\s+         - Matches one or more whitespace characters.
+        (?P<fnc>.*) - This is a named capturing group that captures any characters (except newline)
+                    and assigns them to the named group "fnc".
+        \(          - Matches an opening parenthesis "(".
+        .*          - Matches any characters (except newline).
+        $           - Asserts the end of a line.
+        """
+        python_function_regex = r"^@@.*@@\s+def\s+(?P<fnc>.*)\(.*$"
+        return set(re.findall(python_function_regex, patch, flags=re.M))
+
+
+PATCH_PARSERS = {"py": PythonParser}

tests/sentry/tasks/integrations/github/test_open_pr_comment.py

@@ -13,7 +13,6 @@
     PullRequestFile,
     format_issue_table,
     format_open_pr_comment,
-    get_file_functions,
     get_issue_table_contents,
     get_pr_files,
     get_projects_and_filenames_from_source_file,
@@ -308,24 +307,6 @@ def test_get_projects_and_filenames_from_source_file(self):
         assert project_list == set(projects)
         assert sentry_filenames == set(correct_filenames)
 
-    def test_get_file_functions(self):
-        # from https://github.com/getsentry/sentry/pull/61981
-        patch = """@@ -36,6 +36,7 @@\n from sentry.templatetags.sentry_helpers import small_count\n from sentry.types.referrer_ids import GITHUB_OPEN_PR_BOT_REFERRER\n from sentry.utils import metrics\n+from sentry.utils.json import JSONData\n from sentry.utils.snuba import raw_snql_query\n \n logger = logging.getLogger(__name__)\n@@ -134,10 +135,10 @@ def get_issue_table_contents(issue_list: List[Dict[str, int]]) -> List[PullReque\n # TODO(cathy): Change the client typing to allow for multiple SCM Integrations\n def safe_for_comment(\n     gh_client: GitHubAppsClient, repository: Repository, pull_request: PullRequest\n-) -> bool:\n+) -> Tuple[bool, JSONData]:\n     logger.info("github.open_pr_comment.check_safe_for_comment")\n     try:\n-        pullrequest_resp = gh_client.get_pullrequest(\n+        pr_files = gh_client.get_pullrequest_files(\n             repo=repository.name, pull_number=pull_request.key\n         )\n     except ApiError as e:\n@@ -158,34 +159,47 @@ def safe_for_comment(\n                 tags={"type": GithubAPIErrorType.UNKNOWN.value, "code": e.code},\n             )\n             logger.exception("github.open_pr_comment.unknown_api_error", extra={"error": str(e)})\n-        return False\n+        return False, []\n \n     safe_to_comment = True\n-    if pullrequest_resp["state"] != "open":\n-        metrics.incr(\n-            OPEN_PR_METRICS_BASE.format(key="rejected_comment"), tags={"reason": "incorrect_state"}\n-        )\n-        safe_to_comment = False\n-    if pullrequest_resp["changed_files"] > OPEN_PR_MAX_FILES_CHANGED:\n+\n+    changed_file_count = 0\n+    changed_lines_count = 0\n+\n+    for file in pr_files:\n+        filename = file["filename"]\n+        # don't count the file if it was added or is not a Python file\n+        if file["status"] == "added" or not filename.endswith(".py"):\n+            continue\n+\n+        changed_file_count += 1\n+        changed_lines_count += file["changes"]\n+\n+    if changed_file_count > OPEN_PR_MAX_FILES_CHANGED:\n         metrics.incr(\n             OPEN_PR_METRICS_BASE.format(key="rejected_comment"), tags={"reason": "too_many_files"}\n         )\n         safe_to_comment = False\n-    if pullrequest_resp["additions"] + pullrequest_resp["deletions"] > OPEN_PR_MAX_LINES_CHANGED:\n+    if changed_lines_count > OPEN_PR_MAX_LINES_CHANGED:\n         metrics.incr(\n             OPEN_PR_METRICS_BASE.format(key="rejected_comment"), tags={"reason": "too_many_lines"}\n         )\n         safe_to_comment = False\n-    return safe_to_comment\n \n+    if not safe_to_comment:\n+        pr_files = []\n+\n+    return safe_to_comment, pr_files\n \n-def get_pr_filenames(\n-    gh_client: GitHubAppsClient, repository: Repository, pull_request: PullRequest\n-) -> List[str]:\n-    pr_files = gh_client.get_pullrequest_files(repo=repository.name, pull_number=pull_request.key)\n \n+def get_pr_filenames(pr_files: JSONData) -> List[str]:\n     # new files will not have sentry issues associated with them\n-    pr_filenames: List[str] = [file["filename"] for file in pr_files if file["status"] != "added"]\n+    # only fetch Python files\n+    pr_filenames: List[str] = [\n+        file["filename"]\n+        for file in pr_files\n+        if file["status"] != "added" and file["filename"].endswith(".py")\n+    ]\n \n     logger.info("github.open_pr_comment.pr_filenames", extra={"count": len(pr_filenames)})\n     return pr_filenames\n@@ -316,15 +330,22 @@ def open_pr_comment_workflow(pr_id: int) -> None:\n     client = installation.get_client()\n \n     # CREATING THE COMMENT\n-    if not safe_for_comment(gh_client=client, repository=repo, pull_request=pull_request):\n+    logger.info("github.open_pr_comment.check_safe_for_comment")\n+\n+    # fetch the files in the PR and determine if it is safe to comment\n+    safe_to_comment, pr_files = safe_for_comment(\n+        gh_client=client, repository=repo, pull_request=pull_request\n+    )\n+\n+    if not safe_to_comment:\n         logger.info("github.open_pr_comment.not_safe_for_comment")\n         metrics.incr(\n             OPEN_PR_METRICS_BASE.format(key="error"),\n             tags={"type": "unsafe_for_comment"},\n         )\n         return\n \n-    pr_filenames = get_pr_filenames(gh_client=client, repository=repo, pull_request=pull_request)\n+    pr_filenames = get_pr_filenames(pr_files)\n \n     issue_table_contents = {}\n     top_issues_per_file = []"""
-        assert get_file_functions(patch) == {
-            "get_issue_table_contents",
-            "safe_for_comment",
-            "open_pr_comment_workflow",
-        }
-
-    def test_get_file_functions_in_class(self):
-        # from https://github.com/getsentry/sentry/pull/59152
-        patch = '@@ -274,6 +274,14 @@ def patch(self, request: Request, organization, member):\n \n         result = serializer.validated_data\n \n+        if getattr(member.flags, "partnership:restricted"):\n+            return Response(\n+                {\n+                    "detail": "This member is managed by an active partnership and cannot be modified until the end of the partnership."\n+                },\n+                status=403,\n+            )\n+\n         for operation in result["operations"]:\n             # we only support setting active to False which deletes the orgmember\n             if self._should_delete_member(operation):\n@@ -310,6 +318,14 @@ def delete(self, request: Request, organization, member) -> Response:\n         """\n         Delete an organization member with a SCIM User DELETE Request.\n         """\n+        if getattr(member.flags, "partnership:restricted"):\n+            return Response(\n+                {\n+                    "detail": "This member is managed by an active partnership and cannot be modified until the end of the partnership."\n+                },\n+                status=403,\n+            )\n+\n         self._delete_member(request, organization, member)\n         metrics.incr("sentry.scim.member.delete", tags={"organization": organization})\n         return Response(status=204)\n@@ -348,6 +364,14 @@ def put(self, request: Request, organization, member):\n             )\n             return Response(context, status=200)\n \n+        if getattr(member.flags, "partnership:restricted"):\n+            return Response(\n+                {\n+                    "detail": "This member is managed by an active partnership and cannot be modified until the end of the partnership."\n+                },\n+                status=403,\n+            )\n+\n         if request.data.get("sentryOrgRole"):\n             # Don\'t update if the org role is the same\n             if ('
-        assert get_file_functions(patch) == {
-            "patch",
-            "delete",
-            "put",
-        }
-
 
 @region_silo_test
 class TestGetCommentIssues(CreateEventTestCase):
@@ -656,7 +637,9 @@ def setUp(self):
     @patch(
         "sentry.tasks.integrations.github.open_pr_comment.get_projects_and_filenames_from_source_file"
     )
-    @patch("sentry.tasks.integrations.github.open_pr_comment.get_file_functions")
+    @patch(
+        "sentry.tasks.integrations.github.patch_parsers.PythonParser.extract_functions_from_patch"
+    )
     @patch("sentry.tasks.integrations.github.open_pr_comment.get_top_5_issues_by_count_for_file")
     @patch(
         "sentry.tasks.integrations.github.open_pr_comment.safe_for_comment",
@@ -707,7 +690,9 @@ def test_comment_workflow(
     @patch(
         "sentry.tasks.integrations.github.open_pr_comment.get_projects_and_filenames_from_source_file"
     )
-    @patch("sentry.tasks.integrations.github.open_pr_comment.get_file_functions")
+    @patch(
+        "sentry.tasks.integrations.github.patch_parsers.PythonParser.extract_functions_from_patch"
+    )
     @patch("sentry.tasks.integrations.github.open_pr_comment.get_top_5_issues_by_count_for_file")
     @patch(
         "sentry.tasks.integrations.github.open_pr_comment.safe_for_comment",
@@ -765,7 +750,9 @@ def test_comment_workflow_comment_exists(
     @patch(
         "sentry.tasks.integrations.github.open_pr_comment.get_projects_and_filenames_from_source_file"
     )
-    @patch("sentry.tasks.integrations.github.open_pr_comment.get_file_functions")
+    @patch(
+        "sentry.tasks.integrations.github.patch_parsers.PythonParser.extract_functions_from_patch"
+    )
     @patch("sentry.tasks.integrations.github.open_pr_comment.safe_for_comment")
     @patch("sentry.tasks.integrations.github.open_pr_comment.metrics")
     @responses.activate
@@ -822,7 +809,9 @@ def test_comment_workflow_early_return(
     @patch(
         "sentry.tasks.integrations.github.open_pr_comment.get_projects_and_filenames_from_source_file"
     )
-    @patch("sentry.tasks.integrations.github.open_pr_comment.get_file_functions")
+    @patch(
+        "sentry.tasks.integrations.github.patch_parsers.PythonParser.extract_functions_from_patch"
+    )
     @patch("sentry.tasks.integrations.github.open_pr_comment.get_top_5_issues_by_count_for_file")
     @patch(
         "sentry.tasks.integrations.github.open_pr_comment.safe_for_comment",