Extract textual object list parsing from Differential

Summary:
Ref T2222. Currently, Differential has a fairly hairy piece of logic to parse object lists, like `Reviewers: alincoln, htaft`. Extract, generalize, and cover this.

  - Some of the logic can be simplified with modern ObjectQuery stuff.
  - Make `@username` the formal monogram for users.
  - Make `list@domain.com` the formal monogram for mailing lists.
  - Add test coverage.

Test Plan:
  - Ran unit tests.
  - Called `differential.parsecommitmessage` with a bunch of real-world inputs and got sensible results.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8445

src/__phutil_library_map__.php

@@ -1771,6 +1771,8 @@ phutil_register_library_map(array(
     'PhabricatorObjectHandle' => 'applications/phid/PhabricatorObjectHandle.php',
     'PhabricatorObjectHandleConstants' => 'applications/phid/handle/const/PhabricatorObjectHandleConstants.php',
     'PhabricatorObjectHandleStatus' => 'applications/phid/handle/const/PhabricatorObjectHandleStatus.php',
+    'PhabricatorObjectListQuery' => 'applications/phid/query/PhabricatorObjectListQuery.php',
+    'PhabricatorObjectListQueryTestCase' => 'applications/phid/query/__tests__/PhabricatorObjectListQueryTestCase.php',
     'PhabricatorObjectListView' => 'view/control/PhabricatorObjectListView.php',
     'PhabricatorObjectMailReceiver' => 'applications/metamta/receiver/PhabricatorObjectMailReceiver.php',
     'PhabricatorObjectMailReceiverTestCase' => 'applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php',
@@ -4545,6 +4547,7 @@ phutil_register_library_map(array(
     'PhabricatorOAuthServerTokenController' => 'PhabricatorAuthController',
     'PhabricatorObjectHandle' => 'PhabricatorPolicyInterface',
     'PhabricatorObjectHandleStatus' => 'PhabricatorObjectHandleConstants',
+    'PhabricatorObjectListQueryTestCase' => 'PhabricatorTestCase',
     'PhabricatorObjectListView' => 'AphrontView',
     'PhabricatorObjectMailReceiver' => 'PhabricatorMailReceiver',
     'PhabricatorObjectMailReceiverTestCase' => 'PhabricatorTestCase',

src/applications/differential/field/specification/DifferentialFieldSpecification.php

@@ -783,8 +783,7 @@ abstract class DifferentialFieldSpecification {
     return $this->parseCommitMessageObjectList(
       $value,
       $mailables = false,
-      $allow_partial = false,
+      $allow_partial = false);
-      $projects = true);
   }
 
   /**
@@ -814,89 +813,21 @@ abstract class DifferentialFieldSpecification {
     $include_mailables,
     $allow_partial = false) {
 
-    $value = array_unique(array_filter(preg_split('/[\s,]+/', $value)));
+    $types = array(
-    if (!$value) {
+      PhabricatorPeoplePHIDTypeUser::TYPECONST,
-      return array();
+      PhabricatorProjectPHIDTypeProject::TYPECONST,
-    }
+    );
-
-    $object_map = array();
-
-    $project_names = array();
-    $other_names = array();
-    foreach ($value as $item) {
-      if (preg_match('/^#/', $item)) {
-        $project_names[$item] = ltrim(phutil_utf8_strtolower($item), '#').'/';
-      } else {
-        $other_names[] = $item;
-      }
-    }
-
-    if ($project_names) {
-      // TODO: (T603) This should probably be policy-aware, although maybe not,
-      // since we generally don't want to destroy data and it doesn't leak
-      // anything?
-      $projects = id(new PhabricatorProjectQuery())
-        ->setViewer(PhabricatorUser::getOmnipotentUser())
-        ->withPhrictionSlugs($project_names)
-        ->execute();
-
-      $reverse_map = array_flip($project_names);
-      foreach ($projects as $project) {
-        $reverse_key = $project->getPhrictionSlug();
-        if (isset($reverse_map[$reverse_key])) {
-          $object_map[$reverse_map[$reverse_key]] = $project->getPHID();
-        }
-      }
-    }
-
-    if ($other_names) {
-      $users = id(new PhabricatorUser())->loadAllWhere(
-        '(username IN (%Ls))',
-        $other_names);
-
-      $user_map = mpull($users, 'getPHID', 'getUsername');
-      foreach ($user_map as $username => $phid) {
-        // Usernames may have uppercase letters in them. Put both names in the
-        // map so we can try the original case first, so that username *always*
-        // works in weird edge cases where some other mailable object collides.
-        $object_map[$username] = $phid;
-        $object_map[strtolower($username)] = $phid;
-      }
 
     if ($include_mailables) {
-        $mailables = id(new PhabricatorMetaMTAMailingList())->loadAllWhere(
+      $types[] = PhabricatorMailingListPHIDTypeList::TYPECONST;
-          '(email IN (%Ls)) OR (name IN (%Ls))',
-          $other_names,
-          $other_names);
-        $object_map += mpull($mailables, 'getPHID', 'getName');
-        $object_map += mpull($mailables, 'getPHID', 'getEmail');
-      }
     }
 
-    $invalid = array();
+    return id(new PhabricatorObjectListQuery())
-    $results = array();
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
-    foreach ($value as $name) {
+      ->setAllowPartialResults($allow_partial)
-      if (empty($object_map[$name])) {
+      ->setAllowedTypes($types)
-        if (empty($object_map[phutil_utf8_strtolower($name)])) {
+      ->setObjectList($value)
-          $invalid[] = $name;
+      ->execute();
-        } else {
-          $results[] = $object_map[phutil_utf8_strtolower($name)];
-        }
-      } else {
-        $results[] = $object_map[$name];
-      }
-    }
-
-    if ($invalid && !$allow_partial) {
-      $invalid = implode(', ', $invalid);
-      $what = $include_mailables
-        ? "users and mailing lists"
-        : "users";
-      throw new DifferentialFieldParseException(
-        "Commit message references nonexistent {$what}: {$invalid}.");
-    }
-
-    return array_unique($results);
   }
 
 

src/applications/mailinglists/phid/PhabricatorMailingListPHIDTypeList.php

@@ -37,4 +37,35 @@ final class PhabricatorMailingListPHIDTypeList extends PhabricatorPHIDType {
     }
   }
 
+  public function canLoadNamedObject($name) {
+    return preg_match('/^.+@.+/', $name);
+  }
+
+  public function loadNamedObjects(
+    PhabricatorObjectQuery $query,
+    array $names) {
+
+    $id_map = array();
+    foreach ($names as $name) {
+      // Maybe normalize these some day?
+      $id = $name;
+      $id_map[$id][] = $name;
+    }
+
+    $objects = id(new PhabricatorMailingListQuery())
+      ->setViewer($query->getViewer())
+      ->withEmails(array_keys($id_map))
+      ->execute();
+
+    $results = array();
+    foreach ($objects as $id => $object) {
+      $email = $object->getEmail();
+      foreach (idx($id_map, $email, array()) as $name) {
+        $results[$name] = $object;
+      }
+    }
+
+    return $results;
+  }
+
 }

src/applications/mailinglists/query/PhabricatorMailingListQuery.php

@@ -5,6 +5,8 @@ final class PhabricatorMailingListQuery
 
   private $phids;
   private $ids;
+  private $emails;
+  private $names;
 
   public function withIDs($ids) {
     $this->ids = $ids;
@@ -16,6 +18,16 @@ final class PhabricatorMailingListQuery
     return $this;
   }
 
+  public function withEmails(array $emails) {
+    $this->emails = $emails;
+    return $this;
+  }
+
+  public function withNames(array $names) {
+    $this->names = $names;
+    return $this;
+  }
+
   public function loadPage() {
     $table = new PhabricatorMetaMTAMailingList();
     $conn_r = $table->establishConnection('r');
@@ -48,6 +60,20 @@ final class PhabricatorMailingListQuery
         $this->phids);
     }
 
+    if ($this->names) {
+      $where[] = qsprintf(
+        $conn_r,
+        'name IN (%Ls)',
+        $this->names);
+    }
+
+    if ($this->emails) {
+      $where[] = qsprintf(
+        $conn_r,
+        'email IN (%Ls)',
+        $this->emails);
+    }
+
     $where[] = $this->buildPagingClause($conn_r);
 
     return $this->formatWhereClause($where);

src/applications/people/phid/PhabricatorPeoplePHIDTypeUser.php

@@ -52,4 +52,34 @@ final class PhabricatorPeoplePHIDTypeUser extends PhabricatorPHIDType {
 
   }
 
+  public function canLoadNamedObject($name) {
+    return preg_match('/^@.+/', $name);
+  }
+
+  public function loadNamedObjects(
+    PhabricatorObjectQuery $query,
+    array $names) {
+
+    $id_map = array();
+    foreach ($names as $name) {
+      $id = substr($name, 1);
+      $id_map[$id][] = $name;
+    }
+
+    $objects = id(new PhabricatorPeopleQuery())
+      ->setViewer($query->getViewer())
+      ->withUsernames(array_keys($id_map))
+      ->execute();
+
+    $results = array();
+    foreach ($objects as $id => $object) {
+      $username = $object->getUsername();
+      foreach (idx($id_map, $username, array()) as $name) {
+        $results[$name] = $object;
+      }
+    }
+
+    return $results;
+  }
+
 }

src/applications/phid/query/PhabricatorObjectListQuery.php

@@ -0,0 +1,171 @@
+<?php
+
+final class PhabricatorObjectListQuery {
+
+  private $viewer;
+  private $objectList;
+  private $allowedTypes = array();
+  private $allowPartialResults;
+
+  public function setAllowPartialResults($allow_partial_results) {
+    $this->allowPartialResults = $allow_partial_results;
+    return $this;
+  }
+
+  public function getAllowPartialResults() {
+    return $this->allowPartialResults;
+  }
+
+  public function setAllowedTypes(array $allowed_types) {
+    $this->allowedTypes = $allowed_types;
+    return $this;
+  }
+
+  public function getAllowedTypes() {
+    return $this->allowedTypes;
+  }
+
+  public function setViewer(PhabricatorUser $viewer) {
+    $this->viewer = $viewer;
+    return $this;
+  }
+
+  public function getViewer() {
+    return $this->viewer;
+  }
+
+  public function setObjectList($object_list) {
+    $this->objectList = $object_list;
+    return $this;
+  }
+
+  public function getObjectList() {
+    return $this->objectList;
+  }
+
+  public function execute() {
+    $names = $this->getObjectList();
+    $names = array_unique(array_filter(preg_split('/[\s,]+/', $names)));
+
+    $objects = $this->loadObjects($names);
+
+    $types = array();
+    foreach ($objects as $name => $object) {
+      $types[phid_get_type($object->getPHID())][] = $name;
+    }
+
+    $invalid = array();
+    if ($this->getAllowedTypes()) {
+      $allowed = array_fuse($this->getAllowedTypes());
+      foreach ($types as $type => $names_of_type) {
+        if (empty($allowed[$type])) {
+          $invalid[] = $names_of_type;
+        }
+      }
+    }
+    $invalid = array_mergev($invalid);
+
+    $missing = array();
+    foreach ($names as $name) {
+      if (empty($objects[$name])) {
+        $missing[] = $name;
+      }
+    }
+
+    // NOTE: We could couple this less tightly with Differential, but it is
+    // currently the only thing that uses it, and we'd have to add a lot of
+    // extra API to loosen this. It's not clear that this will be useful
+    // elsewhere any time soon, so let's cross that bridge when we come to it.
+
+    if (!$this->getAllowPartialResults()) {
+      if ($invalid && $missing) {
+        throw new DifferentialFieldParseException(
+          pht(
+            'The objects you have listed include objects of the wrong '.
+            'type (%s) and objects which do not exist (%s).',
+            implode(', ', $invalid),
+            implode(', ', $missing)));
+      } else if ($invalid) {
+        throw new DifferentialFieldParseException(
+          pht(
+            'The objects you have listed include objects of the wrong '.
+            'type (%s).',
+            implode(', ', $invalid)));
+      } else if ($missing) {
+        throw new DifferentialFieldParseException(
+          pht(
+            'The objects you have listed include objects which do not '.
+            'exist (%s).',
+            implode(', ', $missing)));
+      }
+    }
+
+    return array_values(array_unique(mpull($objects, 'getPHID')));
+  }
+
+  private function loadObjects($names) {
+    // First, try to load visible objects using monograms. This covers most
+    // object types, but does not cover users or user email addresses.
+    $query = id(new PhabricatorObjectQuery())
+      ->setViewer($this->getViewer())
+      ->withNames($names);
+
+    $query->execute();
+    $objects = $query->getNamedResults();
+
+    $results = array();
+    foreach ($names as $key => $name) {
+      if (isset($objects[$name])) {
+        $results[$name] = $objects[$name];
+        unset($names[$key]);
+      }
+    }
+
+    if ($names) {
+      // We still have some symbols we haven't been able to resolve, so try to
+      // load users. Try by username first...
+      $users = id(new PhabricatorPeopleQuery())
+        ->setViewer($this->getViewer())
+        ->withUsernames($names)
+        ->execute();
+
+      $user_map = array();
+      foreach ($users as $user) {
+        $user_map[phutil_utf8_strtolower($user->getUsername())] = $user;
+      }
+
+      foreach ($names as $key => $name) {
+        $normal_name = phutil_utf8_strtolower($name);
+        if (isset($user_map[$normal_name])) {
+          $results[$name] = $user_map[$normal_name];
+          unset($names[$key]);
+        }
+      }
+    }
+
+    $mailing_list_app = PhabricatorApplication::getByClass(
+      'PhabricatorApplicationMailingLists');
+    if ($mailing_list_app->isInstalled()) {
+      if ($names) {
+        // We still haven't been able to resolve everything; try mailing lists
+        // by name as a last resort.
+        $lists = id(new PhabricatorMailingListQuery())
+          ->setViewer($this->getViewer())
+          ->withNames($names)
+          ->execute();
+
+        $lists = mpull($lists, null, 'getName');
+        foreach ($names as $key => $name) {
+          if (isset($lists[$name])) {
+            $results[$name] = $lists[$name];
+            unset($names[$key]);
+          }
+        }
+      }
+    }
+
+    return $results;
+  }
+
+
+}

src/applications/phid/query/__tests__/PhabricatorObjectListQueryTestCase.php

@@ -0,0 +1,87 @@
+<?php
+
+final class PhabricatorObjectListQueryTestCase extends PhabricatorTestCase {
+
+  protected function getPhabricatorTestCaseConfiguration() {
+    return array(
+      self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES => true,
+    );
+  }
+
+  public function testObjectListQuery() {
+    $user = $this->generateNewTestUser();
+    $name = $user->getUsername();
+    $phid = $user->getPHID();
+
+
+    $result = $this->parseObjectList("@{$name}");
+    $this->assertEqual(array($phid), $result);
+
+    $result = $this->parseObjectList("{$name}");
+    $this->assertEqual(array($phid), $result);
+
+    $result = $this->parseObjectList("{$name}, {$name}");
+    $this->assertEqual(array($phid), $result);
+
+    $result = $this->parseObjectList("@{$name}, {$name}");
+    $this->assertEqual(array($phid), $result);
+
+    $result = $this->parseObjectList("");
+    $this->assertEqual(array(), $result);
+
+    // Expect failure when loading a user if objects must be of type "DUCK".
+    $caught = null;
+    try {
+      $result = $this->parseObjectList("{$name}", array("DUCK"));
+    } catch (Exception $ex) {
+      $caught = $ex;
+    }
+    $this->assertEqual(true, ($caught instanceof Exception));
+
+
+    // Expect failure when loading an invalid object.
+    $caught = null;
+    try {
+      $result = $this->parseObjectList("invalid");
+    } catch (Exception $ex) {
+      $caught = $ex;
+    }
+    $this->assertEqual(true, ($caught instanceof Exception));
+
+
+    // Expect failure when loading ANY invalid object, by default.
+    $caught = null;
+    try {
+      $result = $this->parseObjectList("{$name}, invalid");
+    } catch (Exception $ex) {
+      $caught = $ex;
+    }
+    $this->assertEqual(true, ($caught instanceof Exception));
+
+
+    // With partial results, this should load the valid user.
+    $result = $this->parseObjectList("{$name}, invalid", array(), true);
+    $this->assertEqual(array($phid), $result);
+  }
+
+  private function parseObjectList(
+    $list,
+    array $types = array(),
+    $allow_partial = false) {
+
+    $query = id(new PhabricatorObjectListQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->setObjectList($list);
+
+    if ($types) {
+      $query->setAllowedTypes($types);
+    }
+
+    if ($allow_partial) {
+      $query->setAllowPartialResults(true);
+    }
+
+    return $query->execute();
+  }
+
+}

src/applications/search/engine/PhabricatorJumpNavHandler.php

@@ -16,9 +16,7 @@ final class PhabricatorJumpNavHandler {
       '/^p$/i'                    => 'uri:/project/',
       '/^u$/i'                    => 'uri:/people/',
       '/^p\s+(.+)$/i'             => 'project',
-      '/^#(.+)$/i'                => 'project',
       '/^u\s+(\S+)$/i'            => 'user',
-      '/^@(.+)$/i'                => 'user',
       '/^task:\s*(.+)/i'          => 'create-task',
       '/^(?:s|symbol)\s+(\S+)/i'  => 'find-symbol',
       '/^r\s+(.+)$/i'             => 'find-repository',