From 810caafa4c48678a5bfc0311e0c0e64624bfa9b7 Mon Sep 17 00:00:00 2001 From: Adrian Date: Wed, 26 Feb 2025 10:34:48 -0800 Subject: [PATCH 1/2] Adjust call used in comparison file to avoid provider api call --- graphql_api/types/comparison/comparison.py | 4 --- services/comparison.py | 39 ++++++++++++++++++---- services/tests/test_comparison.py | 8 +++-- 3 files changed, 39 insertions(+), 12 deletions(-) diff --git a/graphql_api/types/comparison/comparison.py b/graphql_api/types/comparison/comparison.py index 36eeb53fa4..9291b7a05b 100644 --- a/graphql_api/types/comparison/comparison.py +++ b/graphql_api/types/comparison/comparison.py @@ -252,10 +252,6 @@ def resolve_has_different_number_of_head_and_base_reports( if "comparison" not in info.context: return False comparison: Comparison = info.context["comparison"] - try: - comparison.validate() - except Exception: - return False return comparison.has_different_number_of_head_and_base_sessions diff --git a/services/comparison.py b/services/comparison.py index 144f7d2ded..ba45c9a3cd 100644 --- a/services/comparison.py +++ b/services/comparison.py @@ -728,14 +728,41 @@ def head_report(self): pass return report + @cached_property + def head_report_without_applied_diff(self): + """ + This is a variant to the head_report property without having an applied diff. + This is created because applying the diff calls the provider, which adds a + diff_totals key to the head_report object as well as adjusting the diff_total + values in each session. This variant should only be used if you are not using + any diff related data, as it saves an unnecessary request to the provider otherwise. + """ + try: + report = report_service.build_report_from_commit(self.head_commit) + except minio.error.S3Error as e: + if e.code == "NoSuchKey": + raise MissingComparisonReport("Missing head report") + else: + raise e + + return report + @cached_property def has_different_number_of_head_and_base_sessions(self): - log.info("has_different_number_of_head_and_base_sessions - Start") - head_sessions = self.head_report.sessions - base_sessions = self.base_report.sessions - log.info( - f"has_different_number_of_head_and_base_sessions - Retrieved sessions - head {len(head_sessions)} / base {len(base_sessions)}" - ) + """ + 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 + head_report one as it doesn't need diff related data for this computation (see + the description of that property above for more context). + This method should be replaced with a direct call to the report_uploads table instead, + but leaving the implementation the same for now for consistency. + """ + try: + head_sessions = self.head_report_without_applied_diff.sessions + base_sessions = self.base_report.sessions + except Exception: + return False + # We're treating this case as false since considering CFF's complicates the logic if self._has_cff_sessions(head_sessions) or self._has_cff_sessions( base_sessions diff --git a/services/tests/test_comparison.py b/services/tests/test_comparison.py index 34b0c4c4f8..68573932e1 100644 --- a/services/tests/test_comparison.py +++ b/services/tests/test_comparison.py @@ -925,13 +925,17 @@ def test_head_and_base_reports_have_cff_sessions( fc = self.comparison.has_different_number_of_head_and_base_sessions assert fc == False + @patch( + "services.comparison.Comparison.head_report_without_applied_diff", + new_callable=PropertyMock, + ) def test_head_and_base_reports_have_different_number_of_reports( - self, base_report_mock, head_report_mock, _ + self, head_report_no_diff_mock, base_report_mock, head_report_mock, _ ): # Only relevant files keys to the session object head_report_sessions = {"0": {"st": "uploaded"}, "1": {"st": "uploaded"}} head_report = SerializableReport(sessions=head_report_sessions) - head_report_mock.return_value = head_report + head_report_no_diff_mock.return_value = head_report base_report_sessions = {"0": {"st": "uploaded"}} base_report = SerializableReport(sessions=base_report_sessions) base_report_mock.return_value = base_report From f49d5d458a16fc1db5a25f45b0005a047cfbc8c9 Mon Sep 17 00:00:00 2001 From: Adrian Date: Wed, 26 Feb 2025 18:31:02 -0800 Subject: [PATCH 2/2] Remove unecessary log lines --- services/comparison.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/services/comparison.py b/services/comparison.py index ba45c9a3cd..06b071ac9f 100644 --- a/services/comparison.py +++ b/services/comparison.py @@ -773,12 +773,9 @@ def has_different_number_of_head_and_base_sessions(self): # I feel this method should belong to the API Report class, but we're thinking of getting rid of that class soon # In truth, this should be in the shared.Report class def _has_cff_sessions(self, sessions) -> bool: - log.info(f"_has_cff_sessions - sessions count {len(sessions)}") for session in sessions.values(): if session.session_type.value == "carriedforward": - log.info("_has_cff_sessions - Found carriedforward") return True - log.info("_has_cff_sessions - No carriedforward") return False @property