From d58c40e26a571c7593b1e2c464258ef919553ea4 Mon Sep 17 00:00:00 2001 From: Tony Le Date: Tue, 5 Nov 2024 18:16:39 -0500 Subject: [PATCH 01/18] Initial commit --- upload/views/combined_upload.py | 126 ++++++++++++++++++++++++++++++++ upload/views/commits.py | 13 +++- upload/views/reports.py | 32 ++++---- upload/views/uploads.py | 117 +++++++++++++++-------------- 4 files changed, 215 insertions(+), 73 deletions(-) create mode 100644 upload/views/combined_upload.py diff --git a/upload/views/combined_upload.py b/upload/views/combined_upload.py new file mode 100644 index 0000000000..33e75e8a7e --- /dev/null +++ b/upload/views/combined_upload.py @@ -0,0 +1,126 @@ +import logging + +from rest_framework import status +from rest_framework.generics import ListCreateAPIView +from rest_framework.response import Response + +from codecov_auth.authentication.repo_auth import ( + GitHubOIDCTokenAuthentication, + GlobalTokenAuthentication, + OrgLevelTokenAuthentication, + RepositoryLegacyTokenAuthentication, + TokenlessAuthentication, + UploadTokenRequiredAuthenticationCheck, + repo_auth_custom_exception_handler, +) +from upload.serializers import ( + CommitReportSerializer, + CommitSerializer, + UploadSerializer, +) +from upload.throttles import UploadsPerCommitThrottle, UploadsPerWindowThrottle +from upload.views.commits import CommitLogicMixin +from upload.views.reports import ReportLogicMixin +from upload.views.uploads import CanDoCoverageUploadsPermission, UploadLogicMixin + +log = logging.getLogger(__name__) + + +class CombinedUploadMixin(CommitLogicMixin, ReportLogicMixin, UploadLogicMixin): + pass + + +class CombinedUploadView(ListCreateAPIView, CombinedUploadMixin): + permission_classes = [CanDoCoverageUploadsPermission] + authentication_classes = [ + UploadTokenRequiredAuthenticationCheck, + GlobalTokenAuthentication, + OrgLevelTokenAuthentication, + GitHubOIDCTokenAuthentication, + RepositoryLegacyTokenAuthentication, + TokenlessAuthentication, + ] + throttle_classes = [UploadsPerCommitThrottle, UploadsPerWindowThrottle] + + def get_exception_handler(self): + return repo_auth_custom_exception_handler + + def create(self, request, *args, **kwargs): + # Create commit + create_commit_data = dict( + branch=request.data.get("branch"), + commit_sha=request.data.get("commit_sha"), + fail_on_error=True, + git_service=request.data.get("git_service"), + parent_sha=request.data.get("parent_sha"), + pull_request_number=request.data.get("pull_request_number"), + slug=request.data.get("slug"), + token=request.data.get("token"), + ) + commit_serializer = CommitSerializer(data=create_commit_data) + commit_serializer.is_valid(raise_exception=True) + + repository = self.get_repo() + commit = self.create_commit(commit_serializer, repository) + + # Create report + commit_report_data = dict( + code=request.data.get("code"), + commit_sha=request.data.get("commit_sha"), + fail_on_error=True, + git_service=request.data.get("git_service"), + slug=request.data.get("slug"), + token=request.data.get("token"), + ) + commit_report_serializer = CommitReportSerializer(data=commit_report_data) + report = self.create_report(commit_report_serializer, repository, commit) + + # Do upload + upload_data = dict( + branch=request.data.get("branch"), + build_code=request.data.get("build_code"), + build_url=request.data.get("build_url"), + commit_sha=request.data.get("commit_sha"), + disable_file_fixes=request.data.get("disable_file_fixes"), + disable_search=request.data.get("disable_search"), + dry_run=request.data.get("dry_run"), + env_vars=request.data.get("env_vars"), + fail_on_error=request.data.get("fail_on_error"), + files_search_exclude_folders=request.data.get( + "files_search_exclude_folders" + ), + files_search_explicitly_listed_files=request.data.get( + "files_search_explicitly_listed_files" + ), + files_search_root_folder=request.data.get("files_search_root_folder"), + flags=request.data.get("flags"), + gcov_args=request.data.get("gcov_args"), + gcov_executable=request.data.get("gcov_executable"), + gcov_ignore=request.data.get("gcov_ignore"), + gcov_include=request.data.get("gcov_include"), + git_service=request.data.get("git_service"), + handle_no_reports_found=request.data.get("handle_no_reports_found"), + job_code=request.data.get("job_code"), + name=request.data.get("name"), + network_filter=request.data.get("network_filter"), + network_prefix=request.data.get("network_prefix"), + network_root_folder=request.data.get("network_root_folder"), + plugin_names=request.data.get("plugin_names"), + pull_request_number=request.data.get("pull_request_number"), + report_code=report.code, + report_type=request.data.get("report_type"), + slug=request.data.get("slug"), + swift_project=request.data.get("swift_project"), + token=request.data.get("token"), + use_legacy_uploader=request.data.get("use_legacy_uploader"), + ) + upload_serializer = UploadSerializer(data=upload_data) + upload_serializer.is_valid(raise_exception=True) + + instance = self.create_upload(upload_serializer, repository, commit, report) + if instance: + return Response(data=instance.data, status=status.HTTP_201_CREATED) + else: + return Response( + upload_serializer.errors, status=status.HTTP_400_BAD_REQUEST + ) diff --git a/upload/views/commits.py b/upload/views/commits.py index 71ace788ed..2bc251f475 100644 --- a/upload/views/commits.py +++ b/upload/views/commits.py @@ -26,7 +26,14 @@ log = logging.getLogger(__name__) -class CommitViews(ListCreateAPIView, GetterMixin): +class CommitLogicMixin(GetterMixin): + def create_commit(self, serializer, repository): + validate_activated_repo(repository) + commit = serializer.save(repository=repository) + return commit + + +class CommitViews(ListCreateAPIView, CommitLogicMixin): serializer_class = CommitSerializer permission_classes = [CanDoCoverageUploadsPermission] authentication_classes = [ @@ -68,9 +75,7 @@ def perform_create(self, serializer): ), ) repository = self.get_repo() - validate_activated_repo(repository) - - commit = serializer.save(repository=repository) + commit = self.create_commit(serializer, repository) log.info( "Request to create new commit", diff --git a/upload/views/reports.py b/upload/views/reports.py index 9ee4cd6d62..d8380ca77b 100644 --- a/upload/views/reports.py +++ b/upload/views/reports.py @@ -18,7 +18,6 @@ from services.task import TaskService from upload.helpers import ( generate_upload_prometheus_metrics_labels, - validate_activated_repo, ) from upload.metrics import API_UPLOAD_COUNTER from upload.serializers import CommitReportSerializer, ReportResultsSerializer @@ -28,7 +27,23 @@ log = logging.getLogger(__name__) -class ReportViews(ListCreateAPIView, GetterMixin): +class ReportLogicMixin(GetterMixin): + def create_report(self, serializer, repository, commit): + code = serializer.validated_data.get("code") + if code == "default": + serializer.validated_data["code"] = None + instance, was_created = serializer.save( + commit_id=commit.id, + report_type=CommitReport.ReportType.COVERAGE, + ) + if was_created: + TaskService().preprocess_upload( + repository.repoid, commit.commitid, instance.code + ) + return instance + + +class ReportViews(ListCreateAPIView, ReportLogicMixin): serializer_class = CommitReportSerializer permission_classes = [CanDoCoverageUploadsPermission] authentication_classes = [ @@ -55,23 +70,12 @@ def perform_create(self, serializer): ), ) repository = self.get_repo() - validate_activated_repo(repository) commit = self.get_commit(repository) log.info( "Request to create new report", extra=dict(repo=repository.name, commit=commit.commitid), ) - code = serializer.validated_data.get("code") - if code == "default": - serializer.validated_data["code"] = None - instance, was_created = serializer.save( - commit_id=commit.id, - report_type=CommitReport.ReportType.COVERAGE, - ) - if was_created: - TaskService().preprocess_upload( - repository.repoid, commit.commitid, instance.code - ) + instance = self.create_report(serializer, repository, commit) inc_counter( API_UPLOAD_COUNTER, diff --git a/upload/views/uploads.py b/upload/views/uploads.py index 4157b57d82..8697f8f4b8 100644 --- a/upload/views/uploads.py +++ b/upload/views/uploads.py @@ -49,40 +49,8 @@ def has_permission(self, request, view): ) -class UploadViews(ListCreateAPIView, GetterMixin): - serializer_class = UploadSerializer - permission_classes = [ - CanDoCoverageUploadsPermission, - ] - authentication_classes = [ - UploadTokenRequiredAuthenticationCheck, - GlobalTokenAuthentication, - OrgLevelTokenAuthentication, - GitHubOIDCTokenAuthentication, - RepositoryLegacyTokenAuthentication, - TokenlessAuthentication, - ] - throttle_classes = [UploadsPerCommitThrottle, UploadsPerWindowThrottle] - - def get_exception_handler(self): - return repo_auth_custom_exception_handler - - def perform_create(self, serializer: UploadSerializer): - inc_counter( - API_UPLOAD_COUNTER, - labels=generate_upload_prometheus_metrics_labels( - action="coverage", - endpoint="create_upload", - request=self.request, - is_shelter_request=self.is_shelter_request(), - position="start", - ), - ) - repository: Repository = self.get_repo() - validate_activated_repo(repository) - commit: Commit = self.get_commit(repository) - report: CommitReport = self.get_report(commit) - +class UploadLogicMixin(GetterMixin): + def create_upload(self, serializer, repository, commit, report): version = ( serializer.validated_data["version"] if "version" in serializer.validated_data @@ -127,31 +95,10 @@ def perform_create(self, serializer: UploadSerializer): instance.storage_path = path instance.save() self.trigger_upload_task(repository, commit.commitid, instance, report) - inc_counter( - API_UPLOAD_COUNTER, - labels=generate_upload_prometheus_metrics_labels( - action="coverage", - endpoint="create_upload", - request=self.request, - repository=repository, - is_shelter_request=self.is_shelter_request(), - position="end", - ), - ) self.activate_repo(repository) self.send_analytics_data(commit, instance, version) return instance - def list( - self, - request: HttpRequest, - service: str, - repo: str, - commit_sha: str, - report_code: str, - ): - return HttpResponseNotAllowed(permitted_methods=["POST"]) - def trigger_upload_task(self, repository, commit_sha, upload, report): log.info( "Triggering upload task", @@ -234,6 +181,66 @@ def get_token_for_analytics(self, commit: Commit): token = repo.upload_token return token + +class UploadViews(ListCreateAPIView, UploadLogicMixin): + serializer_class = UploadSerializer + permission_classes = [ + CanDoCoverageUploadsPermission, + ] + authentication_classes = [ + UploadTokenRequiredAuthenticationCheck, + GlobalTokenAuthentication, + OrgLevelTokenAuthentication, + GitHubOIDCTokenAuthentication, + RepositoryLegacyTokenAuthentication, + TokenlessAuthentication, + ] + throttle_classes = [UploadsPerCommitThrottle, UploadsPerWindowThrottle] + + def get_exception_handler(self): + return repo_auth_custom_exception_handler + + def perform_create(self, serializer: UploadSerializer): + inc_counter( + API_UPLOAD_COUNTER, + labels=generate_upload_prometheus_metrics_labels( + action="coverage", + endpoint="create_upload", + request=self.request, + is_shelter_request=self.is_shelter_request(), + position="start", + ), + ) + repository: Repository = self.get_repo() + validate_activated_repo(repository) + commit: Commit = self.get_commit(repository) + report: CommitReport = self.get_report(commit) + + instance = self.create_upload(serializer, repository, commit, report) + inc_counter( + API_UPLOAD_COUNTER, + labels=generate_upload_prometheus_metrics_labels( + action="coverage", + endpoint="create_upload", + request=self.request, + repository=repository, + is_shelter_request=self.is_shelter_request(), + position="end", + ), + ) + + return instance + + def list( + self, + request: HttpRequest, + service: str, + repo: str, + commit_sha: str, + report_code: str, + ): + return HttpResponseNotAllowed(permitted_methods=["POST"]) + def get_repo(self) -> Repository: try: repo = super().get_repo() From af557db68fb36f0018129c14d7cf71c81452665e Mon Sep 17 00:00:00 2001 From: Tony Le Date: Tue, 5 Nov 2024 18:36:01 -0500 Subject: [PATCH 02/18] Add back validate_activated_repo --- upload/views/reports.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/upload/views/reports.py b/upload/views/reports.py index d8380ca77b..d531e6981d 100644 --- a/upload/views/reports.py +++ b/upload/views/reports.py @@ -18,6 +18,7 @@ from services.task import TaskService from upload.helpers import ( generate_upload_prometheus_metrics_labels, + validate_activated_repo, ) from upload.metrics import API_UPLOAD_COUNTER from upload.serializers import CommitReportSerializer, ReportResultsSerializer @@ -70,6 +71,7 @@ def perform_create(self, serializer): ), ) repository = self.get_repo() + validate_activated_repo(repository) commit = self.get_commit(repository) log.info( "Request to create new report", From 8e1560b203bee0729dfb176d902ef375e166b503 Mon Sep 17 00:00:00 2001 From: Tony Le Date: Wed, 6 Nov 2024 17:52:38 -0500 Subject: [PATCH 03/18] Combined Upload v1 --- upload/urls.py | 6 +++ upload/views/combined_upload.py | 92 ++++++++++++--------------------- upload/views/commits.py | 4 +- upload/views/reports.py | 4 +- upload/views/uploads.py | 4 +- 5 files changed, 46 insertions(+), 64 deletions(-) diff --git a/upload/urls.py b/upload/urls.py index 9ca6183673..7e5be4f2a0 100644 --- a/upload/urls.py +++ b/upload/urls.py @@ -2,6 +2,7 @@ from upload.views.bundle_analysis import BundleAnalysisView from upload.views.commits import CommitViews +from upload.views.combined_upload import CombinedUploadView from upload.views.empty_upload import EmptyUploadView from upload.views.legacy import UploadDownloadHandler, UploadHandler from upload.views.reports import ReportResultsView, ReportViews @@ -57,6 +58,11 @@ CommitViews.as_view(), name="new_upload.commits", ), + path( + "//combined-upload", + CombinedUploadView.as_view(), + name="new_upload.combined_upload", + ), # This was getting in the way of the new endpoints, so I moved to the end re_path(r"(?P\w+)/?", UploadHandler.as_view(), name="upload-handler"), ] diff --git a/upload/views/combined_upload.py b/upload/views/combined_upload.py index 33e75e8a7e..ca9ec1a484 100644 --- a/upload/views/combined_upload.py +++ b/upload/views/combined_upload.py @@ -1,8 +1,11 @@ import logging +from django.conf import settings +from django.http import HttpRequest from rest_framework import status -from rest_framework.generics import ListCreateAPIView +from rest_framework.views import APIView from rest_framework.response import Response +from services.archive import ArchiveService from codecov_auth.authentication.repo_auth import ( GitHubOIDCTokenAuthentication, @@ -19,6 +22,7 @@ UploadSerializer, ) from upload.throttles import UploadsPerCommitThrottle, UploadsPerWindowThrottle +from upload.views.base import GetterMixin from upload.views.commits import CommitLogicMixin from upload.views.reports import ReportLogicMixin from upload.views.uploads import CanDoCoverageUploadsPermission, UploadLogicMixin @@ -26,11 +30,7 @@ log = logging.getLogger(__name__) -class CombinedUploadMixin(CommitLogicMixin, ReportLogicMixin, UploadLogicMixin): - pass - - -class CombinedUploadView(ListCreateAPIView, CombinedUploadMixin): +class CombinedUploadView(APIView, GetterMixin, CommitLogicMixin, ReportLogicMixin, UploadLogicMixin): permission_classes = [CanDoCoverageUploadsPermission] authentication_classes = [ UploadTokenRequiredAuthenticationCheck, @@ -40,86 +40,62 @@ class CombinedUploadView(ListCreateAPIView, CombinedUploadMixin): RepositoryLegacyTokenAuthentication, TokenlessAuthentication, ] - throttle_classes = [UploadsPerCommitThrottle, UploadsPerWindowThrottle] def get_exception_handler(self): return repo_auth_custom_exception_handler - def create(self, request, *args, **kwargs): + def post(self, request: HttpRequest, *args, **kwargs) -> Response: # Create commit create_commit_data = dict( + commitid=request.data.get("commit_sha"), + parent_commit_id=request.data.get("parent_sha"), + pullid=request.data.get("pull_request_number"), branch=request.data.get("branch"), - commit_sha=request.data.get("commit_sha"), - fail_on_error=True, - git_service=request.data.get("git_service"), - parent_sha=request.data.get("parent_sha"), - pull_request_number=request.data.get("pull_request_number"), - slug=request.data.get("slug"), - token=request.data.get("token"), ) commit_serializer = CommitSerializer(data=create_commit_data) - commit_serializer.is_valid(raise_exception=True) - + if not commit_serializer.is_valid(): + return Response( + commit_serializer.errors, status=status.HTTP_400_BAD_REQUEST + ) + log.info(f"Creating commit for {commit_serializer.validated_data}") repository = self.get_repo() commit = self.create_commit(commit_serializer, repository) # Create report commit_report_data = dict( code=request.data.get("code"), - commit_sha=request.data.get("commit_sha"), - fail_on_error=True, - git_service=request.data.get("git_service"), - slug=request.data.get("slug"), - token=request.data.get("token"), ) commit_report_serializer = CommitReportSerializer(data=commit_report_data) + if not commit_report_serializer.is_valid(): + return Response( + commit_report_serializer.errors, status=status.HTTP_400_BAD_REQUEST + ) report = self.create_report(commit_report_serializer, repository, commit) # Do upload upload_data = dict( - branch=request.data.get("branch"), - build_code=request.data.get("build_code"), - build_url=request.data.get("build_url"), - commit_sha=request.data.get("commit_sha"), - disable_file_fixes=request.data.get("disable_file_fixes"), - disable_search=request.data.get("disable_search"), - dry_run=request.data.get("dry_run"), - env_vars=request.data.get("env_vars"), - fail_on_error=request.data.get("fail_on_error"), - files_search_exclude_folders=request.data.get( - "files_search_exclude_folders" - ), - files_search_explicitly_listed_files=request.data.get( - "files_search_explicitly_listed_files" - ), - files_search_root_folder=request.data.get("files_search_root_folder"), + ci_url=request.data.get("build_url"), + env=request.data.get("env_vars"), flags=request.data.get("flags"), - gcov_args=request.data.get("gcov_args"), - gcov_executable=request.data.get("gcov_executable"), - gcov_ignore=request.data.get("gcov_ignore"), - gcov_include=request.data.get("gcov_include"), - git_service=request.data.get("git_service"), - handle_no_reports_found=request.data.get("handle_no_reports_found"), + ci_service=request.data.get("ci_service"), job_code=request.data.get("job_code"), name=request.data.get("name"), - network_filter=request.data.get("network_filter"), - network_prefix=request.data.get("network_prefix"), - network_root_folder=request.data.get("network_root_folder"), - plugin_names=request.data.get("plugin_names"), - pull_request_number=request.data.get("pull_request_number"), - report_code=report.code, - report_type=request.data.get("report_type"), - slug=request.data.get("slug"), - swift_project=request.data.get("swift_project"), - token=request.data.get("token"), - use_legacy_uploader=request.data.get("use_legacy_uploader"), ) upload_serializer = UploadSerializer(data=upload_data) - upload_serializer.is_valid(raise_exception=True) - + if not upload_serializer.is_valid(): + return Response(upload_serializer.errors, status=status.HTTP_400_BAD_REQUEST) instance = self.create_upload(upload_serializer, repository, commit, report) + + repository = instance.report.commit.repository + commit = instance.report.commit + + url = f"{settings.CODECOV_DASHBOARD_URL}/{repository.author.service}/{repository.author.username}/{repository.name}/commit/{commit.commitid}" + + archive_service = ArchiveService(repository) + raw_upload_location = archive_service.create_presigned_put(instance.storage_path) + if instance: - return Response(data=instance.data, status=status.HTTP_201_CREATED) + return Response({"raw_upload_location": raw_upload_location, "url": url}, status=status.HTTP_201_CREATED) else: return Response( upload_serializer.errors, status=status.HTTP_400_BAD_REQUEST diff --git a/upload/views/commits.py b/upload/views/commits.py index 2bc251f475..60ab490c78 100644 --- a/upload/views/commits.py +++ b/upload/views/commits.py @@ -26,14 +26,14 @@ log = logging.getLogger(__name__) -class CommitLogicMixin(GetterMixin): +class CommitLogicMixin: def create_commit(self, serializer, repository): validate_activated_repo(repository) commit = serializer.save(repository=repository) return commit -class CommitViews(ListCreateAPIView, CommitLogicMixin): +class CommitViews(ListCreateAPIView, GetterMixin, CommitLogicMixin): serializer_class = CommitSerializer permission_classes = [CanDoCoverageUploadsPermission] authentication_classes = [ diff --git a/upload/views/reports.py b/upload/views/reports.py index d531e6981d..f1d046fb82 100644 --- a/upload/views/reports.py +++ b/upload/views/reports.py @@ -28,7 +28,7 @@ log = logging.getLogger(__name__) -class ReportLogicMixin(GetterMixin): +class ReportLogicMixin: def create_report(self, serializer, repository, commit): code = serializer.validated_data.get("code") if code == "default": @@ -44,7 +44,7 @@ def create_report(self, serializer, repository, commit): return instance -class ReportViews(ListCreateAPIView, ReportLogicMixin): +class ReportViews(ListCreateAPIView, GetterMixin, ReportLogicMixin): serializer_class = CommitReportSerializer permission_classes = [CanDoCoverageUploadsPermission] authentication_classes = [ diff --git a/upload/views/uploads.py b/upload/views/uploads.py index 8697f8f4b8..0015bbd108 100644 --- a/upload/views/uploads.py +++ b/upload/views/uploads.py @@ -49,7 +49,7 @@ def has_permission(self, request, view): ) -class UploadLogicMixin(GetterMixin): +class UploadLogicMixin: def create_upload(self, serializer, repository, commit, report): version = ( serializer.validated_data["version"] @@ -182,7 +182,7 @@ def get_token_for_analytics(self, commit: Commit): return token -class UploadViews(ListCreateAPIView, UploadLogicMixin): +class UploadViews(ListCreateAPIView, GetterMixin, UploadLogicMixin): serializer_class = UploadSerializer permission_classes = [ CanDoCoverageUploadsPermission, From 541451a7a2f5913f9f8dd5ddad1b78066f865a42 Mon Sep 17 00:00:00 2001 From: Tony Le Date: Wed, 6 Nov 2024 21:47:41 -0500 Subject: [PATCH 04/18] Linting --- upload/tests/views/test_combined_upload.py | 169 +++++++++++++++++++++ upload/urls.py | 2 +- upload/views/combined_upload.py | 30 ++-- 3 files changed, 189 insertions(+), 12 deletions(-) create mode 100644 upload/tests/views/test_combined_upload.py diff --git a/upload/tests/views/test_combined_upload.py b/upload/tests/views/test_combined_upload.py new file mode 100644 index 0000000000..d641f92a19 --- /dev/null +++ b/upload/tests/views/test_combined_upload.py @@ -0,0 +1,169 @@ +from unittest.mock import patch + +import pytest +from django.urls import reverse +from rest_framework.test import APIClient +from shared.django_apps.core.tests.factories import ( + RepositoryFactory, +) + +from core.models import Commit +from reports.models import ( + CommitReport, + ReportSession, +) + + +class TestCombinedUpload: + def test_get_repo(self, db): + repository = RepositoryFactory( + name="the_repo", author__username="codecov", author__service="github" + ) + repository.save() + repo_slug = f"{repository.author.username}::::{repository.name}" + url = reverse( + "new_upload.combined_upload", + args=[repository.author.service, repo_slug], + ) + client = APIClient() + client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token) + response = client.post(url, {}, format="json") + assert response.status_code == 400 # Bad request due to missing required fields + assert "commit_sha" in response.json() + + def test_get_repo_not_found(self, db): + repository = RepositoryFactory( + name="the_repo", author__username="codecov", author__service="github" + ) + repo_slug = "codecov::::wrong-repo-name" + url = reverse( + "new_upload.combined_upload", + args=[repository.author.service, repo_slug], + ) + client = APIClient() + response = client.post(url, {}, format="json") + assert response.status_code == 400 + assert "Repository not found" in str(response.json()) + + def test_deactivated_repo(self, db): + repository = RepositoryFactory( + name="the_repo", + author__username="codecov", + author__service="github", + active=True, + activated=False, + ) + repository.save() + repo_slug = f"{repository.author.username}::::{repository.name}" + url = reverse( + "new_upload.combined_upload", + args=[repository.author.service, repo_slug], + ) + client = APIClient() + client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token) + response = client.post(url, {"commit_sha": "abc123"}, format="json") + assert response.status_code == 400 + assert "This repository is deactivated" in str(response.json()) + + @patch("services.task.TaskService.update_commit") + def test_successful_combined_upload(self, mock_update_commit, db): + repository = RepositoryFactory( + name="the_repo", author__username="codecov", author__service="github" + ) + repository.save() + repo_slug = f"{repository.author.username}::::{repository.name}" + url = reverse( + "new_upload.combined_upload", + args=[repository.author.service, repo_slug], + ) + + upload_data = { + "commit_sha": "abc123", + "branch": "main", + "pull_request_number": "42", + "code": "coverage-data", + "build_code": "build-1", + "build_url": "http://ci.test/build/1", + "job_code": "job-1", + "flags": ["unit", "integration"], + "name": "Upload 1", + } + + client = APIClient() + client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token) + response = client.post(url, upload_data, format="json") + + assert response.status_code == 201 + + # Verify commit was created + commit = Commit.objects.get(commitid="abc123") + assert commit.branch == "main" + assert commit.pullid == 42 + assert commit.repository == repository + + # Verify report was created + report = CommitReport.objects.get(commit=commit) + assert report is not None + + # Verify upload was created + session = ReportSession.objects.get(report=report) + assert session.build_code == "build-1" + assert session.build_url == "http://ci.test/build/1" + assert session.job_code == "job-1" + assert session.name == "Upload 1" + + mock_update_commit.assert_called_with( + commitid="abc123", repoid=repository.repoid + ) + + @pytest.mark.parametrize("branch", ["main", "someone:main", "someone/fork:main"]) + @pytest.mark.parametrize("private", [True, False]) + def test_combined_upload_tokenless(self, db, branch, private): + repository = RepositoryFactory( + private=private, author__username="codecov", name="the_repo" + ) + repo_slug = f"{repository.author.username}::::{repository.name}" + url = reverse( + "new_upload.combined_upload", + args=[repository.author.service, repo_slug], + ) + + upload_data = { + "commit_sha": "abc123", + "branch": branch, + "code": "coverage-data", + } + + client = APIClient() + response = client.post(url, upload_data, format="json") + + if ":" in branch and private == False: + assert response.status_code == 201 + commit = Commit.objects.get(commitid="abc123") + assert commit.branch == branch + else: + assert response.status_code == 401 + assert not Commit.objects.filter(commitid="abc123").exists() + + def test_combined_upload_with_errors(self, db): + repository = RepositoryFactory() + repo_slug = f"{repository.author.username}::::{repository.name}" + url = reverse( + "new_upload.combined_upload", + args=[repository.author.service, repo_slug], + ) + + client = APIClient() + client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token) + + # Missing required fields + response = client.post(url, {}, format="json") + assert response.status_code == 400 + assert "commit_sha" in response.json() + + # Invalid flag format + response = client.post( + url, {"commit_sha": "abc123", "flags": "not-a-list"}, format="json" + ) + assert response.status_code == 400 + assert "flags" in response.json() diff --git a/upload/urls.py b/upload/urls.py index 7e5be4f2a0..a2eb26dab8 100644 --- a/upload/urls.py +++ b/upload/urls.py @@ -1,8 +1,8 @@ from django.urls import path, re_path from upload.views.bundle_analysis import BundleAnalysisView -from upload.views.commits import CommitViews from upload.views.combined_upload import CombinedUploadView +from upload.views.commits import CommitViews from upload.views.empty_upload import EmptyUploadView from upload.views.legacy import UploadDownloadHandler, UploadHandler from upload.views.reports import ReportResultsView, ReportViews diff --git a/upload/views/combined_upload.py b/upload/views/combined_upload.py index ca9ec1a484..47bf0fe1f5 100644 --- a/upload/views/combined_upload.py +++ b/upload/views/combined_upload.py @@ -3,9 +3,8 @@ from django.conf import settings from django.http import HttpRequest from rest_framework import status -from rest_framework.views import APIView from rest_framework.response import Response -from services.archive import ArchiveService +from rest_framework.views import APIView from codecov_auth.authentication.repo_auth import ( GitHubOIDCTokenAuthentication, @@ -16,12 +15,12 @@ UploadTokenRequiredAuthenticationCheck, repo_auth_custom_exception_handler, ) +from services.archive import ArchiveService from upload.serializers import ( CommitReportSerializer, CommitSerializer, UploadSerializer, ) -from upload.throttles import UploadsPerCommitThrottle, UploadsPerWindowThrottle from upload.views.base import GetterMixin from upload.views.commits import CommitLogicMixin from upload.views.reports import ReportLogicMixin @@ -30,7 +29,9 @@ log = logging.getLogger(__name__) -class CombinedUploadView(APIView, GetterMixin, CommitLogicMixin, ReportLogicMixin, UploadLogicMixin): +class CombinedUploadView( + APIView, GetterMixin, CommitLogicMixin, ReportLogicMixin, UploadLogicMixin +): permission_classes = [CanDoCoverageUploadsPermission] authentication_classes = [ UploadTokenRequiredAuthenticationCheck, @@ -83,19 +84,26 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: ) upload_serializer = UploadSerializer(data=upload_data) if not upload_serializer.is_valid(): - return Response(upload_serializer.errors, status=status.HTTP_400_BAD_REQUEST) + return Response( + upload_serializer.errors, status=status.HTTP_400_BAD_REQUEST + ) instance = self.create_upload(upload_serializer, repository, commit, report) - + repository = instance.report.commit.repository commit = instance.report.commit - + url = f"{settings.CODECOV_DASHBOARD_URL}/{repository.author.service}/{repository.author.username}/{repository.name}/commit/{commit.commitid}" - + archive_service = ArchiveService(repository) - raw_upload_location = archive_service.create_presigned_put(instance.storage_path) - + raw_upload_location = archive_service.create_presigned_put( + instance.storage_path + ) + if instance: - return Response({"raw_upload_location": raw_upload_location, "url": url}, status=status.HTTP_201_CREATED) + return Response( + {"raw_upload_location": raw_upload_location, "url": url}, + status=status.HTTP_201_CREATED, + ) else: return Response( upload_serializer.errors, status=status.HTTP_400_BAD_REQUEST From a0a5bc494c513e8dd3cd134071bea3d26d79b2ca Mon Sep 17 00:00:00 2001 From: Tony Le Date: Wed, 6 Nov 2024 22:42:17 -0500 Subject: [PATCH 05/18] Fix tests --- upload/tests/views/test_combined_upload.py | 93 ++-------------------- 1 file changed, 7 insertions(+), 86 deletions(-) diff --git a/upload/tests/views/test_combined_upload.py b/upload/tests/views/test_combined_upload.py index d641f92a19..2c80e25de2 100644 --- a/upload/tests/views/test_combined_upload.py +++ b/upload/tests/views/test_combined_upload.py @@ -13,7 +13,6 @@ ReportSession, ) - class TestCombinedUpload: def test_get_repo(self, db): repository = RepositoryFactory( @@ -29,9 +28,10 @@ def test_get_repo(self, db): client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token) response = client.post(url, {}, format="json") assert response.status_code == 400 # Bad request due to missing required fields - assert "commit_sha" in response.json() + assert "commitid" in response.json() - def test_get_repo_not_found(self, db): + @patch("services.task.TaskService.upload") + def test_get_repo_not_found(self, upload, db): repository = RepositoryFactory( name="the_repo", author__username="codecov", author__service="github" ) @@ -42,8 +42,9 @@ def test_get_repo_not_found(self, db): ) client = APIClient() response = client.post(url, {}, format="json") - assert response.status_code == 400 - assert "Repository not found" in str(response.json()) + assert response.status_code == 401 + assert response.json() == {"detail": "Not valid tokenless upload"} + assert not upload.called def test_deactivated_repo(self, db): repository = RepositoryFactory( @@ -65,86 +66,6 @@ def test_deactivated_repo(self, db): assert response.status_code == 400 assert "This repository is deactivated" in str(response.json()) - @patch("services.task.TaskService.update_commit") - def test_successful_combined_upload(self, mock_update_commit, db): - repository = RepositoryFactory( - name="the_repo", author__username="codecov", author__service="github" - ) - repository.save() - repo_slug = f"{repository.author.username}::::{repository.name}" - url = reverse( - "new_upload.combined_upload", - args=[repository.author.service, repo_slug], - ) - - upload_data = { - "commit_sha": "abc123", - "branch": "main", - "pull_request_number": "42", - "code": "coverage-data", - "build_code": "build-1", - "build_url": "http://ci.test/build/1", - "job_code": "job-1", - "flags": ["unit", "integration"], - "name": "Upload 1", - } - - client = APIClient() - client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token) - response = client.post(url, upload_data, format="json") - - assert response.status_code == 201 - - # Verify commit was created - commit = Commit.objects.get(commitid="abc123") - assert commit.branch == "main" - assert commit.pullid == 42 - assert commit.repository == repository - - # Verify report was created - report = CommitReport.objects.get(commit=commit) - assert report is not None - - # Verify upload was created - session = ReportSession.objects.get(report=report) - assert session.build_code == "build-1" - assert session.build_url == "http://ci.test/build/1" - assert session.job_code == "job-1" - assert session.name == "Upload 1" - - mock_update_commit.assert_called_with( - commitid="abc123", repoid=repository.repoid - ) - - @pytest.mark.parametrize("branch", ["main", "someone:main", "someone/fork:main"]) - @pytest.mark.parametrize("private", [True, False]) - def test_combined_upload_tokenless(self, db, branch, private): - repository = RepositoryFactory( - private=private, author__username="codecov", name="the_repo" - ) - repo_slug = f"{repository.author.username}::::{repository.name}" - url = reverse( - "new_upload.combined_upload", - args=[repository.author.service, repo_slug], - ) - - upload_data = { - "commit_sha": "abc123", - "branch": branch, - "code": "coverage-data", - } - - client = APIClient() - response = client.post(url, upload_data, format="json") - - if ":" in branch and private == False: - assert response.status_code == 201 - commit = Commit.objects.get(commitid="abc123") - assert commit.branch == branch - else: - assert response.status_code == 401 - assert not Commit.objects.filter(commitid="abc123").exists() - def test_combined_upload_with_errors(self, db): repository = RepositoryFactory() repo_slug = f"{repository.author.username}::::{repository.name}" @@ -159,7 +80,7 @@ def test_combined_upload_with_errors(self, db): # Missing required fields response = client.post(url, {}, format="json") assert response.status_code == 400 - assert "commit_sha" in response.json() + assert "commitid" in response.json() # Invalid flag format response = client.post( From c5ce5d343dfafad79671022f98df420d1673abac Mon Sep 17 00:00:00 2001 From: Tony Le Date: Wed, 6 Nov 2024 22:46:04 -0500 Subject: [PATCH 06/18] Lints --- upload/tests/views/test_combined_upload.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/upload/tests/views/test_combined_upload.py b/upload/tests/views/test_combined_upload.py index 2c80e25de2..43bdf4fd31 100644 --- a/upload/tests/views/test_combined_upload.py +++ b/upload/tests/views/test_combined_upload.py @@ -1,17 +1,11 @@ from unittest.mock import patch -import pytest from django.urls import reverse from rest_framework.test import APIClient from shared.django_apps.core.tests.factories import ( RepositoryFactory, ) -from core.models import Commit -from reports.models import ( - CommitReport, - ReportSession, -) class TestCombinedUpload: def test_get_repo(self, db): From 3f74fa49ab50adf88ee16d3f7002eae7ee8a84d4 Mon Sep 17 00:00:00 2001 From: Tony Le Date: Fri, 8 Nov 2024 12:46:57 -0500 Subject: [PATCH 07/18] Add throttling for combined-upload --- upload/throttles.py | 5 +++-- upload/views/combined_upload.py | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/upload/throttles.py b/upload/throttles.py index 29c46c989d..2f148c64dd 100644 --- a/upload/throttles.py +++ b/upload/throttles.py @@ -3,6 +3,7 @@ from django.conf import settings from django.core.exceptions import ObjectDoesNotExist from django.db.models import Q +from rest_framework.exceptions import ValidationError from rest_framework.throttling import BaseThrottle from shared.reports.enums import UploadType from shared.upload.utils import query_monthly_coverage_measurements @@ -38,7 +39,7 @@ def allow_request(self, request, view): ) return False return True - except ObjectDoesNotExist: + except (ObjectDoesNotExist, ValidationError): return True @@ -71,5 +72,5 @@ def allow_request(self, request, view): ) return False return True - except ObjectDoesNotExist: + except (ObjectDoesNotExist, ValidationError): return True diff --git a/upload/views/combined_upload.py b/upload/views/combined_upload.py index 47bf0fe1f5..f5a10ff378 100644 --- a/upload/views/combined_upload.py +++ b/upload/views/combined_upload.py @@ -21,6 +21,7 @@ CommitSerializer, UploadSerializer, ) +from upload.throttles import UploadsPerCommitThrottle, UploadsPerWindowThrottle from upload.views.base import GetterMixin from upload.views.commits import CommitLogicMixin from upload.views.reports import ReportLogicMixin @@ -41,6 +42,7 @@ class CombinedUploadView( RepositoryLegacyTokenAuthentication, TokenlessAuthentication, ] + throttle_classes = [UploadsPerCommitThrottle, UploadsPerWindowThrottle] def get_exception_handler(self): return repo_auth_custom_exception_handler From 87e2a2bd3f497251539ad42d15cfae288c1b8424 Mon Sep 17 00:00:00 2001 From: Tony Le Date: Fri, 8 Nov 2024 13:06:27 -0500 Subject: [PATCH 08/18] Better code --- upload/views/combined_upload.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/upload/views/combined_upload.py b/upload/views/combined_upload.py index f5a10ff378..ff99ee3a12 100644 --- a/upload/views/combined_upload.py +++ b/upload/views/combined_upload.py @@ -84,26 +84,26 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: job_code=request.data.get("job_code"), name=request.data.get("name"), ) + upload_serializer = UploadSerializer(data=upload_data) if not upload_serializer.is_valid(): return Response( upload_serializer.errors, status=status.HTTP_400_BAD_REQUEST ) - instance = self.create_upload(upload_serializer, repository, commit, report) - - repository = instance.report.commit.repository - commit = instance.report.commit - url = f"{settings.CODECOV_DASHBOARD_URL}/{repository.author.service}/{repository.author.username}/{repository.name}/commit/{commit.commitid}" + upload = self.create_upload(upload_serializer, repository, commit, report) - archive_service = ArchiveService(repository) - raw_upload_location = archive_service.create_presigned_put( - instance.storage_path - ) - - if instance: + if upload: + url = f"{settings.CODECOV_DASHBOARD_URL}/{repository.author.service}/{repository.author.username}/{repository.name}/commit/{commit.commitid}" + archive_service = ArchiveService(repository) + raw_upload_location = archive_service.create_presigned_put( + upload.storage_path + ) return Response( - {"raw_upload_location": raw_upload_location, "url": url}, + { + "url": url, + "raw_upload_location": raw_upload_location, + }, status=status.HTTP_201_CREATED, ) else: From 7987e604c40b62111aa5a8569e0be785b56f1a83 Mon Sep 17 00:00:00 2001 From: Tony Le Date: Fri, 8 Nov 2024 13:13:21 -0500 Subject: [PATCH 09/18] Add metrics --- upload/views/combined_upload.py | 57 +++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/upload/views/combined_upload.py b/upload/views/combined_upload.py index ff99ee3a12..247034fa83 100644 --- a/upload/views/combined_upload.py +++ b/upload/views/combined_upload.py @@ -5,6 +5,7 @@ from rest_framework import status from rest_framework.response import Response from rest_framework.views import APIView +from shared.metrics import inc_counter from codecov_auth.authentication.repo_auth import ( GitHubOIDCTokenAuthentication, @@ -16,6 +17,8 @@ repo_auth_custom_exception_handler, ) from services.archive import ArchiveService +from upload.helpers import generate_upload_prometheus_metrics_labels +from upload.metrics import API_UPLOAD_COUNTER from upload.serializers import ( CommitReportSerializer, CommitSerializer, @@ -48,6 +51,17 @@ def get_exception_handler(self): return repo_auth_custom_exception_handler def post(self, request: HttpRequest, *args, **kwargs) -> Response: + inc_counter( + API_UPLOAD_COUNTER, + labels=generate_upload_prometheus_metrics_labels( + action="coverage", + endpoint="combined_upload", + request=self.request, + is_shelter_request=self.is_shelter_request(), + position="start", + ), + ) + # Create commit create_commit_data = dict( commitid=request.data.get("commit_sha"), @@ -62,6 +76,17 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: ) log.info(f"Creating commit for {commit_serializer.validated_data}") repository = self.get_repo() + + inc_counter( + API_UPLOAD_COUNTER, + labels=generate_upload_prometheus_metrics_labels( + action="coverage", + endpoint="combined_upload", + request=self.request, + is_shelter_request=self.is_shelter_request(), + position="create_commit", + ), + ) commit = self.create_commit(commit_serializer, repository) # Create report @@ -73,6 +98,17 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: return Response( commit_report_serializer.errors, status=status.HTTP_400_BAD_REQUEST ) + + inc_counter( + API_UPLOAD_COUNTER, + labels=generate_upload_prometheus_metrics_labels( + action="coverage", + endpoint="combined_upload", + request=self.request, + is_shelter_request=self.is_shelter_request(), + position="create_report", + ), + ) report = self.create_report(commit_report_serializer, repository, commit) # Do upload @@ -91,8 +127,29 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: upload_serializer.errors, status=status.HTTP_400_BAD_REQUEST ) + inc_counter( + API_UPLOAD_COUNTER, + labels=generate_upload_prometheus_metrics_labels( + action="coverage", + endpoint="combined_upload", + request=self.request, + is_shelter_request=self.is_shelter_request(), + position="create_upload", + ), + ) upload = self.create_upload(upload_serializer, repository, commit, report) + inc_counter( + API_UPLOAD_COUNTER, + labels=generate_upload_prometheus_metrics_labels( + action="coverage", + endpoint="combined_upload", + request=self.request, + is_shelter_request=self.is_shelter_request(), + position="end", + ), + ) + if upload: url = f"{settings.CODECOV_DASHBOARD_URL}/{repository.author.service}/{repository.author.username}/{repository.name}/commit/{commit.commitid}" archive_service = ArchiveService(repository) From deb6e15eee72854b72314f45a4ddca217e00752f Mon Sep 17 00:00:00 2001 From: Tony Le Date: Fri, 8 Nov 2024 13:19:05 -0500 Subject: [PATCH 10/18] Use updated repository obj --- upload/views/combined_upload.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/upload/views/combined_upload.py b/upload/views/combined_upload.py index 247034fa83..6810c7bc9b 100644 --- a/upload/views/combined_upload.py +++ b/upload/views/combined_upload.py @@ -151,8 +151,10 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: ) if upload: - url = f"{settings.CODECOV_DASHBOARD_URL}/{repository.author.service}/{repository.author.username}/{repository.name}/commit/{commit.commitid}" - archive_service = ArchiveService(repository) + commitid = upload.report.commit.commitid + upload_repository = upload.report.commit.repository + url = f"{settings.CODECOV_DASHBOARD_URL}/{upload_repository.author.service}/{upload_repository.author.username}/{upload_repository.name}/commit/{commitid}" + archive_service = ArchiveService(upload_repository) raw_upload_location = archive_service.create_presigned_put( upload.storage_path ) From fd7dcf48b40d851796c66ad8b9953f928cbbc6c1 Mon Sep 17 00:00:00 2001 From: Tony Le Date: Fri, 8 Nov 2024 13:36:38 -0500 Subject: [PATCH 11/18] Lints --- upload/views/combined_upload.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/upload/views/combined_upload.py b/upload/views/combined_upload.py index 6810c7bc9b..e2d7be7465 100644 --- a/upload/views/combined_upload.py +++ b/upload/views/combined_upload.py @@ -76,7 +76,7 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: ) log.info(f"Creating commit for {commit_serializer.validated_data}") repository = self.get_repo() - + inc_counter( API_UPLOAD_COUNTER, labels=generate_upload_prometheus_metrics_labels( @@ -98,7 +98,7 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: return Response( commit_report_serializer.errors, status=status.HTTP_400_BAD_REQUEST ) - + inc_counter( API_UPLOAD_COUNTER, labels=generate_upload_prometheus_metrics_labels( From 001c993ec4969fc44971556457afc5eef148d24c Mon Sep 17 00:00:00 2001 From: Tony Le Date: Tue, 12 Nov 2024 03:28:11 -0500 Subject: [PATCH 12/18] Use emit_metrics for better readability --- upload/views/combined_upload.py | 53 +++++++-------------------------- 1 file changed, 10 insertions(+), 43 deletions(-) diff --git a/upload/views/combined_upload.py b/upload/views/combined_upload.py index e2d7be7465..1e6e2507fa 100644 --- a/upload/views/combined_upload.py +++ b/upload/views/combined_upload.py @@ -49,8 +49,8 @@ class CombinedUploadView( def get_exception_handler(self): return repo_auth_custom_exception_handler - - def post(self, request: HttpRequest, *args, **kwargs) -> Response: + + def emit_metrics(self, position: str) -> None: inc_counter( API_UPLOAD_COUNTER, labels=generate_upload_prometheus_metrics_labels( @@ -58,10 +58,13 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: endpoint="combined_upload", request=self.request, is_shelter_request=self.is_shelter_request(), - position="start", + position=position, ), ) + def post(self, request: HttpRequest, *args, **kwargs) -> Response: + self.emit_metrics(position="start") + # Create commit create_commit_data = dict( commitid=request.data.get("commit_sha"), @@ -77,16 +80,7 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: log.info(f"Creating commit for {commit_serializer.validated_data}") repository = self.get_repo() - inc_counter( - API_UPLOAD_COUNTER, - labels=generate_upload_prometheus_metrics_labels( - action="coverage", - endpoint="combined_upload", - request=self.request, - is_shelter_request=self.is_shelter_request(), - position="create_commit", - ), - ) + self.emit_metrics(position="create_commit") commit = self.create_commit(commit_serializer, repository) # Create report @@ -99,16 +93,7 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: commit_report_serializer.errors, status=status.HTTP_400_BAD_REQUEST ) - inc_counter( - API_UPLOAD_COUNTER, - labels=generate_upload_prometheus_metrics_labels( - action="coverage", - endpoint="combined_upload", - request=self.request, - is_shelter_request=self.is_shelter_request(), - position="create_report", - ), - ) + self.emit_metrics(position="create_report") report = self.create_report(commit_report_serializer, repository, commit) # Do upload @@ -127,28 +112,10 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: upload_serializer.errors, status=status.HTTP_400_BAD_REQUEST ) - inc_counter( - API_UPLOAD_COUNTER, - labels=generate_upload_prometheus_metrics_labels( - action="coverage", - endpoint="combined_upload", - request=self.request, - is_shelter_request=self.is_shelter_request(), - position="create_upload", - ), - ) + self.emit_metrics(position="create_upload") upload = self.create_upload(upload_serializer, repository, commit, report) - inc_counter( - API_UPLOAD_COUNTER, - labels=generate_upload_prometheus_metrics_labels( - action="coverage", - endpoint="combined_upload", - request=self.request, - is_shelter_request=self.is_shelter_request(), - position="end", - ), - ) + self.emit_metrics(position="end") if upload: commitid = upload.report.commit.commitid From 24f9c94779e3140b11914f17e370a6d06f63fd26 Mon Sep 17 00:00:00 2001 From: Tony Le Date: Tue, 12 Nov 2024 03:31:16 -0500 Subject: [PATCH 13/18] Return early if broken upload --- upload/views/combined_upload.py | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/upload/views/combined_upload.py b/upload/views/combined_upload.py index 1e6e2507fa..ae070f57d8 100644 --- a/upload/views/combined_upload.py +++ b/upload/views/combined_upload.py @@ -49,7 +49,7 @@ class CombinedUploadView( def get_exception_handler(self): return repo_auth_custom_exception_handler - + def emit_metrics(self, position: str) -> None: inc_counter( API_UPLOAD_COUNTER, @@ -117,22 +117,20 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: self.emit_metrics(position="end") - if upload: - commitid = upload.report.commit.commitid - upload_repository = upload.report.commit.repository - url = f"{settings.CODECOV_DASHBOARD_URL}/{upload_repository.author.service}/{upload_repository.author.username}/{upload_repository.name}/commit/{commitid}" - archive_service = ArchiveService(upload_repository) - raw_upload_location = archive_service.create_presigned_put( - upload.storage_path - ) - return Response( - { - "url": url, - "raw_upload_location": raw_upload_location, - }, - status=status.HTTP_201_CREATED, - ) - else: + if not upload: return Response( upload_serializer.errors, status=status.HTTP_400_BAD_REQUEST ) + + commitid = upload.report.commit.commitid + upload_repository = upload.report.commit.repository + url = f"{settings.CODECOV_DASHBOARD_URL}/{upload_repository.author.service}/{upload_repository.author.username}/{upload_repository.name}/commit/{commitid}" + archive_service = ArchiveService(upload_repository) + raw_upload_location = archive_service.create_presigned_put(upload.storage_path) + return Response( + { + "url": url, + "raw_upload_location": raw_upload_location, + }, + status=status.HTTP_201_CREATED, + ) From b527507b7abee1e67d206f78e624aa123ce3b782 Mon Sep 17 00:00:00 2001 From: Tony Le Date: Tue, 12 Nov 2024 05:10:41 -0500 Subject: [PATCH 14/18] Add external_id and created_at outputs for combined_upload --- upload/views/combined_upload.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/upload/views/combined_upload.py b/upload/views/combined_upload.py index ae070f57d8..2a0ec1f149 100644 --- a/upload/views/combined_upload.py +++ b/upload/views/combined_upload.py @@ -77,9 +77,9 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: return Response( commit_serializer.errors, status=status.HTTP_400_BAD_REQUEST ) - log.info(f"Creating commit for {commit_serializer.validated_data}") - repository = self.get_repo() + repository = self.get_repo() + log.info(f"Creating commit for {commit_serializer.validated_data}") self.emit_metrics(position="create_commit") commit = self.create_commit(commit_serializer, repository) @@ -93,6 +93,7 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: commit_report_serializer.errors, status=status.HTTP_400_BAD_REQUEST ) + log.info(f"Creating report for {commit_report_serializer.validated_data}") self.emit_metrics(position="create_report") report = self.create_report(commit_report_serializer, repository, commit) @@ -104,14 +105,19 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: ci_service=request.data.get("ci_service"), job_code=request.data.get("job_code"), name=request.data.get("name"), + version=request.data.get("version"), ) + if self.is_shelter_request(): + upload_data["storage_path"] = request.data.get("storage_path") + upload_serializer = UploadSerializer(data=upload_data) if not upload_serializer.is_valid(): return Response( upload_serializer.errors, status=status.HTTP_400_BAD_REQUEST ) + log.info(f"Creating upload for {upload_serializer.validated_data}") self.emit_metrics(position="create_upload") upload = self.create_upload(upload_serializer, repository, commit, report) @@ -129,8 +135,10 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: raw_upload_location = archive_service.create_presigned_put(upload.storage_path) return Response( { - "url": url, + "external_id": str(upload.external_id), + "created_at": upload.created_at.strftime("%Y-%m-%dT%H:%M:%S.%fZ"), "raw_upload_location": raw_upload_location, + "url": url, }, status=status.HTTP_201_CREATED, ) From 93b0f4896b76291b9a3fd5b9155248c99c494762 Mon Sep 17 00:00:00 2001 From: Tony Le Date: Tue, 12 Nov 2024 05:10:58 -0500 Subject: [PATCH 15/18] Add successful combined_upload test for normal use case and shelter --- upload/tests/views/test_combined_upload.py | 345 ++++++++++++++++----- 1 file changed, 275 insertions(+), 70 deletions(-) diff --git a/upload/tests/views/test_combined_upload.py b/upload/tests/views/test_combined_upload.py index 43bdf4fd31..4d73a4a9d1 100644 --- a/upload/tests/views/test_combined_upload.py +++ b/upload/tests/views/test_combined_upload.py @@ -1,84 +1,289 @@ from unittest.mock import patch +from django.conf import settings +from django.test import override_settings from django.urls import reverse from rest_framework.test import APIClient from shared.django_apps.core.tests.factories import ( + CommitFactory, RepositoryFactory, ) +from reports.models import ( + ReportSession, + RepositoryFlag, + UploadFlagMembership, +) +from services.archive import ArchiveService, MinioEndpoints +from upload.views.combined_upload import CanDoCoverageUploadsPermission -class TestCombinedUpload: - def test_get_repo(self, db): - repository = RepositoryFactory( - name="the_repo", author__username="codecov", author__service="github" - ) - repository.save() - repo_slug = f"{repository.author.username}::::{repository.name}" - url = reverse( - "new_upload.combined_upload", - args=[repository.author.service, repo_slug], - ) - client = APIClient() - client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token) - response = client.post(url, {}, format="json") - assert response.status_code == 400 # Bad request due to missing required fields - assert "commitid" in response.json() - - @patch("services.task.TaskService.upload") - def test_get_repo_not_found(self, upload, db): - repository = RepositoryFactory( - name="the_repo", author__username="codecov", author__service="github" - ) - repo_slug = "codecov::::wrong-repo-name" - url = reverse( - "new_upload.combined_upload", - args=[repository.author.service, repo_slug], - ) - client = APIClient() - response = client.post(url, {}, format="json") - assert response.status_code == 401 - assert response.json() == {"detail": "Not valid tokenless upload"} - assert not upload.called - - def test_deactivated_repo(self, db): - repository = RepositoryFactory( - name="the_repo", - author__username="codecov", - author__service="github", - active=True, - activated=False, - ) - repository.save() - repo_slug = f"{repository.author.username}::::{repository.name}" - url = reverse( - "new_upload.combined_upload", - args=[repository.author.service, repo_slug], - ) - client = APIClient() - client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token) - response = client.post(url, {"commit_sha": "abc123"}, format="json") - assert response.status_code == 400 - assert "This repository is deactivated" in str(response.json()) - - def test_combined_upload_with_errors(self, db): - repository = RepositoryFactory() - repo_slug = f"{repository.author.username}::::{repository.name}" - url = reverse( - "new_upload.combined_upload", - args=[repository.author.service, repo_slug], + +def test_get_repo(db): + repository = RepositoryFactory( + name="the_repo", author__username="codecov", author__service="github" + ) + repository.save() + repo_slug = f"{repository.author.username}::::{repository.name}" + url = reverse( + "new_upload.combined_upload", + args=[repository.author.service, repo_slug], + ) + client = APIClient() + client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token) + response = client.post(url, {}, format="json") + assert response.status_code == 400 # Bad request due to missing required fields + assert "commitid" in response.json() + + +@patch("services.task.TaskService.upload") +def test_get_repo_not_found(upload, db): + repository = RepositoryFactory( + name="the_repo", author__username="codecov", author__service="github" + ) + repo_slug = "codecov::::wrong-repo-name" + url = reverse( + "new_upload.combined_upload", + args=[repository.author.service, repo_slug], + ) + client = APIClient() + response = client.post(url, {}, format="json") + assert response.status_code == 401 + assert response.json() == {"detail": "Not valid tokenless upload"} + assert not upload.called + + +def test_deactivated_repo(db): + repository = RepositoryFactory( + name="the_repo", + author__username="codecov", + author__service="github", + active=True, + activated=False, + ) + repository.save() + repo_slug = f"{repository.author.username}::::{repository.name}" + url = reverse( + "new_upload.combined_upload", + args=[repository.author.service, repo_slug], + ) + client = APIClient() + client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token) + response = client.post(url, {"commit_sha": "abc123"}, format="json") + assert response.status_code == 400 + assert "This repository is deactivated" in str(response.json()) + + +def test_combined_upload_with_errors(db): + repository = RepositoryFactory() + repo_slug = f"{repository.author.username}::::{repository.name}" + url = reverse( + "new_upload.combined_upload", + args=[repository.author.service, repo_slug], + ) + + client = APIClient() + client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token) + + # Missing required fields + response = client.post(url, {}, format="json") + assert response.status_code == 400 + assert "commitid" in response.json() + + # Invalid flag format + response = client.post( + url, {"commit_sha": "abc123", "flags": "not-a-list"}, format="json" + ) + assert response.status_code == 400 + assert "flags" in response.json() + + +def test_combined_upload_post(db, mocker): + mocker.patch.object( + CanDoCoverageUploadsPermission, "has_permission", return_value=True + ) + presigned_put_mock = mocker.patch( + "services.archive.StorageService.create_presigned_put", + return_value="presigned put", + ) + upload_task_mock = mocker.patch( + "upload.views.uploads.UploadLogicMixin.trigger_upload_task", return_value=True + ) + + repository = RepositoryFactory( + name="the_repo1", author__username="codecov", author__service="github" + ) + commit = CommitFactory(repository=repository) + repository.save() + commit.save() + + owner = repository.author + client = APIClient() + client.force_authenticate(user=owner) + repo_slug = f"{repository.author.username}::::{repository.name}" + url = reverse( + "new_upload.combined_upload", + args=[repository.author.service, repo_slug], + ) + response = client.post( + url, + { + "branch": "branch", + "ci_service": "ci_service", + "ci_url": "ci_url", + "code": "code", + "commit_sha": commit.commitid, + "flags": ["flag1", "flag2"], + "job_code": "job_code", + "version": "version", + }, + format="json", + ) + response_json = response.json() + upload = ReportSession.objects.filter( + report__commit=commit, + report__code="code", + upload_extras={"format_version": "v1"}, + ).first() + assert response.status_code == 201 + assert all( + map( + lambda x: x in response_json.keys(), + ["external_id", "created_at", "raw_upload_location", "url"], ) + ) + assert ( + response_json.get("url") + == f"{settings.CODECOV_DASHBOARD_URL}/{repository.author.service}/{repository.author.username}/{repository.name}/commit/{commit.commitid}" + ) + + assert ReportSession.objects.filter( + report__commit=commit, + report__code="code", + upload_extras={"format_version": "v1"}, + ).exists() + assert RepositoryFlag.objects.filter( + repository_id=repository.repoid, flag_name="flag1" + ).exists() + assert RepositoryFlag.objects.filter( + repository_id=repository.repoid, flag_name="flag2" + ).exists() + flag1 = RepositoryFlag.objects.filter( + repository_id=repository.repoid, flag_name="flag1" + ).first() + flag2 = RepositoryFlag.objects.filter( + repository_id=repository.repoid, flag_name="flag2" + ).first() + assert UploadFlagMembership.objects.filter( + report_session_id=upload.id, flag_id=flag1.id + ).exists() + assert UploadFlagMembership.objects.filter( + report_session_id=upload.id, flag_id=flag2.id + ).exists() + assert [flag for flag in upload.flags.all()] == [flag1, flag2] + + archive_service = ArchiveService(repository) + assert upload.storage_path == MinioEndpoints.raw_with_upload_id.get_path( + version="v4", + date=upload.created_at.strftime("%Y-%m-%d"), + repo_hash=archive_service.storage_hash, + commit_sha=commit.commitid, + reportid=upload.report.external_id, + uploadid=upload.external_id, + ) + presigned_put_mock.assert_called_with("archive", upload.storage_path, 10) + upload_task_mock.assert_called() - client = APIClient() - client.credentials(HTTP_AUTHORIZATION="token " + repository.upload_token) - # Missing required fields - response = client.post(url, {}, format="json") - assert response.status_code == 400 - assert "commitid" in response.json() +@override_settings(SHELTER_SHARED_SECRET="shelter-shared-secret") +def test_combined_upload_post_shelter(db, mocker): + mocker.patch.object( + CanDoCoverageUploadsPermission, "has_permission", return_value=True + ) + presigned_put_mock = mocker.patch( + "services.archive.StorageService.create_presigned_put", + return_value="presigned put", + ) + upload_task_mock = mocker.patch( + "upload.views.uploads.UploadLogicMixin.trigger_upload_task", return_value=True + ) - # Invalid flag format - response = client.post( - url, {"commit_sha": "abc123", "flags": "not-a-list"}, format="json" + repository = RepositoryFactory( + name="the_repo", author__username="codecov", author__service="github" + ) + commit = CommitFactory(repository=repository) + repository.save() + commit.save() + + owner = repository.author + client = APIClient() + client.force_authenticate(user=owner) + repo_slug = f"{repository.author.username}::::{repository.name}" + url = reverse( + "new_upload.combined_upload", + args=[repository.author.service, repo_slug], + ) + response = client.post( + url, + { + "branch": "branch", + "ci_service": "ci_service", + "ci_url": "ci_url", + "code": "code", + "commit_sha": commit.commitid, + "flags": ["flag1", "flag2"], + "job_code": "job_code", + "storage_path": "shelter/test/path.txt", + "version": "version", + }, + headers={ + "X-Shelter-Token": "shelter-shared-secret", + "User-Agent": "codecov-cli/0.4.7", + }, + format="json", + ) + response_json = response.json() + upload = ReportSession.objects.filter( + report__commit=commit, + report__code="code", + upload_extras={"format_version": "v1"}, + ).first() + assert response.status_code == 201 + assert all( + map( + lambda x: x in response_json.keys(), + ["external_id", "created_at", "raw_upload_location", "url"], ) - assert response.status_code == 400 - assert "flags" in response.json() + ) + assert ( + response_json.get("url") + == f"{settings.CODECOV_DASHBOARD_URL}/{repository.author.service}/{repository.author.username}/{repository.name}/commit/{commit.commitid}" + ) + + assert ReportSession.objects.filter( + report__commit=commit, + report__code="code", + upload_extras={"format_version": "v1"}, + ).exists() + assert RepositoryFlag.objects.filter( + repository_id=repository.repoid, flag_name="flag1" + ).exists() + assert RepositoryFlag.objects.filter( + repository_id=repository.repoid, flag_name="flag2" + ).exists() + flag1 = RepositoryFlag.objects.filter( + repository_id=repository.repoid, flag_name="flag1" + ).first() + flag2 = RepositoryFlag.objects.filter( + repository_id=repository.repoid, flag_name="flag2" + ).first() + assert UploadFlagMembership.objects.filter( + report_session_id=upload.id, flag_id=flag1.id + ).exists() + assert UploadFlagMembership.objects.filter( + report_session_id=upload.id, flag_id=flag2.id + ).exists() + assert [flag for flag in upload.flags.all()] == [flag1, flag2] + + assert upload.storage_path == "shelter/test/path.txt" + presigned_put_mock.assert_called_with("archive", upload.storage_path, 10) + upload_task_mock.assert_called() From 52ce8a41e450409dfd6d483b825265275cd644b7 Mon Sep 17 00:00:00 2001 From: Tony Le Date: Tue, 12 Nov 2024 06:30:29 -0500 Subject: [PATCH 16/18] Use functions instead of Mix ins --- upload/tests/views/test_combined_upload.py | 4 +- upload/tests/views/test_uploads.py | 32 ++- upload/views/combined_upload.py | 38 ++- upload/views/commits.py | 13 +- upload/views/reports.py | 29 ++- upload/views/uploads.py | 278 +++++++++++---------- 6 files changed, 207 insertions(+), 187 deletions(-) diff --git a/upload/tests/views/test_combined_upload.py b/upload/tests/views/test_combined_upload.py index 4d73a4a9d1..d52fc1fc1c 100644 --- a/upload/tests/views/test_combined_upload.py +++ b/upload/tests/views/test_combined_upload.py @@ -106,7 +106,7 @@ def test_combined_upload_post(db, mocker): return_value="presigned put", ) upload_task_mock = mocker.patch( - "upload.views.uploads.UploadLogicMixin.trigger_upload_task", return_value=True + "upload.views.uploads.trigger_upload_task", return_value=True ) repository = RepositoryFactory( @@ -204,7 +204,7 @@ def test_combined_upload_post_shelter(db, mocker): return_value="presigned put", ) upload_task_mock = mocker.patch( - "upload.views.uploads.UploadLogicMixin.trigger_upload_task", return_value=True + "upload.views.uploads.trigger_upload_task", return_value=True ) repository = RepositoryFactory( diff --git a/upload/tests/views/test_uploads.py b/upload/tests/views/test_uploads.py index 2be26e5ba8..0fb8788424 100644 --- a/upload/tests/views/test_uploads.py +++ b/upload/tests/views/test_uploads.py @@ -23,7 +23,12 @@ ) from reports.tests.factories import CommitReportFactory, UploadFactory from services.archive import ArchiveService, MinioEndpoints -from upload.views.uploads import CanDoCoverageUploadsPermission, UploadViews +from upload.views.uploads import ( + CanDoCoverageUploadsPermission, + UploadViews, + activate_repo, + trigger_upload_task, +) def test_upload_permission_class_pass(db, mocker): @@ -197,7 +202,7 @@ def test_uploads_post(db, mocker, mock_redis): return_value="presigned put", ) upload_task_mock = mocker.patch( - "upload.views.uploads.UploadViews.trigger_upload_task", return_value=True + "upload.views.uploads.trigger_upload_task", return_value=True ) repository = RepositoryFactory( @@ -293,7 +298,7 @@ def test_uploads_post_tokenless(db, mocker, mock_redis, private, branch, branch_ return_value="presigned put", ) upload_task_mock = mocker.patch( - "upload.views.uploads.UploadViews.trigger_upload_task", return_value=True + "upload.views.uploads.trigger_upload_task", return_value=True ) analytics_service_mock = mocker.patch("upload.views.uploads.AnalyticsService") @@ -433,7 +438,7 @@ def test_uploads_post_token_required_auth_check( return_value="presigned put", ) upload_task_mock = mocker.patch( - "upload.views.uploads.UploadViews.trigger_upload_task", return_value=True + "upload.views.uploads.trigger_upload_task", return_value=True ) analytics_service_mock = mocker.patch("upload.views.uploads.AnalyticsService") @@ -577,7 +582,7 @@ def test_uploads_post_github_oidc_auth( return_value="presigned put", ) upload_task_mock = mocker.patch( - "upload.views.uploads.UploadViews.trigger_upload_task", return_value=True + "upload.views.uploads.trigger_upload_task", return_value=True ) repository = RepositoryFactory( @@ -696,9 +701,7 @@ def test_uploads_post_shelter(db, mocker, mock_redis): "services.archive.StorageService.create_presigned_put", return_value="presigned put", ) - mocker.patch( - "upload.views.uploads.UploadViews.trigger_upload_task", return_value=True - ) + mocker.patch("upload.views.uploads.trigger_upload_task", return_value=True) mock_prometheus_metrics = mocker.patch("upload.metrics.API_UPLOAD_COUNTER.labels") repository = RepositoryFactory( @@ -798,14 +801,13 @@ def test_deactivated_repo(db, mocker, mock_redis): def test_trigger_upload_task(db, mocker): - upload_views = UploadViews() repo = RepositoryFactory.create() upload = UploadFactory.create() report = CommitReportFactory.create() commitid = "commit id" mocked_redis = mocker.patch("upload.views.uploads.get_redis_connection") mocked_dispatched_task = mocker.patch("upload.views.uploads.dispatch_upload_task") - upload_views.trigger_upload_task(repo, commitid, upload, report) + trigger_upload_task(repo, commitid, upload, report) mocked_redis.assert_called() mocked_dispatched_task.assert_called() @@ -814,8 +816,7 @@ def test_activate_repo(db): repo = RepositoryFactory( active=False, deleted=True, activated=False, coverage_enabled=False ) - upload_views = UploadViews() - upload_views.activate_repo(repo) + activate_repo(repo) assert repo.active assert repo.activated assert not repo.deleted @@ -826,8 +827,7 @@ def test_activate_already_activated_repo(db): repo = RepositoryFactory( active=True, activated=True, deleted=False, coverage_enabled=True ) - upload_views = UploadViews() - upload_views.activate_repo(repo) + activate_repo(repo) assert repo.active @@ -855,9 +855,7 @@ def test_uploads_post_github_enterprise_oidc_auth_jwks_url( "services.archive.StorageService.create_presigned_put", return_value="presigned put", ) - self.mocker.patch( - "upload.views.uploads.UploadViews.trigger_upload_task", return_value=True - ) + self.mocker.patch("upload.views.uploads.trigger_upload_task", return_value=True) repository = RepositoryFactory( name="the_repo", diff --git a/upload/views/combined_upload.py b/upload/views/combined_upload.py index 2a0ec1f149..b5f80a025f 100644 --- a/upload/views/combined_upload.py +++ b/upload/views/combined_upload.py @@ -26,16 +26,18 @@ ) from upload.throttles import UploadsPerCommitThrottle, UploadsPerWindowThrottle from upload.views.base import GetterMixin -from upload.views.commits import CommitLogicMixin -from upload.views.reports import ReportLogicMixin -from upload.views.uploads import CanDoCoverageUploadsPermission, UploadLogicMixin +from upload.views.commits import create_commit +from upload.views.reports import create_report +from upload.views.uploads import ( + CanDoCoverageUploadsPermission, + create_upload, + get_token_for_analytics, +) log = logging.getLogger(__name__) -class CombinedUploadView( - APIView, GetterMixin, CommitLogicMixin, ReportLogicMixin, UploadLogicMixin -): +class CombinedUploadView(APIView, GetterMixin): permission_classes = [CanDoCoverageUploadsPermission] authentication_classes = [ UploadTokenRequiredAuthenticationCheck, @@ -79,9 +81,16 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: ) repository = self.get_repo() - log.info(f"Creating commit for {commit_serializer.validated_data}") self.emit_metrics(position="create_commit") - commit = self.create_commit(commit_serializer, repository) + commit = create_commit(commit_serializer, repository) + + log.info( + "Request to create new combined upload", + extra=dict( + repo=repository.name, + commit=commit.commitid, + ), + ) # Create report commit_report_data = dict( @@ -93,9 +102,8 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: commit_report_serializer.errors, status=status.HTTP_400_BAD_REQUEST ) - log.info(f"Creating report for {commit_report_serializer.validated_data}") self.emit_metrics(position="create_report") - report = self.create_report(commit_report_serializer, repository, commit) + report = create_report(commit_report_serializer, repository, commit) # Do upload upload_data = dict( @@ -117,9 +125,15 @@ def post(self, request: HttpRequest, *args, **kwargs) -> Response: upload_serializer.errors, status=status.HTTP_400_BAD_REQUEST ) - log.info(f"Creating upload for {upload_serializer.validated_data}") self.emit_metrics(position="create_upload") - upload = self.create_upload(upload_serializer, repository, commit, report) + upload = create_upload( + upload_serializer, + repository, + commit, + report, + self.is_shelter_request(), + get_token_for_analytics(commit, self.request), + ) self.emit_metrics(position="end") diff --git a/upload/views/commits.py b/upload/views/commits.py index 60ab490c78..804b42af09 100644 --- a/upload/views/commits.py +++ b/upload/views/commits.py @@ -26,14 +26,13 @@ log = logging.getLogger(__name__) -class CommitLogicMixin: - def create_commit(self, serializer, repository): - validate_activated_repo(repository) - commit = serializer.save(repository=repository) - return commit +def create_commit(serializer, repository): + validate_activated_repo(repository) + commit = serializer.save(repository=repository) + return commit -class CommitViews(ListCreateAPIView, GetterMixin, CommitLogicMixin): +class CommitViews(ListCreateAPIView, GetterMixin): serializer_class = CommitSerializer permission_classes = [CanDoCoverageUploadsPermission] authentication_classes = [ @@ -75,7 +74,7 @@ def perform_create(self, serializer): ), ) repository = self.get_repo() - commit = self.create_commit(serializer, repository) + commit = create_commit(serializer, repository) log.info( "Request to create new commit", diff --git a/upload/views/reports.py b/upload/views/reports.py index f1d046fb82..3685990f65 100644 --- a/upload/views/reports.py +++ b/upload/views/reports.py @@ -28,23 +28,22 @@ log = logging.getLogger(__name__) -class ReportLogicMixin: - def create_report(self, serializer, repository, commit): - code = serializer.validated_data.get("code") - if code == "default": - serializer.validated_data["code"] = None - instance, was_created = serializer.save( - commit_id=commit.id, - report_type=CommitReport.ReportType.COVERAGE, +def create_report(serializer, repository, commit): + code = serializer.validated_data.get("code") + if code == "default": + serializer.validated_data["code"] = None + instance, was_created = serializer.save( + commit_id=commit.id, + report_type=CommitReport.ReportType.COVERAGE, + ) + if was_created: + TaskService().preprocess_upload( + repository.repoid, commit.commitid, instance.code ) - if was_created: - TaskService().preprocess_upload( - repository.repoid, commit.commitid, instance.code - ) - return instance + return instance -class ReportViews(ListCreateAPIView, GetterMixin, ReportLogicMixin): +class ReportViews(ListCreateAPIView, GetterMixin): serializer_class = CommitReportSerializer permission_classes = [CanDoCoverageUploadsPermission] authentication_classes = [ @@ -77,7 +76,7 @@ def perform_create(self, serializer): "Request to create new report", extra=dict(repo=repository.name, commit=commit.commitid), ) - instance = self.create_report(serializer, repository, commit) + instance = create_report(serializer, repository, commit) inc_counter( API_UPLOAD_COUNTER, diff --git a/upload/views/uploads.py b/upload/views/uploads.py index 0015bbd108..d34fe3bee4 100644 --- a/upload/views/uploads.py +++ b/upload/views/uploads.py @@ -39,150 +39,142 @@ log = logging.getLogger(__name__) -class CanDoCoverageUploadsPermission(BasePermission): - def has_permission(self, request, view): - repository = view.get_repo() - return ( - request.auth is not None - and "upload" in request.auth.get_scopes() - and request.auth.allows_repo(repository) - ) +def create_upload(serializer, repository, commit, report, is_shelter_request, token): + version = ( + serializer.validated_data["version"] + if "version" in serializer.validated_data + else None + ) + archive_service = ArchiveService(repository) + # Create upload record + instance: ReportSession = serializer.save( + report_id=report.id, + upload_extras={"format_version": "v1"}, + ) + # Inserts mirror upload record into measurements table. CLI hits this endpoint + insert_coverage_measurement( + owner_id=repository.author.ownerid, + repo_id=repository.repoid, + commit_id=commit.id, + upload_id=instance.id, + uploader_used=UploaderType.CLI.value, + private_repo=repository.private, + report_type=report.report_type, + ) -class UploadLogicMixin: - def create_upload(self, serializer, repository, commit, report): - version = ( - serializer.validated_data["version"] - if "version" in serializer.validated_data - else None - ) - log.info( - "Request to create new upload", - extra=dict( - repo=repository.name, - commit=commit.commitid, - cli_version=version, - ), - ) - archive_service = ArchiveService(repository) - # Create upload record - instance: ReportSession = serializer.save( - report_id=report.id, - upload_extras={"format_version": "v1"}, + # only Shelter requests are allowed to set their own `storage_path` + if instance.storage_path is None or not is_shelter_request: + path = MinioEndpoints.raw_with_upload_id.get_path( + version="v4", + date=timezone.now().strftime("%Y-%m-%d"), + repo_hash=archive_service.storage_hash, + commit_sha=commit.commitid, + reportid=report.external_id, + uploadid=instance.external_id, ) + instance.storage_path = path + instance.save() + trigger_upload_task(repository, commit.commitid, instance, report) + activate_repo(repository) + send_analytics_data(commit, instance, version, token) + return instance - # Inserts mirror upload record into measurements table. CLI hits this endpoint - insert_coverage_measurement( - owner_id=repository.author.ownerid, - repo_id=repository.repoid, - commit_id=commit.id, - upload_id=instance.id, - uploader_used=UploaderType.CLI.value, - private_repo=repository.private, - report_type=report.report_type, - ) - # only Shelter requests are allowed to set their own `storage_path` - if instance.storage_path is None or not self.is_shelter_request(): - path = MinioEndpoints.raw_with_upload_id.get_path( - version="v4", - date=timezone.now().strftime("%Y-%m-%d"), - repo_hash=archive_service.storage_hash, - commit_sha=commit.commitid, - reportid=report.external_id, - uploadid=instance.external_id, - ) - instance.storage_path = path - instance.save() - self.trigger_upload_task(repository, commit.commitid, instance, report) - self.activate_repo(repository) - self.send_analytics_data(commit, instance, version) - return instance +def trigger_upload_task(repository, commit_sha, upload, report): + log.info( + "Triggering upload task", + extra=dict( + repo=repository.name, + commit=commit_sha, + upload_id=upload.id, + report_code=report.code, + ), + ) + redis = get_redis_connection() + task_arguments = { + "commit": commit_sha, + "upload_id": upload.id, + "version": "v4", + "report_code": report.code, + "reportid": str(report.external_id), + } + dispatch_upload_task(task_arguments, repository, redis) - def trigger_upload_task(self, repository, commit_sha, upload, report): - log.info( - "Triggering upload task", - extra=dict( - repo=repository.name, - commit=commit_sha, - upload_id=upload.id, - report_code=report.code, - ), - ) - redis = get_redis_connection() - task_arguments = { - "commit": commit_sha, - "upload_id": upload.id, - "version": "v4", - "report_code": report.code, - "reportid": str(report.external_id), - } - dispatch_upload_task(task_arguments, repository, redis) - def activate_repo(self, repository): - # Only update the fields if needed - if ( - repository.activated - and repository.active - and not repository.deleted - and repository.coverage_enabled - ): - return - repository.activated = True - repository.active = True - repository.deleted = False - repository.coverage_enabled = True - repository.save( - update_fields=[ - "activated", - "active", - "deleted", - "coverage_enabled", - "updatestamp", - ] - ) +def activate_repo(repository): + # Only update the fields if needed + if ( + repository.activated + and repository.active + and not repository.deleted + and repository.coverage_enabled + ): + return + repository.activated = True + repository.active = True + repository.deleted = False + repository.coverage_enabled = True + repository.save( + update_fields=[ + "activated", + "active", + "deleted", + "coverage_enabled", + "updatestamp", + ] + ) - def send_analytics_data(self, commit: Commit, upload: ReportSession, version): - token = self.get_token_for_analytics(commit) - analytics_upload_data = { - "commit": commit.commitid, - "branch": commit.branch, - "pr": commit.pullid, - "repo": commit.repository.name, - "repository_name": commit.repository.name, - "repository_id": commit.repository.repoid, - "service": commit.repository.service, - "build": upload.build_code, - "build_url": upload.build_url, - # we were previously using upload.flag_names here, and this query might not be optimized - # we weren't doing it in the legacy endpoint, but in the new one we are, and it may be causing problems - # therefore we are removing this for now to see if it is the source of the issue - "flags": "", - "owner": commit.repository.author.ownerid, - "token": str(token), - "version": version, - "uploader_type": "CLI", - } - AnalyticsService().account_uploaded_coverage_report( - commit.repository.author.ownerid, analytics_upload_data - ) - def get_token_for_analytics(self, commit: Commit): - repo = commit.repository - if isinstance(self.request.auth, TokenlessAuth): - token = "tokenless_upload" - elif isinstance(self.request.auth, OrgLevelTokenRepositoryAuth): - token = ( - OrganizationLevelToken.objects.filter(owner=repo.author).first().token - ) - elif isinstance(self.request.auth, OIDCTokenRepositoryAuth): - token = "oidc_token_upload" - else: - token = repo.upload_token - return token +def send_analytics_data(commit: Commit, upload: ReportSession, version, token): + analytics_upload_data = { + "commit": commit.commitid, + "branch": commit.branch, + "pr": commit.pullid, + "repo": commit.repository.name, + "repository_name": commit.repository.name, + "repository_id": commit.repository.repoid, + "service": commit.repository.service, + "build": upload.build_code, + "build_url": upload.build_url, + # we were previously using upload.flag_names here, and this query might not be optimized + # we weren't doing it in the legacy endpoint, but in the new one we are, and it may be causing problems + # therefore we are removing this for now to see if it is the source of the issue + "flags": "", + "owner": commit.repository.author.ownerid, + "token": str(token), + "version": version, + "uploader_type": "CLI", + } + AnalyticsService().account_uploaded_coverage_report( + commit.repository.author.ownerid, analytics_upload_data + ) + +def get_token_for_analytics(commit: Commit, request): + repo = commit.repository + if isinstance(request.auth, TokenlessAuth): + token = "tokenless_upload" + elif isinstance(request.auth, OrgLevelTokenRepositoryAuth): + token = OrganizationLevelToken.objects.filter(owner=repo.author).first().token + elif isinstance(request.auth, OIDCTokenRepositoryAuth): + token = "oidc_token_upload" + else: + token = repo.upload_token + return token -class UploadViews(ListCreateAPIView, GetterMixin, UploadLogicMixin): + +class CanDoCoverageUploadsPermission(BasePermission): + def has_permission(self, request, view): + repository = view.get_repo() + return ( + request.auth is not None + and "upload" in request.auth.get_scopes() + and request.auth.allows_repo(repository) + ) + + +class UploadViews(ListCreateAPIView, GetterMixin): serializer_class = UploadSerializer permission_classes = [ CanDoCoverageUploadsPermission, @@ -216,7 +208,25 @@ def perform_create(self, serializer: UploadSerializer): commit: Commit = self.get_commit(repository) report: CommitReport = self.get_report(commit) - instance = self.create_upload(serializer, repository, commit, report) + log.info( + "Request to create new upload", + extra=dict( + repo=repository.name, + commit=commit.commitid, + cli_version=serializer.validated_data["version"] + if "version" in serializer.validated_data + else None, + ), + ) + + instance = create_upload( + serializer, + repository, + commit, + report, + self.is_shelter_request(), + get_token_for_analytics(commit, self.request), + ) inc_counter( API_UPLOAD_COUNTER, labels=generate_upload_prometheus_metrics_labels( From 6becd24ea442113daf5bc6267650d26b9ddaea69 Mon Sep 17 00:00:00 2001 From: Tony Le Date: Tue, 12 Nov 2024 10:28:38 -0500 Subject: [PATCH 17/18] Use analytics_token instead of token --- upload/views/uploads.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/upload/views/uploads.py b/upload/views/uploads.py index d34fe3bee4..d8a65a7288 100644 --- a/upload/views/uploads.py +++ b/upload/views/uploads.py @@ -39,7 +39,7 @@ log = logging.getLogger(__name__) -def create_upload(serializer, repository, commit, report, is_shelter_request, token): +def create_upload(serializer, repository, commit, report, is_shelter_request, analytics_token): version = ( serializer.validated_data["version"] if "version" in serializer.validated_data @@ -77,7 +77,7 @@ def create_upload(serializer, repository, commit, report, is_shelter_request, to instance.save() trigger_upload_task(repository, commit.commitid, instance, report) activate_repo(repository) - send_analytics_data(commit, instance, version, token) + send_analytics_data(commit, instance, version, analytics_token) return instance @@ -126,7 +126,7 @@ def activate_repo(repository): ) -def send_analytics_data(commit: Commit, upload: ReportSession, version, token): +def send_analytics_data(commit: Commit, upload: ReportSession, version, analytics_token): analytics_upload_data = { "commit": commit.commitid, "branch": commit.branch, @@ -142,7 +142,7 @@ def send_analytics_data(commit: Commit, upload: ReportSession, version, token): # therefore we are removing this for now to see if it is the source of the issue "flags": "", "owner": commit.repository.author.ownerid, - "token": str(token), + "token": str(analytics_token), "version": version, "uploader_type": "CLI", } @@ -154,14 +154,14 @@ def send_analytics_data(commit: Commit, upload: ReportSession, version, token): def get_token_for_analytics(commit: Commit, request): repo = commit.repository if isinstance(request.auth, TokenlessAuth): - token = "tokenless_upload" + analytics_token = "tokenless_upload" elif isinstance(request.auth, OrgLevelTokenRepositoryAuth): - token = OrganizationLevelToken.objects.filter(owner=repo.author).first().token + analytics_token = OrganizationLevelToken.objects.filter(owner=repo.author).first().token elif isinstance(request.auth, OIDCTokenRepositoryAuth): - token = "oidc_token_upload" + analytics_token = "oidc_token_upload" else: - token = repo.upload_token - return token + analytics_token = repo.upload_token + return analytics_token class CanDoCoverageUploadsPermission(BasePermission): From 210033215029e4f5218f243b0f29aa179e073ab5 Mon Sep 17 00:00:00 2001 From: Tony Le Date: Tue, 12 Nov 2024 10:34:34 -0500 Subject: [PATCH 18/18] Lints --- upload/views/uploads.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/upload/views/uploads.py b/upload/views/uploads.py index d8a65a7288..837937ab69 100644 --- a/upload/views/uploads.py +++ b/upload/views/uploads.py @@ -39,7 +39,9 @@ log = logging.getLogger(__name__) -def create_upload(serializer, repository, commit, report, is_shelter_request, analytics_token): +def create_upload( + serializer, repository, commit, report, is_shelter_request, analytics_token +): version = ( serializer.validated_data["version"] if "version" in serializer.validated_data @@ -126,7 +128,9 @@ def activate_repo(repository): ) -def send_analytics_data(commit: Commit, upload: ReportSession, version, analytics_token): +def send_analytics_data( + commit: Commit, upload: ReportSession, version, analytics_token +): analytics_upload_data = { "commit": commit.commitid, "branch": commit.branch, @@ -156,7 +160,9 @@ def get_token_for_analytics(commit: Commit, request): if isinstance(request.auth, TokenlessAuth): analytics_token = "tokenless_upload" elif isinstance(request.auth, OrgLevelTokenRepositoryAuth): - analytics_token = OrganizationLevelToken.objects.filter(owner=repo.author).first().token + analytics_token = ( + OrganizationLevelToken.objects.filter(owner=repo.author).first().token + ) elif isinstance(request.auth, OIDCTokenRepositoryAuth): analytics_token = "oidc_token_upload" else: