Repository: SFDO-Tooling/CumulusCI
Pull request #3379 (2022-09-30 20:19:06)

Use parent_tree in CommitDir  to fix timeouts with large repositories
Users have reported 502 errors (i.e., timeouts) when using CommitDir to create a commit when the repository is above a certain size. Because CommitDir uses GitHub's [Git Database API] directly to create blob and tree objects before authoring a commit, it made too many calls and triggered rate limiting. For very large repositories (e.g., NPSP), even very small changes will throw an exception because CommitDir constructs the entire Tree and creates a blob for each file that is new or modified.

This PR resolves this issue with two changes:

1. CommitDir now creates a smaller tree object by omitting unchanged files. By using the `parent_tree` property in the `create_tree` request, we only need to include the blobs that are new, modified, or deleted, meaning the overall tree is much smaller.
2. CommitDir no longer calls the `blob` endpoint for each new or modified file before constructing the new tree. Github will accept property `contents` with the text contents of a file, so only binary files need to create a blob first. This saves a number of API calls and avoids triggering secondary rate limits.

[Git Database API]: https://docs.github.com/en/rest/git

Fixes SFDO-Tooling/Metecho#1303


Patch:
diff --git a/cumulusci/tasks/github/util.py b/cumulusci/tasks/github/util.py
index e87c5af73..c838a20bd 100644
--- a/cumulusci/tasks/github/util.py
+++ b/cumulusci/tasks/github/util.py
@@ -1,8 +1,11 @@
 import base64
 import hashlib
-import io
 import logging
 import os
+from pathlib import Path
+from typing import Optional, Tuple
+
+from github3.git import Commit, Tree
 
 from cumulusci.core.exceptions import GithubException
 
@@ -20,12 +23,12 @@ class CommitDir(object):
         if not logger:
             logger = logging.getLogger(os.path.basename(__file__))
         self.logger = logger
-        self.author = author if author else {}
+        self.author = author or {}
 
     def _validate_dirs(self, local_dir, repo_dir):
         local_dir = os.path.abspath(local_dir)
         if not os.path.isdir(local_dir):
-            raise GithubException("Not a dir: {}".format(local_dir))
+            raise GithubException(f"Not a dir: {local_dir}")
         # do not use os.path because repo_dir is not local
         if repo_dir is None:
             repo_dir = ""
@@ -57,11 +60,11 @@ class CommitDir(object):
         self.new_tree_list = [item for item in self.new_tree_list if item]
         self._add_new_files_to_tree(self.new_tree_list)
 
-        tree_unchanged = self._summarize_changes(self.new_tree_list)
-        if tree_unchanged:
+        if not self.new_tree_list:
             self.logger.warning("No changes found, aborting commit")
             return self.parent_commit
 
+        self._summarize_changes(self.tree, self.new_tree_list)
         new_tree = self._create_tree(self.new_tree_list)
         new_commit = self._create_commit(commit_message, new_tree)
         self._update_head(new_commit)
@@ -69,94 +72,101 @@ class CommitDir(object):
 
     def _set_git_data(self, branch):
         # get ref to branch HEAD
-        self.head = self.repo.ref("heads/{}".format(branch))
+        self.head = self.repo.ref(f"heads/{branch}")
 
         # get commit pointed to by HEAD
-        self.parent_commit = self.repo.git_commit(self.head.object.sha)
+        self.parent_commit: Commit = self.repo.git_commit(self.head.object.sha)
 
         # get tree of commit as dict
-        self.tree = self.repo.tree(self.parent_commit.tree.sha, recursive=True)
+        orig_tree: Tree = self.repo.tree(self.parent_commit.tree.sha, recursive=True)
         self.tree = [
-            self._git_hash_to_dict(git_hash)
-            for git_hash in self.tree.tree
-            if git_hash.type != "tree"
+            git_hash.as_dict() for git_hash in orig_tree.tree if git_hash.type != "tree"
         ]
 
