From 36a50ccbae1e004eb1bb20003d726593cba6d814 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 15 Aug 2015 13:06:10 -0700 Subject: [PATCH] Provide a more modern way to load packages owning a set of files Summary: Ref T8320. Ref T8004. This just tries to generally modernize It also replaces the nonfunctional "Find Owners" link with a new property that just shows owning packages. Test Plan: - Created and edited packages. {F720478} Reviewers: chad Reviewed By: chad Maniphest Tasks: T8004, T8320 Differential Revision: https://secure.phabricator.com/D13911 --- .../controller/DiffusionBrowseController.php | 62 ++++--- .../PhabricatorOwnersDetailController.php | 3 +- .../PhabricatorOwnersPathsController.php | 3 +- ...bricatorOwnersPackageTransactionEditor.php | 6 +- .../query/PhabricatorOwnersPackageQuery.php | 151 +++++++++++++++++- .../storage/PhabricatorOwnersPackage.php | 14 +- .../owners/storage/PhabricatorOwnersPath.php | 32 ++++ 7 files changed, 242 insertions(+), 29 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 3d7bfb78fd..6fd164f2a7 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -111,25 +111,6 @@ abstract class DiffusionBrowseController extends DiffusionController { ->setIcon('fa-home') ->setDisabled(!$behind_head)); - // TODO: Ideally, this should live in Owners and be event-triggered, but - // there's no reasonable object for it to react to right now. - - $owners = 'PhabricatorOwnersApplication'; - if (PhabricatorApplication::isClassInstalled($owners)) { - $owners_uri = id(new PhutilURI('/owners/view/search/')) - ->setQueryParams( - array( - 'repository' => $drequest->getCallsign(), - 'path' => '/'.$drequest->getPath(), - )); - - $view->addAction( - id(new PhabricatorActionView()) - ->setName(pht('Find Owners')) - ->setHref((string)$owners_uri) - ->setIcon('fa-users')); - } - return $view; } @@ -137,7 +118,7 @@ abstract class DiffusionBrowseController extends DiffusionController { DiffusionRequest $drequest, PhabricatorActionListView $actions) { - $viewer = $this->getRequest()->getUser(); + $viewer = $this->getViewer(); $view = id(new PHUIPropertyListView()) ->setUser($viewer) @@ -180,6 +161,47 @@ abstract class DiffusionBrowseController extends DiffusionController { } } + $repository = $drequest->getRepository(); + + $owners = 'PhabricatorOwnersApplication'; + if (PhabricatorApplication::isClassInstalled($owners)) { + $package_query = id(new PhabricatorOwnersPackageQuery()) + ->setViewer($viewer) + ->withControl( + $repository->getPHID(), + array( + $drequest->getPath(), + )); + + $package_query->execute(); + + $packages = $package_query->getControllingPackagesForPath( + $repository->getPHID(), + $drequest->getPath()); + + if ($packages) { + $ownership = id(new PHUIStatusListView()) + ->setUser($viewer); + + + + foreach ($packages as $package) { + $icon = 'fa-list-alt'; + $color = 'grey'; + + $item = id(new PHUIStatusItemView()) + ->setIcon($icon, $color) + ->setTarget($viewer->renderHandle($package->getPHID())); + + $ownership->addItem($item); + } + } else { + $ownership = phutil_tag('em', array(), pht('None')); + } + + $view->addProperty(pht('Packages'), $ownership); + } + return $view; } diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php index 6ec31eadfc..c3f0ec42f4 100644 --- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php +++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php @@ -13,12 +13,13 @@ final class PhabricatorOwnersDetailController $package = id(new PhabricatorOwnersPackageQuery()) ->setViewer($viewer) ->withIDs(array($request->getURIData('id'))) + ->needPaths(true) ->executeOne(); if (!$package) { return new Aphront404Response(); } - $paths = $package->loadPaths(); + $paths = $package->getPaths(); $repository_phids = array(); foreach ($paths as $path) { diff --git a/src/applications/owners/controller/PhabricatorOwnersPathsController.php b/src/applications/owners/controller/PhabricatorOwnersPathsController.php index 95df2cb807..ad278d5296 100644 --- a/src/applications/owners/controller/PhabricatorOwnersPathsController.php +++ b/src/applications/owners/controller/PhabricatorOwnersPathsController.php @@ -15,6 +15,7 @@ final class PhabricatorOwnersPathsController // TODO: Support this capability. // PhabricatorPolicyCapability::CAN_EDIT, )) + ->needPaths(true) ->executeOne(); if (!$package) { return new Aphront404Response(); @@ -66,7 +67,7 @@ final class PhabricatorOwnersPathsController return id(new AphrontRedirectResponse()) ->setURI('/owners/package/'.$package->getID().'/'); } else { - $paths = $package->loadPaths(); + $paths = $package->getPaths(); $path_refs = mpull($paths, 'getRef'); } diff --git a/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php b/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php index 8ce1e48c29..311cf642a0 100644 --- a/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php +++ b/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php @@ -43,8 +43,7 @@ final class PhabricatorOwnersPackageTransactionEditor case PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION: return $object->getDescription(); case PhabricatorOwnersPackageTransaction::TYPE_PATHS: - // TODO: needPaths() this on the query - $paths = $object->loadPaths(); + $paths = $object->getPaths(); return mpull($paths, 'getRef'); } } @@ -152,8 +151,7 @@ final class PhabricatorOwnersPackageTransactionEditor $old = $xaction->getOldValue(); $new = $xaction->getNewValue(); - // TODO: needPaths this - $paths = $object->loadPaths(); + $paths = $object->getPaths(); $diffs = PhabricatorOwnersPath::getTransactionValueChanges($old, $new); list($rem, $add) = $diffs; diff --git a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php index 1d34d7be58..8dbb74e528 100644 --- a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php +++ b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php @@ -8,6 +8,10 @@ final class PhabricatorOwnersPackageQuery private $ownerPHIDs; private $repositoryPHIDs; private $namePrefix; + private $needPaths; + + private $controlMap = array(); + private $controlResults; /** * Owners are direct owners, and members of owning projects. @@ -32,19 +36,64 @@ final class PhabricatorOwnersPackageQuery return $this; } + public function withControl($repository_phid, array $paths) { + if (empty($this->controlMap[$repository_phid])) { + $this->controlMap[$repository_phid] = array(); + } + + foreach ($paths as $path) { + $this->controlMap[$repository_phid][$path] = $path; + } + + // We need to load paths to execute control queries. + $this->needPaths = true; + + return $this; + } + public function withNamePrefix($prefix) { $this->namePrefix = $prefix; return $this; } + public function needPaths($need_paths) { + $this->needPaths = $need_paths; + return $this; + } + public function newResultObject() { return new PhabricatorOwnersPackage(); } + protected function willExecute() { + $this->controlResults = array(); + } + protected function loadPage() { return $this->loadStandardPage(new PhabricatorOwnersPackage()); } + protected function didFilterPage(array $packages) { + if ($this->needPaths) { + $package_ids = mpull($packages, 'getID'); + + $paths = id(new PhabricatorOwnersPath())->loadAllWhere( + 'packageID IN (%Ld)', + $package_ids); + $paths = mgroup($paths, 'getPackageID'); + + foreach ($packages as $package) { + $package->attachPaths(idx($paths, $package->getID(), array())); + } + } + + if ($this->controlMap) { + $this->controlResults += mpull($packages, null, 'getID'); + } + + return $packages; + } + protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { $joins = parent::buildJoinClauseParts($conn); @@ -55,7 +104,7 @@ final class PhabricatorOwnersPackageQuery id(new PhabricatorOwnersOwner())->getTableName()); } - if ($this->repositoryPHIDs !== null) { + if ($this->shouldJoinOwnersPathTable()) { $joins[] = qsprintf( $conn, 'JOIN %T rpath ON rpath.packageID = p.id', @@ -115,11 +164,30 @@ final class PhabricatorOwnersPackageQuery phutil_utf8_strtolower($this->namePrefix)); } + if ($this->controlMap) { + $clauses = array(); + foreach ($this->controlMap as $repository_phid => $paths) { + $fragments = array(); + foreach ($paths as $path) { + foreach (PhabricatorOwnersPackage::splitPath($path) as $fragment) { + $fragments[$fragment] = $fragment; + } + } + + $clauses[] = qsprintf( + $conn, + '(rpath.repositoryPHID = %s AND rpath.path IN (%Ls))', + $repository_phid, + $fragments); + } + $where[] = implode(' OR ', $clauses); + } + return $where; } protected function shouldGroupQueryResultRows() { - if ($this->repositoryPHIDs) { + if ($this->shouldJoinOwnersPathTable()) { return true; } @@ -167,4 +235,83 @@ final class PhabricatorOwnersPackageQuery return 'p'; } + private function shouldJoinOwnersPathTable() { + if ($this->repositoryPHIDs !== null) { + return true; + } + + if ($this->controlMap) { + return true; + } + + return false; + } + + +/* -( Path Control )------------------------------------------------------- */ + + + /** + * Get the package which controls a path, if one exists. + * + * @return PhabricatorOwnersPackage|null Package, if one exists. + */ + public function getControllingPackageForPath($repository_phid, $path) { + $packages = $this->getControllingPackagesForPath($repository_phid, $path); + + if (!$packages) { + return null; + } + + return head($packages); + } + + + /** + * Get a list of all packages which control a path or its parent directories, + * ordered from weakest to strongest. + * + * The first package has the most specific claim on the path; the last + * package has the most general claim. + * + * @return list List of controlling packages. + */ + public function getControllingPackagesForPath($repository_phid, $path) { + if (!isset($this->controlMap[$repository_phid][$path])) { + throw new PhutilInvalidStateException('withControl'); + } + + if ($this->controlResults === null) { + throw new PhutilInvalidStateException('execute'); + } + + $packages = $this->controlResults; + + $matches = array(); + foreach ($packages as $package_id => $package) { + $best_match = null; + $include = false; + + foreach ($package->getPaths() as $package_path) { + $strength = $package_path->getPathMatchStrength($path); + if ($strength > $best_match) { + $best_match = $strength; + $include = !$package_path->getExcluded(); + } + } + + if ($best_match && $include) { + $matches[$package_id] = array( + 'strength' => $best_match, + 'package' => $package, + ); + } + } + + $matches = isort($matches, 'strength'); + $matches = array_reverse($matches); + + return array_values(ipull($matches, 'package')); + } + } diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php index ecf1862323..82c8ca9074 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php @@ -13,6 +13,8 @@ final class PhabricatorOwnersPackage protected $primaryOwnerPHID; protected $mailKey; + private $paths = self::ATTACHABLE; + public static function initializeNewPackage(PhabricatorUser $actor) { return id(new PhabricatorOwnersPackage()) ->setAuditingEnabled(0) @@ -225,7 +227,7 @@ final class PhabricatorOwnersPackage return $ids; } - private static function splitPath($path) { + public static function splitPath($path) { $result = array('/'); $trailing_slash = preg_match('@/$@', $path) ? '/' : ''; $path = trim($path, '/'); @@ -238,6 +240,16 @@ final class PhabricatorOwnersPackage return $result; } + public function attachPaths(array $paths) { + assert_instances_of($paths, 'PhabricatorOwnersPath'); + $this->paths = $paths; + return $this; + } + + public function getPaths() { + return $this->assertAttached($this->paths); + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/owners/storage/PhabricatorOwnersPath.php b/src/applications/owners/storage/PhabricatorOwnersPath.php index f65d6052db..33ab109719 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPath.php +++ b/src/applications/owners/storage/PhabricatorOwnersPath.php @@ -70,4 +70,36 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO { return isset($set[$ref['repositoryPHID']][$ref['path']][$ref['excluded']]); } + /** + * Get the number of directory matches between this path specification and + * some real path. + */ + public function getPathMatchStrength($path) { + $this_path = $this->getPath(); + + if ($this_path === '/') { + // The root path "/" just matches everything with strength 1. + return 1; + } + + $self_fragments = PhabricatorOwnersPackage::splitPath($this_path); + $path_fragments = PhabricatorOwnersPackage::splitPath($path); + + $self_count = count($self_fragments); + $path_count = count($path_fragments); + if ($self_count > $path_count) { + // If this path is longer (and therefor more specific) than the target + // path, we don't match it at all. + return 0; + } + + for ($ii = 0; $ii < $self_count; $ii++) { + if ($self_fragments[$ii] != $path_fragments[$ii]) { + return 0; + } + } + + return $self_count; + } + }