From 642113708a83429f0928e04fc15875e54b148210 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 20 May 2019 14:02:38 -0700 Subject: [PATCH] Build a rough transaction-level view of Feed Summary: Ref T13294. An install is interested in a way to easily answer audit-focused questions like "what edits were made to any Herald rule in Q1 2019?". We can answer this kind of question with a more granular version of feed that focuses on being exhaustive rather than being human-readable. This starts a rough version of it and deals with the two major tricky pieces: transactions are in a lot of different tables; and paging across them is not trivial. To solve "lots of tables", we just query every table. There's a little bit of sleight-of-hand to get this working, but nothing too awful. To solve "paging is hard", we order by "". The "phid" part of this order doesn't have much meaning, but it lets us put every transaction in a single, stable, global order and identify a place in that ordering given only one transaction PHID. Test Plan: {F6463076} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13294 Differential Revision: https://secure.phabricator.com/D20531 --- src/__phutil_library_map__.php | 6 + .../PhabricatorFeedApplication.php | 4 + .../controller/PhabricatorFeedController.php | 24 +-- .../PhabricatorFeedListController.php | 21 ++- ...abricatorFeedTransactionListController.php | 16 ++ .../query/PhabricatorFeedTransactionQuery.php | 178 ++++++++++++++++++ ...PhabricatorFeedTransactionSearchEngine.php | 113 +++++++++++ ...PhabricatorApplicationTransactionQuery.php | 73 +++++++ .../PhabricatorApplicationTransaction.php | 6 + ...PhabricatorCursorPagedPolicyAwareQuery.php | 9 +- 10 files changed, 416 insertions(+), 34 deletions(-) create mode 100644 src/applications/feed/controller/PhabricatorFeedTransactionListController.php create mode 100644 src/applications/feed/query/PhabricatorFeedTransactionQuery.php create mode 100644 src/applications/feed/query/PhabricatorFeedTransactionSearchEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6b5f07eb8c..dc331be164 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3265,6 +3265,9 @@ phutil_register_library_map(array( 'PhabricatorFeedStoryNotification' => 'applications/notification/storage/PhabricatorFeedStoryNotification.php', 'PhabricatorFeedStoryPublisher' => 'applications/feed/PhabricatorFeedStoryPublisher.php', 'PhabricatorFeedStoryReference' => 'applications/feed/storage/PhabricatorFeedStoryReference.php', + 'PhabricatorFeedTransactionListController' => 'applications/feed/controller/PhabricatorFeedTransactionListController.php', + 'PhabricatorFeedTransactionQuery' => 'applications/feed/query/PhabricatorFeedTransactionQuery.php', + 'PhabricatorFeedTransactionSearchEngine' => 'applications/feed/query/PhabricatorFeedTransactionSearchEngine.php', 'PhabricatorFerretEngine' => 'applications/search/ferret/PhabricatorFerretEngine.php', 'PhabricatorFerretEngineTestCase' => 'applications/search/ferret/__tests__/PhabricatorFerretEngineTestCase.php', 'PhabricatorFerretFulltextEngineExtension' => 'applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php', @@ -9330,6 +9333,9 @@ phutil_register_library_map(array( 'PhabricatorFeedStoryNotification' => 'PhabricatorFeedDAO', 'PhabricatorFeedStoryPublisher' => 'Phobject', 'PhabricatorFeedStoryReference' => 'PhabricatorFeedDAO', + 'PhabricatorFeedTransactionListController' => 'PhabricatorFeedController', + 'PhabricatorFeedTransactionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorFeedTransactionSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorFerretEngine' => 'Phobject', 'PhabricatorFerretEngineTestCase' => 'PhabricatorTestCase', 'PhabricatorFerretFulltextEngineExtension' => 'PhabricatorFulltextEngineExtension', diff --git a/src/applications/feed/application/PhabricatorFeedApplication.php b/src/applications/feed/application/PhabricatorFeedApplication.php index 287cf2d387..7d267394e9 100644 --- a/src/applications/feed/application/PhabricatorFeedApplication.php +++ b/src/applications/feed/application/PhabricatorFeedApplication.php @@ -31,6 +31,10 @@ final class PhabricatorFeedApplication extends PhabricatorApplication { '/feed/' => array( '(?P\d+)/' => 'PhabricatorFeedDetailController', '(?:query/(?P[^/]+)/)?' => 'PhabricatorFeedListController', + 'transactions/' => array( + $this->getQueryRoutePattern() + => 'PhabricatorFeedTransactionListController', + ), ), ); } diff --git a/src/applications/feed/controller/PhabricatorFeedController.php b/src/applications/feed/controller/PhabricatorFeedController.php index 6e4d353518..cb7059a554 100644 --- a/src/applications/feed/controller/PhabricatorFeedController.php +++ b/src/applications/feed/controller/PhabricatorFeedController.php @@ -1,24 +1,4 @@ getRequest()->getUser(); - - $nav = new AphrontSideNavFilterView(); - $nav->setBaseURI(new PhutilURI($this->getApplicationURI())); - - id(new PhabricatorFeedSearchEngine()) - ->setViewer($user) - ->addNavigationItems($nav->getMenu()); - - $nav->selectFilter(null); - - return $nav; - } - - public function buildApplicationMenu() { - return $this->buildSideNavView()->getMenu(); - } - -} +abstract class PhabricatorFeedController + extends PhabricatorController {} diff --git a/src/applications/feed/controller/PhabricatorFeedListController.php b/src/applications/feed/controller/PhabricatorFeedListController.php index 8451592362..2acc06689e 100644 --- a/src/applications/feed/controller/PhabricatorFeedListController.php +++ b/src/applications/feed/controller/PhabricatorFeedListController.php @@ -1,20 +1,27 @@ getURIData('queryKey'); + $navigation = array(); - $controller = id(new PhabricatorApplicationSearchController()) - ->setQueryKey($querykey) - ->setSearchEngine(new PhabricatorFeedSearchEngine()) - ->setNavigation($this->buildSideNavView()); + $navigation[] = id(new PHUIListItemView()) + ->setType(PHUIListItemView::TYPE_LABEL) + ->setName(pht('Transactions')); - return $this->delegateToController($controller); + $navigation[] = id(new PHUIListItemView()) + ->setName(pht('Transaction Logs')) + ->setHref($this->getApplicationURI('transactions/')); + + return id(new PhabricatorFeedSearchEngine()) + ->setController($this) + ->setNavigationItems($navigation) + ->buildResponse(); } } diff --git a/src/applications/feed/controller/PhabricatorFeedTransactionListController.php b/src/applications/feed/controller/PhabricatorFeedTransactionListController.php new file mode 100644 index 0000000000..16e5b0673b --- /dev/null +++ b/src/applications/feed/controller/PhabricatorFeedTransactionListController.php @@ -0,0 +1,16 @@ +setController($this) + ->buildResponse(); + } + +} diff --git a/src/applications/feed/query/PhabricatorFeedTransactionQuery.php b/src/applications/feed/query/PhabricatorFeedTransactionQuery.php new file mode 100644 index 0000000000..00da566532 --- /dev/null +++ b/src/applications/feed/query/PhabricatorFeedTransactionQuery.php @@ -0,0 +1,178 @@ +phids = $phids; + return $this; + } + + public function withDateCreatedBetween($min, $max) { + $this->createdMin = $min; + $this->createdMax = $max; + return $this; + } + + protected function loadPage() { + $queries = $this->newTransactionQueries(); + + $xactions = array(); + + if ($this->shouldLimitResults()) { + $limit = $this->getRawResultLimit(); + if (!$limit) { + $limit = null; + } + } else { + $limit = null; + } + + // We're doing a bit of manual work to get paging working, because this + // query aggregates the results of a large number of subqueries. + + // Overall, we're ordering transactions by "". Ordering + // by PHID is not very meaningful, but we don't need the ordering to be + // especially meaningful, just consistent. Using PHIDs is easy and does + // everything we need it to technically. + + // To actually configure paging, if we have an external cursor, we load + // the internal cursor first. Then we pass it to each subquery and the + // subqueries pretend they just loaded a page where it was the last object. + // This configures their queries properly and we can aggregate a cohesive + // set of results by combining all the queries. + + $cursor = $this->getExternalCursorString(); + if ($cursor !== null) { + $cursor_object = $this->newInternalCursorFromExternalCursor($cursor); + } else { + $cursor_object = null; + } + + $is_reversed = $this->getIsQueryOrderReversed(); + + $created_min = $this->createdMin; + $created_max = $this->createdMax; + + $xaction_phids = $this->phids; + + foreach ($queries as $query) { + $query->withDateCreatedBetween($created_min, $created_max); + + if ($xaction_phids !== null) { + $query->withPHIDs($xaction_phids); + } + + if ($limit !== null) { + $query->setLimit($limit); + } + + if ($cursor_object !== null) { + $query + ->setAggregatePagingCursor($cursor_object) + ->setIsQueryOrderReversed($is_reversed); + } + + $query->setOrder('global'); + + $query_xactions = $query->execute(); + foreach ($query_xactions as $query_xaction) { + $xactions[] = $query_xaction; + } + + $xactions = msortv($xactions, 'newGlobalSortVector'); + if ($is_reversed) { + $xactions = array_reverse($xactions); + } + + if ($limit !== null) { + $xactions = array_slice($xactions, 0, $limit); + + // If we've found enough transactions to fill up the entire requested + // page size, we can narrow the search window: transactions after the + // last transaction we've found so far can't possibly be part of the + // result set. + + if (count($xactions) === $limit) { + $last_date = last($xactions)->getDateCreated(); + if ($is_reversed) { + if ($created_max === null) { + $created_max = $last_date; + } else { + $created_max = min($created_max, $last_date); + } + } else { + if ($created_min === null) { + $created_min = $last_date; + } else { + $created_min = max($created_min, $last_date); + } + } + } + } + } + + return $xactions; + } + + public function getQueryApplicationClass() { + return 'PhabricatorFeedApplication'; + } + + private function newTransactionQueries() { + $viewer = $this->getViewer(); + + $queries = id(new PhutilClassMapQuery()) + ->setAncestorClass('PhabricatorApplicationTransactionQuery') + ->execute(); + + $type_map = array(); + + // If we're querying for specific transaction PHIDs, we only need to + // consider queries which may load transactions with subtypes present + // in the list. + + // For example, if we're loading Maniphest Task transaction PHIDs, we know + // we only have to look at Maniphest Task transactions, since other types + // of objects will never have the right transaction PHIDs. + + $xaction_phids = $this->phids; + if ($xaction_phids) { + foreach ($xaction_phids as $xaction_phid) { + $type_map[phid_get_subtype($xaction_phid)] = true; + } + } + + $results = array(); + foreach ($queries as $query) { + if ($type_map) { + $type = $query->getTemplateApplicationTransaction() + ->getApplicationTransactionType(); + if (!isset($type_map[$type])) { + continue; + } + } + + $results[] = id(clone $query) + ->setViewer($viewer) + ->setParentQuery($this); + } + + return $results; + } + + protected function newExternalCursorStringForResult($object) { + return (string)$object->getPHID(); + } + + protected function applyExternalCursorConstraintsToQuery( + PhabricatorCursorPagedPolicyAwareQuery $subquery, + $cursor) { + $subquery->withPHIDs(array($cursor)); + } + +} diff --git a/src/applications/feed/query/PhabricatorFeedTransactionSearchEngine.php b/src/applications/feed/query/PhabricatorFeedTransactionSearchEngine.php new file mode 100644 index 0000000000..bc0d27c70c --- /dev/null +++ b/src/applications/feed/query/PhabricatorFeedTransactionSearchEngine.php @@ -0,0 +1,113 @@ +newQuery(); + + return $query; + } + + protected function getURI($path) { + return '/feed/transactions/'.$path; + } + + protected function getBuiltinQueryNames() { + $names = array( + 'all' => pht('All Transactions'), + ); + + return $names; + } + + public function buildSavedQueryFromBuiltin($query_key) { + $query = $this->newSavedQuery() + ->setQueryKey($query_key); + + switch ($query_key) { + case 'all': + return $query; + } + + return parent::buildSavedQueryFromBuiltin($query_key); + } + + protected function renderResultList( + array $objects, + PhabricatorSavedQuery $query, + array $handles) { + assert_instances_of($objects, 'PhabricatorApplicationTransaction'); + + $viewer = $this->requireViewer(); + + $handle_phids = array(); + foreach ($objects as $object) { + $author_phid = $object->getAuthorPHID(); + if ($author_phid !== null) { + $handle_phids[] = $author_phid; + } + $object_phid = $object->getObjectPHID(); + if ($object_phid !== null) { + $handle_phids[] = $object_phid; + } + } + + $handles = $viewer->loadHandles($handle_phids); + + $rows = array(); + foreach ($objects as $object) { + $author_phid = $object->getAuthorPHID(); + $object_phid = $object->getObjectPHID(); + + try { + $title = $object->getTitle(); + } catch (Exception $ex) { + $title = null; + } + + $rows[] = array( + $handles[$author_phid]->renderLink(), + $handles[$object_phid]->renderLink(), + AphrontTableView::renderSingleDisplayLine($title), + phabricator_datetime($object->getDateCreated(), $viewer), + ); + } + + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + pht('Actor'), + pht('Object'), + pht('Transaction'), + pht('Date'), + )) + ->setColumnClasses( + array( + null, + null, + 'wide', + 'right', + )); + + return id(new PhabricatorApplicationSearchResultView()) + ->setTable($table); + } + +} diff --git a/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php b/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php index 1db622163d..195de601f0 100644 --- a/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php +++ b/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php @@ -9,6 +9,9 @@ abstract class PhabricatorApplicationTransactionQuery private $authorPHIDs; private $transactionTypes; private $withComments; + private $createdMin; + private $createdMax; + private $aggregatePagingCursor; private $needComments = true; private $needHandles = true; @@ -66,6 +69,12 @@ abstract class PhabricatorApplicationTransactionQuery return $this; } + public function withDateCreatedBetween($min, $max) { + $this->createdMin = $min; + $this->createdMax = $max; + return $this; + } + public function needComments($need) { $this->needComments = $need; return $this; @@ -76,6 +85,22 @@ abstract class PhabricatorApplicationTransactionQuery return $this; } + public function setAggregatePagingCursor(PhabricatorQueryCursor $cursor) { + $this->aggregatePagingCursor = $cursor; + return $this; + } + + public function getAggregatePagingCursor() { + return $this->aggregatePagingCursor; + } + + protected function willExecute() { + $cursor_object = $this->getAggregatePagingCursor(); + if ($cursor_object) { + $this->nextPage(array($cursor_object->getObject())); + } + } + protected function loadPage() { $table = $this->getTemplateApplicationTransaction(); @@ -206,6 +231,20 @@ abstract class PhabricatorApplicationTransactionQuery } } + if ($this->createdMin !== null) { + $where[] = qsprintf( + $conn, + 'x.dateCreated >= %d', + $this->createdMin); + } + + if ($this->createdMax !== null) { + $where[] = qsprintf( + $conn, + 'x.dateCreated <= %d', + $this->createdMax); + } + return $where; } @@ -262,4 +301,38 @@ abstract class PhabricatorApplicationTransactionQuery return 'x'; } + protected function newPagingMapFromPartialObject($object) { + return parent::newPagingMapFromPartialObject($object) + array( + 'created' => $object->getDateCreated(), + 'phid' => $object->getPHID(), + ); + } + + public function getBuiltinOrders() { + return parent::getBuiltinOrders() + array( + 'global' => array( + 'vector' => array('created', 'phid'), + 'name' => pht('Global'), + ), + ); + } + + public function getOrderableColumns() { + return parent::getOrderableColumns() + array( + 'created' => array( + 'table' => 'x', + 'column' => 'dateCreated', + 'type' => 'int', + ), + 'phid' => array( + 'table' => 'x', + 'column' => 'phid', + 'type' => 'string', + 'reverse' => true, + 'unique' => true, + ), + ); + } + + } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 1250b7cb16..6fa9446911 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -1711,6 +1711,12 @@ abstract class PhabricatorApplicationTransaction return array($done, $undone); } + public function newGlobalSortVector() { + return id(new PhutilSortVector()) + ->addInt(-$this->getDateCreated()) + ->addString($this->getPHID()); + } + /* -( PhabricatorPolicyInterface Implementation )-------------------------- */ diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index f5586fd90f..978de0fcf8 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -104,7 +104,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } // Now that we made sure the viewer can actually see the object the - // external cursor identifies, return the internal cursor the query + // external cursor identifies, return the internal cursor the query // generated as a side effect while loading the object. return $query->getInternalCursorObject(); } @@ -134,7 +134,6 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery ); } - final private function getExternalCursorStringForResult($object) { $cursor = $this->newExternalCursorStringForResult($object); @@ -150,7 +149,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery return $cursor; } - final private function getExternalCursorString() { + final protected function getExternalCursorString() { return $this->externalCursorString; } @@ -159,11 +158,11 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery return $this; } - final private function getIsQueryOrderReversed() { + final protected function getIsQueryOrderReversed() { return $this->isQueryOrderReversed; } - final private function setIsQueryOrderReversed($is_reversed) { + final protected function setIsQueryOrderReversed($is_reversed) { $this->isQueryOrderReversed = $is_reversed; return $this; } -- 2.51.2