Refactor and optimize querying pathContents (#1141)

Author: Swatinem

graphql_api/actions/path_contents.py

@@ -1,46 +1,18 @@
-from typing import Iterable, Union
-
 import sentry_sdk
 
 from graphql_api.types.enums import PathContentDisplayType
 from services.path import Dir, File
 
 
-def partition_list_into_files_and_directories(
-    items: Iterable[Union[File, Dir]],
-) -> tuple[Union[File, Dir]]:
-    files = []
-    directories = []
-
-    # Separate files and directories
-    for item in items:
-        if isinstance(item, Dir):
-            directories.append(item)
-        else:
-            files.append(item)
-
-    return (files, directories)
-
-
-def sort_list_by_directory(
-    items: Iterable[Union[File, Dir]],
-) -> Iterable[Union[File, Dir]]:
-    (files, directories) = partition_list_into_files_and_directories(items=items)
-    return directories + files
-
-
 @sentry_sdk.trace
-def sort_path_contents(
-    items: Iterable[Union[File, Dir]], filters={}
-) -> Iterable[Union[File, Dir]]:
+def sort_path_contents(items: list[File | Dir], filters: dict = {}) -> list[File | Dir]:
     filter_parameter = filters.get("ordering", {}).get("parameter")
     filter_direction = filters.get("ordering", {}).get("direction")
 
     if filter_parameter and filter_direction:
         parameter_value = filter_parameter.value
         direction_value = filter_direction.value
-        items = sorted(
-            items,
+        items.sort(
             key=lambda item: getattr(item, parameter_value),
             reverse=direction_value == "descending",
         )
@@ -49,6 +21,6 @@ def sort_path_contents(
             parameter_value == "name"
             and display_type is not PathContentDisplayType.LIST
         ):
-            items = sort_list_by_directory(items=items)
+            items.sort(key=lambda item: isinstance(item, File))
 
     return items

graphql_api/tests/test_branch.py

@@ -154,6 +154,11 @@ class MockSession(object):
     pass
 
 
+class MockFile:
+    def __init__(self, name: str):
+        self.name = name
+
+
 class MockReport(object):
     def __init__(self):
         self.sessions = {1: MockSession()}
@@ -171,6 +176,10 @@ def files(self):
             "folder/subfolder/fileD.py",
         ]
 
+    def __iter__(self):
+        for name in self.files:
+            yield MockFile(name)
+
     @property
     def flags(self):
         return {"flag-a": MockFlag()}
@@ -774,11 +783,9 @@ def test_fetch_path_contents_component_filter_missing_coverage(
     @patch("services.components.component_filtered_report")
     @patch("services.components.commit_components")
     @patch("shared.reports.api_report_service.build_report_from_commit")
-    @patch("services.report.files_in_sessions")
     def test_fetch_path_contents_component_filter_has_coverage(
-        self, session_files_mock, report_mock, commit_components_mock, filtered_mock
+        self, report_mock, commit_components_mock, filtered_mock
     ):
-        session_files_mock.return_value = MockReport().files
         components = ["Global"]
         variables = {
             "org": self.org.username,

graphql_api/types/commit/commit.py

@@ -14,6 +14,7 @@
 import services.components as components_service
 import services.path as path_service
 from codecov.db import sync_to_async
+from codecov_auth.models import Owner
 from core.models import Commit
 from graphql_api.actions.commits import commit_status, commit_uploads
 from graphql_api.actions.comparison import validate_commit_comparison
@@ -44,7 +45,7 @@
 from services.bundle_analysis import BundleAnalysisComparison, BundleAnalysisReport
 from services.comparison import Comparison, ComparisonReport
 from services.components import Component
-from services.path import ReportPaths
+from services.path import Dir, File, ReportPaths
 from services.profiling import CriticalFile, ProfilingSummary
 from services.yaml import (
     YamlStates,
@@ -146,23 +147,19 @@ def resolve_critical_files(commit: Commit, info, **kwargs) -> List[CriticalFile]
     return profiling_summary.critical_files
 
 
-@sentry_sdk.trace
-@commit_bindable.field("pathContents")
-@sync_to_async
-def resolve_path_contents(commit: Commit, info, path: str = None, filters=None):
-    """
-    The file directory tree is a list of all the files and directories
-    extracted from the commit report of the latest, head commit.
-    The is resolver results in a list that represent the tree with files
-    and nested directories.
-    """
-    current_owner = info.context["request"].current_owner
-
+def get_sorted_path_contents(
+    current_owner: Owner,
+    commit: Commit,
+    path: str | None = None,
+    filters: dict | None = None,
+) -> (
+    list[File | Dir] | MissingHeadReport | MissingCoverage | UnknownFlags | UnknownPath
+):
     # TODO: Might need to add reports here filtered by flags in the future
-    commit_report = report_service.build_report_from_commit(
+    report = report_service.build_report_from_commit(
         commit, report_class=ReadOnlyReport
     )
-    if not commit_report:
+    if not report:
         return MissingHeadReport()
 
     if filters is None:
@@ -189,9 +186,9 @@ def resolve_path_contents(commit: Commit, info, path: str = None, filters=None):
 
         for component in filtered_components:
             component_paths.extend(component.paths)
-            if commit_report.flags:
+            if report.flags:
                 component_flags.extend(
-                    component.get_matching_flags(commit_report.flags.keys())
+                    component.get_matching_flags(report.flags.keys())
                 )
 
     if component_flags:
@@ -200,11 +197,11 @@ def resolve_path_contents(commit: Commit, info, path: str = None, filters=None):
         else:
             flags_filter = component_flags
 
-    if flags_filter and not commit_report.flags:
+    if flags_filter and not report.flags:
         return UnknownFlags(f"No coverage with chosen flags: {flags_filter}")
 
     report_paths = ReportPaths(
-        report=commit_report,
+        report=report,
         path=path,
         search_term=search_value,
         filter_flags=flags_filter,
@@ -221,25 +218,23 @@ def resolve_path_contents(commit: Commit, info, path: str = None, filters=None):
         # we're just missing coverage for the file
         return MissingCoverage(f"missing coverage for path: {path}")
 
+    items: list[File | Dir]
     if search_value or display_type == PathContentDisplayType.LIST:
         items = report_paths.full_filelist()
     else:
         items = report_paths.single_directory()
-    return {"results": sort_path_contents(items, filters)}
+    return sort_path_contents(items, filters)
 
 
-@commit_bindable.field("deprecatedPathContents")
+@sentry_sdk.trace
+@commit_bindable.field("pathContents")
 @sync_to_async
-def resolve_deprecated_path_contents(
+def resolve_path_contents(
     commit: Commit,
-    info,
-    path: str = None,
-    filters=None,
-    first=None,
-    after=None,
-    last=None,
-    before=None,
-):
+    info: GraphQLResolveInfo,
+    path: str | None = None,
+    filters: dict | None = None,
+) -> Any:
     """
     The file directory tree is a list of all the files and directories
     extracted from the commit report of the latest, head commit.
@@ -248,80 +243,43 @@ def resolve_deprecated_path_contents(
     """
     current_owner = info.context["request"].current_owner
 
-    # TODO: Might need to add reports here filtered by flags in the future
-    commit_report = report_service.build_report_from_commit(
-        commit, report_class=ReadOnlyReport
-    )
-    if not commit_report:
-        return MissingHeadReport()
-
-    if filters is None:
-        filters = {}
-    search_value = filters.get("search_value")
-    display_type = filters.get("display_type")
-
-    flags_filter = filters.get("flags", [])
-    component_filter = filters.get("components", [])
-
-    component_paths = []
-    component_flags = []
-
-    if component_filter:
-        all_components = components_service.commit_components(commit, current_owner)
-        filtered_components = components_service.filter_components_by_name_or_id(
-            all_components, component_filter
-        )
-
-        if not filtered_components:
-            return MissingCoverage(
-                f"missing coverage for report with components: {component_filter}"
-            )
-
-        for component in filtered_components:
-            component_paths.extend(component.paths)
-            if commit_report.flags:
-                component_flags.extend(
-                    component.get_matching_flags(commit_report.flags.keys())
-                )
-
-    if component_flags:
-        if flags_filter:
-            flags_filter = list(set(flags_filter) & set(component_flags))
-        else:
-            flags_filter = component_flags
+    contents = get_sorted_path_contents(current_owner, commit, path, filters)
+    if isinstance(contents, list):
+        return {"results": contents}
+    return contents
 
-    if flags_filter and not commit_report.flags:
-        return UnknownFlags(f"No coverage with chosen flags: {flags_filter}")
 
-    report_paths = ReportPaths(
-        report=commit_report,
-        path=path,
-        search_term=search_value,
-        filter_flags=flags_filter,
-        filter_paths=component_paths,
-    )
-
-    if len(report_paths.paths) == 0:
-        # we do not know about this path
-
-        if path_service.provider_path_exists(path, commit, current_owner) is False:
-            # file doesn't exist
-            return UnknownPath(f"path does not exist: {path}")
-
-        # we're just missing coverage for the file
-        return MissingCoverage(f"missing coverage for path: {path}")
-
-    if search_value or display_type == PathContentDisplayType.LIST:
-        items = report_paths.full_filelist()
-    else:
-        items = report_paths.single_directory()
-
-    sorted_items = sort_path_contents(items, filters)
+@commit_bindable.field("deprecatedPathContents")
+@sync_to_async
+def resolve_deprecated_path_contents(
+    commit: Commit,
+    info: GraphQLResolveInfo,
+    path: str | None = None,
+    filters: dict | None = None,
+    first: Any = None,
+    after: Any = None,
+    last: Any = None,
+    before: Any = None,
+) -> Any:
+    """
+    The file directory tree is a list of all the files and directories
+    extracted from the commit report of the latest, head commit.
+    The is resolver results in a list that represent the tree with files
+    and nested directories.
+    """
+    current_owner = info.context["request"].current_owner
 
-    kwargs = {"first": first, "after": after, "last": last, "before": before}
+    contents = get_sorted_path_contents(current_owner, commit, path, filters)
+    if not isinstance(contents, list):
+        return contents
 
     return queryset_to_connection_sync(
-        sorted_items, ordering_direction=OrderingDirection.ASC, **kwargs
+        contents,
+        ordering_direction=OrderingDirection.ASC,
+        first=first,
+        last=last,
+        before=before,
+        after=after,
     )