Do not double-filter Reports in ReportPaths

Previously, the `ReportPaths.files` accessor would manually filter through the `unfiltered_report`.
Even though the `self.report` was already filtered.

Now the custom filtering logic is a bit updated, but taking care of existing bugs in `FilteredReport`, as that is still yielding files even though they are not matching any `flags`.

Author: Swatinem

graphql_api/tests/test_branch.py

@@ -772,18 +772,15 @@ 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.path.ReportPaths.files", new_callable=PropertyMock)
     def test_fetch_path_contents_component_filter_has_coverage(
-        self, report_mock, commit_components_mock, filtered_mock
+        self, files_mock, report_mock, commit_components_mock, filtered_mock
     ):
-        components = ["Global"]
-        variables = {
-            "org": self.org.username,
-            "repo": self.repo.name,
-            "branch": self.branch.name,
-            "path": "",
-            "filters": {"components": components},
-        }
-
+        files_mock.return_value = [
+            "folder/fileB.py",
+            "folder/subfolder/fileC.py",
+            "folder/subfolder/fileD.py",
+        ]
         report_mock.return_value = MockReport()
         commit_components_mock.return_value = [
             Component.from_dict(
@@ -810,6 +807,14 @@ def test_fetch_path_contents_component_filter_has_coverage(
         ]
         filtered_mock.return_value = MockReport()
 
+        components = ["Global"]
+        variables = {
+            "org": self.org.username,
+            "repo": self.repo.name,
+            "branch": self.branch.name,
+            "path": "",
+            "filters": {"components": components},
+        }
         data = self.gql_request(query_files, variables=variables)
 
         assert data == {
@@ -841,18 +846,15 @@ def test_fetch_path_contents_component_filter_has_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_belonging_to_flags")
-    @patch("services.report.files_in_sessions")
+    @patch("services.path.ReportPaths.files", new_callable=PropertyMock)
     def test_fetch_path_contents_component_and_flag_filters(
         self,
-        session_files_mock,
-        flag_files_mock,
+        files_mock,
         report_mock,
         commit_components_mock,
         filtered_mock,
     ):
-        session_files_mock.return_value = ["fileA.py"]
-        flag_files_mock.return_value = ["fileA.py"]
+        files_mock.return_value = ["fileA.py"]
         report_mock.return_value = MockReport()
         commit_components_mock.return_value = [
             Component.from_dict(
@@ -945,11 +947,11 @@ def test_fetch_path_contents_component_and_flag_filters(
     @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_belonging_to_flags")
+    @patch("services.path.ReportPaths.files", new_callable=PropertyMock)
     def test_fetch_path_contents_component_and_flag_filters_unknown_flags(
-        self, flag_files_mock, report_mock, commit_components_mock, filtered_mock
+        self, files_mock, report_mock, commit_components_mock, filtered_mock
     ):
-        flag_files_mock.return_value = ["fileA.py"]
+        files_mock.return_value = ["fileA.py"]
         report_mock.return_value = MockNoFlagsReport()
         commit_components_mock.return_value = [
             Component.from_dict(
@@ -1035,18 +1037,15 @@ def test_fetch_path_contents_component_and_flag_filters_unknown_flags(
     @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_belonging_to_flags")
-    @patch("services.report.files_in_sessions")
+    @patch("services.path.ReportPaths.files", new_callable=PropertyMock)
     def test_fetch_path_contents_component_flags_filters(
         self,
-        session_files_mock,
-        flag_files_mock,
+        files_mock,
         report_mock,
         commit_components_mock,
         filtered_mock,
     ):
-        session_files_mock.return_value = ["fileA.py"]
-        flag_files_mock.return_value = ["fileA.py"]
+        files_mock.return_value = ["fileA.py"]
         report_mock.return_value = MockReport()
         commit_components_mock.return_value = [
             Component.from_dict(

services/path.py

@@ -9,6 +9,7 @@
 from asgiref.sync import async_to_sync
 from django.conf import settings
 from shared.reports.resources import Report
+from shared.reports.filtered import FilteredReport, FilteredReportFile
 from shared.reports.types import ReportTotals
 from shared.torngit.exceptions import TorngitClientError
 from shared.utils.match import Matcher
@@ -150,13 +151,12 @@ def __init__(
         report: Report,
         path: str | None = None,
         search_term: str | None = None,
-        filter_flags: list[str] = [],
-        filter_paths: list[str] = [],
+        filter_flags: list[str] = None,
+        filter_paths: list[str] = None,
     ):
-        self.report = report
-        self.unfiltered_report = report
-        self.filter_flags = filter_flags
-        self.filter_paths = filter_paths
+        self.report: Report | FilteredReport = report
+        self.filter_flags = filter_flags or []
+        self.filter_paths = filter_paths or []
         self.prefix = path or ""
 
         # Filter report if flags or paths exist
@@ -172,37 +172,37 @@ def __init__(
         ]
 
         if search_term:
+            search_term = search_term.lower()
             self._paths = [
                 path
-                for path in self.paths
-                if search_term.lower() in path.relative_path.lower()
+                for path in self._paths
+                if search_term in path.relative_path.lower()
             ]
 
     @cached_property
     def files(self) -> list[str]:
-        # No filtering, just return files in Report
-        if not self.filter_flags and not self.filter_paths:
+        # No flags filtering, just return (path-filtered) files in Report
+        if not self.filter_flags:
             return self.report.files
 
+        # When there is a flag filter, `FilteredReport` currently yields
+        # `FilteredReportFile`s without actually checking whether they match the sessions.
+        # Before that bug is fixed, lets do the filtering manually here. Once that bug is fixed,
+        # this should just forward to `self.report.files` like above.
         files = []
-        # Do flag filtering if needed
-        if self.filter_flags:
-            files = report_service.files_belonging_to_flags(
-                commit_report=self.unfiltered_report, flags=self.filter_flags
-            )
-        else:
-            files = [file.name for file in self.unfiltered_report]
-
-        # Do path filtering if needed
-        if self.filter_paths:
-            matcher = Matcher(self.filter_paths)
-            files = [file for file in files if matcher.match(file)]
-
+        for file in self.report:
+            if isinstance(file, FilteredReportFile):
+                found = False
+                for _ln, line in file.lines:
+                    if line and any(s.id in file.session_ids for s in line.sessions):
+                        found = True
+                        break
+                if not found:
+                    continue
+
+            files.append(file.name)
         return files
 
-    def _filter_commit_report(self) -> None:
-        self.report = self.report.filter(flags=self.filter_flags)
-
     @property
     def paths(self) -> list[PrefixedPath]:
         return self._paths