Make InlineCommentQueries more robust/consistent

Summary:
Ref T13513. Improve consistency and robustness of the "InlineComment" queries.

The only real change here is that these queries now implicitly add a clause for selecting inlines ("pathID IS NULL" or "changesetID IS NULL").

Test Plan: Browed, created, edited, and submitted inlines.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21227

src/applications/audit/storage/PhabricatorAuditInlineComment.php

@@ -65,7 +65,6 @@ final class PhabricatorAuditInlineComment
       ->withAuthorPHIDs(array($viewer->getPHID()))
       ->withCommitPHIDs(array($commit_phid))
       ->withHasTransaction(false)
-      ->withHasPath(true)
       ->withIsDeleted(false)
       ->needReplyToComments(true)
       ->execute();
@@ -85,7 +84,6 @@ final class PhabricatorAuditInlineComment
       ->setViewer($viewer)
       ->withCommitPHIDs(array($commit_phid))
       ->withHasTransaction(true)
-      ->withHasPath(true)
       ->execute();
 
     return self::buildProxies($inlines);

src/applications/differential/query/DifferentialDiffInlineCommentQuery.php

@@ -5,23 +5,40 @@ final class DifferentialDiffInlineCommentQuery
 
   private $revisionPHIDs;
 
+  protected function newApplicationTransactionCommentTemplate() {
+    return new DifferentialTransactionComment();
+  }
+
   public function withRevisionPHIDs(array $phids) {
     $this->revisionPHIDs = $phids;
     return $this;
   }
 
-  protected function getTemplate() {
+  public function withObjectPHIDs(array $phids) {
-    return new DifferentialTransactionComment();
+    return $this->withRevisionPHIDs($phids);
   }
 
-  protected function buildWhereClauseComponents(
+  protected function buildInlineCommentWhereClauseParts(
-    AphrontDatabaseConnection $conn_r) {
+    AphrontDatabaseConnection $conn) {
-    $where = parent::buildWhereClauseComponents($conn_r);
+    $where = array();
+    $alias = $this->getPrimaryTableAlias();
+
+    $where[] = qsprintf(
+      $conn,
+      'changesetID IS NOT NULL');
+
+    return $where;
+  }
+
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
+    $where = parent::buildWhereClauseParts($conn);
+    $alias = $this->getPrimaryTableAlias();
 
     if ($this->revisionPHIDs !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
-        'revisionPHID IN (%Ls)',
+        '%T.revisionPHID IN (%Ls)',
+        $alias,
         $this->revisionPHIDs);
     }
 

src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php

@@ -4,17 +4,19 @@ final class DiffusionDiffInlineCommentQuery
   extends PhabricatorDiffInlineCommentQuery {
 
   private $commitPHIDs;
-  private $hasPath;
   private $pathIDs;
 
+  protected function newApplicationTransactionCommentTemplate() {
+    return new PhabricatorAuditTransactionComment();
+  }
+
   public function withCommitPHIDs(array $phids) {
     $this->commitPHIDs = $phids;
     return $this;
   }
 
-  public function withHasPath($has_path) {
+  public function withObjectPHIDs(array $phids) {
-    $this->hasPath = $has_path;
+    return $this->withCommitPHIDs($phids);
-    return $this;
   }
 
   public function withPathIDs(array $path_ids) {
@@ -22,37 +24,36 @@ final class DiffusionDiffInlineCommentQuery
     return $this;
   }
 
-  protected function getTemplate() {
+  protected function buildInlineCommentWhereClauseParts(
-    return new PhabricatorAuditTransactionComment();
+    AphrontDatabaseConnection $conn) {
+    $where = array();
+    $alias = $this->getPrimaryTableAlias();
+
+    $where[] = qsprintf(
+      $conn,
+      '%T.pathID IS NOT NULL',
+      $alias);
+
+    return $where;
   }
 
-  protected function buildWhereClauseComponents(
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
-    AphrontDatabaseConnection $conn_r) {
+    $where = parent::buildWhereClauseParts($conn);
-    $where = parent::buildWhereClauseComponents($conn_r);
+    $alias = $this->getPrimaryTableAlias();
 
     if ($this->commitPHIDs !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
-        'xcomment.commitPHID IN (%Ls)',
+        '%T.commitPHID IN (%Ls)',
+        $alias,
         $this->commitPHIDs);
     }
 
-    if ($this->hasPath !== null) {
-      if ($this->hasPath) {
-        $where[] = qsprintf(
-          $conn_r,
-          'xcomment.pathID IS NOT NULL');
-      } else {
-        $where[] = qsprintf(
-          $conn_r,
-          'xcomment.pathID IS NULL');
-      }
-    }
-
     if ($this->pathIDs !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
-        'xcomment.pathID IN (%Ld)',
+        '%T.pathID IN (%Ld)',
+        $alias,
         $this->pathIDs);
     }
 

src/applications/transactions/query/PhabricatorApplicationTransactionCommentQuery.php

@@ -10,7 +10,7 @@ abstract class PhabricatorApplicationTransactionCommentQuery
   private $isDeleted;
   private $hasTransaction;
 
-  abstract protected function getTemplate();
+  abstract protected function newApplicationTransactionCommentTemplate();
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -42,64 +42,55 @@ abstract class PhabricatorApplicationTransactionCommentQuery
     return $this;
   }
 
+  public function newResultObject() {
+    return $this->newApplicationTransactionCommentTemplate();
+  }
+
   protected function loadPage() {
-    $table = $this->getTemplate();
+    return $this->loadStandardPage($this->newResultObject());
-    $conn_r = $table->establishConnection('r');
-
-    $data = queryfx_all(
-      $conn_r,
-      'SELECT * FROM %T xcomment %Q %Q %Q',
-      $table->getTableName(),
-      $this->buildWhereClause($conn_r),
-      $this->buildOrderClause($conn_r),
-      $this->buildLimitClause($conn_r));
-
-    return $table->loadAllFromArray($data);
   }
 
-  protected function buildWhereClause(AphrontDatabaseConnection $conn) {
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
-    return $this->formatWhereClause(
+    $where = parent::buildWhereClauseParts($conn);
-      $conn,
+    $alias = $this->getPrimaryTableAlias();
-      $this->buildWhereClauseComponents($conn));
-  }
-
-  protected function buildWhereClauseComponents(
-    AphrontDatabaseConnection $conn) {
-
-    $where = array();
 
     if ($this->ids !== null) {
       $where[] = qsprintf(
         $conn,
-        'xcomment.id IN (%Ld)',
+        '%T.id IN (%Ld)',
+        $alias,
         $this->ids);
     }
 
     if ($this->phids !== null) {
       $where[] = qsprintf(
         $conn,
-        'xcomment.phid IN (%Ls)',
+        '%T.phid IN (%Ls)',
+        $alias,
         $this->phids);
     }
 
     if ($this->authorPHIDs !== null) {
       $where[] = qsprintf(
         $conn,
-        'xcomment.authorPHID IN (%Ls)',
+        '%T.authorPHID IN (%Ls)',
+        $alias,
         $this->authorPHIDs);
     }
 
     if ($this->transactionPHIDs !== null) {
       $where[] = qsprintf(
         $conn,
-        'xcomment.transactionPHID IN (%Ls)',
+        '%T.transactionPHID IN (%Ls)',
+        $alias,
         $this->transactionPHIDs);
     }
 
     if ($this->isDeleted !== null) {
       $where[] = qsprintf(
         $conn,
-        'xcomment.isDeleted = %d',
+        '%T.isDeleted = %d',
+        $alias,
         (int)$this->isDeleted);
     }
 
@@ -107,21 +98,26 @@ abstract class PhabricatorApplicationTransactionCommentQuery
       if ($this->hasTransaction) {
         $where[] = qsprintf(
           $conn,
-          'xcomment.transactionPHID IS NOT NULL');
+          '%T.transactionPHID IS NOT NULL',
+          $alias);
       } else {
         $where[] = qsprintf(
           $conn,
-          'xcomment.transactionPHID IS NULL');
+          '%T.transactionPHID IS NULL',
+          $alias);
       }
     }
 
     return $where;
   }
 
