Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust call used in comparison file to avoid provider api call #1176

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions graphql_api/types/comparison/comparison.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didnt specify but this is implicitly moved in the try/except inside the resolver logic

comparison.validate()
except Exception:
return False
return comparison.has_different_number_of_head_and_base_sessions


Expand Down
39 changes: 33 additions & 6 deletions services/comparison.py
Original file line number Diff line number Diff line change
Expand Up @@ -728,14 +728,41 @@
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")

Check warning on line 744 in services/comparison.py

View check run for this annotation

Codecov Notifications / codecov/patch

services/comparison.py#L742-L744

Added lines #L742 - L744 were not covered by tests
else:
raise e

Check warning on line 746 in services/comparison.py

View check run for this annotation

Codecov Notifications / codecov/patch

services/comparison.py#L746

Added line #L746 was not covered by tests

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
Expand Down
8 changes: 6 additions & 2 deletions services/tests/test_comparison.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading