[owners] Refactor OwnersClient.

- Remove GetChangeApprovalStatus and GetFilesApprovalStatus.
- Make host, project and branch part of GerritClient.
- Rename ListOwnersForFile to ListOwners.

Change-Id: I1f610a64f0217ce54e5ce4a210026535adc1c2e5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2587268
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Gavin Mak <gavinmak@google.com>

git_cl.py

@@ -42,7 +42,6 @@ import git_footers
 import git_new_branch
 import metrics
 import metrics_utils
-import owners
 import owners_client
 import owners_finder
 import presubmit_canned_checks
@@ -1376,14 +1375,11 @@ class Changelist(object):
       add_owners = []
       if options.add_owners_to:
         # Fill gaps in OWNERS coverage to tbrs/reviewers if requested.
-        project = self.GetGerritProject()
-        branch = self.GetCommonAncestorWithUpstream()
         client = owners_client.DepotToolsClient(
-            host=self.GetGerritHost(),
             root=settings.GetRoot(),
-            branch=branch)
+            branch=self.GetCommonAncestorWithUpstream())
         status = client.GetFilesApprovalStatus(
-            project, branch, files, [], options.tbrs + options.reviewers)
+            files, [], options.tbrs + options.reviewers)
         missing_files = [
             f for f in files
             if status[f] == owners_client.INSUFFICIENT_REVIEWERS
@@ -4789,16 +4785,13 @@ def CMDowners(parser, args):
     if len(args) == 0:
       print('No files specified for --show-all. Nothing to do.')
       return 0
-    project = cl.GetGerritProject()
-    branch = cl.GetCommonAncestorWithUpstream()
     client = owners_client.DepotToolsClient(
-        host=cl.GetGerritHost(),
         root=settings.GetRoot(),
-        branch=branch)
-    for arg in args:
-      print('Owners for %s:' % arg)
-      for owner in client.ListOwnersForFile(project, branch, arg):
-        print(' - %s' % owner)
+        branch=cl.GetCommonAncestorWithUpstream())
+    owners_by_file = client.BatchListOwners(args)
+    for f, owners in owners_by_file.items():
+      print('Owners for %s:' % f)
+      print('\n'.join(' - %s' % owner for owner in owners))
     return 0
 
   if args:
@@ -4813,13 +4806,10 @@ def CMDowners(parser, args):
   affected_files = cl.GetAffectedFiles(base_branch)
 
   if options.batch:
-    project = cl.GetGerritProject()
-    branch = cl.GetCommonAncestorWithUpstream()
     client = owners_client.DepotToolsClient(
-        host=cl.GetGerritHost(),
         root=settings.GetRoot(),
-        branch=branch)
-    print('\n'.join(client.SuggestOwners(project, branch, affected_files)))
+        branch=cl.GetCommonAncestorWithUpstream())
+    print('\n'.join(client.SuggestOwners(affected_files)))
     return 0
 
   owner_files = [f for f in affected_files if 'OWNERS' in os.path.basename(f)]

owners_client.py

@@ -5,6 +5,7 @@
 import itertools
 import os
 import random
+import threading
 
 import gerrit_util
 import git_common
@@ -39,10 +40,6 @@ def _owner_combinations(owners, num_owners):
   return reversed(list(itertools.combinations(reversed(owners), num_owners)))
 
 
-class InvalidOwnersConfig(Exception):
-  pass
-
-
 class OwnersClient(object):
   """Interact with OWNERS files in a repository.
 
@@ -50,46 +47,30 @@ class OwnersClient(object):
   Gerrit Code-Owners plugin REST API, and the owners database implemented by
   Depot Tools in owners.py:
 
-   - List all the owners for a change.
-   - Check if a change has been approved.
-   - Check if the OWNERS configuration in a change is valid.
+   - List all the owners for a group of files.
+   - Check if files have been approved.
+   - Suggest owners for a group of files.
 
   All code should use this class to interact with OWNERS files instead of the
   owners database in owners.py
   """