-    def _git_hash_to_dict(self, git_hash):
-        return {
-            "sha": git_hash.sha,
-            "mode": git_hash.mode,
-            "path": git_hash.path,
-            "size": git_hash.size,
-            "type": git_hash.type,
-        }
-
-    def _create_new_tree_item(self, item):
+    def _create_new_tree_item(self, item: dict) -> Optional[dict]:
+        """Given an Git tree element, returns None if it is unchanged, or
+        a new element if it has been deleted or updated.
+        """
         if not item["path"].startswith(self.repo_dir):
             # outside target dir in repo - keep in tree
-            self.logger.debug(
-                "Unchanged (outside target path): {}".format(item["path"])
-            )
-            return item
+            self.logger.debug(f'Unchanged (outside target path): {item["path"]}')
+            return None
 
-        local_file, content = self._read_item_content(item)
+        local_path, content = self._find_and_read_item(item)
         new_item = item.copy()
         if content is None:
-            # delete blob from tree
+            # delete blob from tree by setting 'sha' to null
             self.logger.debug(f"Delete: {item['path']}")
-            return content
+            new_item["sha"] = None
         elif self._item_changed(item, content):
-            self.logger.debug("Update: {}".format(local_file))
-            blob_sha = self._create_blob(content, local_file)
-            new_item["sha"] = blob_sha
+            # we need to delete the sha key because GH returns an error if
+            # both 'sha' and 'contents' are passed.
+            self.logger.debug(f"Update: {local_path}")
+            new_item.pop("sha", None)
+            new_item.update(self._get_content_or_sha(content, self.dry_run))
         else:
-            self.logger.debug("Unchanged: {}".format(item["path"]))
+            self.logger.debug(f'Unchanged: {item["path"]}')
+            return None
         return new_item
 
     def _add_new_files_to_tree(self, new_tree_list):
         new_tree_target_subpaths = [
             self._get_item_sub_path(item)
-            for item in new_tree_list
+            for item in self.tree
             if item["path"].startswith(self.repo_dir)
         ]
 
-        for root, dirs, files in os.walk(self.local_dir):
+        for root, _, files in os.walk(self.local_dir):
             for filename in files:
                 if not filename.startswith("."):
                     local_file = os.path.join(root, filename)
                     local_file_subpath = local_file[(len(self.local_dir) + 1) :]
                     if local_file_subpath not in new_tree_target_subpaths:
-                        with io.open(local_file, "rb") as f:
-                            content = f.read()
-                        repo_path = (self.repo_dir + "/") if self.repo_dir else ""
+                        repo_path = f"{self.repo_dir}/" if self.repo_dir else ""
                         new_item = {
-                            "path": "{}{}".format(
-                                repo_path, local_file_subpath.replace(os.sep, "/")
-                            ),
-                            "mode": "100644",
+                            "path": f'{repo_path}{local_file_subpath.replace(os.sep, "/")}',
+                            "mode": "100644",  # FIXME: This is wrong
                             "type": "blob",
-                            "sha": self._create_blob(content, local_file),
                         }
+                        new_item.update(
+                            self._get_content_or_sha(
+                                Path(local_file).read_bytes(), self.dry_run
+                            )
+                        )
                         new_tree_list.append(new_item)
 
-    def _summarize_changes(self, new_tree_list):
+    def _summarize_changes(self, old_tree_list, new_tree_list):
         self.logger.info("Summary of changes:")
-        new_shas = [item["sha"] for item in new_tree_list]
-        new_paths = [item["path"] for item in new_tree_list]
-        old_paths = [item["path"] for item in self.tree]
-        old_tree_list = []
-        for item in self.tree:
-            if item["type"] == "tree":
-                continue
-            old_tree_list.append(item)
-            if item["path"] not in new_paths:
-                self.logger.warning("Delete:\t{}".format(item["path"]))
-            elif item["sha"] not in new_shas:
-                self.logger.info("Update:\t{}".format(item["path"]))
-        for item in new_tree_list:
-            if item["path"] not in old_paths:
-                self.logger.info("Add:\t{}".format(item["path"]))
-        return new_tree_list == old_tree_list
+        old_paths = [item["path"] for item in old_tree_list]
+
+        def join_paths(paths):
+            return "\n".join(f"\t{path}" for path in paths)
+
+        deleted_paths = join_paths(
+            [item["path"] for item in new_tree_list if item.get("sha", "") is None]
+        )
+        if deleted_paths:
+            self.logger.warning(f"Deleted:\n{deleted_paths}")
+
+        new_paths = join_paths(
+            [
+                item["path"]
+                for item in new_tree_list
+                if "content" in item and item["path"] not in old_paths
+            ]
+        )
+        if new_paths:
+            self.logger.info(f"Added:\n{new_paths}")
+
+        updated_paths = join_paths(
+            [
+                item["path"]
+                for item in new_tree_list
+                if "content" in item and item["path"] in old_paths
+            ]
+        )
+        if updated_paths:
+            self.logger.info(f"Updated:\n{updated_paths}")
 
     def _create_tree(self, new_tree_list):
         new_tree = None
