stacks: refactor build_stack_graph endpoint and tests (bug 1819408) (#279)

- use prefetched revision data to retrieve stack graph
- parse edges and nodes from above data
- update tests and fixtures

Author: zzzeid

landoapi/api/stacks.py

@@ -8,7 +8,7 @@
 from flask import current_app
 from landoapi.commit_message import format_commit_message
 from landoapi.decorators import require_phabricator_api_key
-from landoapi.phabricator import PhabricatorClient, PhabricatorAPIException
+from landoapi.phabricator import PhabricatorClient
 from landoapi.projects import (
     get_release_managers,
     get_sec_approval_project_phid,
@@ -67,12 +67,7 @@ def get(phab: PhabricatorClient, revision_id: str):
     if revision is None:
         return not_found_problem
 
-    try:
-        nodes, edges = build_stack_graph(phab, phab.expect(revision, "phid"))
-    except PhabricatorAPIException:
-        # If a revision within the stack causes an API exception, treat the whole stack
-        # as not found.
-        return not_found_problem
+    nodes, edges = build_stack_graph(revision)
     stack_data = request_extended_revision_data(phab, [phid for phid in nodes])
 
     supported_repos = get_repos_for_env(current_app.config.get("ENVIRONMENT"))

landoapi/api/transplants.py

@@ -130,11 +130,7 @@ def _find_stack_from_landing_path(
             "The stack does not exist or you lack permission to see it.",
             type="https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/404",
         )
-
-    # TODO: This assumes that all revisions and related objects in the stack
-    # have uniform view permissions for the requesting user. Some revisions
-    # being restricted could cause this to fail.
-    return build_stack_graph(phab, phab.expect(revision, "phid"))
+    return build_stack_graph(revision)
 
 
 def _assess_transplant_request(
@@ -524,11 +520,7 @@ def get_list(phab: PhabricatorClient, stack_revision_id: str):
             "The revision does not exist or you lack permission to see it.",
             type="https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/404",
         )
-
-    # TODO: This assumes that all revisions and related objects in the stack
-    # have uniform view permissions for the requesting user. Some revisions
-    # being restricted could cause this to fail.
-    nodes, edges = build_stack_graph(phab, phab.expect(revision, "phid"))
+    nodes, edges = build_stack_graph(revision)
     revision_phids = list(nodes)
     revs = phab.call_conduit(
         "differential.revision.search",

landoapi/stacks.py

@@ -25,18 +25,11 @@
 logger = logging.getLogger(__name__)
 
 
-def build_stack_graph(
-    phab: PhabricatorClient, revision_phid: str
-) -> tuple[set[str], set[tuple[str, str]]]:
+def build_stack_graph(revision: dict) -> tuple[set[str], set[tuple[str, str]]]:
     """Return a graph representation of a revision stack.
 
-    This function is expensive and can make up to approximately
-    n/2 calls to phabricator for a linear stack where n is the
-    number of revisions in the stack.
-
     Args:
-        phab: A landoapi.phabricator.PhabricatorClient.
-        revision_phid: String PHID of a revision in the stack.
+        revision: A dictionary containing Phabricator revision data.
 
     Returns:
         A tuple of (nodes, edges). `nodes` is a set of strings
@@ -45,36 +38,13 @@ def build_stack_graph(
         between two nodes. `child` and `parent` are also string
         PHIDs.
     """
-    phids = set()
-    new_phids = {revision_phid}
-    edges = []
-
-    # Repeatedly request all related edges, adding connected revisions
-    # each time until no new revisions are found.
-    while new_phids:
-        phids.update(new_phids)
-        edges = phab.call_conduit(
-            "edge.search",
-            types=["revision.parent", "revision.child"],
-            sourcePHIDs=[phid for phid in phids],
-            limit=10000,
-        )
-        edges = phab.expect(edges, "data")
-        new_phids = set()
-        for edge in edges:
-            new_phids.add(edge["sourcePHID"])
-            new_phids.add(edge["destinationPHID"])
-
-        new_phids = new_phids - phids
-
-    # Treat the stack like a commit DAG, we only care about edges going
-    # from child to parent. This is enough to represent the graph.
-    edges = {
-        (edge["sourcePHID"], edge["destinationPHID"])
-        for edge in edges
-        if edge["edgeType"] == "revision.parent"
-    }
+    stack_graph = PhabricatorClient.expect(revision, "fields", "stackGraph")
+    phids = set(stack_graph.keys())
+    edges = set()
 
+    for node, predecessors in stack_graph.items():
+        for predecessor in predecessors:
+            edges.add((node, predecessor))
     return phids, edges
 
 

landoapi/uplift.py

@@ -21,7 +21,7 @@
 
 from landoapi import bmo
 from landoapi.cache import cache, DEFAULT_CACHE_KEY_TIMEOUT_SECONDS
-from landoapi.phabricator import PhabricatorClient, PhabricatorAPIException
+from landoapi.phabricator import PhabricatorClient
 from landoapi.phabricator_patch import patch_to_changes
 from landoapi.repos import (
     Repo,
@@ -127,15 +127,7 @@ def get_uplift_conduit_state(
     if not revision:
         raise ValueError(f"No revision found with id {revision_id}")
 
-    try:
-        nodes, edges = build_stack_graph(phab, phab.expect(revision, "phid"))
-    except PhabricatorAPIException as e:
-        # If a revision within the stack causes an API exception, treat the whole stack
-        # as not found.
-        logger.exception(
-            f"Phabricator returned an error searching for {revision_id}: {str(e)}"
-        )
-        raise ValueError(f"Missing revision info for stack ending in {revision_id}")
+    nodes, edges = build_stack_graph(revision)
 
     stack_data = request_extended_revision_data(phab, [phid for phid in nodes])
 

tests/mocks.py

@@ -3,6 +3,7 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 import hashlib
 import json
+from collections import defaultdict
 from copy import deepcopy
 
 from landoapi.phabricator import (
@@ -59,7 +60,7 @@ def validate_hunk(hunk):
     assert isinstance(hunk["corpus"], str)
     lines = hunk["corpus"].splitlines()
     assert len(lines) > 0
-    assert all([l[0] in (" ", "-", "+") for l in lines])
+    assert all([line[0] in (" ", "-", "+") for line in lines])
 
     return True
 
@@ -96,6 +97,51 @@ def validate_change(change):
     return True
 
 
+def get_stack(phid, phabdouble):
+    phids = set()
+    new_phids = {phid}
+    edges = []
+
+    # Repeatedly request all related edges, adding connected revisions
+    # each time until no new revisions are found.
+    # NOTE: this was adapted from previous implementation of build_stack_graph.
+    while new_phids:
+        phids.update(new_phids)
+        edges = [
+            edge
+            for edge in phabdouble._edges
+            if edge["sourcePHID"] in phids
+            and edge["edgeType"] in ("revision.parent", "revision.child")
+        ]
+        new_phids = set()
+        for edge in edges:
+            new_phids.add(edge["sourcePHID"])
+            new_phids.add(edge["destinationPHID"])
+
+        new_phids = new_phids - phids
+
+    # Treat the stack like a commit DAG, we only care about edges going
+    # from child to parent. This is enough to represent the graph.
+    edges = {
+        (edge["sourcePHID"], edge["destinationPHID"])
+        for edge in edges
+        if edge["edgeType"] == "revision.parent"
+    }
+
+    stack_graph = defaultdict(list)
+    sources = [source for source, dest in edges]
+    for source, dest in edges:
+        # Check that destination phid has a corresponding source phid.
+        if dest not in sources:
+            # We are at a root node.
+            stack_graph[dest] = []
+        stack_graph[source].append(dest)
+    if not stack_graph:
+        # There is only one node, the root node.
+        stack_graph[phid] = []
+    return dict(stack_graph)
+
+
 class PhabricatorDouble:
     """Phabricator test double.
 
@@ -190,6 +236,30 @@ def api_object_for(self, mock_object: dict, **kwargs) -> dict:
     def get_phabricator_client():
         return PhabricatorClient("https://localhost", "DOESNT-MATTER")
 
+    def update_revision_dependencies(self, phid: str, depends_on: list[str]):
+        """Updates edges of `phid` so they match `depends_on`."""
+        # Remove all previous edges related to this revision.
+        def philter(edge):
+            return phid not in (edge["sourcePHID"], edge["destinationPHID"])
+
+        self._edges = list(filter(philter, self._edges))
+
+        for rev in depends_on:
+            self._edges.append(
+                {
+                    "edgeType": "revision.parent",
+                    "sourcePHID": phid,
+                    "destinationPHID": rev["phid"],
+                }
+            )
+            self._edges.append(
+                {
+                    "edgeType": "revision.child",
+                    "sourcePHID": rev["phid"],
+                    "destinationPHID": phid,
+                }
+            )
+
     def revision(
         self,
         *,
@@ -905,6 +975,7 @@ def to_response(i):
                 "fields": {
                     "title": i["title"],
                     "authorPHID": i["authorPHID"],
+                    "stackGraph": i["stackGraph"],
                     "status": {
                         "value": i["status"].value,
                         "name": i["status"].output_name,
@@ -913,6 +984,7 @@ def to_response(i):
                     },
                     "repositoryPHID": i["repositoryPHID"],
                     "diffPHID": diffs[-1]["phid"],
+                    "diffID": diffs[-1]["id"],
                     "summary": i["summary"],
                     "dateCreated": i["dateCreated"],
                     "dateModified": i["dateModified"],
@@ -955,7 +1027,10 @@ def to_response(i):
 
             return deepcopy(resp)
 
-        items = [r for r in self._revisions]
+        items = []
+        for revision in self._revisions:
+            revision["stackGraph"] = get_stack(revision["phid"], self)
+            items.append(revision)
 
         if constraints and "ids" in constraints:
             items = [i for i in items if i["id"] in constraints["ids"]]