-  def __init__(self, host):
-    self._host = host
-
-  def ListOwnersForFile(self, project, branch, path):
+  def ListOwners(self, path):
     """List all owners for a file.
 
     The returned list is sorted so that better owners appear first.
     """
     raise Exception('Not implemented')
 
-  def BatchListOwners(self, project, branch, paths):
-    """Returns a dictionary {path: [owners]}."""
-    with git_common.ScopedPool(kind='threads') as pool:
-      return dict(pool.imap_unordered(
-          lambda p: (p, self.ListOwnersForFile(project, branch, p)), paths))
-
-  def GetChangeApprovalStatus(self, change_id):
-    """Check the approval status for the latest revision_id in a change.
+  def BatchListOwners(self, paths):
+    """List all owners for a group of files.
 
-    Returns a map of path to approval status, where the status can be one of:
-    - APPROVED: An owner of the file has reviewed the change.
-    - PENDING:  An owner of the file has been added as a reviewer, but no owner
-      has approved.
-    - INSUFFICIENT_REVIEWERS: No owner of the file has been added as a reviewer.
+    Returns a dictionary {path: [owners]}.
     """
-    raise Exception('Not implemented')
-
-  def ValidateOwnersConfig(self, change_id):
-    """Check if the owners configuration in a change is valid."""
-    raise Exception('Not implemented')
+    with git_common.ScopedPool(kind='threads') as pool:
+      return dict(pool.imap_unordered(
+          lambda p: (p, self.ListOwners(p)), paths))
 
-  def GetFilesApprovalStatus(
-      self, project, branch, paths, approvers, reviewers):
+  def GetFilesApprovalStatus(self, paths, approvers, reviewers):
     """Check the approval status for the given paths.
 
     Utility method to check for approval status when a change has not yet been
@@ -100,22 +81,23 @@ class OwnersClient(object):
     approvers = set(approvers)
     reviewers = set(reviewers)
     status = {}
-    for path in paths:
-      path_owners = set(self.ListOwnersForFile(project, branch, path))
-      if path_owners.intersection(approvers):
+    owners_by_path = self.BatchListOwners(paths)
+    for path, owners in owners_by_path.items():
+      owners = set(owners)
+      if owners.intersection(approvers):
         status[path] = APPROVED
-      elif path_owners.intersection(reviewers):
+      elif owners.intersection(reviewers):
         status[path] = PENDING
       else:
         status[path] = INSUFFICIENT_REVIEWERS
     return status
 
-  def SuggestOwners(self, project, branch, paths):
+  def SuggestOwners(self, paths):
     """Suggest a set of owners for the given paths."""
     paths_by_owner = {}
     score_by_owner = {}
-    for path in paths:
-      owners = self.ListOwnersForFile(project, branch, path)
+    owners_by_path = self.BatchListOwners(paths)
+    for path, owners in owners_by_path.items():
       for i, owner in enumerate(owners):
         paths_by_owner.setdefault(owner, set()).add(path)
         # Gerrit API lists owners of a path sorted by an internal score, so
@@ -124,8 +106,10 @@ class OwnersClient(object):
         # paths.
         score_by_owner[owner] = min(i, score_by_owner.get(owner, i))
 
-    # Sort owners by their score.
-    owners = sorted(score_by_owner, key=lambda o: score_by_owner[o])
+    # Sort owners by their score. Randomize order of owners with same score.
+    owners = sorted(
+        score_by_owner,
+        key=lambda o: (score_by_owner[o], random.random()))
 
     # Select the minimum number of owners that can approve all paths.
     # We start at 2 to avoid sending all changes that require multiple reviewers
@@ -139,19 +123,22 @@ class OwnersClient(object):
       for selected in _owner_combinations(owners, num_owners):
         covered = set.union(*(paths_by_owner[o] for o in selected))
         if len(covered) == len(paths):
-          return selected
+          return list(selected)
 
 
 class DepotToolsClient(OwnersClient):
   """Implement OwnersClient using owners.py Database."""
