From d2e5afb0959c61a8f2ad2b0319617cde32ab7b9f Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 21 Jul 2013 09:27:00 -0700 Subject: [PATCH] Use application PHIDs in Releeph, plus more Summary: Ref T2715. Ref T3551. Ref T603. This does a few things, but they're all sort of small: - We commonly use a `getX()` / `attachX()` pattern, but have very similar code in the `getX()` method every time. Provide a convenience method to make this pattern easier to write. - We use `willFilterPage()` in many queries, but it currently is called with zero or more results. This means we have a lot of "if no results, return nothing" boilerplate. Make it call only for one or more results. - Implement `PhabricatorPolicyInterface` on `ReleephBranch`. A branch has the same policy as its project. - Implement `ReleephBranchQuery`. - Move the branch PHID type to application PHID infrastructure. Test Plan: Browsed Releeph. Used `phid.query` to query branch PHIDs. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T603, T2715, T3551 Differential Revision: https://secure.phabricator.com/D6512 --- src/__phutil_library_map__.php | 13 +++- .../releeph/ReleephObjectHandleLoader.php | 45 ----------- .../releeph/phid/ReleephPHIDTypeBranch.php | 47 ++++++++++++ .../releeph/query/ReleephBranchQuery.php | 76 +++++++++++++++++++ .../releeph/query/ReleephProjectQuery.php | 13 ++++ .../releeph/storage/ReleephBranch.php | 33 +++++++- .../policy/PhabricatorPolicyAwareQuery.php | 10 ++- .../PhabricatorDataNotAttachedException.php | 35 +++++++++ .../storage/lisk/PhabricatorLiskDAO.php | 9 +++ 9 files changed, 230 insertions(+), 51 deletions(-) delete mode 100644 src/applications/releeph/ReleephObjectHandleLoader.php create mode 100644 src/applications/releeph/phid/ReleephPHIDTypeBranch.php create mode 100644 src/applications/releeph/query/ReleephBranchQuery.php create mode 100644 src/infrastructure/storage/lisk/PhabricatorDataNotAttachedException.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index de45ec10f0..0fb3cbf42d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1032,6 +1032,7 @@ phutil_register_library_map(array( 'PhabricatorDaemonManagementStopWorkflow' => 'applications/daemon/management/PhabricatorDaemonManagementStopWorkflow.php', 'PhabricatorDaemonManagementWorkflow' => 'applications/daemon/management/PhabricatorDaemonManagementWorkflow.php', 'PhabricatorDaemonReference' => 'infrastructure/daemon/control/PhabricatorDaemonReference.php', + 'PhabricatorDataNotAttachedException' => 'infrastructure/storage/lisk/PhabricatorDataNotAttachedException.php', 'PhabricatorDebugController' => 'applications/system/PhabricatorDebugController.php', 'PhabricatorDefaultFileStorageEngineSelector' => 'applications/files/engineselector/PhabricatorDefaultFileStorageEngineSelector.php', 'PhabricatorDefaultSearchEngineSelector' => 'applications/search/selector/PhabricatorDefaultSearchEngineSelector.php', @@ -1876,6 +1877,7 @@ phutil_register_library_map(array( 'ReleephBranchEditor' => 'applications/releeph/editor/ReleephBranchEditor.php', 'ReleephBranchNamePreviewController' => 'applications/releeph/controller/branch/ReleephBranchNamePreviewController.php', 'ReleephBranchPreviewView' => 'applications/releeph/view/branch/ReleephBranchPreviewView.php', + 'ReleephBranchQuery' => 'applications/releeph/query/ReleephBranchQuery.php', 'ReleephBranchTemplate' => 'applications/releeph/view/branch/ReleephBranchTemplate.php', 'ReleephBranchViewController' => 'applications/releeph/controller/branch/ReleephBranchViewController.php', 'ReleephCommitFinder' => 'applications/releeph/commitfinder/ReleephCommitFinder.php', @@ -1896,9 +1898,9 @@ phutil_register_library_map(array( 'ReleephFieldSpecificationIncompleteException' => 'applications/releeph/field/exception/ReleephFieldSpecificationIncompleteException.php', 'ReleephIntentFieldSpecification' => 'applications/releeph/field/specification/ReleephIntentFieldSpecification.php', 'ReleephLevelFieldSpecification' => 'applications/releeph/field/specification/ReleephLevelFieldSpecification.php', - 'ReleephObjectHandleLoader' => 'applications/releeph/ReleephObjectHandleLoader.php', 'ReleephOriginalCommitFieldSpecification' => 'applications/releeph/field/specification/ReleephOriginalCommitFieldSpecification.php', 'ReleephPHIDConstants' => 'applications/releeph/ReleephPHIDConstants.php', + 'ReleephPHIDTypeBranch' => 'applications/releeph/phid/ReleephPHIDTypeBranch.php', 'ReleephPHIDTypeProject' => 'applications/releeph/phid/ReleephPHIDTypeProject.php', 'ReleephPHIDTypeRequest' => 'applications/releeph/phid/ReleephPHIDTypeRequest.php', 'ReleephProject' => 'applications/releeph/storage/ReleephProject.php', @@ -3007,6 +3009,7 @@ phutil_register_library_map(array( 'PhabricatorDaemonManagementStatusWorkflow' => 'PhabricatorDaemonManagementWorkflow', 'PhabricatorDaemonManagementStopWorkflow' => 'PhabricatorDaemonManagementWorkflow', 'PhabricatorDaemonManagementWorkflow' => 'PhutilArgumentWorkflow', + 'PhabricatorDataNotAttachedException' => 'Exception', 'PhabricatorDebugController' => 'PhabricatorController', 'PhabricatorDefaultFileStorageEngineSelector' => 'PhabricatorFileStorageEngineSelector', 'PhabricatorDefaultSearchEngineSelector' => 'PhabricatorSearchEngineSelector', @@ -3910,7 +3913,11 @@ phutil_register_library_map(array( 'ProjectRemarkupRule' => 'PhabricatorRemarkupRuleObject', 'QueryFormattingTestCase' => 'PhabricatorTestCase', 'ReleephAuthorFieldSpecification' => 'ReleephFieldSpecification', - 'ReleephBranch' => 'ReleephDAO', + 'ReleephBranch' => + array( + 0 => 'ReleephDAO', + 1 => 'PhabricatorPolicyInterface', + ), 'ReleephBranchAccessController' => 'ReleephProjectController', 'ReleephBranchBoxView' => 'AphrontView', 'ReleephBranchCommitFieldSpecification' => 'ReleephFieldSpecification', @@ -3919,6 +3926,7 @@ phutil_register_library_map(array( 'ReleephBranchEditor' => 'PhabricatorEditor', 'ReleephBranchNamePreviewController' => 'ReleephController', 'ReleephBranchPreviewView' => 'AphrontFormControl', + 'ReleephBranchQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'ReleephBranchViewController' => 'ReleephProjectController', 'ReleephCommitFinderException' => 'Exception', 'ReleephCommitMessageFieldSpecification' => 'ReleephFieldSpecification', @@ -3936,6 +3944,7 @@ phutil_register_library_map(array( 'ReleephIntentFieldSpecification' => 'ReleephFieldSpecification', 'ReleephLevelFieldSpecification' => 'ReleephFieldSpecification', 'ReleephOriginalCommitFieldSpecification' => 'ReleephFieldSpecification', + 'ReleephPHIDTypeBranch' => 'PhabricatorPHIDType', 'ReleephPHIDTypeProject' => 'PhabricatorPHIDType', 'ReleephPHIDTypeRequest' => 'PhabricatorPHIDType', 'ReleephProject' => diff --git a/src/applications/releeph/ReleephObjectHandleLoader.php b/src/applications/releeph/ReleephObjectHandleLoader.php deleted file mode 100644 index e47ef2ee04..0000000000 --- a/src/applications/releeph/ReleephObjectHandleLoader.php +++ /dev/null @@ -1,45 +0,0 @@ - $phids) { - switch ($type) { - - case ReleephPHIDConstants::PHID_TYPE_REBR: - $object = new ReleephBranch(); - - $branches = $object->loadAllWhere('phid IN (%Ls)', $phids); - $branches = mpull($branches, null, 'getPHID'); - - foreach ($phids as $phid) { - $branch = $branches[$phid]; - $handle = new PhabricatorObjectHandle(); - $handle->setPHID($phid); - $handle->setType($type); - $handle->setURI($branch->getURI()); - $handle->setName($branch->getBasename()); - $handle->setFullName($branch->getName()); - $handle->setComplete(true); - $handles[$phid] = $handle; - } - break; - - default: - throw new Exception('unknown type '.$type); - } - } - - return $handles; - } - -} diff --git a/src/applications/releeph/phid/ReleephPHIDTypeBranch.php b/src/applications/releeph/phid/ReleephPHIDTypeBranch.php new file mode 100644 index 0000000000..b4a51a1399 --- /dev/null +++ b/src/applications/releeph/phid/ReleephPHIDTypeBranch.php @@ -0,0 +1,47 @@ +setViewer($query->getViewer()) + ->withPHIDs($phids) + ->execute(); + } + + public function loadHandles( + PhabricatorHandleQuery $query, + array $handles, + array $objects) { + + foreach ($handles as $phid => $handle) { + $branch = $objects[$phid]; + + $handle->setURI($branch->getURI()); + $handle->setName($branch->getBasename()); + $handle->setFullName($branch->getName()); + } + } + + public function canLoadNamedObject($name) { + return false; + } + +} diff --git a/src/applications/releeph/query/ReleephBranchQuery.php b/src/applications/releeph/query/ReleephBranchQuery.php new file mode 100644 index 0000000000..82fe8d4e14 --- /dev/null +++ b/src/applications/releeph/query/ReleephBranchQuery.php @@ -0,0 +1,76 @@ +ids = $ids; + return $this; + } + + public function withPHIDs(array $phids) { + $this->phids = $phids; + return $this; + } + + public function loadPage() { + $table = new ReleephBranch(); + $conn_r = $table->establishConnection('r'); + + $data = queryfx_all( + $conn_r, + 'SELECT * FROM %T %Q %Q %Q', + $table->getTableName(), + $this->buildWhereClause($conn_r), + $this->buildOrderClause($conn_r), + $this->buildLimitClause($conn_r)); + + return $table->loadAllFromArray($data); + } + + public function willFilterPage(array $branches) { + $project_ids = mpull($branches, 'getReleephProjectID'); + + $projects = id(new ReleephProjectQuery()) + ->withIDs($project_ids) + ->setViewer($this->getViewer()) + ->execute(); + + foreach ($branches as $key => $branch) { + $project_id = $project_ids[$key]; + if (isset($projects[$project_id])) { + $branch->attachProject($projects[$project_id]); + } else { + unset($branches[$key]); + } + } + + return $branches; + } + + private function buildWhereClause(AphrontDatabaseConnection $conn_r) { + $where = array(); + + if ($this->ids) { + $where[] = qsprintf( + $conn_r, + 'id IN (%Ld)', + $this->ids); + } + + if ($this->phids) { + $where[] = qsprintf( + $conn_r, + 'phid IN (%Ls)', + $this->phids); + } + + $where[] = $this->buildPagingClause($conn_r); + + return $this->formatWhereClause($where); + } + +} diff --git a/src/applications/releeph/query/ReleephProjectQuery.php b/src/applications/releeph/query/ReleephProjectQuery.php index bbd4ad0744..2104fde217 100644 --- a/src/applications/releeph/query/ReleephProjectQuery.php +++ b/src/applications/releeph/query/ReleephProjectQuery.php @@ -4,6 +4,7 @@ final class ReleephProjectQuery extends PhabricatorCursorPagedPolicyAwareQuery { private $active; + private $ids; private $phids; private $order = 'order-id'; @@ -20,6 +21,11 @@ final class ReleephProjectQuery return $this; } + public function withIDs(array $ids) { + $this->ids = $ids; + return $this; + } + public function withPHIDs(array $phids) { $this->phids = $phids; return $this; @@ -50,6 +56,13 @@ final class ReleephProjectQuery $this->active); } + if ($this->ids) { + $where[] = qsprintf( + $conn_r, + 'id IN (%Ls)', + $this->ids); + } + if ($this->phids) { $where[] = qsprintf( $conn_r, diff --git a/src/applications/releeph/storage/ReleephBranch.php b/src/applications/releeph/storage/ReleephBranch.php index 12887a383c..9053b53d91 100644 --- a/src/applications/releeph/storage/ReleephBranch.php +++ b/src/applications/releeph/storage/ReleephBranch.php @@ -1,6 +1,7 @@ true, @@ -31,8 +34,7 @@ final class ReleephBranch extends ReleephDAO { } public function generatePHID() { - return PhabricatorPHID::generateNewPHID( - ReleephPHIDConstants::PHID_TYPE_REBR); + return PhabricatorPHID::generateNewPHID(ReleephPHIDTypeBranch::TYPECONST); } public function getDetail($key, $default = null) { @@ -151,4 +153,29 @@ final class ReleephBranch extends ReleephDAO { return $this->getIsActive(); } + public function attachProject(ReleephProject $project) { + $this->project = $project; + return $this; + } + + public function getProject() { + return $this->assertAttached($this->project); + } + + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + + public function getCapabilities() { + return $this->getProject()->getCapabilities(); + } + + public function getPolicy($capability) { + return $this->getProject()->getPolicy($capability); + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return $this->getProject()->hasAutomaticCapability($capability, $viewer); + } + } diff --git a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php index 1d5ca96504..f544401ddc 100644 --- a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php @@ -176,7 +176,12 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { $page = array(); } - $visible = $this->willFilterPage($page); + if ($page) { + $visible = $this->willFilterPage($page); + } else { + $visible = array(); + } + $visible = $filter->apply($visible); foreach ($visible as $key => $result) { ++$count; @@ -272,6 +277,9 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { * you to drop some items from the result set without creating problems with * pagination or cursor updates. * + * This method will only be called if data is available. Implementations + * do not need to handle the case of no results specially. + * * @param list Results from `loadPage()`. * @return list Objects for policy filtering. * @task policyimpl diff --git a/src/infrastructure/storage/lisk/PhabricatorDataNotAttachedException.php b/src/infrastructure/storage/lisk/PhabricatorDataNotAttachedException.php new file mode 100644 index 0000000000..7c1453b6c0 --- /dev/null +++ b/src/infrastructure/storage/lisk/PhabricatorDataNotAttachedException.php @@ -0,0 +1,35 @@ +"; /* -( Managing Edges )----------------------------------------------------- */ @@ -207,4 +208,12 @@ abstract class PhabricatorLiskDAO extends LiskDAO { return $result; } + + protected function assertAttached($property) { + if ($property === self::ATTACHABLE) { + throw new PhabricatorDataNotAttachedException($this); + } + return $property; + } + }