From 76577df5061403784b6a399dfcf4745c389bd6cf Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 7 Mar 2014 17:44:44 -0800 Subject: [PATCH] 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 | 3 + .../DifferentialFieldSpecification.php | 97 ++-------- .../PhabricatorMailingListPHIDTypeList.php | 31 ++++ .../query/PhabricatorMailingListQuery.php | 26 +++ .../phid/PhabricatorPeoplePHIDTypeUser.php | 30 +++ .../phid/query/PhabricatorObjectListQuery.php | 171 ++++++++++++++++++ .../PhabricatorObjectListQueryTestCase.php | 87 +++++++++ .../engine/PhabricatorJumpNavHandler.php | 2 - 8 files changed, 362 insertions(+), 85 deletions(-) create mode 100644 src/applications/phid/query/PhabricatorObjectListQuery.php create mode 100644 src/applications/phid/query/__tests__/PhabricatorObjectListQueryTestCase.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 900dc0d6fe..22beb582db 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/differential/field/specification/DifferentialFieldSpecification.php b/src/applications/differential/field/specification/DifferentialFieldSpecification.php index 4f4a2cace2..a7d45ab2f9 100644 --- a/src/applications/differential/field/specification/DifferentialFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialFieldSpecification.php @@ -783,8 +783,7 @@ abstract class DifferentialFieldSpecification { return $this->parseCommitMessageObjectList( $value, $mailables = false, - $allow_partial = false, - $projects = true); + $allow_partial = false); } /** @@ -814,89 +813,21 @@ abstract class DifferentialFieldSpecification { $include_mailables, $allow_partial = false) { - $value = array_unique(array_filter(preg_split('/[\s,]+/', $value))); - if (!$value) { - return array(); + $types = array( + PhabricatorPeoplePHIDTypeUser::TYPECONST, + PhabricatorProjectPHIDTypeProject::TYPECONST, + ); + + if ($include_mailables) { + $types[] = PhabricatorMailingListPHIDTypeList::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( - '(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(); - $results = array(); - foreach ($value as $name) { - if (empty($object_map[$name])) { - if (empty($object_map[phutil_utf8_strtolower($name)])) { - $invalid[] = $name; - } 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); + return id(new PhabricatorObjectListQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->setAllowPartialResults($allow_partial) + ->setAllowedTypes($types) + ->setObjectList($value) + ->execute(); } diff --git a/src/applications/mailinglists/phid/PhabricatorMailingListPHIDTypeList.php b/src/applications/mailinglists/phid/PhabricatorMailingListPHIDTypeList.php index 1b6d08c844..df814dfa3a 100644 --- a/src/applications/mailinglists/phid/PhabricatorMailingListPHIDTypeList.php +++ b/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; + } + } diff --git a/src/applications/mailinglists/query/PhabricatorMailingListQuery.php b/src/applications/mailinglists/query/PhabricatorMailingListQuery.php index 77551d3984..66b128123c 100644 --- a/src/applications/mailinglists/query/PhabricatorMailingListQuery.php +++ b/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); diff --git a/src/applications/people/phid/PhabricatorPeoplePHIDTypeUser.php b/src/applications/people/phid/PhabricatorPeoplePHIDTypeUser.php index 1bc144c6d0..c84607994f 100644 --- a/src/applications/people/phid/PhabricatorPeoplePHIDTypeUser.php +++ b/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; + } + } diff --git a/src/applications/phid/query/PhabricatorObjectListQuery.php b/src/applications/phid/query/PhabricatorObjectListQuery.php new file mode 100644 index 0000000000..97a0c16938 --- /dev/null +++ b/src/applications/phid/query/PhabricatorObjectListQuery.php @@ -0,0 +1,171 @@ +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; + } + + +} diff --git a/src/applications/phid/query/__tests__/PhabricatorObjectListQueryTestCase.php b/src/applications/phid/query/__tests__/PhabricatorObjectListQueryTestCase.php new file mode 100644 index 0000000000..5d0cdb968c --- /dev/null +++ b/src/applications/phid/query/__tests__/PhabricatorObjectListQueryTestCase.php @@ -0,0 +1,87 @@ + 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(); + } + +} diff --git a/src/applications/search/engine/PhabricatorJumpNavHandler.php b/src/applications/search/engine/PhabricatorJumpNavHandler.php index e3081dfebd..e8790e1635 100644 --- a/src/applications/search/engine/PhabricatorJumpNavHandler.php +++ b/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',