Skip to content

Commit

Permalink
Avoid fetching unnecessary data when posting review requests.
Browse files Browse the repository at this point in the history
The way the RBTools Python API works, it's annoyingly easy to make GET
requests to list resources when we don't actually care about the
existing data, we just want an instance of the list resource in order to
to do POST operations.

This change fixes `rbt post` to minimize this in two big cases:
- We were fetching the review request list when creating a new review
  request. We had `only-fields=` on this request, so it wasn't actually
  serializing very much data, but it would still query and return a list
  of up to 25 review requests.
- We were fetching up to 25 diff file attachments when we wanted to
  upload binary files, including all of their data. Moreover, we were
  doing this for each binary file we wanted to upload.

With this fix, we now pass max_results=0 to the get operation. This
means we'll get the proper MIME types and links which are used to do
further operations, but we won't fetch any existing data from the
database.

The file attachments resource is now saved once instead of fetching it
for each file.

This also fixes up the DiffFileAttachmentListResource upload method to
use the `create` link (before it would inherit the ?max-results=0 from
the GET).

Testing Done:
Posted changes and looked carefully at the --debug output. Verified that
we were no longer doing unnecessary or unnecessarily heavy API requests.

Reviewed at https://reviews.reviewboard.org/r/13785/
  • Loading branch information
davidt committed Apr 25, 2024
1 parent 6090b90 commit 40069e0
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 10 deletions.
2 changes: 1 addition & 1 deletion rbtools/api/resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -1452,7 +1452,7 @@ def upload_attachment(
rbtools.api.request.HttpRequest:
The request object.
"""
request = HttpRequest(self._url, method='POST', query_args=kwargs)
request = self.create(query_args=kwargs, internal=True)
request.add_file('path', filename, content)
request.add_field('filediff', filediff_id)

Expand Down
41 changes: 32 additions & 9 deletions rbtools/commands/post.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

if TYPE_CHECKING:
from rbtools.api.resource import (
DiffFileAttachmentListResource,
DraftDiffCommitItemResource,
FileDiffResource,
ReviewRequestResource,
Expand Down Expand Up @@ -434,6 +435,22 @@ class Post(BaseCommand):
BaseCommand.tfs_options,
]

######################
# Instance variables #
######################

#: Whether both the server and tool can upload binary files to diffs.
#:
#: Version Added:
#: 5.0
can_upload_binary_files: bool

#: The DiffFileAttachments resource for the repository.
#:
#: Version Added:
#: 5.0
diff_file_attachments_resource: Optional[DiffFileAttachmentListResource]

def post_process_options(self):
super(Post, self).post_process_options()

Expand Down Expand Up @@ -653,7 +670,7 @@ def post_request(

try:
review_requests = self.api_root.get_review_requests(
only_fields='',
max_results=0,
only_links='create')
review_request = review_requests.create(**request_data)
except APIError as e:
Expand Down Expand Up @@ -876,6 +893,12 @@ def main(self, *args):
server_supports_history = self.capabilities.has_capability(
'review_requests', 'supports_history')

self.can_upload_binary_files = (
self.capabilities.has_capability('diffs', 'file_attachments') and
self.tool.can_get_file_content)

self.diff_file_attachments_resource = None

# If we are passing --diff-filename, we attempt to read the diff before
# we normally would. This allows us to exit early if the file does not
# exist or cannot be read and save several network requests.
Expand Down Expand Up @@ -1544,8 +1567,7 @@ def _post_diff_history(
# We now need to go through and upload any binary files.
assert self.tool is not None

if (self.capabilities.has_capability('diffs', 'file_attachments') and
self.tool.can_get_file_content):
if self.can_upload_binary_files:
files_to_upload: list[FileDiffResource] = []

for commit in commits:
Expand Down Expand Up @@ -1603,9 +1625,7 @@ def _post_squashed_diff(
diff = diff_resource.upload_diff(
squashed_diff.diff, **diff_kwargs)

if (self.capabilities.has_capability('diffs', 'file_attachments') and
self.tool.can_get_file_content):

if self.can_upload_binary_files:
files_to_upload = list(
diff.get_draft_files(binary=True).all_items)

Expand All @@ -1628,6 +1648,11 @@ def _upload_binary_files(
assert self.capabilities is not None
assert self.repository is not None
assert self.tool is not None
assert self.can_upload_binary_files

if self.diff_file_attachments_resource is None:
self.diff_file_attachments_resource = \
self.repository.get_diff_file_attachments(max_results=0)

supported_mimetypes = [
parse_mimetype(mimetype) for mimetype in
Expand All @@ -1638,8 +1663,6 @@ def _upload_binary_files(
valid_mimetypes = set()
invalid_mimetypes = set()

diff_file_attachments = self.repository.get_diff_file_attachments()

iterable = self._show_progress(
iterable=files_to_upload,
desc='Uploading binary files...',
Expand Down Expand Up @@ -1748,7 +1771,7 @@ def _upload_binary_files(
logger.debug('Uploading file "%s" revision %s (%s)',
filename, revision, mimetype)

diff_file_attachments.upload_attachment(
self.diff_file_attachments_resource.upload_attachment(
filename=os.path.basename(filename),
content=file_content,
filediff_id=file.id)
Expand Down

0 comments on commit 40069e0

Please sign in to comment.