@@ -164,16 +174,17 @@ class CommitDir(object):
             self.logger.info("[dry_run] Skipping creation of new tree")
         else:
             self.logger.info("Creating new tree")
-            new_tree = self.repo.create_tree(new_tree_list, None)
+            new_tree = self.repo.create_tree(
+                new_tree_list, base_tree=self.parent_commit.tree.sha
+            )
             if not new_tree:
                 raise GithubException("Failed to create tree")
         return new_tree
 
     def _create_commit(self, commit_message, new_tree):
         if commit_message is None:
-            commit_message = "Commit dir {} to {}/{}/{} via CumulusCI".format(
-                self.local_dir, self.repo.owner, self.repo.name, self.repo_dir
-            )
+            commit_message = f"Commit dir {self.local_dir} to {self.repo.owner}/{self.repo.name}/{self.repo_dir} via CumulusCI"
+
         new_commit = None
         if self.dry_run:
             self.logger.info("[dry_run] Skipping creation of new commit")
@@ -205,35 +216,36 @@ class CommitDir(object):
         len_path = (len(self.repo_dir) + 1) if self.repo_dir else 0
         return item["path"][len_path:]
 
-    def _read_item_content(self, item):
+    def _find_and_read_item(self, item) -> Tuple[Path, Optional[bytes]]:
         item_subpath = self._get_item_sub_path(item)
-        local_file = os.path.join(self.local_dir, item_subpath)
-        if not os.path.isfile(local_file):
-            return local_file, None
-        with io.open(local_file, "rb") as f:
-            content = f.read()
-        return local_file, content
-
-    def _item_changed(self, item, content):
+        pth = Path(self.local_dir, item_subpath)
+        contents = pth.read_bytes() if pth.exists() else None
+        return pth, contents
+
+    def _item_changed(self, item: dict, content: bytes) -> bool:
         header = b"blob " + str(len(content)).encode() + b"\0"
         return hashlib.sha1(header + content).hexdigest() != item["sha"]
 
-    def _create_blob(self, content, local_file):
-        if self.dry_run:
-            self.logger.info(
-                "[dry_run] Skipping creation of "
-                + "blob for new file: {}".format(local_file)
-            )
-            blob_sha = None
-        else:
-            self.logger.info("Creating blob for new file: {}".format(local_file))
-            try:
-                content = content.decode("utf-8")
-                blob_sha = self.repo.create_blob(content, "utf-8")
-            except UnicodeDecodeError:
-                content = base64.b64encode(content)
-                blob_sha = self.repo.create_blob(content.decode("utf-8"), "base64")
-            if not blob_sha:
-                raise GithubException("Failed to create blob")
-        self.logger.debug("Blob created: {}".format(blob_sha))
-        return blob_sha
+    def _get_content_or_sha(self, content, dry_run) -> dict:
+        """Given file contents as bytes, returns a dict with file contents.
+
+        If the contents are text: {'contents': contents as str}
+        If the contents are binary: {'sha': hash of the uploaded blob}
+        """
+        if dry_run:
+            self.logger.info("[dry_run] Skipping creation of blob for new file")
+            return {"content": None}
+        try:
+            content = content.decode("utf-8")
+            return {"content": content}
+        except UnicodeDecodeError:
+            return self._create_blob(content)
+
+    def _create_blob(self, contents: bytes) -> dict:
+        self.logger.info("Creating blob for binary file")
+        content: bytes = base64.b64encode(contents)
+        blob_sha: str = self.repo.create_blob(content.decode("utf-8"), "base64")
+        if not blob_sha:
+            raise GithubException("Failed to create blob")
+        self.logger.debug(f"Blob created: {blob_sha}")
+        return {"sha": blob_sha}


