From a7826d8322777b465825668fba87ce2a3243119a Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Fri, 11 Apr 2025 12:15:52 +0200 Subject: [PATCH 1/2] Improve typing and optimize `Comparison` This primarily improves some typings related to `Comparison/Report`, adds some tracing, and removes the need to initialize the related `Repository` when initializing the `ArchiveService` as that is not needed. --- api/shared/compare/serializers.py | 2 +- graphql_api/actions/comparison.py | 6 ++---- graphql_api/types/commit/commit.py | 5 ++--- graphql_api/types/comparison/comparison.py | 6 ++---- .../types/impacted_file/impacted_file.py | 7 ++++--- graphql_api/types/pull/pull.py | 1 - services/comparison.py | 21 ++++++++++--------- upload/serializers.py | 3 +-- 8 files changed, 23 insertions(+), 28 deletions(-) diff --git a/api/shared/compare/serializers.py b/api/shared/compare/serializers.py index 162fd32387..a851fe289a 100644 --- a/api/shared/compare/serializers.py +++ b/api/shared/compare/serializers.py @@ -131,7 +131,7 @@ def get_lines(self, segment: Segment) -> serializers.ListField: class ImpactedFileSegmentsSerializer(serializers.Serializer): segments = serializers.SerializerMethodField() - def get_segments(self, file_path: str) -> ImpactedFileSegmentSerializer: + def get_segments(self, file_path: str) -> list[ImpactedFileSegmentSerializer]: file_comparison = self.context["comparison"].get_file_comparison( file_path, with_src=True, bypass_max_diff=True ) diff --git a/graphql_api/actions/comparison.py b/graphql_api/actions/comparison.py index 4ad267a7b4..405ef05d42 100644 --- a/graphql_api/actions/comparison.py +++ b/graphql_api/actions/comparison.py @@ -1,5 +1,3 @@ -from typing import Optional, Union - from compare.models import CommitComparison from graphql_api.types.comparison.comparison import ( MissingBaseReport, @@ -9,8 +7,8 @@ def validate_commit_comparison( - commit_comparison: Optional[CommitComparison], -) -> Union[MissingBaseReport, MissingHeadReport, MissingComparison]: + commit_comparison: CommitComparison | None, +) -> MissingBaseReport | MissingHeadReport | MissingComparison | None: if not commit_comparison: return MissingComparison() diff --git a/graphql_api/types/commit/commit.py b/graphql_api/types/commit/commit.py index 1c8173d27c..9516bf9b64 100644 --- a/graphql_api/types/commit/commit.py +++ b/graphql_api/types/commit/commit.py @@ -118,8 +118,8 @@ def resolve_list_uploads(commit: Commit, info: GraphQLResolveInfo, **kwargs): @commit_bindable.field("compareWithParent") @sentry_sdk.trace async def resolve_compare_with_parent( - commit: Commit, info: GraphQLResolveInfo, **kwargs -): + commit: Commit, info: GraphQLResolveInfo, **kwargs: Any +) -> ComparisonReport | Any: if not commit.parent_commit_id: return MissingBaseCommit() @@ -129,7 +129,6 @@ async def resolve_compare_with_parent( ) comparison_error = validate_commit_comparison(commit_comparison=commit_comparison) - if comparison_error: return comparison_error diff --git a/graphql_api/types/comparison/comparison.py b/graphql_api/types/comparison/comparison.py index 19e6651fd3..acf8962c49 100644 --- a/graphql_api/types/comparison/comparison.py +++ b/graphql_api/types/comparison/comparison.py @@ -1,5 +1,5 @@ from asyncio import gather -from typing import List, Optional +from typing import Any, List, Optional import sentry_sdk from ariadne import ObjectType, UnionType @@ -245,9 +245,7 @@ def resolve_flag_comparisons_count( @sync_to_async @sentry_sdk.trace def resolve_has_different_number_of_head_and_base_reports( - comparison: ComparisonReport, - info: GraphQLResolveInfo, - **kwargs, # type: ignore + comparison: ComparisonReport, info: GraphQLResolveInfo, **kwargs: Any ) -> bool: # TODO: can we remove the need for `info.context["comparison"]` here? if "comparison" not in info.context: diff --git a/graphql_api/types/impacted_file/impacted_file.py b/graphql_api/types/impacted_file/impacted_file.py index 19441eb70c..b484299b7c 100644 --- a/graphql_api/types/impacted_file/impacted_file.py +++ b/graphql_api/types/impacted_file/impacted_file.py @@ -1,9 +1,10 @@ import hashlib -from typing import List, Union +from typing import List import sentry_sdk from ariadne import ObjectType, UnionType from asgiref.sync import sync_to_async +from graphql import GraphQLResolveInfo from shared.reports.types import ReportTotals from shared.torngit.exceptions import TorngitClientError @@ -67,8 +68,8 @@ def resolve_hashed_path(impacted_file: ImpactedFile, info) -> str: @sync_to_async @sentry_sdk.trace def resolve_segments( - impacted_file: ImpactedFile, info, filters=None -) -> Union[UnknownPath, ProviderError, SegmentComparisons]: + impacted_file: ImpactedFile, info: GraphQLResolveInfo, filters: dict | None = None +) -> SegmentComparisons | UnknownPath | ProviderError: if filters is None: filters = {} if "comparison" not in info.context: diff --git a/graphql_api/types/pull/pull.py b/graphql_api/types/pull/pull.py index 2845e57148..baf7222b2d 100644 --- a/graphql_api/types/pull/pull.py +++ b/graphql_api/types/pull/pull.py @@ -76,7 +76,6 @@ async def resolve_compare_with_base( commit_comparison = await comparison_loader.load((pull.compared_to, pull.head)) comparison_error = validate_commit_comparison(commit_comparison=commit_comparison) - if comparison_error: return comparison_error diff --git a/services/comparison.py b/services/comparison.py index df6ca410c0..db1e02229f 100644 --- a/services/comparison.py +++ b/services/comparison.py @@ -9,6 +9,7 @@ import minio import pytz +import sentry_sdk import shared.reports.api_report_service as report_service from asgiref.sync import async_to_sync from django.db.models import Prefetch, QuerySet @@ -756,7 +757,7 @@ def head_report_without_applied_diff(self): return report @cached_property - def has_different_number_of_head_and_base_sessions(self): + def has_different_number_of_head_and_base_sessions(self) -> bool: """ This method checks if the head and the base have different number of sessions. It makes use of the head_report_without_applied_diff property instead of the @@ -989,10 +990,10 @@ class ComparisonReport(object): on a `CommitComparison` """ - commit_comparison: CommitComparison = None + commit_comparison: CommitComparison @cached_property - def files(self) -> List[ImpactedFile]: + def files(self) -> list[ImpactedFile]: if not self.commit_comparison.report_storage_path: return [] @@ -1001,29 +1002,29 @@ def files(self) -> List[ImpactedFile]: ImpactedFile.create(**data) for data in comparison_data.get("files", []) ] - def impacted_file(self, path: str) -> Optional[ImpactedFile]: + def impacted_file(self, path: str) -> ImpactedFile | None: for file in self.files: if file.head_name == path: return file - @cached_property - def impacted_files(self) -> List[ImpactedFile]: + @property + def impacted_files(self) -> list[ImpactedFile]: return self.files @cached_property - def impacted_files_with_unintended_changes(self) -> List[ImpactedFile]: + def impacted_files_with_unintended_changes(self) -> list[ImpactedFile]: return [file for file in self.files if file.has_changes] @cached_property - def impacted_files_with_direct_changes(self) -> List[ImpactedFile]: + def impacted_files_with_direct_changes(self) -> list[ImpactedFile]: return [file for file in self.files if file.has_diff or not file.has_changes] + @sentry_sdk.trace def _fetch_raw_comparison_data(self) -> dict: """ Fetches the raw comparison data from storage """ - repository = self.commit_comparison.compare_commit.repository - archive_service = ArchiveService(repository) + archive_service = ArchiveService(repository=None) try: data = archive_service.read_file(self.commit_comparison.report_storage_path) return json.loads(data) diff --git a/upload/serializers.py b/upload/serializers.py index f092c6f7d5..b46e047863 100644 --- a/upload/serializers.py +++ b/upload/serializers.py @@ -51,8 +51,7 @@ class Meta: raw_upload_location = serializers.SerializerMethodField() def get_raw_upload_location(self, obj: ReportSession) -> str: - repo = obj.report.commit.repository - archive_service = ArchiveService(repo) + archive_service = ArchiveService(repository=None) return archive_service.create_presigned_put(obj.storage_path) def get_url(self, obj: ReportSession) -> str: From e11aaf3c402a9c621a6c364017625d45187bda9a Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Fri, 11 Apr 2025 13:17:27 +0200 Subject: [PATCH 2/2] update shared as well --- pyproject.toml | 2 +- uv.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index da15598fef..27cd56fc05 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -74,4 +74,4 @@ dev-dependencies = [ ] [tool.uv.sources] -shared = { git = "https://github.com/codecov/shared", rev = "a2350552f09580e26a9356b30353d02fe0832855" } +shared = { git = "https://github.com/codecov/shared", rev = "940103554b6072f61ad8534fcbefdab217138295" } diff --git a/uv.lock b/uv.lock index f02958c815..970286595f 100644 --- a/uv.lock +++ b/uv.lock @@ -391,7 +391,7 @@ requires-dist = [ { name = "requests", specifier = "==2.32.3" }, { name = "sentry-sdk", specifier = ">=2.25.1" }, { name = "sentry-sdk", extras = ["celery"], specifier = ">=2.25.1" }, - { name = "shared", git = "https://github.com/codecov/shared?rev=a2350552f09580e26a9356b30353d02fe0832855" }, + { name = "shared", git = "https://github.com/codecov/shared?rev=940103554b6072f61ad8534fcbefdab217138295" }, { name = "simplejson", specifier = "==3.17.2" }, { name = "starlette", specifier = "==0.40.0" }, { name = "stripe", specifier = ">=11.4.1" }, @@ -1599,7 +1599,7 @@ celery = [ [[package]] name = "shared" version = "0.1.0" -source = { git = "https://github.com/codecov/shared?rev=a2350552f09580e26a9356b30353d02fe0832855#a2350552f09580e26a9356b30353d02fe0832855" } +source = { git = "https://github.com/codecov/shared?rev=940103554b6072f61ad8534fcbefdab217138295#940103554b6072f61ad8534fcbefdab217138295" } dependencies = [ { name = "amplitude-analytics" }, { name = "boto3" },