-  def __init__(self, host, root, branch, fopen=open, os_path=os.path):
-    super(DepotToolsClient, self).__init__(host)
+  def __init__(self, root, branch, fopen=open, os_path=os.path):
+    super(DepotToolsClient, self).__init__()
+
     self._root = root
+    self._branch = branch
     self._fopen = fopen
     self._os_path = os_path
-    self._branch = branch
+
     self._db = owners_db.Database(root, fopen, os_path)
     self._db.override_files = self._GetOriginalOwnersFiles()
+    self._db_lock = threading.Lock()
 
   def _GetOriginalOwnersFiles(self):
     return {
@@ -160,57 +147,33 @@ class DepotToolsClient(OwnersClient):
       if os.path.basename(f) == 'OWNERS'
     }
 
-  def ListOwnersForFile(self, _project, _branch, path):
-    # all_possible_owners returns a dict {owner: [(path, distance)]}. We want to
-    # return a list of owners sorted by increasing distance.
-    distance_by_owner = self._db.all_possible_owners([path], None)
-    # We add a small random number to the distance, so that owners at the same
-    # distance are returned in random order to avoid overloading those who would
-    # appear first.
-    return sorted(
-        distance_by_owner,
-        key=lambda o: distance_by_owner[o][0][1] + random.random())
-
-  def GetChangeApprovalStatus(self, change_id):
-    data = gerrit_util.GetChange(
-        self._host, change_id,
-        ['DETAILED_ACCOUNTS', 'DETAILED_LABELS', 'CURRENT_FILES',
-         'CURRENT_REVISION'])
-
-    reviewers = [r['email'] for r in data['reviewers']['REVIEWER']]
-
-    # Get reviewers that have approved this change
-    label = data['labels']['Code-Review']
-    max_value = max(int(v) for v in label['values'])
-    approvers = [v['email'] for v in label['all'] if v['value'] == max_value]
-
-    files = data['revisions'][data['current_revision']]['files']
-    return self.GetFilesApprovalStatus(None, None, files, approvers, reviewers)
-
-  def ValidateOwnersConfig(self, change_id):
-    data = gerrit_util.GetChange(
-        self._host, change_id,
-        ['DETAILED_ACCOUNTS', 'DETAILED_LABELS', 'CURRENT_FILES',
-         'CURRENT_REVISION'])
-
-    files = data['revisions'][data['current_revision']]['files']
-
-    db = owners_db.Database(self._root, self._fopen, self._os_path)
-    try:
-      db.load_data_needed_for(
-          [f for f in files if os.path.basename(f) == 'OWNERS'])
-    except Exception as e:
-      raise InvalidOwnersConfig('Error parsing OWNERS files:\n%s' % e)
+  def ListOwners(self, path):
+    # all_possible_owners is not thread safe.
+    with self._db_lock:
+      # all_possible_owners returns a dict {owner: [(path, distance)]}. We want
+      # to return a list of owners sorted by increasing distance.
+      distance_by_owner = self._db.all_possible_owners([path], None)
+      # We add a small random number to the distance, so that owners at the same
+      # distance are returned in random order to avoid overloading those who
+      # would appear first.
+      return sorted(
+          distance_by_owner,
+          key=lambda o: distance_by_owner[o][0][1] + random.random())
 
 
 class GerritClient(OwnersClient):
   """Implement OwnersClient using OWNERS REST API."""
-  def __init__(self, host):
-    super(GerritClient, self).__init__(host)
+  def __init__(self, host, project, branch):
+    super(GerritClient, self).__init__()
+
+    self._host = host
+    self._project = project
+    self._branch = branch
 
-  def ListOwnersForFile(self, project, branch, path):
+  def ListOwners(self, path):
     # GetOwnersForFile returns a list of account details sorted by order of
     # best reviewer for path. If code owners have the same score, the order is
     # random.
-    data = gerrit_util.GetOwnersForFile(self._host, project, branch, path)
+    data = gerrit_util.GetOwnersForFile(
+        self._host, self._project, self._branch, path)
     return [d['account']['email'] for d in data]