+  protected function getPrimaryTableAlias() {
+    return 'xcomment';
+  }
+
   public function getQueryApplicationClass() {
     // TODO: Figure out the app via the template?
     return null;
   }
 
-
 }

src/applications/transactions/query/PhabricatorApplicationTransactionTemplatedCommentQuery.php

@@ -11,8 +11,8 @@ final class PhabricatorApplicationTransactionTemplatedCommentQuery
     return $this;
   }
 
-  protected function getTemplate() {
+  protected function newApplicationTransactionCommentTemplate() {
-    return $this->template;
+    return id(clone $this->template);
   }
 
 }

src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php

@@ -6,6 +6,10 @@ abstract class PhabricatorDiffInlineCommentQuery
   private $fixedStates;
   private $needReplyToComments;
 
+  abstract protected function buildInlineCommentWhereClauseParts(
+    AphrontDatabaseConnection $conn);
+  abstract public function withObjectPHIDs(array $phids);
+
   public function withFixedStates(array $states) {
     $this->fixedStates = $states;
     return $this;
@@ -16,14 +20,19 @@ abstract class PhabricatorDiffInlineCommentQuery
     return $this;
   }
 
-  protected function buildWhereClauseComponents(
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
-    AphrontDatabaseConnection $conn_r) {
+    $where = parent::buildWhereClauseParts($conn);
-    $where = parent::buildWhereClauseComponents($conn_r);
+    $alias = $this->getPrimaryTableAlias();
+
+    foreach ($this->buildInlineCommentWhereClauseParts($conn) as $part) {
+      $where[] = $part;
+    }
 
     if ($this->fixedStates !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
-        'fixedState IN (%Ls)',
+        '%T.fixedState IN (%Ls)',
+        $alias,
         $this->fixedStates);
     }