owners_client: Handle missing email in AccountInfo

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

gerrit_util.py

@@ -779,13 +779,13 @@ def IsCodeOwnersEnabled(host):
 
 
 def GetOwnersForFile(host, project, branch, path, limit=100,
-                     o_params=('DETAILS',)):
+                     resolve_all_users=True, o_params=('DETAILS',)):
   """Gets information about owners attached to a file."""
   path = 'projects/%s/branches/%s/code_owners/%s' % (
       urllib.parse.quote(project, ''),
       urllib.parse.quote(branch, ''),
       urllib.parse.quote(path, ''))
-  q = []
+  q = ['resolve-all-users=%s' % json.dumps(resolve_all_users)]
   if limit:
     q.append('n=%d' % limit)
   if o_params:

owners_client.py

@@ -215,11 +215,17 @@ class GerritClient(OwnersClient):
       # best reviewer for path. If owners have the same score, the order is
       # random.
       data = gerrit_util.GetOwnersForFile(
-          self._host, self._project, self._branch, path)
+          self._host, self._project, self._branch, path,
+          resolve_all_users=False)
       self._owners_cache[path] = [
         d['account']['email']
         for d in data['code_owners']
+        if 'account' in d and 'email' in d['account']
       ]
+      # If owned_by_all_users is true, add everyone as an owner at the end of
+      # the owners list.
+      if data.get('owned_by_all_users', False):
+        self._owners_cache[path].append(self.EVERYONE)
     return self._owners_cache[path]
 
 

tests/owners_client_test.py

@@ -58,6 +58,9 @@ class DepotToolsClientTest(unittest.TestCase):
 class GerritClientTest(unittest.TestCase):
   def setUp(self):
     self.client = owners_client.GerritClient('host', 'project', 'branch')
+    self.addCleanup(mock.patch.stopall)
+
+  def testListOwners(self):
     mock.patch(
         'gerrit_util.GetOwnersForFile',
         return_value={
@@ -76,12 +79,12 @@ class GerritClientTest(unittest.TestCase):
               "account": {
                 "email": 'missing@example.com'
               },
+            },
+            {
+              "account": {},
             }
           ]
         }).start()
-    self.addCleanup(mock.patch.stopall)
-
-  def testListOwners(self):
     self.assertEquals(
         ['approver@example.com', 'reviewer@example.com', 'missing@example.com'],
         self.client.ListOwners(os.path.join('bar', 'everyone', 'foo.txt')))
@@ -92,7 +95,41 @@ class GerritClientTest(unittest.TestCase):
         self.client.ListOwners(os.path.join('bar', 'everyone', 'foo.txt')))
     # Always use slashes as separators.
     gerrit_util.GetOwnersForFile.assert_called_once_with(
-        'host', 'project', 'branch', 'bar/everyone/foo.txt')
+        'host', 'project', 'branch', 'bar/everyone/foo.txt',
+        resolve_all_users=False)
+
+  def testListOwnersOwnedByAll(self):
+    mock.patch(
+      'gerrit_util.GetOwnersForFile',
+      side_effect=[
+        {
+          "code_owners": [
+            {
+              "account": {
+                "email": 'foo@example.com'
+              },
+            },
+          ],
+          "owned_by_all_users": True,
+        },
+        {
+          "code_owners": [
+            {
+              "account": {
+                "email": 'bar@example.com'
+              },
+            },
+          ],
+          "owned_by_all_users": False,
+        },
+      ]
+    ).start()
+    self.assertEquals(
+        ['foo@example.com', self.client.EVERYONE],
+        self.client.ListOwners('foo.txt'))
+    self.assertEquals(
+        ['bar@example.com'],
+        self.client.ListOwners('bar.txt'))
 
 
 class TestClient(owners_client.OwnersClient):