tests/owners_client_test.py

@@ -26,43 +26,6 @@ dave = 'dave@example.com'
 emily = 'emily@example.com'
 
 
-def _get_change():
-  return {
-    "labels": {
-      "Code-Review": {
-        "all": [
-          {
-            "value": 1,
-            "email": "approver@example.com",
-          }
-        ],
-        "values": {
-          "-1": "Don't submit as-is",
-          " 0": "No score",
-          "+1": "Looks good to me"
-        },
-      },
-    },
-    "reviewers": {
-      "REVIEWER": [
-        {"email": "approver@example.com"},
-        {"email": "reviewer@example.com"},
-      ],
-    },
-    "current_revision": "cb90826d03533d6c4e1f0e974ebcbfd7a6f42406",
-    "revisions": {
-      "cb90826d03533d6c4e1f0e974ebcbfd7a6f42406": {
-        "files": {
-          "approved.cc": {},
-          "reviewed.h": {},
-          "bar/insufficient.py": {},
-        },
-      },
-    },
-  }
-
-
-
 def _get_owners():
   return [
     {
@@ -105,65 +68,38 @@ class DepotToolsClientTest(unittest.TestCase):
         return_value={}).start()
     self.addCleanup(mock.patch.stopall)
     self.client = owners_client.DepotToolsClient(
-        'host', '/', 'branch', self.fopen, self.repo)
+        '/', 'branch', self.fopen, self.repo)
 
   def testListOwners(self):
-    self.assertEquals(
+    self.assertEqual(
         ['*', 'missing@example.com'],
-        self.client.ListOwnersForFile(
-            'project', 'branch', 'bar/everyone/foo.txt'))
-
-  @mock.patch('gerrit_util.GetChange', return_value=_get_change())
-  def testGetChangeApprovalStatus(self, _mock):
-    self.assertEquals(
-        {
-            'approved.cc': owners_client.APPROVED,
-            'reviewed.h': owners_client.PENDING,
-            'bar/insufficient.py': owners_client.INSUFFICIENT_REVIEWERS,
-        },
-        self.client.GetChangeApprovalStatus('changeid'))
-
-  @mock.patch('gerrit_util.GetChange', return_value=_get_change())
-  def testValidateOwnersConfig_OK(self, get_change_mock):
-    self.client.ValidateOwnersConfig('changeid')
-
-  @mock.patch('gerrit_util.GetChange', return_value=_get_change())
-  def testValidateOwnersConfig_Invalid(self, get_change_mock):
-    change = get_change_mock()
-    change['revisions'][change['current_revision']]['files'] = {'/OWNERS': {}}
-    self.repo.files['/OWNERS'] = '\n'.join([
-        'foo@example.com',
-        'invalid directive',
-    ])
-    with self.assertRaises(owners_client.InvalidOwnersConfig):
-      self.client.ValidateOwnersConfig('changeid')
+        self.client.ListOwners('bar/everyone/foo.txt'))
 
 
 class GerritClientTest(unittest.TestCase):
   def setUp(self):
-    self.client = owners_client.GerritClient('host')
+    self.client = owners_client.GerritClient('host', 'project', 'branch')
 
   @mock.patch('gerrit_util.GetOwnersForFile', return_value=_get_owners())
-  def testListOwners(self, get_owners_mock):
+  def testListOwners(self, _get_owners_mock):
     self.assertEquals(
         ['approver@example.com', 'reviewer@example.com', 'missing@example.com'],
-        self.client.ListOwnersForFile('project', 'branch',
-                                      'bar/everyone/foo.txt'))
+        self.client.ListOwners('bar/everyone/foo.txt'))
 
 
 class TestClient(owners_client.OwnersClient):
-  def __init__(self, host, owners_by_path):
-    super(TestClient, self).__init__(host)
+  def __init__(self, owners_by_path):
+    super(TestClient, self).__init__()
     self.owners_by_path = owners_by_path
 
