Bug 1554447 - ERR-CONDUIT-CORE: Error while reading "ids" when pushing a stack with duplicate revisions; r=zalun

* Rewrite get_revisions to avoid duplicated code and checks.
* Don't query Phabricator with an empty list.
* Update tests to pass in real data to get_revisions and clear revision
  mapping cache between tests.

Differential Revision: https://phabricator.services.mozilla.com/D32663

Author: globau

moz-phab

@@ -2878,88 +2878,72 @@ def self_update(args):
     logger.info("%s updated" % SELF_FILE)
 
 
-def order_revisions_by_id(phids_by_id, revs, ids):
-    return [revs[phids_by_id[str(id)]] for id in ids if phids_by_id[str(id)]]
-
-
-def order_revisions_by_phid(revs, phids):
-    return [revs[phid] for phid in phids]
-
-
 def get_revisions(cwd, ids=None, phids=None):
     """Get revisions info from Phabricator.
 
     Args:
         cwd - current working directory
-        ids - list of ids
+        ids - list of revision ids
         phids - list of revision phids
 
     Returns a list of revisions ordered by ids or phids
-
-    Raises Exception if both ids and phids are provided
     """
-    if ids and phids:
-        # Ordering would be too complicated
-        raise ValueError("ids and phids are mutually exclusive")
+
+    if (ids and phids) or (ids is None and phids is None):
+        raise ValueError("Internal Error: Invalid args to get_revisions")
 
     if not ids and not phids:
         return []
 
-    collected_ids = (
-        dict(
+    # Initialise depending on if we're passed revision IDs or PHIDs.
+    if ids:
+        ids = [str(rev_id) for rev_id in ids]
+        phids_by_id = dict(
             [
-                (str(id), cache.get("rev-id-%s" % id))
-                for id in ids
-                if "rev-id-%s" % id in cache
+                (rev_id, cache.get("rev-id-%s" % rev_id))
+                for rev_id in ids
+                if "rev-id-%s" % rev_id in cache
             ]
         )
-        if ids
-        else {}
-    )
+        found_phids = phids_by_id.values()
+        query_field = "ids"
+        query_values = [int(rev_id) for rev_id in set(ids) - set(phids_by_id.keys())]
 
-    found_phids = collected_ids.values() if ids else phids[:]
-
-    collected_revs = (
-        dict(
-            [
-                (phid, cache.get("rev-%s" % phid))
-                for phid in found_phids
-                if "rev-%s" % phid in cache
-            ]
-        )
-        if found_phids
-        else {}
+    else:
+        phids_by_id = {}
+        found_phids = phids[:]
+        query_field = "phids"
+        query_values = list(set(phids) - set(phids_by_id.values()))
+
+    # Revisions metadata keyed by PHID.
+    revisions = dict(
+        [
+            (phid, cache.get("rev-%s" % phid))
+            for phid in found_phids
+            if "rev-%s" % phid in cache
+        ]
     )
 
-    if ids and len(ids) == len(collected_ids):
-        return order_revisions_by_id(collected_ids, collected_revs, ids)
-
-    if phids and len(phids) == len(collected_revs):
-        return order_revisions_by_phid(collected_revs, phids)
-
-    api_call_args = {"constraints": {}, "attachments": {"reviewers": True}}
+    # Query Phabricator if we don't have cached values for revisions.
+    if query_values:
+        api_call_args = {
+            "constraints": {query_field: query_values},
+            "attachments": {"reviewers": True},
+        }
+        response = arc_call_conduit("differential.revision.search", api_call_args, cwd)
+        rev_list = response.get("data")
+
+        for r in rev_list:
+            phids_by_id[str(r["id"])] = r["phid"]
+            revisions[r["phid"]] = r
+            cache.set("rev-id-%s" % r["id"], r["phid"])
+            cache.set("rev-%s" % r["phid"], r)
+
+    # Return revisions in the same order requested.
     if ids:
-        to_collect = list(set(ids) - set([int(key) for key in collected_ids.keys()]))
-        api_call_args["constraints"]["ids"] = to_collect
+        return [revisions[phids_by_id[rev_id]] for rev_id in ids if phids_by_id[rev_id]]
     else:
-        to_collect = list(set(phids) - set(collected_ids.values()))
-        api_call_args["constraints"]["phids"] = to_collect
-
-    response = arc_call_conduit("differential.revision.search", api_call_args, cwd)
-    rev_list = response.get("data")
-    if not rev_list:
-        return
-
-    for r in rev_list:
-        collected_ids[str(r["id"])] = r["phid"]
-        collected_revs[r["phid"]] = r
-        cache.set("rev-id-%s" % r["id"], r["phid"])
-        cache.set("rev-%s" % r["phid"], r)
-
-    if ids:
-        return order_revisions_by_id(collected_ids, collected_revs, ids)
-
-    return order_revisions_by_phid(collected_revs, phids)
+        return [revisions[phid] for phid in phids]
 
 
 def get_diffs(cwd, phids):

tests/test_patch.py

@@ -42,25 +42,58 @@ def test_prepare_body():
 @mock.patch("mozphab.arc_call_conduit")
 def test_get_revisions(m_arc):
     get_revs = mozphab.get_revisions
+
+    # sanity checks
+    with pytest.raises(ValueError):
+        get_revs("x", ids=[1], phids=["PHID-1"])
+    with pytest.raises(ValueError):
+        get_revs("x", ids=None)
     with pytest.raises(ValueError):
-        get_revs("x", ids=[1], phids=[1])
+        get_revs("x", phids=None)
 
-    m_arc.return_value = {}
-    assert get_revs("x", ids=[1]) is None
+    m_arc.return_value = {"data": [dict(id=1, phid="PHID-1")]}
+
+    # differential.revision.search by revision-id
+    assert len(get_revs("x", ids=[1])) == 1
     m_arc.assert_called_with(
         "differential.revision.search",
         dict(constraints=dict(ids=[1]), attachments=dict(reviewers=True)),
         "x",
     )
+
+    # differential.revision.search by phid
+    m_arc.reset_mock()
+    mozphab.cache.reset()
+    assert len(get_revs("x", phids=["PHID-1"])) == 1
+    m_arc.assert_called_with(
+        "differential.revision.search",
+        dict(constraints=dict(phids=["PHID-1"]), attachments=dict(reviewers=True)),
+        "x",
+    )
+
+    # differential.revision.search by revision-id with duplicates
+    m_arc.reset_mock()
+    mozphab.cache.reset()
+    assert len(get_revs("x", ids=[1, 1])) == 2
+    m_arc.assert_called_with(
+        "differential.revision.search",
+        dict(constraints=dict(ids=[1]), attachments=dict(reviewers=True)),
+        "x",
+    )
+
+    # differential.revision.search by phid with duplicates
     m_arc.reset_mock()
-    assert get_revs("x", phids=[1]) is None
+    mozphab.cache.reset()
+    assert len(get_revs("x", phids=["PHID-1", "PHID-1"])) == 2
     m_arc.assert_called_with(
         "differential.revision.search",
-        dict(constraints=dict(phids=[1]), attachments=dict(reviewers=True)),
+        dict(constraints=dict(phids=["PHID-1"]), attachments=dict(reviewers=True)),
         "x",
     )
 
+    # ordering of results must match input
     m_arc.reset_mock()
+    mozphab.cache.reset()
     m_arc.return_value = {
         "data": [
             dict(id=1, phid="PHID-1"),