From 8efdc4aabf09f799fbbf99fab7d97c81e129cb3d Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 12 Apr 2015 16:44:15 -0700 Subject: [PATCH] Replace getPagingValue() with cursor methods Summary: Ref T7803. Prior to this change sequence, Query classes conflated paging values (the actual thing that goes in a "x > 3" clause) with cursor values (arbitrary identifiers which track where the user is in a result list). Although the two can sometimes be the same, the vast majority of implementations are simpler and better when object IDs are used as cursors and paging values are derived from them. The new stuff handles this in a consistent way, so we're free to separate getPagingValue() from paging. The new method is essentially getResultCursor(). This also implements getPageCursors(), which allows queries to return directional cursors. The inability to do this was a practical limitation blocking the implementation of T7803. Test Plan: - Browsed a bunch of results and paged through queries. - Grepped for removed methods. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7803 Differential Revision: https://secure.phabricator.com/D12383 --- .../feed/query/PhabricatorFeedQuery.php | 2 +- .../maniphest/query/ManiphestTaskQuery.php | 16 ++----- .../query/PhabricatorNotificationQuery.php | 2 +- .../project/query/PhabricatorProjectQuery.php | 7 +-- .../query/PhabricatorSearchDocumentQuery.php | 2 +- ...PhabricatorCursorPagedPolicyAwareQuery.php | 46 +++++++++++++------ 6 files changed, 41 insertions(+), 34 deletions(-) diff --git a/src/applications/feed/query/PhabricatorFeedQuery.php b/src/applications/feed/query/PhabricatorFeedQuery.php index a0851aeca0..5f3efe5aaf 100644 --- a/src/applications/feed/query/PhabricatorFeedQuery.php +++ b/src/applications/feed/query/PhabricatorFeedQuery.php @@ -112,7 +112,7 @@ final class PhabricatorFeedQuery ); } - protected function getPagingValue($item) { + protected function getResultCursor($item) { if ($item instanceof PhabricatorFeedStory) { return $item->getChronologicalKey(); } diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php index 433a4cec7b..cf83f469ae 100644 --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -896,20 +896,14 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { return array_mergev($phids); } - protected function getPagingValue($result) { + protected function getResultCursor($result) { $id = $result->getID(); - switch ($this->groupBy) { - case self::GROUP_NONE: - case self::GROUP_STATUS: - case self::GROUP_PRIORITY: - case self::GROUP_OWNER: - return $id; - case self::GROUP_PROJECT: - return rtrim($id.'.'.$result->getGroupByProjectPHID(), '.'); - default: - throw new Exception("Unknown group query '{$this->groupBy}'!"); + if ($this->groupBy == self::GROUP_PROJECT) { + return rtrim($id.'.'.$result->getGroupByProjectPHID(), '.');; } + + return $id; } public function getOrderableColumns() { diff --git a/src/applications/notification/query/PhabricatorNotificationQuery.php b/src/applications/notification/query/PhabricatorNotificationQuery.php index 263aea04db..a1f8ac63c7 100644 --- a/src/applications/notification/query/PhabricatorNotificationQuery.php +++ b/src/applications/notification/query/PhabricatorNotificationQuery.php @@ -103,7 +103,7 @@ final class PhabricatorNotificationQuery return $this->formatWhereClause($where); } - protected function getPagingValue($item) { + protected function getResultCursor($item) { return $item->getChronologicalKey(); } diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php index 89d1f31dba..1dfc466712 100644 --- a/src/applications/project/query/PhabricatorProjectQuery.php +++ b/src/applications/project/query/PhabricatorProjectQuery.php @@ -112,15 +112,12 @@ final class PhabricatorProjectQuery } protected function getPagingValueMap($cursor, array $keys) { + $project = $this->loadCursorObject($cursor); return array( - 'name' => $cursor, + 'name' => $project->getName(), ); } - protected function getPagingValue($result) { - return $result->getName(); - } - protected function loadPage() { $table = new PhabricatorProject(); $conn_r = $table->establishConnection('r'); diff --git a/src/applications/search/query/PhabricatorSearchDocumentQuery.php b/src/applications/search/query/PhabricatorSearchDocumentQuery.php index aa69fc1783..b5591d8eca 100644 --- a/src/applications/search/query/PhabricatorSearchDocumentQuery.php +++ b/src/applications/search/query/PhabricatorSearchDocumentQuery.php @@ -69,7 +69,7 @@ final class PhabricatorSearchDocumentQuery return 'PhabricatorSearchApplication'; } - protected function getPagingValue($result) { + protected function getResultCursor($result) { throw new Exception( pht( 'This query does not support cursor paging; it must be offset '. diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 068229c9ef..f0cff78e7c 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -19,23 +19,35 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery private $orderVector; private $builtinOrder; - protected function getPagingValue($result) { - if (!is_object($result)) { - // This interface can't be typehinted and PHP gets really angry if we - // call a method on a non-object, so add an explicit check here. - throw new Exception(pht('Expected object, got "%s"!', gettype($result))); + protected function getPageCursors(array $page) { + return array( + $this->getResultCursor(head($page)), + $this->getResultCursor(last($page)), + ); + } + + protected function getResultCursor($object) { + if (!is_object($object)) { + throw new Exception( + pht( + 'Expected object, got "%s".', + gettype($object))); } - return $result->getID(); + + return $object->getID(); } protected function nextPage(array $page) { // See getPagingViewer() for a description of this flag. $this->internalPaging = true; - if ($this->beforeID) { - $this->beforeID = $this->getPagingValue(last($page)); + if ($this->beforeID !== null) { + $page = array_reverse($page, $preserve_keys = true); + list($before, $after) = $this->getPageCursors($page); + $this->beforeID = $before; } else { - $this->afterID = $this->getPagingValue(last($page)); + list($before, $after) = $this->getPageCursors($page); + $this->afterID = $after; } } @@ -113,7 +125,9 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } final public function executeWithCursorPager(AphrontCursorPagerView $pager) { - $this->setLimit($pager->getPageSize() + 1); + $limit = $pager->getPageSize(); + + $this->setLimit($limit + 1); if ($pager->getAfterID()) { $this->setAfterID($pager->getAfterID()); @@ -122,17 +136,19 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } $results = $this->execute(); + $count = count($results); $sliced_results = $pager->sliceResults($results); - if ($sliced_results) { - if ($pager->getBeforeID() || (count($results) > $pager->getPageSize())) { - $pager->setNextPageID($this->getPagingValue(last($sliced_results))); + list($before, $after) = $this->getPageCursors($sliced_results); + + if ($pager->getBeforeID() || ($count > $limit)) { + $pager->setNextPageID($after); } if ($pager->getAfterID() || - ($pager->getBeforeID() && (count($results) > $pager->getPageSize()))) { - $pager->setPrevPageID($this->getPagingValue(head($sliced_results))); + ($pager->getBeforeID() && ($count > $limit))) { + $pager->setPrevPageID($before); } }