From 51c4b199d027de99f844c585cc87a2b78dab5683 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 23 Oct 2012 12:01:11 -0700 Subject: [PATCH] Allow policy-aware queries to prefilter results Summary: Provides a simple way for policy-aware queries to pre-filter results without needing to maintain separate cursors, and fixes a bunch of filter-related edge cases. - For reverse-paged cursor queries, we previously reversed each individual set of results. If the final result set is built out of multiple pages, it's in the wrong order overall, with each page in the correct order in sequence. Instead, reverse everything at the end. This also simplifies construction of queries. - `AphrontCursorPagerView` would always render a "<< First" link when paging backward, even if we were on the first page of results. - Add a filtering hook to let queries perform in-application pre-policy filtering as simply as possible (i.e., without maintaing their own cursors over the result sets). Test Plan: Made feed randomly prefilter half the results, and paged forward and backward. Observed correct result ordering, pagination, and next/previous links. Reviewers: btrahan, vrana Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D3787 --- .../chatlog/PhabricatorChatLogQuery.php | 2 +- .../feed/PhabricatorFeedQuery.php | 11 +++++-- .../paste/query/PhabricatorPasteQuery.php | 2 +- ...PhabricatorCursorPagedPolicyAwareQuery.php | 11 +++---- .../policy/PhabricatorPolicyAwareQuery.php | 33 ++++++++++++++++++- src/view/control/AphrontCursorPagerView.php | 4 ++- 6 files changed, 50 insertions(+), 13 deletions(-) diff --git a/src/applications/chatlog/PhabricatorChatLogQuery.php b/src/applications/chatlog/PhabricatorChatLogQuery.php index 02b1e1e56c..06e1f1ee5b 100644 --- a/src/applications/chatlog/PhabricatorChatLogQuery.php +++ b/src/applications/chatlog/PhabricatorChatLogQuery.php @@ -46,7 +46,7 @@ final class PhabricatorChatLogQuery $logs = $table->loadAllFromArray($data); - return $this->processResults($logs); + return $logs; } private function buildWhereClause($conn_r) { diff --git a/src/applications/feed/PhabricatorFeedQuery.php b/src/applications/feed/PhabricatorFeedQuery.php index 2c954c8b49..30331ecafd 100644 --- a/src/applications/feed/PhabricatorFeedQuery.php +++ b/src/applications/feed/PhabricatorFeedQuery.php @@ -41,9 +41,11 @@ final class PhabricatorFeedQuery $this->buildOrderClause($conn), $this->buildLimitClause($conn)); - $results = PhabricatorFeedStory::loadAllFromRows($data); + return $data; + } - return $this->processResults($results); + protected function willFilterPage(array $data) { + return PhabricatorFeedStory::loadAllFromRows($data); } private function buildJoinClause(AphrontDatabaseConnection $conn_r) { @@ -88,7 +90,10 @@ final class PhabricatorFeedQuery } protected function getPagingValue($item) { - return $item->getChronologicalKey(); + if ($item instanceof PhabricatorFeedStory) { + return $item->getChronologicalKey(); + } + return $item['chronologicalKey']; } } diff --git a/src/applications/paste/query/PhabricatorPasteQuery.php b/src/applications/paste/query/PhabricatorPasteQuery.php index fdbb14251a..0666e60a08 100644 --- a/src/applications/paste/query/PhabricatorPasteQuery.php +++ b/src/applications/paste/query/PhabricatorPasteQuery.php @@ -81,7 +81,7 @@ final class PhabricatorPasteQuery } } - return $this->processResults($pastes); + return $pastes; } protected function buildWhereClause($conn_r) { diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 9c070455bb..5ec7fd408e 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -40,7 +40,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery protected function nextPage(array $page) { if ($this->beforeID) { - $this->beforeID = $this->getPagingValue(head($page)); + $this->beforeID = $this->getPagingValue(last($page)); } else { $this->afterID = $this->getPagingValue(last($page)); } @@ -102,14 +102,13 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } } - final protected function processResults(array $results) { + final protected function didLoadResults(array $results) { if ($this->beforeID) { $results = array_reverse($results, $preserve_keys = true); } return $results; } - final public function executeWithCursorPager(AphrontCursorPagerView $pager) { $this->setLimit($pager->getPageSize() + 1); @@ -123,12 +122,12 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $sliced_results = $pager->sliceResults($results); - if ($this->beforeID || (count($results) > $pager->getPageSize())) { + if ($pager->getBeforeID() || (count($results) > $pager->getPageSize())) { $pager->setNextPageID($this->getPagingValue(last($sliced_results))); } - if ($this->afterID || - ($this->beforeID && (count($results) > $pager->getPageSize()))) { + if ($pager->getAfterID() || + ($pager->getBeforeID() && (count($results) > $pager->getPageSize()))) { $pager->setPrevPageID($this->getPagingValue(head($sliced_results))); } diff --git a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php index fb204dead1..b33901aa2f 100644 --- a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php @@ -188,7 +188,8 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { $page = $this->loadPage(); - $visible = $filter->apply($page); + $visible = $this->willFilterPage($page); + $visible = $filter->apply($visible); foreach ($visible as $key => $result) { ++$count; @@ -222,6 +223,8 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { $this->nextPage($page); } while (true); + $results = $this->didLoadResults($results); + return $results; } @@ -275,4 +278,32 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { */ abstract protected function nextPage(array $page); + + /** + * Hook for applying a page filter prior to the privacy filter. This allows + * you to drop some items from the result set without creating problems with + * pagination or cursor updates. + * + * @param list Results from `loadPage()`. + * @return list Objects for policy filtering. + * @task policyimpl + */ + protected function willFilterPage(array $page) { + return $page; + } + + + /** + * Hook for applying final adjustments before results are returned. This is + * used by @{class:PhabricatorCursorPagedPolicyAwareQuery} to reverse results + * that are queried during reverse paging. + * + * @param list Query results. + * @return list Final results. + * @task policyimpl + */ + protected function didLoadResults(array $results) { + return $results; + } + } diff --git a/src/view/control/AphrontCursorPagerView.php b/src/view/control/AphrontCursorPagerView.php index 04c5798523..36aecacac6 100644 --- a/src/view/control/AphrontCursorPagerView.php +++ b/src/view/control/AphrontCursorPagerView.php @@ -25,6 +25,7 @@ final class AphrontCursorPagerView extends AphrontView { private $nextPageID; private $prevPageID; + private $moreResults; private $uri; @@ -89,6 +90,7 @@ final class AphrontCursorPagerView extends AphrontView { if (count($results) > $this->getPageSize()) { $offset = ($this->beforeID ? count($results) - $this->getPageSize() : 0); $results = array_slice($results, $offset, $this->getPageSize(), true); + $this->moreResults = true; } return $results; } @@ -101,7 +103,7 @@ final class AphrontCursorPagerView extends AphrontView { $links = array(); - if ($this->beforeID || $this->afterID) { + if ($this->afterID || ($this->beforeID && $this->moreResults)) { $links[] = phutil_render_tag( 'a', array(