From 8d48f2be77b0b221bd130eb254b53278bbdbac32 Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Tue, 16 Jul 2024 10:53:34 -0700 Subject: [PATCH] option to set limit for number of issues returned --- .../organization_pull_request_file_issues.py | 17 +++++++-- .../integrations/github/open_pr_comment.py | 11 ++++-- ...t_organization_pull_request_file_issues.py | 38 +++++++++++++++++++ .../github/test_open_pr_comment.py | 34 ++++++++--------- 4 files changed, 76 insertions(+), 24 deletions(-) diff --git a/src/sentry/api/endpoints/organization_pull_request_file_issues.py b/src/sentry/api/endpoints/organization_pull_request_file_issues.py index 27225c64f646b..f2b385e9114dc 100644 --- a/src/sentry/api/endpoints/organization_pull_request_file_issues.py +++ b/src/sentry/api/endpoints/organization_pull_request_file_issues.py @@ -14,7 +14,7 @@ from sentry.tasks.integrations.github.language_parsers import PATCH_PARSERS from sentry.tasks.integrations.github.open_pr_comment import ( get_projects_and_filenames_from_source_file, - get_top_5_issues_by_count_for_file, + get_top_issues_by_count_for_file, ) @@ -22,6 +22,7 @@ class PullRequestFileSerializer(serializers.Serializer): filename = serializers.CharField(required=True) repo = serializers.CharField(required=True) patch = serializers.CharField(required=True) + limit = serializers.IntegerField(required=False, default=5) def validate_filename(self, value): if not value: @@ -34,6 +35,12 @@ def validate_filename(self, value): return value + def validate_limit(self, value): + if value and value < 1 or value > 100: + raise serializers.ValidationError("Issue count must be between 1 and 100") + + return value + @region_silo_endpoint class OrganizationPullRequestFilesIssuesEndpoint(OrganizationEndpoint): @@ -50,6 +57,7 @@ def post(self, request: Request, organization: Organization) -> Response: filename = serializer.validated_data["filename"] repo_name = serializer.validated_data["repo"] patch = serializer.validated_data["patch"] + limit = serializer.validated_data["limit"] projects, sentry_filenames = get_projects_and_filenames_from_source_file( org_id=organization.id, repo_name=repo_name, pr_filename=filename @@ -66,8 +74,11 @@ def post(self, request: Request, organization: Organization) -> Response: if not len(function_names): return Response([]) - top_issues = get_top_5_issues_by_count_for_file( - list(projects), list(sentry_filenames), list(function_names) + top_issues = get_top_issues_by_count_for_file( + projects=list(projects), + sentry_filenames=list(sentry_filenames), + function_names=list(function_names), + limit=limit, ) group_id_to_info = {} diff --git a/src/sentry/tasks/integrations/github/open_pr_comment.py b/src/sentry/tasks/integrations/github/open_pr_comment.py index e4a363d841ae4..2b7ac0afc838a 100644 --- a/src/sentry/tasks/integrations/github/open_pr_comment.py +++ b/src/sentry/tasks/integrations/github/open_pr_comment.py @@ -280,8 +280,11 @@ def get_projects_and_filenames_from_source_file( return project_list, sentry_filenames -def get_top_5_issues_by_count_for_file( - projects: list[Project], sentry_filenames: list[str], function_names: list[str] +def get_top_issues_by_count_for_file( + projects: list[Project], + sentry_filenames: list[str], + function_names: list[str], + limit: int | None = 5, ) -> list[dict[str, Any]]: """ Given a list of projects, Github filenames reverse-codemapped into filenames in Sentry, @@ -401,7 +404,7 @@ def get_top_5_issues_by_count_for_file( ] ) .set_orderby([OrderBy(Column("event_count"), Direction.DESC)]) - .set_limit(5) + .set_limit(limit) ) request = SnubaRequest( @@ -554,7 +557,7 @@ def open_pr_comment_workflow(pr_id: int) -> None: if not len(function_names): continue - top_issues = get_top_5_issues_by_count_for_file( + top_issues = get_top_issues_by_count_for_file( list(projects), list(sentry_filenames), list(function_names) ) if not len(top_issues): diff --git a/tests/sentry/api/endpoints/test_organization_pull_request_file_issues.py b/tests/sentry/api/endpoints/test_organization_pull_request_file_issues.py index 58874aae1c4ae..a1866603e9b53 100644 --- a/tests/sentry/api/endpoints/test_organization_pull_request_file_issues.py +++ b/tests/sentry/api/endpoints/test_organization_pull_request_file_issues.py @@ -1,5 +1,7 @@ from unittest.mock import patch +from rest_framework import status + from sentry.models.group import Group from sentry.testutils.cases import APITestCase from tests.sentry.tasks.integrations.github.test_open_pr_comment import CreateEventTestCase @@ -69,6 +71,42 @@ def test_simple(self, mock_reverse_codemappings): f"http://testserver/organizations/{self.organization.slug}/issues/{self.group_id_1}/", ] + def test_limit_validation(self, mock_reverse_codemappings): + mock_reverse_codemappings.return_value = ([self.project], ["foo.py"]) + + patch = """@@ -36,6 +36,7 @@\n def blue(self):""" + self.get_error_response( + self.organization.slug, + **{ + "filename": "foo.py", + "repo": self.gh_repo.name, + "patch": patch, + "limit": -1, + }, + status_code=status.HTTP_400_BAD_REQUEST, + ) + + self.get_error_response( + self.organization.slug, + **{ + "filename": "foo.py", + "repo": self.gh_repo.name, + "patch": patch, + "limit": 101, + }, + status_code=status.HTTP_400_BAD_REQUEST, + ) + + self.get_success_response( + self.organization.slug, + **{ + "filename": "foo.py", + "repo": self.gh_repo.name, + "patch": patch, + "limit": 100, + }, + ) + def test_no_codemappings(self, mock_reverse_codemappings): mock_reverse_codemappings.return_value = ([], []) diff --git a/tests/sentry/tasks/integrations/github/test_open_pr_comment.py b/tests/sentry/tasks/integrations/github/test_open_pr_comment.py index a0d10eac86755..3dda68d8112d7 100644 --- a/tests/sentry/tasks/integrations/github/test_open_pr_comment.py +++ b/tests/sentry/tasks/integrations/github/test_open_pr_comment.py @@ -16,7 +16,7 @@ get_issue_table_contents, get_pr_files, get_projects_and_filenames_from_source_file, - get_top_5_issues_by_count_for_file, + get_top_issues_by_count_for_file, open_pr_comment_workflow, safe_for_comment, ) @@ -367,7 +367,7 @@ def test_simple(self): group_id = [ self._create_event(function_names=["blue", "planet"], user_id=str(i)) for i in range(7) ][0].group.id - top_5_issues = get_top_5_issues_by_count_for_file( + top_5_issues = get_top_issues_by_count_for_file( [self.project], ["baz.py"], ["world", "planet"] ) @@ -394,7 +394,7 @@ def test_javascript_simple(self): ) for i in range(6) ][0].group.id - top_5_issues = get_top_5_issues_by_count_for_file( + top_5_issues = get_top_issues_by_count_for_file( [self.project], ["baz.js"], ["world", "planet"] ) top_5_issue_ids = [issue["group_id"] for issue in top_5_issues] @@ -420,7 +420,7 @@ def test_php_simple(self): ) for i in range(6) ][0].group.id - top_5_issues = get_top_5_issues_by_count_for_file( + top_5_issues = get_top_issues_by_count_for_file( [self.project], ["baz.php"], ["world", "planet"] ) top_5_issue_ids = [issue["group_id"] for issue in top_5_issues] @@ -446,7 +446,7 @@ def test_ruby_simple(self): ) for i in range(6) ][0].group.id - top_5_issues = get_top_5_issues_by_count_for_file( + top_5_issues = get_top_issues_by_count_for_file( [self.project], ["baz.rb"], ["world", "planet"] ) top_5_issue_ids = [issue["group_id"] for issue in top_5_issues] @@ -460,13 +460,13 @@ def test_filters_resolved_issue(self): group.status = GroupStatus.RESOLVED group.save() - top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) + top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) assert len(top_5_issues) == 0 def test_filters_handled_issue(self): group_id = self._create_event(filenames=["bar.py", "baz.py"], handled=True).group.id - top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) + top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) top_5_issue_ids = [issue["group_id"] for issue in top_5_issues] assert group_id != self.group_id assert top_5_issue_ids == [self.group_id] @@ -475,7 +475,7 @@ def test_project_group_id_mismatch(self): # we fetch all group_ids that belong to the projects passed into the function self._create_event(project_id=self.another_org_project.id) - top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) + top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) top_5_issue_ids = [issue["group_id"] for issue in top_5_issues] assert top_5_issue_ids == [self.group_id] @@ -484,7 +484,7 @@ def test_filename_mismatch(self): filenames=["foo.py", "bar.py"], ).group.id - top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) + top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) top_5_issue_ids = [issue["group_id"] for issue in top_5_issues] assert group_id != self.group_id assert top_5_issue_ids == [self.group_id] @@ -494,7 +494,7 @@ def test_function_name_mismatch(self): function_names=["world", "hello"], ).group.id - top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) + top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) top_5_issue_ids = [issue["group_id"] for issue in top_5_issues] assert group_id != self.group_id assert top_5_issue_ids == [self.group_id] @@ -504,7 +504,7 @@ def test_not_first_frame(self): function_names=["world", "hello"], filenames=["baz.py", "bar.py"], culprit="hi" ).group.id - top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) + top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) top_5_issue_ids = [issue["group_id"] for issue in top_5_issues] function_names = [issue["function_name"] for issue in top_5_issues] assert group_id != self.group_id @@ -516,7 +516,7 @@ def test_not_within_frame_limit(self): filenames = ["baz.py"] + ["foo.py" for _ in range(STACKFRAME_COUNT)] group_id = self._create_event(function_names=function_names, filenames=filenames).group.id - top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) + top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) top_5_issue_ids = [issue["group_id"] for issue in top_5_issues] assert group_id != self.group_id assert top_5_issue_ids == [self.group_id] @@ -526,7 +526,7 @@ def test_event_too_old(self): timestamp=iso_format(before_now(days=15)), filenames=["bar.py", "baz.py"] ).group.id - top_5_issues = get_top_5_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) + top_5_issues = get_top_issues_by_count_for_file([self.project], ["baz.py"], ["world"]) top_5_issue_ids = [issue["group_id"] for issue in top_5_issues] assert group_id != self.group_id assert top_5_issue_ids == [self.group_id] @@ -553,7 +553,7 @@ def test_squashes_same_title_culprit_issues(self): for i in range(5) ][0].group_id - top_5_issues = get_top_5_issues_by_count_for_file( + top_5_issues = get_top_issues_by_count_for_file( [self.project], ["baz.py"], ["world", "planet"] ) top_5_issue_ids = [issue["group_id"] for issue in top_5_issues] @@ -607,7 +607,7 @@ def test_fetches_top_five_issues(self): # unrelated issue with same stack trace in different project self._create_event(project_id=self.another_org_project.id) - top_5_issues = get_top_5_issues_by_count_for_file( + top_5_issues = get_top_issues_by_count_for_file( [self.project], ["baz.py"], ["world", "planet"] ) top_5_issue_ids = [issue["group_id"] for issue in top_5_issues] @@ -659,7 +659,7 @@ def test_get_issue_table_contents(self): for i in range(2) ][0].group.id - top_5_issues = get_top_5_issues_by_count_for_file( + top_5_issues = get_top_issues_by_count_for_file( [self.project], ["baz.py"], ["world", "planet"] ) affected_users = [6, 5, 4, 3, 2] @@ -821,7 +821,7 @@ def test_comment_format_missing_language(self): @patch( "sentry.tasks.integrations.github.language_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.get_top_issues_by_count_for_file") @patch("sentry.tasks.integrations.github.open_pr_comment.safe_for_comment") @patch("sentry.tasks.integrations.github.utils.metrics") class TestOpenPRCommentWorkflow(IntegrationTestCase, CreateEventTestCase):