-  def ListOwnersForFile(self, _project, _branch, path):
+  def ListOwners(self, path):
     return self.owners_by_path[path]
 
 
 class OwnersClientTest(unittest.TestCase):
   def setUp(self):
     self.owners = {}
-    self.client = TestClient('host', self.owners)
+    self.client = TestClient(self.owners)
 
   def testGetFilesApprovalStatus(self):
     self.client.owners_by_path = {
@@ -172,7 +108,6 @@ class OwnersClientTest(unittest.TestCase):
       'insufficient': ['insufficient@example.com'],
     }
     status = self.client.GetFilesApprovalStatus(
-        'project', 'branch',
         ['approved', 'pending', 'insufficient'],
         ['approver@example.com'], ['reviewer@example.com'])
     self.assertEqual(
@@ -201,13 +136,13 @@ class OwnersClientTest(unittest.TestCase):
   def testSuggestOwners(self):
     self.client.owners_by_path = {'a': [alice]}
     self.assertEqual(
-        self.client.SuggestOwners('project', 'branch', ['a']),
+        self.client.SuggestOwners(['a']),
         [alice])
 
     self.client.owners_by_path = {'abcd': [alice, bob, chris, dave]}
     self.assertEqual(
-        self.client.SuggestOwners('project', 'branch', ['abcd']),
-        (bob, alice))
+        sorted(self.client.SuggestOwners(['abcd'])),
+        [alice, bob])
 
     self.client.owners_by_path = {
         'ae': [alice, emily],
@@ -215,10 +150,10 @@ class OwnersClientTest(unittest.TestCase):
         'ce': [chris, emily],
         'de': [dave, emily],
     }
-    self.assertEqual(
-        self.client.SuggestOwners(
-            'project', 'branch', ['ae', 'be', 'ce', 'de']),
-        (emily, bob))
+    suggested = self.client.SuggestOwners(['ae', 'be', 'ce', 'de'])
+    # emily should be selected along with anyone else.
+    self.assertIn(emily, suggested)
+    self.assertEqual(2, len(suggested))
 
     self.client.owners_by_path = {
         'ad': [alice, dave],
@@ -227,9 +162,8 @@ class OwnersClientTest(unittest.TestCase):
         'bd': [bob, dave],
     }
     self.assertEqual(
-        self.client.SuggestOwners(
-            'project', 'branch', ['ad', 'cad', 'ead', 'bd']),
-        (bob, alice))
+        sorted(self.client.SuggestOwners(['ad', 'cad', 'ead', 'bd'])),
+        [alice, bob])
 
     self.client.owners_by_path = {
         'a': [alice],
@@ -238,9 +172,8 @@ class OwnersClientTest(unittest.TestCase):
         'ad': [alice, dave],
     }
     self.assertEqual(
-        self.client.SuggestOwners(
-            'project', 'branch', ['a', 'b', 'c', 'ad']),
-        (alice, chris, bob))
+        sorted(self.client.SuggestOwners(['a', 'b', 'c', 'ad'])),
+        [alice, bob, chris])
 
     self.client.owners_by_path = {
         'abc': [alice, bob, chris],
@@ -250,11 +183,10 @@ class OwnersClientTest(unittest.TestCase):
         'cab': [chris, alice, bob],
         'cba': [chris, bob, alice]
     }
-    self.assertEqual(
-        self.client.SuggestOwners(
-            'project', 'branch',
-            ['abc', 'acb', 'bac', 'bca', 'cab', 'cba']),
-        (chris, bob))
+    suggested = self.client.SuggestOwners(
+        ['abc', 'acb', 'bac', 'bca', 'cab', 'cba'])
+    # Any two owners.
+    self.assertEqual(2, len(suggested))
 
   def testBatchListOwners(self):
     self.client.owners_by_path = {
@@ -270,7 +202,6 @@ class OwnersClientTest(unittest.TestCase):
             'bar/foo/': [bob, chris]
         },
         self.client.BatchListOwners(
-            'project', 'branch',
             ['bar/everyone/foo.txt', 'bar/everyone/bar.txt', 'bar/foo/']))