From 4d86d51125fe6815cdb9172267e9c8298218287c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Mar 2015 12:15:54 -0700 Subject: [PATCH] Prepare TransactionCommentQuery for extension Summary: Ref T2009. Ref T1460. The way Diffusion and Differential load inlines is horrible garbage right now: - Differential does an ad-hoc query to get the PHIDs, then does a real load to policy check. - Diffusion completely fakes things. In practice this is not a policy violation, but it's dangerous. Make TransactionCommentQuery extensible so we can subclass it and get the query building correctly in the right Query layer. Specifically, the Diffusion and Differential subclasses of this Query will add appropriate `withX()` methods to let us express the query in SQL. Test Plan: Loaded, previewed, edited, and submitted inlines in Differential and Diffusion Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T2009, T1460 Differential Revision: https://secure.phabricator.com/D12026 --- src/__phutil_library_map__.php | 2 + .../query/DifferentialTransactionQuery.php | 8 +-- ...ionTransactionCommentHistoryController.php | 2 +- ...atorApplicationTransactionCommentQuery.php | 59 +++++++++++++++---- ...PhabricatorApplicationTransactionQuery.php | 11 ++-- ...cationTransactionTemplatedCommentQuery.php | 18 ++++++ .../PhabricatorInlineCommentController.php | 11 ++-- 7 files changed, 81 insertions(+), 30 deletions(-) create mode 100644 src/applications/transactions/query/PhabricatorApplicationTransactionTemplatedCommentQuery.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8a2a20bdc6..05b8ad18e9 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1298,6 +1298,7 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionResponse' => 'applications/transactions/response/PhabricatorApplicationTransactionResponse.php', 'PhabricatorApplicationTransactionShowOlderController' => 'applications/transactions/controller/PhabricatorApplicationTransactionShowOlderController.php', 'PhabricatorApplicationTransactionStructureException' => 'applications/transactions/exception/PhabricatorApplicationTransactionStructureException.php', + 'PhabricatorApplicationTransactionTemplatedCommentQuery' => 'applications/transactions/query/PhabricatorApplicationTransactionTemplatedCommentQuery.php', 'PhabricatorApplicationTransactionTextDiffDetailView' => 'applications/transactions/view/PhabricatorApplicationTransactionTextDiffDetailView.php', 'PhabricatorApplicationTransactionTransactionPHIDType' => 'applications/transactions/phid/PhabricatorApplicationTransactionTransactionPHIDType.php', 'PhabricatorApplicationTransactionValidationError' => 'applications/transactions/error/PhabricatorApplicationTransactionValidationError.php', @@ -4541,6 +4542,7 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionResponse' => 'AphrontProxyResponse', 'PhabricatorApplicationTransactionShowOlderController' => 'PhabricatorApplicationTransactionController', 'PhabricatorApplicationTransactionStructureException' => 'Exception', + 'PhabricatorApplicationTransactionTemplatedCommentQuery' => 'PhabricatorApplicationTransactionCommentQuery', 'PhabricatorApplicationTransactionTextDiffDetailView' => 'AphrontView', 'PhabricatorApplicationTransactionTransactionPHIDType' => 'PhabricatorPHIDType', 'PhabricatorApplicationTransactionValidationError' => 'Phobject', diff --git a/src/applications/differential/query/DifferentialTransactionQuery.php b/src/applications/differential/query/DifferentialTransactionQuery.php index 64ef271655..72e247aa36 100644 --- a/src/applications/differential/query/DifferentialTransactionQuery.php +++ b/src/applications/differential/query/DifferentialTransactionQuery.php @@ -11,11 +11,7 @@ final class DifferentialTransactionQuery PhabricatorUser $viewer, DifferentialRevision $revision) { - // TODO: This probably needs to move somewhere more central as we move - // away from DifferentialInlineCommentQuery, but - // PhabricatorApplicationTransactionCommentQuery is currently `final` and - // I'm not yet decided on how to approach that. For now, just get the PHIDs - // and then execute a PHID-based query through the standard stack. + // TODO: Subclass ApplicationTransactionCommentQuery to do this for real. $table = new DifferentialTransactionComment(); $conn_r = $table->establishConnection('r'); @@ -36,7 +32,7 @@ final class DifferentialTransactionQuery return array(); } - $comments = id(new PhabricatorApplicationTransactionCommentQuery()) + $comments = id(new PhabricatorApplicationTransactionTemplatedCommentQuery()) ->setTemplate(new DifferentialTransactionComment()) ->setViewer($viewer) ->withPHIDs($phids) diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentHistoryController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentHistoryController.php index d9bd061947..dcff0d76e2 100644 --- a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentHistoryController.php +++ b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentHistoryController.php @@ -36,7 +36,7 @@ final class PhabricatorApplicationTransactionCommentHistoryController return new Aphront400Response(); } - $comments = id(new PhabricatorApplicationTransactionCommentQuery()) + $comments = id(new PhabricatorApplicationTransactionTemplatedCommentQuery()) ->setViewer($user) ->setTemplate($xaction->getApplicationTransactionCommentObject()) ->withTransactionPHIDs(array($xaction->getPHID())) diff --git a/src/applications/transactions/query/PhabricatorApplicationTransactionCommentQuery.php b/src/applications/transactions/query/PhabricatorApplicationTransactionCommentQuery.php index 038fadb9cb..df188829f0 100644 --- a/src/applications/transactions/query/PhabricatorApplicationTransactionCommentQuery.php +++ b/src/applications/transactions/query/PhabricatorApplicationTransactionCommentQuery.php @@ -1,16 +1,18 @@ template = $template; + abstract protected function getTemplate(); + + public function withIDs(array $ids) { + $this->ids = $ids; return $this; } @@ -24,13 +26,23 @@ final class PhabricatorApplicationTransactionCommentQuery return $this; } + public function withAuthorPHIDs(array $phids) { + $this->authorPHIDs = $phids; + return $this; + } + + public function withDeleted($deleted) { + $this->isDeleted = $deleted; + return $this; + } + protected function loadPage() { - $table = $this->template; + $table = $this->getTemplate(); $conn_r = $table->establishConnection('r'); $data = queryfx_all( $conn_r, - 'SELECT * FROM %T xc %Q %Q %Q', + 'SELECT * FROM %T xcomment %Q %Q %Q', $table->getTableName(), $this->buildWhereClause($conn_r), $this->buildOrderClause($conn_r), @@ -39,23 +51,44 @@ final class PhabricatorApplicationTransactionCommentQuery return $table->loadAllFromArray($data); } - private function buildWhereClause(AphrontDatabaseConnection $conn_r) { + protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { $where = array(); - if ($this->phids) { + if ($this->ids !== null) { $where[] = qsprintf( $conn_r, - 'phid IN (%Ls)', + 'xcomment.id IN (%Ld)', + $this->ids); + } + + if ($this->phids !== null) { + $where[] = qsprintf( + $conn_r, + 'xcomment.phid IN (%Ls)', $this->phids); } - if ($this->transactionPHIDs) { + if ($this->authorPHIDs !== null) { $where[] = qsprintf( $conn_r, - 'transactionPHID IN (%Ls)', + 'xcomment.authorPHID IN (%Ls)', + $this->authorPHIDs); + } + + if ($this->transactionPHIDs !== null) { + $where[] = qsprintf( + $conn_r, + 'xcomment.transactionPHID IN (%Ls)', $this->transactionPHIDs); } + if ($this->isDeleted !== null) { + $where[] = qsprintf( + $conn_r, + 'xcomment.isDeleted = %d', + (int)$this->isDeleted); + } + return $this->formatWhereClause($where); } diff --git a/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php b/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php index 3cec8821f6..45dcbfd8cf 100644 --- a/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php +++ b/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php @@ -80,11 +80,12 @@ abstract class PhabricatorApplicationTransactionQuery $comments = array(); if ($comment_phids) { - $comments = id(new PhabricatorApplicationTransactionCommentQuery()) - ->setTemplate($table->getApplicationTransactionCommentObject()) - ->setViewer($this->getViewer()) - ->withPHIDs($comment_phids) - ->execute(); + $comments = + id(new PhabricatorApplicationTransactionTemplatedCommentQuery()) + ->setTemplate($table->getApplicationTransactionCommentObject()) + ->setViewer($this->getViewer()) + ->withPHIDs($comment_phids) + ->execute(); $comments = mpull($comments, null, 'getPHID'); } diff --git a/src/applications/transactions/query/PhabricatorApplicationTransactionTemplatedCommentQuery.php b/src/applications/transactions/query/PhabricatorApplicationTransactionTemplatedCommentQuery.php new file mode 100644 index 0000000000..41758a4f26 --- /dev/null +++ b/src/applications/transactions/query/PhabricatorApplicationTransactionTemplatedCommentQuery.php @@ -0,0 +1,18 @@ +template = $template; + return $this; + } + + protected function getTemplate() { + return $this->template; + } + +} diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index c400b965d2..960957794a 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -330,11 +330,12 @@ abstract class PhabricatorInlineCommentController } if ($reply_phids) { - $reply_comments = id(new PhabricatorApplicationTransactionCommentQuery()) - ->setTemplate($template) - ->setViewer($viewer) - ->withPHIDs($reply_phids) - ->execute(); + $reply_comments = + id(new PhabricatorApplicationTransactionTemplatedCommentQuery()) + ->setTemplate($template) + ->setViewer($viewer) + ->withPHIDs($reply_phids) + ->execute(); $reply_comments = mpull($reply_comments, null, 'getPHID'); } else { $reply_comments = array();