Test patch:
diff --git a/cumulusci/tasks/github/tests/test_util.py b/cumulusci/tasks/github/tests/test_util.py
index 96ec9ff0d..0fb215fa3 100644
--- a/cumulusci/tasks/github/tests/test_util.py
+++ b/cumulusci/tasks/github/tests/test_util.py
@@ -1,4 +1,5 @@
 import hashlib
+from pathlib import Path
 from unittest import mock
 
 import pytest
@@ -10,88 +11,102 @@ from cumulusci.tasks.github.util import CommitDir
 from cumulusci.utils import temporary_dir
 
 
+@pytest.fixture
+def repo():
+    repo = mock.Mock(spec=Repository)
+    repo.owner = "SalesforceFoundation"
+    repo.name = "TestRepo"
+    return repo
+
+
 class TestCommitDir:
-    def test_call(self):
-        with temporary_dir() as d:
-            repo = mock.Mock(spec=Repository)
-            repo.owner = "SalesforceFoundation"
-            repo.name = "TestRepo"
-            repo.tree = mock.Mock()
-            repo.tree.return_value = Tree(
-                {
-                    "url": "string",
-                    "sha": "tree-ish-hash",
-                    "tree": [
-                        {
-                            "type": "tree",
-                            "mode": "100644",
-                            "path": "dir",
-                            "sha": "bogus1",
-                        },
-                        {
-                            "type": "blob",
-                            "mode": "100644",
-                            "path": "file_outside_dir",
-                            "sha": "bogus2",
-                        },
-                        {
-                            "type": "blob",
-                            "mode": "100644",
-                            "path": "dir/deleteme",
-                            "sha": "deletedfilesha",
-                        },
-                        {
-                            "type": "blob",
-                            "mode": "100644",
-                            "path": "dir/unchanged",
-                            "sha": hashlib.sha1(b"blob 0\0").hexdigest(),
-                        },
-                        {
-                            "type": "blob",
-                            "mode": "100644",
-                            "path": "dir/modified",
-                            "sha": "bogus3",
-                        },
-                    ],
-                },
-                None,
-            )
+    def test_call(self, repo, tmp_path):
+        repo.git_commit.tree.sha = mock.Mock()
+        repo.create_blob.return_value = "mocked_sha_hash"
+        repo.tree = mock.Mock()
+        repo.tree.return_value = Tree(
+            {
+                "url": "string",
+                "sha": "tree-ish-hash",
+                "tree": [
+                    {
+                        "type": "tree",
+                        "mode": "100644",
+                        "path": "dir",
+                        "sha": "bogus1",
+                    },
+                    {
+                        "type": "blob",
+                        "mode": "100644",
+                        "path": "file_outside_dir",
+                        "sha": "bogus2",
+                    },
+                    {
+                        "type": "blob",
+                        "mode": "100644",
+                        "path": "dir/deleteme",
+                        "sha": "deletedfilesha",
+                    },
+                    {
+                        "type": "blob",
+                        "mode": "100644",
+                        "path": "dir/unchanged",
+                        "sha": hashlib.sha1(b"blob 0\0").hexdigest(),
+                    },
+                    {
+                        "type": "blob",
+                        "mode": "100644",
+                        "path": "dir/modified",
+                        "sha": "bogus3",
+                    },
+                    {
+                        "type": "blob",
+                        "mode": "100644",
+                        "path": "dir/modified_bin",
+                        "sha": "bogus4",
+                    },
+                ],
+            },
+            None,
+        )
 
