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

fix: a couple of rest api unhandled errors #1139

Merged
merged 2 commits into from
Feb 10, 2025
Merged
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
4 changes: 4 additions & 0 deletions api/public/v1/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,7 @@ class Meta:
"state",
)
fields = read_only_fields + ("user_provided_base_sha",)


class PullIdSerializer(serializers.Serializer):
pullid = serializers.IntegerField()
7 changes: 7 additions & 0 deletions api/public/v1/tests/views/test_pull_viewset.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,10 @@ def test_post_pull_user_provided_base(self, pulls_sync_mock):
)
self.assertEqual(response.status_code, 405)
assert not pulls_sync_mock.called

def test_get_pull_no_pullid_provided(self):
self.client.credentials(HTTP_AUTHORIZATION="Token " + self.repo.upload_token)
response = self.client.get("/api/github/codecov/testRepoName/pulls/abc")
self.assertEqual(response.status_code, 400)
content = json.loads(response.content.decode())
self.assertEqual(content["pullid"], ["A valid integer is required."])
17 changes: 10 additions & 7 deletions api/public/v1/views.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
import logging

from django.db.models import OuterRef, Subquery
from django.db.models import OuterRef, QuerySet, Subquery
from django.shortcuts import get_object_or_404
from django_filters.rest_framework import DjangoFilterBackend
from rest_framework import filters, mixins, viewsets

from api.shared.mixins import RepoPropertyMixin
from codecov_auth.authentication.repo_auth import RepositoryLegacyTokenAuthentication
from core.models import Commit
from core.models import Commit, Pull
from services.task import TaskService

from .permissions import PullUpdatePermission
from .serializers import PullSerializer
from .serializers import PullIdSerializer, PullSerializer

log = logging.getLogger(__name__)

Expand All @@ -30,8 +30,11 @@ class PullViewSet(
authentication_classes = [RepositoryLegacyTokenAuthentication]
permission_classes = [PullUpdatePermission]

def get_object(self):
pullid = self.kwargs.get("pk")
def get_object(self) -> Pull:
serializer = PullIdSerializer(data={"pullid": self.kwargs.get("pk")})
serializer.is_valid(raise_exception=True)
pullid = serializer.validated_data["pullid"]

if self.request.method == "PUT":
# Note: We create a new pull if needed to make sure that they can be updated
# with a base before the upload has finished processing.
Expand All @@ -41,7 +44,7 @@ def get_object(self):
return obj
return get_object_or_404(self.get_queryset(), pullid=pullid)

def get_queryset(self):
def get_queryset(self) -> QuerySet:
return self.repo.pull_requests.annotate(
ci_passed=Subquery(
Commit.objects.filter(
Expand All @@ -50,7 +53,7 @@ def get_queryset(self):
)
)

def perform_update(self, serializer):
def perform_update(self, serializer: PullSerializer) -> Pull:
result = super().perform_update(serializer)
TaskService().pulls_sync(repoid=self.repo.repoid, pullid=self.kwargs.get("pk"))
return result
9 changes: 5 additions & 4 deletions api/public/v2/compare/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ class ComparisonSerializer(BaseComparisonSerializer):

def get_files(self, comparison: Comparison) -> List[dict]:
data = []
for filename in comparison.head_report.files:
file = comparison.get_file_comparison(filename, bypass_max_diff=True)
if self._should_include_file(file):
data.append(FileComparisonSerializer(file).data)
if comparison.head_report is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if we should raise a validation indicating no head report for comparison?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the event it falls out of the if I mean

for filename in comparison.head_report.files:
file = comparison.get_file_comparison(filename, bypass_max_diff=True)
if self._should_include_file(file):
data.append(FileComparisonSerializer(file).data)
return data


Expand Down
Loading