Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(scm): pull request file issues API #74352

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 106 additions & 0 deletions src/sentry/api/endpoints/organization_pull_request_file_issues.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
from __future__ import annotations

from rest_framework import serializers, status
from rest_framework.request import Request
from rest_framework.response import Response

from sentry.api.api_owners import ApiOwner
from sentry.api.api_publish_status import ApiPublishStatus
from sentry.api.base import region_silo_endpoint
from sentry.api.bases.organization import OrganizationEndpoint
from sentry.models.group import Group
from sentry.models.organization import Organization
from sentry.snuba.referrer import Referrer
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_issues_by_count_for_file,
)


class PullRequestFileSerializer(serializers.Serializer):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this gets related to a PR? Is it by patch? Or I can pass any file and get issues for it regardless of PR?

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:
raise serializers.ValidationError("Filename is required")

file_extension = value.split(".")[-1]
language_parser = PATCH_PARSERS.get(file_extension, None)
if not language_parser:
raise serializers.ValidationError("Unsupported file type")

return value

def validate_limit(self, value):
if value and value < 1 or value > 100:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be if value is not None? It looks like this won't raise an error on an int value of 0.

Also, nit: value < 1 or value > 100 can be written as not (1 <= value <= 100).

raise serializers.ValidationError("Issue count must be between 1 and 100")

return value


@region_silo_endpoint
class OrganizationPullRequestFilesIssuesEndpoint(OrganizationEndpoint):
owner = ApiOwner.ECOSYSTEM
publish_status = {
"POST": ApiPublishStatus.EXPERIMENTAL,
}

def post(self, request: Request, organization: Organization) -> Response:
serializer = PullRequestFileSerializer(data=request.data)
if not serializer.is_valid():
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)

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
)

if not len(projects) or not len(sentry_filenames):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: empty collections are falsy, so you don't need len. Just if not projects or not sentry_filenames will work.

I'd also consider writing if not (projects and sentry_filenames), but that's a matter of taste.

return Response([])

file_extension = filename.split(".")[-1]
language_parser = PATCH_PARSERS[file_extension]

function_names = language_parser.extract_functions_from_patch(patch)

if not len(function_names):
return Response([])

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 = {}
for issue in top_issues:
group_id = issue["group_id"]
group_id_to_info[group_id] = dict(filter(lambda k: k[0] != "group_id", issue.items()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
group_id_to_info[group_id] = dict(filter(lambda k: k[0] != "group_id", issue.items()))
group_id_to_info[group_id] = {k: v for (k, v) in issue.items() if k != "group_id"}


issues = Group.objects.filter(id__in=list(group_id_to_info.keys())).all()

pr_file_issues = [
{
"title": issue.title,
"culprit": issue.culprit,
"url": issue.get_absolute_url(),
"affected_users": issue.count_users_seen(
referrer=Referrer.TAGSTORE_GET_GROUPS_USER_COUNTS_OPEN_PR_COMMENT.value
),
"event_count": group_id_to_info[issue.id]["event_count"],
"function_name": group_id_to_info[issue.id]["function_name"],
}
for issue in issues
]
pr_file_issues.sort(key=lambda k: k.get("event_count", 0), reverse=True)

return Response(pr_file_issues)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - if the end goal is for others to use this api, i would use a serializer here to return to make life less painful when ur documenting the endpoint. or u can just kick the can when u get there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a serializer everywhere actually even if it's not going to be publicly documented. It's more typed and readable.

8 changes: 8 additions & 0 deletions src/sentry/api/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
from sentry.api.endpoints.organization_projects_experiment import (
OrganizationProjectsExperimentEndpoint,
)
from sentry.api.endpoints.organization_pull_request_file_issues import (
OrganizationPullRequestFilesIssuesEndpoint,
)
from sentry.api.endpoints.organization_spans_aggregation import OrganizationSpansAggregationEndpoint
from sentry.api.endpoints.organization_stats_summary import OrganizationStatsSummaryEndpoint
from sentry.api.endpoints.organization_unsubscribe import (
Expand Down Expand Up @@ -1861,6 +1864,11 @@ def create_group_urls(name_prefix: str) -> list[URLPattern | URLResolver]:
OrganizationPluginsConfigsEndpoint.as_view(),
name="sentry-api-0-organization-plugins-configs",
),
re_path(
r"^(?P<organization_id_or_slug>[^\/]+)/pr-file-issues/$",
OrganizationPullRequestFilesIssuesEndpoint.as_view(),
name="sentry-api-0-organization-pr-file-issues",
),
re_path(
r"^(?P<organization_id_or_slug>[^\/]+)/releases/$",
OrganizationReleasesEndpoint.as_view(),
Expand Down
Loading
Loading