-            commit = CommitDir(repo)
-            with open("unchanged", "w") as f:
-                f.write("")
-            with open("modified", "w") as f:
-                f.write("modified")
-            with open("new", "w") as f:
-                f.write("new")
-            with open(".hidden", "w") as f:
-                pass
-            commit(d, "main", "dir", dry_run=True)
-            assert commit.new_tree_list == [
-                {
-                    "sha": "bogus2",
-                    "mode": "100644",
-                    "path": "file_outside_dir",
-                    "size": None,
-                    "type": "blob",
-                },
-                {
-                    "sha": hashlib.sha1(b"blob 0\0").hexdigest(),
-                    "mode": "100644",
-                    "path": "dir/unchanged",
-                    "size": None,
-                    "type": "blob",
-                },
-                {
-                    "sha": None,
-                    "mode": "100644",
-                    "path": "dir/modified",
-                    "size": None,
-                    "type": "blob",
-                },
-                {"path": "dir/new", "mode": "100644", "type": "blob", "sha": None},
-            ]
-            commit(d, "main", "dir", commit_message="msg")
+        commit = CommitDir(repo)
+        Path(tmp_path, "unchanged").write_text("")
+        Path(tmp_path, "modified").write_text("modified")
+        Path(tmp_path, "new").write_text("new")
+        Path(tmp_path, ".hidden").write_text(".hidden")
+        Path(tmp_path, "modified_bin").write_bytes(b"\x9c")
+        Path(tmp_path, "new_bin").write_bytes(b"\x9c")
+
+        commit(tmp_path, "main", "dir", dry_run=True)
+        expected_tree = [
+            {"type": "blob", "mode": "100644", "path": "dir/deleteme", "sha": None},
+            {
+                "type": "blob",
+                "mode": "100644",
+                "path": "dir/modified",
+                "content": "modified",
+            },
+            {
+                "type": "blob",
+                "mode": "100644",
+                "path": "dir/modified_bin",
+                "sha": "mocked_sha_hash",
+            },
+            {"path": "dir/new", "mode": "100644", "type": "blob", "content": "new"},
+            {
+                "path": "dir/new_bin",
+                "mode": "100644",
+                "type": "blob",
+                "sha": "mocked_sha_hash",
+            },
+        ]
+        commit(tmp_path, "main", "dir", commit_message="msg")
+        assert len(commit.new_tree_list) == len(expected_tree)
+        assert all(item in commit.new_tree_list for item in expected_tree)
+        repo.create_tree.assert_called_once()
+        assert commit.new_tree_list == repo.create_tree.call_args.args[0]
+        assert "base_tree" in repo.create_tree.call_args.kwargs
         repo.create_commit.assert_called_once()
 
     def test_call__no_changes(self):
@@ -163,16 +178,25 @@ class TestCommitDir:
             with pytest.raises(GithubException):
                 commit(d, "main", commit_message="msg")
 
-    def test_create_blob__handles_decode_error(self):
-        repo = mock.Mock(spec=Repository)
+    def test_content_or_sha__dry_run(self, repo):
         commit = CommitDir(repo)
-        commit.dry_run = False
-        assert commit._create_blob(b"\x9c", "local_path")
+        result = commit._get_content_or_sha(b"content goes here", True)
+        assert result == {"content": None}
 
-    def test_create_blob__error(self):
-        repo = mock.Mock(spec=Repository)
-        repo.create_blob.return_value = None
+    def test_content_or_sha__text_content(self, repo):
         commit = CommitDir(repo)
-        commit.dry_run = False
+        result = commit._get_content_or_sha(b"content goes here", False)
+        assert result == {"content": "content goes here"}
+
+    def test_content_or_sha__binary_sha(self, repo):
+        commit = CommitDir(repo)
+        repo.create_blob.return_value = "binary-sha"
+        result = commit._get_content_or_sha(b"\x9c", False)
+        repo.create_blob.assert_called_once_with("nA==", "base64")
+        assert result == {"sha": "binary-sha"}
+
+    def test_create_blob__error(self, repo):
+        commit = CommitDir(repo)
+        repo.create_blob.return_value = None
         with pytest.raises(GithubException):
-            assert commit._create_blob(b"", "local_path")
+            commit._get_content_or_sha(b"\x9c", False)