diff --git a/fixtures/gitlab.py b/fixtures/gitlab.py index 0ce0948e070d7b..b1c4fe896b9689 100644 --- a/fixtures/gitlab.py +++ b/fixtures/gitlab.py @@ -168,7 +168,8 @@ def create_repo(self, name, external_id=15, url=None, organization_id=None): "work_in_progress": false, "total_time_spent": 0, "human_total_time_spent": null, - "human_time_estimate": null + "human_time_estimate": null, + "action": "open" }, "labels": null, "repository": { diff --git a/src/sentry/api/endpoints/organization_details.py b/src/sentry/api/endpoints/organization_details.py index c2f44372767dfd..f6f7d96c9ccac7 100644 --- a/src/sentry/api/endpoints/organization_details.py +++ b/src/sentry/api/endpoints/organization_details.py @@ -207,6 +207,12 @@ bool, GITLAB_COMMENT_BOT_DEFAULT, ), + ( + "gitlabOpenPRBot", + "sentry:gitlab_open_pr_bot", + bool, + GITLAB_COMMENT_BOT_DEFAULT, + ), ( "issueAlertsThreadFlag", "sentry:issue_alerts_thread_flag", @@ -279,6 +285,7 @@ class OrganizationSerializer(BaseOrganizationSerializer): githubNudgeInvite = serializers.BooleanField(required=False) githubPRBot = serializers.BooleanField(required=False) gitlabPRBot = serializers.BooleanField(required=False) + gitlabOpenPRBot = serializers.BooleanField(required=False) issueAlertsThreadFlag = serializers.BooleanField(required=False) metricAlertsThreadFlag = serializers.BooleanField(required=False) require2FA = serializers.BooleanField(required=False) @@ -831,6 +838,10 @@ class OrganizationDetailsPutSerializer(serializers.Serializer): help_text="Specify `true` to allow Sentry to comment on recent pull requests suspected of causing issues. Requires a GitLab integration.", required=False, ) + gitlabOpenPRBot = serializers.BooleanField( + help_text="Specify `true` to allow Sentry to comment on open pull requests to show recent error issues for the code being changed. Requires a GitLab integration.", + required=False, + ) # slack features issueAlertsThreadFlag = serializers.BooleanField( diff --git a/src/sentry/api/serializers/models/organization.py b/src/sentry/api/serializers/models/organization.py index f3bbf49658493a..fdcd615114c7b0 100644 --- a/src/sentry/api/serializers/models/organization.py +++ b/src/sentry/api/serializers/models/organization.py @@ -548,6 +548,7 @@ class DetailedOrganizationSerializerResponse(_DetailedOrganizationSerializerResp githubOpenPRBot: bool githubNudgeInvite: bool gitlabPRBot: bool + gitlabOpenPRBot: bool aggregatedDataConsent: bool genAIConsent: bool isDynamicallySampled: bool @@ -688,6 +689,9 @@ def serialize( # type: ignore[explicit-override, override] obj.get_option("sentry:github_nudge_invite", GITHUB_COMMENT_BOT_DEFAULT) ), "gitlabPRBot": bool(obj.get_option("sentry:gitlab_pr_bot", GITLAB_COMMENT_BOT_DEFAULT)), + "gitlabOpenPRBot": bool( + obj.get_option("sentry:gitlab_open_pr_bot", GITLAB_COMMENT_BOT_DEFAULT) + ), "genAIConsent": bool( obj.get_option("sentry:gen_ai_consent_v2024_11_14", DATA_CONSENT_DEFAULT) ), diff --git a/src/sentry/apidocs/examples/organization_examples.py b/src/sentry/apidocs/examples/organization_examples.py index 336c04c6a8972d..907903f1914277 100644 --- a/src/sentry/apidocs/examples/organization_examples.py +++ b/src/sentry/apidocs/examples/organization_examples.py @@ -321,6 +321,7 @@ class OrganizationExamples: "githubOpenPRBot": True, "githubNudgeInvite": True, "gitlabPRBot": True, + "gitlabOpenPRBot": True, "aggregatedDataConsent": False, "defaultAutofixAutomationTuning": "off", "issueAlertsThreadFlag": True, diff --git a/src/sentry/integrations/gitlab/client.py b/src/sentry/integrations/gitlab/client.py index 8696532bed265d..d9e36cb271606d 100644 --- a/src/sentry/integrations/gitlab/client.py +++ b/src/sentry/integrations/gitlab/client.py @@ -412,3 +412,8 @@ def get_blame_for_files( "org_integration_id": self.org_integration_id, }, ) + + def get_pr_diffs(self, repo: Repository, pr: PullRequest) -> list[dict[str, Any]]: + project_id = repo.config["project_id"] + path = GitLabApiClientPath.build_pr_diffs(project=project_id, pr_key=pr.key, unidiff=True) + return self.get(path) diff --git a/src/sentry/integrations/gitlab/integration.py b/src/sentry/integrations/gitlab/integration.py index 0c09492d32d56d..6520091d144f07 100644 --- a/src/sentry/integrations/gitlab/integration.py +++ b/src/sentry/integrations/gitlab/integration.py @@ -1,5 +1,6 @@ from __future__ import annotations +import logging from collections.abc import Callable, Mapping from typing import Any from urllib.parse import urlparse @@ -21,9 +22,17 @@ ) from sentry.integrations.services.repository.model import RpcRepository from sentry.integrations.source_code_management.commit_context import ( + OPEN_PR_MAX_FILES_CHANGED, + OPEN_PR_MAX_LINES_CHANGED, + OPEN_PR_METRICS_BASE, CommitContextIntegration, + OpenPRCommentWorkflow, PRCommentWorkflow, + PullRequestFile, + PullRequestIssue, + _open_pr_comment_log, ) +from sentry.integrations.source_code_management.language_parsers import PATCH_PARSERS from sentry.integrations.source_code_management.repository import RepositoryIntegration from sentry.models.group import Group from sentry.models.organization import Organization @@ -36,17 +45,21 @@ IntegrationProviderError, ) from sentry.snuba.referrer import Referrer -from sentry.types.referrer_ids import GITLAB_PR_BOT_REFERRER +from sentry.templatetags.sentry_helpers import small_count +from sentry.types.referrer_ids import GITLAB_OPEN_PR_BOT_REFERRER, GITLAB_PR_BOT_REFERRER from sentry.users.models.identity import Identity from sentry.utils import metrics from sentry.utils.hashlib import sha1_text from sentry.utils.http import absolute_uri +from sentry.utils.patch_set import patch_to_file_modifications from sentry.web.helpers import render_to_response from .client import GitLabApiClient, GitLabSetupApiClient from .issues import GitlabIssuesSpec from .repository import GitlabRepositoryProvider +logger = logging.getLogger("sentry.integrations.gitlab") + DESCRIPTION = """ Connect your Sentry organization to an organization in your GitLab instance or gitlab.com, enabling the following features: """ @@ -203,6 +216,9 @@ def search_issues(self, query: str | None, **kwargs) -> list[dict[str, Any]]: def get_pr_comment_workflow(self) -> PRCommentWorkflow: return GitlabPRCommentWorkflow(integration=self) + def get_open_pr_comment_workflow(self) -> OpenPRCommentWorkflow: + return GitlabOpenPRCommentWorkflow(integration=self) + MERGED_PR_COMMENT_BODY_TEMPLATE = """\ ## Suspect Issues @@ -257,6 +273,217 @@ def get_comment_data( } +OPEN_PR_COMMENT_BODY_TEMPLATE = """\ +## 🔍 Existing Issues For Review +Your merge request is modifying functions with the following pre-existing issues: + +{issue_tables}""" + +OPEN_PR_ISSUE_TABLE_TEMPLATE = """\ +📄 File: **{filename}** + +| Function | Unhandled Issue | +| :------- | :----- | +{issue_rows}""" + +OPEN_PR_ISSUE_TABLE_TOGGLE_TEMPLATE = """\ +
+📄 File: {filename} (Click to Expand) + +| Function | Unhandled Issue | +| :------- | :----- | +{issue_rows} +
""" + +OPEN_PR_ISSUE_DESCRIPTION_LENGTH = 52 + + +class GitlabOpenPRCommentWorkflow(OpenPRCommentWorkflow): + integration: GitlabIntegration + organization_option_key = "sentry:gitlab_open_pr_bot" + referrer = Referrer.GITLAB_PR_COMMENT_BOT + referrer_id = GITLAB_OPEN_PR_BOT_REFERRER + + def safe_for_comment(self, repo: Repository, pr: PullRequest) -> list[dict[str, Any]]: + client = self.integration.get_client() + + try: + diffs = client.get_pr_diffs(repo=repo, pr=pr) + except ApiError as e: + logger.info( + _open_pr_comment_log( + integration_name=self.integration.integration_name, suffix="api_error" + ) + ) + if e.code == 404: + metrics.incr( + OPEN_PR_METRICS_BASE.format( + integration=self.integration.integration_name, key="api_error" + ), + tags={"type": "missing_pr", "code": e.code}, + ) + else: + metrics.incr( + OPEN_PR_METRICS_BASE.format( + integration=self.integration.integration_name, key="api_error" + ), + tags={"type": "unknown_api_error", "code": e.code}, + ) + logger.exception( + _open_pr_comment_log( + integration_name=self.integration.integration_name, + suffix="unknown_api_error", + ), + extra={"error": str(e)}, + ) + return [] + + changed_file_count = 0 + changed_lines_count = 0 + filtered_diffs = [] + + patch_parsers = PATCH_PARSERS + + for diff in diffs: + filename = diff["new_path"] + # we only count the file if it's modified and if the file extension is in the list of supported file extensions + # we cannot look at deleted or newly added files because we cannot extract functions from the diffs + + if filename.split(".")[-1] not in patch_parsers: + continue + + try: + file_modifications = patch_to_file_modifications(diff["diff"]) + except Exception: + logger.exception( + _open_pr_comment_log( + integration_name=self.integration.integration_name, + suffix="patch_parsing_error", + ), + ) + continue + + if not file_modifications.modified: + continue + + changed_file_count += len(file_modifications.modified) + changed_lines_count += sum( + modification.lines_modified for modification in file_modifications.modified + ) + + filtered_diffs.append(diff) + + if changed_file_count > OPEN_PR_MAX_FILES_CHANGED: + metrics.incr( + OPEN_PR_METRICS_BASE.format( + integration=self.integration.integration_name, key="rejected_comment" + ), + tags={"reason": "too_many_files"}, + ) + return [] + if changed_lines_count > OPEN_PR_MAX_LINES_CHANGED: + metrics.incr( + OPEN_PR_METRICS_BASE.format( + integration=self.integration.integration_name, key="rejected_comment" + ), + tags={"reason": "too_many_lines"}, + ) + return [] + + return filtered_diffs + + def get_pr_files_safe_for_comment( + self, repo: Repository, pr: PullRequest + ) -> list[PullRequestFile]: + pr_diffs = self.safe_for_comment(repo=repo, pr=pr) + + if len(pr_diffs) == 0: + logger.info( + _open_pr_comment_log( + integration_name=self.integration.integration_name, + suffix="not_safe_for_comment", + ), + extra={"file_count": len(pr_diffs)}, + ) + metrics.incr( + OPEN_PR_METRICS_BASE.format( + integration=self.integration.integration_name, key="error" + ), + tags={"type": "unsafe_for_comment"}, + ) + return [] + + pr_files = [ + PullRequestFile(filename=diff["new_path"], patch=diff["diff"]) for diff in pr_diffs + ] + + logger.info( + _open_pr_comment_log( + integration_name=self.integration.integration_name, + suffix="pr_filenames", + ), + extra={"count": len(pr_files)}, + ) + + return pr_files + + def get_comment_data(self, comment_body: str) -> dict[str, Any]: + return { + "body": comment_body, + } + + @staticmethod + def format_comment_url(url: str, referrer: str) -> str: + return url + "?referrer=" + referrer + + @staticmethod + def format_open_pr_comment(issue_tables: list[str]) -> str: + return OPEN_PR_COMMENT_BODY_TEMPLATE.format(issue_tables="\n".join(issue_tables)) + + @staticmethod + def format_open_pr_comment_subtitle(title_length, subtitle): + # the title length + " " + subtitle should be <= 52 + subtitle_length = OPEN_PR_ISSUE_DESCRIPTION_LENGTH - title_length - 1 + return ( + subtitle[: subtitle_length - 3] + "..." if len(subtitle) > subtitle_length else subtitle + ) + + def format_issue_table( + self, + diff_filename: str, + issues: list[PullRequestIssue], + patch_parsers: dict[str, Any], + toggle: bool, + ) -> str: + language_parser = patch_parsers.get(diff_filename.split(".")[-1], None) + + if not language_parser: + return "" + + issue_row_template = language_parser.issue_row_template + + issue_rows = "\n".join( + [ + issue_row_template.format( + title=issue.title, + subtitle=self.format_open_pr_comment_subtitle(len(issue.title), issue.subtitle), + url=self.format_comment_url(issue.url, GITLAB_OPEN_PR_BOT_REFERRER), + event_count=small_count(issue.event_count), + function_name=issue.function_name, + affected_users=small_count(issue.affected_users), + ) + for issue in issues + ] + ) + + if toggle: + return OPEN_PR_ISSUE_TABLE_TOGGLE_TEMPLATE.format( + filename=diff_filename, issue_rows=issue_rows + ) + + return OPEN_PR_ISSUE_TABLE_TEMPLATE.format(filename=diff_filename, issue_rows=issue_rows) + + class InstallationForm(forms.Form): url = forms.CharField( label=_("GitLab URL"), diff --git a/src/sentry/integrations/gitlab/utils.py b/src/sentry/integrations/gitlab/utils.py index 4fcc31a3e3e12f..a7e561069163c5 100644 --- a/src/sentry/integrations/gitlab/utils.py +++ b/src/sentry/integrations/gitlab/utils.py @@ -1,5 +1,6 @@ from collections.abc import Mapping from datetime import datetime +from urllib.parse import urlencode from sentry.shared_integrations.response.base import BaseApiResponse @@ -39,6 +40,7 @@ class GitLabApiClientPath: update_issue_note = "/projects/{project}/issues/{issue_id}/notes/{note_id}" create_pr_note = "/projects/{project}/merge_requests/{pr_key}/notes" update_pr_note = "/projects/{project}/merge_requests/{pr_key}/notes/{note_id}" + pr_diffs = "/projects/{project}/merge_requests/{pr_key}/diffs" project = "/projects/{project}" project_issues = "/projects/{project}/issues" project_hooks = "/projects/{project}/hooks" @@ -50,6 +52,14 @@ class GitLabApiClientPath: def build_api_url(base_url, path): return f"{base_url.rstrip('/')}{API_VERSION}{path}" + @classmethod + def build_pr_diffs(cls, project: str, pr_key: str, unidiff: bool = False) -> str: + params = {} + if unidiff: + params["unidiff"] = "true" + + return f"{cls.pr_diffs.format(project=project, pr_key=pr_key)}?{urlencode(params)}" + def get_rate_limit_info_from_response( response: BaseApiResponse, diff --git a/src/sentry/integrations/gitlab/webhooks.py b/src/sentry/integrations/gitlab/webhooks.py index 0a0123c842f5f0..7746755aadafed 100644 --- a/src/sentry/integrations/gitlab/webhooks.py +++ b/src/sentry/integrations/gitlab/webhooks.py @@ -20,11 +20,13 @@ from sentry.integrations.base import IntegrationDomain from sentry.integrations.services.integration import integration_service from sentry.integrations.services.integration.model import RpcIntegration +from sentry.integrations.source_code_management.commit_context import CommitContextIntegration from sentry.integrations.source_code_management.webhook import SCMWebhook from sentry.integrations.utils.metrics import IntegrationWebhookEvent, IntegrationWebhookEventType from sentry.integrations.utils.scope import clear_tags_and_context from sentry.models.commit import Commit from sentry.models.commitauthor import CommitAuthor +from sentry.models.organization import Organization from sentry.models.pullrequest import PullRequest from sentry.models.repository import Repository from sentry.organizations.services.organization import organization_service @@ -154,6 +156,8 @@ def __call__(self, event: Mapping[str, Any], **kwargs): if last_commit: author_email = last_commit["author"]["email"] author_name = last_commit["author"]["name"] + + action = event["object_attributes"].get("action") except KeyError as e: logger.info( "gitlab.webhook.invalid-merge-data", @@ -172,7 +176,7 @@ def __call__(self, event: Mapping[str, Any], **kwargs): author.preload_users() try: - PullRequest.objects.update_or_create( + pr, created = PullRequest.objects.update_or_create( organization_id=organization.id, repository_id=repo.id, key=number, @@ -184,6 +188,12 @@ def __call__(self, event: Mapping[str, Any], **kwargs): "date_added": parse_date(created_at).astimezone(timezone.utc), }, ) + + installation = integration.get_installation(organization_id=organization.id) + if action == "open" and created and isinstance(installation, CommitContextIntegration): + org = Organization.objects.get_from_cache(id=organization.id) + installation.queue_open_pr_comment_task_if_needed(pr=pr, organization=org) + except IntegrityError: pass diff --git a/src/sentry/integrations/source_code_management/tasks.py b/src/sentry/integrations/source_code_management/tasks.py index 8cd58c64a3d270..fc1e6180d53e53 100644 --- a/src/sentry/integrations/source_code_management/tasks.py +++ b/src/sentry/integrations/source_code_management/tasks.py @@ -299,6 +299,7 @@ def open_pr_comment_workflow(pr_id: int) -> None: extra={ "organization_id": org_id, "repository_id": repo.id, + "file_name": file.filename, "extension": file_extension, }, ) @@ -307,22 +308,35 @@ def open_pr_comment_workflow(pr_id: int) -> None: if not language_parser: logger.info( _open_pr_comment_log(integration_name=integration_name, suffix="missing_parser"), - extra={"extension": file_extension}, + extra={"file_name": file.filename, "extension": file_extension}, ) metrics.incr( OPEN_PR_METRICS_BASE.format(integration=integration_name, key="missing_parser"), - tags={"extension": file_extension}, + tags={"file_name": file.filename, "extension": file_extension}, ) continue function_names = language_parser.extract_functions_from_patch(file.patch) + if file_extension == "py": + logger.info( + _open_pr_comment_log(integration_name=integration_name, suffix="python"), + extra={ + "organization_id": org_id, + "repository_id": repo.id, + "file_name": file.filename, + "extension": file_extension, + "has_function_names": bool(function_names), + }, + ) + if file_extension in ["js", "jsx"]: logger.info( _open_pr_comment_log(integration_name=integration_name, suffix="javascript"), extra={ "organization_id": org_id, "repository_id": repo.id, + "file_name": file.filename, "extension": file_extension, "has_function_names": bool(function_names), }, @@ -334,6 +348,7 @@ def open_pr_comment_workflow(pr_id: int) -> None: extra={ "organization_id": org_id, "repository_id": repo.id, + "file_name": file.filename, "extension": file_extension, "has_function_names": bool(function_names), }, @@ -345,6 +360,7 @@ def open_pr_comment_workflow(pr_id: int) -> None: extra={ "organization_id": org_id, "repository_id": repo.id, + "file_name": file.filename, "extension": file_extension, "has_function_names": bool(function_names), }, diff --git a/src/sentry/types/referrer_ids.py b/src/sentry/types/referrer_ids.py index 7ed60687d91fd9..60f94bc374f637 100644 --- a/src/sentry/types/referrer_ids.py +++ b/src/sentry/types/referrer_ids.py @@ -4,3 +4,4 @@ GITHUB_PR_BOT_REFERRER = "github-pr-bot" GITHUB_OPEN_PR_BOT_REFERRER = "github-open-pr-bot" GITLAB_PR_BOT_REFERRER = "gitlab-pr-bot" +GITLAB_OPEN_PR_BOT_REFERRER = "gitlab-open-pr-bot" diff --git a/src/sentry/utils/patch_set.py b/src/sentry/utils/patch_set.py index 9216642b2e3725..85621bebd71fff 100644 --- a/src/sentry/utils/patch_set.py +++ b/src/sentry/utils/patch_set.py @@ -1,5 +1,6 @@ from __future__ import annotations +from dataclasses import dataclass from typing import Literal, TypedDict import unidiff @@ -24,3 +25,68 @@ def patch_to_file_changes(patch: str) -> list[FileChange]: file_changes.append({"path": patched_file.path, "type": "M"}) return file_changes + + +@dataclass(frozen=True, kw_only=True, slots=True) +class FileModification: + path: str + lines_added: int + lines_removed: int + lines_modified: int + + +@dataclass(frozen=True, kw_only=True, slots=True) +class FileModifications: + added: list[FileModification] + removed: list[FileModification] + modified: list[FileModification] + + +def patch_to_file_modifications(patch: str) -> FileModifications: + patch_set = unidiff.PatchSet.from_string(patch) + + return FileModifications( + added=_patched_files_to_file_modifications(patch_set.added_files), + removed=_patched_files_to_file_modifications(patch_set.removed_files), + modified=_patched_files_to_file_modifications(patch_set.modified_files), + ) + + +def _patched_files_to_file_modifications( + patched_files: list[unidiff.PatchedFile], +) -> list[FileModification]: + result: list[FileModification] = [] + + for patched_file in patched_files: + lines_added = 0 + lines_removed = 0 + lines_modified = 0 + + for hunk in patched_file: + lines = list(hunk) + i = 0 + + while i < len(lines): + line = lines[i] + if line.is_removed: + # Check if next line is an adjacent addition (potential modification) + if i + 1 < len(lines) and lines[i + 1].is_added: + lines_modified += 1 + i += 2 # Skip the added line as well + continue + else: + lines_removed += 1 + elif line.is_added: + lines_added += 1 + i += 1 + + result.append( + FileModification( + path=patched_file.path, + lines_added=lines_added, + lines_removed=lines_removed, + lines_modified=lines_modified, + ) + ) + + return result diff --git a/tests/sentry/api/endpoints/test_organization_details.py b/tests/sentry/api/endpoints/test_organization_details.py index 45df91f237ea81..cdd5fde8125de2 100644 --- a/tests/sentry/api/endpoints/test_organization_details.py +++ b/tests/sentry/api/endpoints/test_organization_details.py @@ -765,6 +765,8 @@ def test_various_options(self, mock_get_repositories): "githubOpenPRBot": False, "githubNudgeInvite": False, "githubPRBot": False, + "gitlabPRBot": False, + "gitlabOpenPRBot": False, "allowSharedIssues": False, "enhancedPrivacy": True, "dataScrubber": True, @@ -857,6 +859,8 @@ def test_various_options(self, mock_get_repositories): assert "to {}".format(data["githubPRBot"]) in log.data["githubPRBot"] assert "to {}".format(data["githubOpenPRBot"]) in log.data["githubOpenPRBot"] assert "to {}".format(data["githubNudgeInvite"]) in log.data["githubNudgeInvite"] + assert "to {}".format(data["gitlabPRBot"]) in log.data["gitlabPRBot"] + assert "to {}".format(data["gitlabOpenPRBot"]) in log.data["gitlabOpenPRBot"] assert "to {}".format(data["issueAlertsThreadFlag"]) in log.data["issueAlertsThreadFlag"] assert "to {}".format(data["metricAlertsThreadFlag"]) in log.data["metricAlertsThreadFlag"] assert "to Default Mode" in log.data["samplingMode"] diff --git a/tests/sentry/integrations/gitlab/tasks/test_open_pr_comment.py b/tests/sentry/integrations/gitlab/tasks/test_open_pr_comment.py new file mode 100644 index 00000000000000..cdb40b00519847 --- /dev/null +++ b/tests/sentry/integrations/gitlab/tasks/test_open_pr_comment.py @@ -0,0 +1,575 @@ +from unittest.mock import patch + +import responses +from django.utils import timezone + +from sentry.integrations.source_code_management.commit_context import ( + OPEN_PR_MAX_FILES_CHANGED, + OPEN_PR_MAX_LINES_CHANGED, + PullRequestFile, +) +from sentry.integrations.source_code_management.tasks import open_pr_comment_workflow +from sentry.models.group import Group +from sentry.models.pullrequest import CommentType, PullRequestComment +from sentry.testutils.helpers.datetime import before_now +from sentry.testutils.skips import requires_snuba +from sentry.utils import json +from tests.sentry.integrations.gitlab.tasks.test_pr_comment import GitlabCommentTestCase + +pytestmark = [requires_snuba] + +DIFFS = { + "pure_addition": """diff --git a/test.py b/test.py +index 0000001..0000002 100644 +--- a/test.py ++++ b/test.py +@@ -0,0 +1,3 @@ ++def hello(): ++ print("Hello") ++ return True +""", + "pure_deletion": """diff --git a/test.py b/test.py +index 0000001..0000002 100644 +--- a/test.py ++++ b/test.py +@@ -5,3 +0,0 @@ +-def goodbye(): +- print("Goodbye") +- return False +""", + "simple_modification": """diff --git a/test.py b/test.py +index 0000001..0000002 100644 +--- a/test.py ++++ b/test.py +@@ -10,1 +10,1 @@ +- print("Hello World") ++ print("Hello Universe") +""", + "add_and_remove": """diff --git a/test.py b/test.py +index 0000001..0000002 100644 +--- a/test.py ++++ b/test.py +@@ -8,0 +9,1 @@ ++ print("Extra logging") +@@ -10,1 +11,0 @@ +- print("Old debug") +""", + "mixed": """diff --git a/test.py b/test.py +index 0000001..0000002 100644 +--- a/test.py ++++ b/test.py +@@ -4,1 +4,1 @@ +- name = "OldName" ++ name = "NewName" +@@ -6,0 +7,2 @@ ++ age = 30 ++ country = "UK" +@@ -10,1 +12,0 @@ +- unused_variable = None +""", + "consecutive_modifications": """diff --git a/test.py b/test.py +index 0000001..0000002 100644 +--- a/test.py ++++ b/test.py +@@ -20,2 +20,2 @@ +- foo = 1 ++ foo = 10 +- bar = 2 ++ bar = 20 +""", +} + + +class TestSafeForComment(GitlabCommentTestCase): + def setUp(self): + super().setUp() + + self.pr = self.create_pr_issues() + + mock_integration_metrics_patcher = patch("sentry.integrations.gitlab.integration.metrics") + self.mock_integration_metrics = mock_integration_metrics_patcher.start() + self.addCleanup(mock_integration_metrics_patcher.stop) + + @responses.activate + def test_simple(self): + data = [ + { + "diff": DIFFS["pure_addition"], + "new_path": "foo.py", + "old_path": "foo.py", + "a_mode": "100644", + "b_mode": "100644", + "new_file": True, + "renamed_file": False, + "deleted_file": False, + "generated_file": False, + }, + { + "diff": DIFFS["pure_deletion"], + "new_path": "foo.py", + "old_path": "foo.py", + "a_mode": "100644", + "b_mode": "100644", + "new_file": False, + "renamed_file": False, + "deleted_file": True, + "generated_file": False, + }, + { + "diff": DIFFS["simple_modification"], + "new_path": "foo.py", + "old_path": "foo.py", + "a_mode": "100644", + "b_mode": "100644", + "new_file": False, + "renamed_file": False, + "deleted_file": False, + "generated_file": False, + }, + { + "diff": DIFFS["add_and_remove"], + "new_path": "foo.py", + "old_path": "foo.py", + "a_mode": "100644", + "b_mode": "100644", + "new_file": False, + "renamed_file": False, + "deleted_file": False, + "generated_file": False, + }, + { + "diff": DIFFS["mixed"], + "new_path": "foo.py", + "old_path": "foo.py", + "a_mode": "100644", + "b_mode": "100644", + "new_file": False, + "renamed_file": False, + "deleted_file": False, + "generated_file": False, + }, + { + "diff": DIFFS["consecutive_modifications"], + "new_path": "foo.py", + "old_path": "foo.py", + "a_mode": "100644", + "b_mode": "100644", + "new_file": False, + "renamed_file": False, + "deleted_file": False, + "generated_file": False, + }, + ] + + responses.add( + responses.GET, + f"https://example.gitlab.com/api/v4/projects/{self.repo.config['project_id']}/merge_requests/{self.pr.key}/diffs?unidiff=true", + status=200, + json=data, + ) + + pr_files = self.open_pr_comment_workflow.safe_for_comment(repo=self.repo, pr=self.pr) + + assert pr_files == [ + data[2], + data[3], + data[4], + data[5], + ] + + @responses.activate + def test_too_many_files(self): + files = OPEN_PR_MAX_FILES_CHANGED + 1 + + data = [ + { + "diff": DIFFS["simple_modification"], + "new_path": f"foo-{i}.py", + "old_path": f"foo-{i}.py", + "a_mode": "100644", + "b_mode": "100644", + "new_file": True, + "renamed_file": False, + "deleted_file": False, + "generated_file": False, + } + for i in range(files) + ] + + responses.add( + responses.GET, + f"https://example.gitlab.com/api/v4/projects/{self.repo.config['project_id']}/merge_requests/{self.pr.key}/diffs?unidiff=true", + status=200, + json=data, + ) + + pr_files = self.open_pr_comment_workflow.safe_for_comment(repo=self.repo, pr=self.pr) + + assert pr_files == [] + self.mock_integration_metrics.incr.assert_called_with( + "gitlab.open_pr_comment.rejected_comment", tags={"reason": "too_many_files"} + ) + + @responses.activate + def test_too_many_lines(self): + lines = OPEN_PR_MAX_LINES_CHANGED + 1 + + diff = f"""diff --git a/test.py b/test.py +index 0000001..0000002 100644 +--- a/test.py ++++ b/test.py +@@ -10,{lines} +10,{lines} @@""" + ( + """ +- print("Hello World") ++ print("Hello Universe")""" + * lines + ) + + data = [ + { + "diff": diff, + "new_path": "foo.py", + "old_path": "foo.py", + "a_mode": "100644", + "b_mode": "100644", + "new_file": False, + "renamed_file": False, + "deleted_file": False, + "generated_file": False, + }, + ] + + responses.add( + responses.GET, + f"https://example.gitlab.com/api/v4/projects/{self.repo.config['project_id']}/merge_requests/{self.pr.key}/diffs?unidiff=true", + status=200, + json=data, + ) + + pr_files = self.open_pr_comment_workflow.safe_for_comment(repo=self.repo, pr=self.pr) + + assert pr_files == [] + self.mock_integration_metrics.incr.assert_called_with( + "gitlab.open_pr_comment.rejected_comment", tags={"reason": "too_many_lines"} + ) + + @responses.activate + def test_error__missing_pr(self): + responses.add( + responses.GET, + f"https://example.gitlab.com/api/v4/projects/{self.repo.config['project_id']}/merge_requests/{self.pr.key}/diffs?unidiff=true", + status=404, + ) + + pr_files = self.open_pr_comment_workflow.safe_for_comment(repo=self.repo, pr=self.pr) + + assert pr_files == [] + self.mock_integration_metrics.incr.assert_called_with( + "gitlab.open_pr_comment.api_error", tags={"type": "missing_pr", "code": 404} + ) + + @responses.activate + def test_error__unknown_api_error(self): + responses.add( + responses.GET, + f"https://example.gitlab.com/api/v4/projects/{self.repo.config['project_id']}/merge_requests/{self.pr.key}/diffs?unidiff=true", + status=500, + ) + + pr_files = self.open_pr_comment_workflow.safe_for_comment(repo=self.repo, pr=self.pr) + + assert pr_files == [] + self.mock_integration_metrics.incr.assert_called_with( + "gitlab.open_pr_comment.api_error", tags={"type": "unknown_api_error", "code": 500} + ) + + +@patch( + "sentry.integrations.gitlab.integration.GitlabOpenPRCommentWorkflow.get_pr_files_safe_for_comment" +) +@patch( + "sentry.integrations.gitlab.integration.GitlabOpenPRCommentWorkflow.get_projects_and_filenames_from_source_file" +) +@patch( + "sentry.integrations.gitlab.integration.GitlabOpenPRCommentWorkflow.get_top_5_issues_by_count_for_file" +) +@patch( + "sentry.integrations.source_code_management.language_parsers.PythonParser.extract_functions_from_patch" +) +@patch("sentry.integrations.gitlab.integration.metrics") +@patch("sentry.integrations.source_code_management.tasks.metrics") +@patch("sentry.integrations.source_code_management.commit_context.metrics") +@patch("sentry.analytics.record") +class TestOpenPRCommentWorkflow(GitlabCommentTestCase): + def _create_event( + self, + culprit=None, + timestamp=None, + filenames=None, + function_names=None, + project_id=None, + user_id=None, + handled=False, + ): + if culprit is None: + culprit = "issue0" + if timestamp is None: + timestamp = before_now(seconds=5).isoformat() + if filenames is None: + filenames = ["foo.py", "baz.py"] + if function_names is None: + function_names = ["hello", "world"] + if project_id is None: + project_id = self.project.id + + assert len(function_names) == len(filenames) + + frames = [] + for i, filename in enumerate(filenames): + frames.append({"filename": filename, "function": function_names[i]}) + + return self.store_event( + data={ + "message": "hello!", + "culprit": culprit, + "platform": "python", + "timestamp": timestamp, + "exception": { + "values": [ + { + "type": "Error", + "stacktrace": { + "frames": frames, + }, + "mechanism": {"handled": handled, "type": "generic"}, + }, + ] + }, + "user": {"id": user_id}, + }, + project_id=project_id, + ) + + def setUp(self): + super().setUp() + self.pr = self.create_pr_issues() + + self.user_id = "user_1" + self.app_id = "app_1" + + self.group_id_1 = [self._create_event(culprit="issue1", user_id=str(i)) for i in range(5)][ + 0 + ].group.id + self.group_id_2 = [ + self._create_event( + culprit="issue2", + filenames=["foo.py", "bar.py"], + function_names=["blue", "planet"], + user_id=str(i), + ) + for i in range(6) + ][0].group.id + + self.groups = [ + { + "group_id": g.id, + "event_count": 1000 * (i + 1), + "function_name": "function_" + str(i), + } + for i, g in enumerate(Group.objects.all()) + ] + self.groups.reverse() + + @responses.activate + def test_comment_workflow( + self, + mock_analytics, + mock_commit_context_metrics, + mock_task_metrics, + mock_integration_metrics, + mock_extract_functions_from_patch, + mock_get_top_5_issues_by_count_for_file, + mock_get_projects_and_filenames_from_source_file, + mock_get_pr_files_safe_for_comment, + ): + # two filenames, the second one has a toggle table + mock_get_pr_files_safe_for_comment.return_value = [ + PullRequestFile(filename="foo.py", patch="a"), + PullRequestFile(filename="bar.py", patch="b"), + ] + mock_get_projects_and_filenames_from_source_file.return_value = ([self.project], ["foo.py"]) + mock_extract_functions_from_patch.return_value = ["world", "planet"] + + mock_get_top_5_issues_by_count_for_file.return_value = self.groups + + responses.add( + responses.POST, + f"https://example.gitlab.com/api/v4/projects/{self.repo.config['project_id']}/merge_requests/{self.pr.key}/notes", + json={"id": 1}, + ) + + open_pr_comment_workflow(self.pr.id) + + data = json.loads(responses.calls[0].request.body) + assert data == { + "body": f"""\ +## 🔍 Existing Issues For Review +Your merge request is modifying functions with the following pre-existing issues: + +📄 File: **foo.py** + +| Function | Unhandled Issue | +| :------- | :----- | +| **`function_3`** | [**Error**](http://testserver/organizations/baz/issues/{self.groups[0]['group_id']}/?referrer=gitlab-open-pr-bot) issue2
`Event Count:` **4k** | +| **`function_2`** | [**issue 2**](http://testserver/organizations/foobar/issues/{self.groups[1]['group_id']}/?referrer=gitlab-open-pr-bot) issue2
`Event Count:` **3k** | +| **`function_1`** | [**issue 1**](http://testserver/organizations/baz/issues/{self.groups[2]['group_id']}/?referrer=gitlab-open-pr-bot) issue1
`Event Count:` **2k** | +| **`function_0`** | [**Error**](http://testserver/organizations/baz/issues/{self.groups[3]['group_id']}/?referrer=gitlab-open-pr-bot) issue1
`Event Count:` **1k** | +
+📄 File: bar.py (Click to Expand) + +| Function | Unhandled Issue | +| :------- | :----- | +| **`function_3`** | [**Error**](http://testserver/organizations/baz/issues/{self.groups[0]['group_id']}/?referrer=gitlab-open-pr-bot) issue2
`Event Count:` **4k** | +| **`function_2`** | [**issue 2**](http://testserver/organizations/foobar/issues/{self.groups[1]['group_id']}/?referrer=gitlab-open-pr-bot) issue2
`Event Count:` **3k** | +| **`function_1`** | [**issue 1**](http://testserver/organizations/baz/issues/{self.groups[2]['group_id']}/?referrer=gitlab-open-pr-bot) issue1
`Event Count:` **2k** | +| **`function_0`** | [**Error**](http://testserver/organizations/baz/issues/{self.groups[3]['group_id']}/?referrer=gitlab-open-pr-bot) issue1
`Event Count:` **1k** | +
""" + } + + comment = PullRequestComment.objects.get() + assert comment.external_id == 1 + assert comment.comment_type == CommentType.OPEN_PR + + mock_commit_context_metrics.incr.assert_called_with( + "gitlab.open_pr_comment.comment_created" + ) + assert mock_task_metrics.mock_calls == [] + assert mock_integration_metrics.mock_calls == [] + mock_analytics.assert_any_call( + "open_pr_comment.created", + comment_id=comment.id, + org_id=self.organization.id, + pr_id=comment.pull_request.id, + language="python", + ) + + @responses.activate + def test_comment_workflow_comment_exists( + self, + mock_analytics, + mock_commit_context_metrics, + mock_task_metrics, + mock_integration_metrics, + mock_extract_functions_from_patch, + mock_get_top_5_issues_by_count_for_file, + mock_get_projects_and_filenames_from_source_file, + mock_get_pr_files_safe_for_comment, + ): + # two filenames, the second one has a toggle table + mock_get_pr_files_safe_for_comment.return_value = [ + PullRequestFile(filename="foo.py", patch="a"), + PullRequestFile(filename="bar.py", patch="b"), + ] + mock_get_projects_and_filenames_from_source_file.return_value = ([self.project], ["foo.py"]) + mock_extract_functions_from_patch.return_value = ["world", "planet"] + + mock_get_top_5_issues_by_count_for_file.return_value = self.groups + + now = timezone.now() + PullRequestComment.objects.create( + external_id=1, + pull_request=self.pr, + created_at=now, + updated_at=now, + group_ids=[0, 1], + comment_type=CommentType.OPEN_PR, + ) + + responses.add( + responses.PUT, + f"https://example.gitlab.com/api/v4/projects/{self.repo.config['project_id']}/merge_requests/{self.pr.key}/notes/1", + json={"id": 1}, + ) + + open_pr_comment_workflow(self.pr.id) + + comment = PullRequestComment.objects.get() + assert comment.external_id == 1 + assert comment.comment_type == CommentType.OPEN_PR + assert comment.created_at != comment.updated_at + + mock_commit_context_metrics.incr.assert_called_with( + "gitlab.open_pr_comment.comment_updated" + ) + assert mock_task_metrics.mock_calls == [] + assert mock_integration_metrics.mock_calls == [] + assert mock_analytics.mock_calls == [] + + @responses.activate + def test_comment_workflow_early_return( + self, + mock_analytics, + mock_commit_context_metrics, + mock_task_metrics, + mock_integration_metrics, + mock_extract_functions_from_patch, + mock_get_top_5_issues_by_count_for_file, + mock_get_projects_and_filenames_from_source_file, + mock_get_pr_files_safe_for_comment, + ): + # no python files + mock_get_pr_files_safe_for_comment.return_value = [] + + open_pr_comment_workflow(self.pr.id) + + comments = PullRequestComment.objects.all() + assert len(comments) == 0 + + assert mock_commit_context_metrics.mock_calls == [] + mock_task_metrics.incr.assert_called_with("gitlab.open_pr_comment.no_issues") + assert mock_integration_metrics.mock_calls == [] + assert mock_analytics.mock_calls == [] + + # no codemappings + mock_get_pr_files_safe_for_comment.return_value = [ + PullRequestFile(filename="foo.py", patch="a"), + PullRequestFile(filename="bar.py", patch="b"), + ] + mock_get_projects_and_filenames_from_source_file.return_value = ([], []) + + open_pr_comment_workflow(self.pr.id) + + comments = PullRequestComment.objects.all() + assert len(comments) == 0 + + assert mock_commit_context_metrics.mock_calls == [] + mock_task_metrics.incr.assert_called_with("gitlab.open_pr_comment.no_issues") + assert mock_integration_metrics.mock_calls == [] + assert mock_analytics.mock_calls == [] + + # has codemappings but no functions in diff + mock_get_projects_and_filenames_from_source_file.return_value = ([self.project], ["foo.py"]) + mock_extract_functions_from_patch.return_value = [] + + open_pr_comment_workflow(self.pr.id) + + comments = PullRequestComment.objects.all() + assert len(comments) == 0 + + assert mock_commit_context_metrics.mock_calls == [] + mock_task_metrics.incr.assert_called_with("gitlab.open_pr_comment.no_issues") + assert mock_integration_metrics.mock_calls == [] + assert mock_analytics.mock_calls == [] + + # has codemappings and functions but no issues + mock_extract_functions_from_patch.return_value = ["world"] + + open_pr_comment_workflow(self.pr.id) + + comments = PullRequestComment.objects.all() + assert len(comments) == 0 + + assert mock_commit_context_metrics.mock_calls == [] + mock_task_metrics.incr.assert_called_with("gitlab.open_pr_comment.no_issues") + assert mock_integration_metrics.mock_calls == [] + assert mock_analytics.mock_calls == [] diff --git a/tests/sentry/integrations/gitlab/tasks/test_pr_comment.py b/tests/sentry/integrations/gitlab/tasks/test_pr_comment.py index 6c68a525a33dbe..52afa01b4ed42e 100644 --- a/tests/sentry/integrations/gitlab/tasks/test_pr_comment.py +++ b/tests/sentry/integrations/gitlab/tasks/test_pr_comment.py @@ -1,5 +1,6 @@ import logging from datetime import UTC, datetime, timedelta +from typing import cast from unittest.mock import patch import pytest @@ -7,7 +8,7 @@ from django.utils import timezone from fixtures.gitlab import GitLabTestCase -from sentry.integrations.gitlab.integration import GitlabIntegration +from sentry.integrations.gitlab.integration import GitlabIntegration, GitlabOpenPRCommentWorkflow from sentry.integrations.source_code_management.tasks import pr_comment_workflow from sentry.models.commit import Commit from sentry.models.group import Group @@ -34,6 +35,9 @@ def setUp(self): GitlabIntegration, integration=self.integration, org_id=self.organization.id ) self.pr_comment_workflow = self.installation.get_pr_comment_workflow() + self.open_pr_comment_workflow = cast( + GitlabOpenPRCommentWorkflow, self.installation.get_open_pr_comment_workflow() + ) self.another_integration = self.create_integration( organization=self.organization, external_id="1", provider="github" ) diff --git a/tests/sentry/integrations/gitlab/test_webhook.py b/tests/sentry/integrations/gitlab/test_webhook.py index 667d6681751fc5..bfbaca98457fe9 100644 --- a/tests/sentry/integrations/gitlab/test_webhook.py +++ b/tests/sentry/integrations/gitlab/test_webhook.py @@ -335,7 +335,8 @@ def test_merge_event_no_last_commit(self, mock_record): assert_success_metric(mock_record) - def test_merge_event_create_pull_request(self): + @patch("sentry.integrations.source_code_management.tasks.open_pr_comment_workflow.delay") + def test_merge_event_create_pull_request(self, mock_delay): self.create_repo("getsentry/sentry") group = self.create_group(project=self.project, short_id=9) response = self.client.post( @@ -353,7 +354,10 @@ def test_merge_event_create_pull_request(self): self.assert_pull_request(pull, author) self.assert_group_link(group, pull) - def test_merge_event_update_pull_request(self): + mock_delay.assert_called_once_with(pr_id=pull.id) + + @patch("sentry.integrations.source_code_management.tasks.open_pr_comment_workflow.delay") + def test_merge_event_update_pull_request(self, mock_delay): repo = self.create_repo("getsentry/sentry") group = self.create_group(project=self.project, short_id=9) PullRequest.objects.create( @@ -382,6 +386,8 @@ def test_merge_event_update_pull_request(self): self.assert_pull_request(pull, author) self.assert_group_link(group, pull) + assert mock_delay.call_count == 0 + def test_update_repo_path(self): repo_out_of_date_path = self.create_repo( name="Cool Group / Sentry", url="http://example.com/cool-group/sentry" diff --git a/tests/sentry/utils/test_patch_set.py b/tests/sentry/utils/test_patch_set.py index 9dd352a47f6c43..e00ce0a933d0d3 100644 --- a/tests/sentry/utils/test_patch_set.py +++ b/tests/sentry/utils/test_patch_set.py @@ -1,4 +1,11 @@ -from sentry.utils.patch_set import patch_to_file_changes +import pytest + +from sentry.utils.patch_set import ( + FileModification, + FileModifications, + patch_to_file_changes, + patch_to_file_modifications, +) def test_filename_containing_spaces(): @@ -14,3 +21,142 @@ def test_filename_containing_spaces(): """ expected = [{"path": "has spaces/t.sql", "type": "A"}] assert patch_to_file_changes(patch) == expected + + +@pytest.mark.parametrize( + "diff_text, expected", + [ + # Test 1: Pure addition + ( + """@@ -0,0 +1,3 @@ ++def hello(): ++ print("Hello") ++ return True +""", + FileModifications( + added=[ + FileModification( + path="test.py", + lines_added=3, + lines_removed=0, + lines_modified=0, + ) + ], + removed=[], + modified=[], + ), + ), + # Test 2: Pure deletion + ( + """@@ -5,3 +0,0 @@ +-def goodbye(): +- print("Goodbye") +- return False +""", + FileModifications( + added=[], + removed=[ + FileModification( + path="test.py", + lines_added=0, + lines_removed=3, + lines_modified=0, + ) + ], + modified=[], + ), + ), + # Test 3: Simple modification + ( + """@@ -10,1 +10,1 @@ +- print("Hello World") ++ print("Hello Universe") +""", + FileModifications( + added=[], + removed=[], + modified=[ + FileModification( + path="test.py", + lines_added=0, + lines_removed=0, + lines_modified=1, + ) + ], + ), + ), + # Test 4: Add + Remove not as modification + ( + """@@ -8,0 +9,1 @@ ++ print("Extra logging") +@@ -10,1 +11,0 @@ +- print("Old debug") +""", + FileModifications( + added=[], + removed=[], + modified=[ + FileModification( + path="test.py", + lines_added=1, + lines_removed=1, + lines_modified=0, + ) + ], + ), + ), + # Test 5: Mixed case + ( + """@@ -4,1 +4,1 @@ +- name = "OldName" ++ name = "NewName" +@@ -6,0 +7,2 @@ ++ age = 30 ++ country = "UK" +@@ -10,1 +12,0 @@ +- unused_variable = None +""", + FileModifications( + added=[], + removed=[], + modified=[ + FileModification( + path="test.py", + lines_added=2, + lines_removed=1, + lines_modified=1, + ) + ], + ), + ), + # Test 6: Consecutive modifications + ( + """@@ -20,2 +20,2 @@ +- foo = 1 ++ foo = 10 +- bar = 2 ++ bar = 20 +""", + FileModifications( + added=[], + removed=[], + modified=[ + FileModification( + path="test.py", + lines_added=0, + lines_removed=0, + lines_modified=2, + ) + ], + ), + ), + ], +) +def test_diff_line_counts(diff_text, expected): + patch = f"""diff --git a/test.py b/test.py +index 0000001..0000002 100644 +--- a/test.py ++++ b/test.py +{diff_text} +""" + assert patch_to_file_modifications(patch) == expected