From bdb3adeee485d00a2f8779a1029111cd56d58203 Mon Sep 17 00:00:00 2001
From: Bob Trahan <btrahan@phacility.com>
Date: Fri, 30 Jan 2015 11:51:16 -0800
Subject: [PATCH] Policy - clean up the deprecated diffusion.getcommits

Summary: Ref T7094. Could just delete this end point too I guess? Needed to add "withCommitPHIDs" to the differentialrevisionquery to get this done.

Test Plan: used diffusion.getcommits from conduit console and got a sensible result for a query for two commits, one with a diff and one without.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7094

Differential Revision: https://secure.phabricator.com/D11581
---
 .../query/DifferentialRevisionQuery.php       | 29 ++++++++++++
 .../DiffusionGetCommitsConduitAPIMethod.php   | 46 +++++++++----------
 2 files changed, 51 insertions(+), 24 deletions(-)

diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php
index ef9712b644..ef3bacd028 100644
--- a/src/applications/differential/query/DifferentialRevisionQuery.php
+++ b/src/applications/differential/query/DifferentialRevisionQuery.php
@@ -32,6 +32,7 @@ final class DifferentialRevisionQuery
   private $reviewers = array();
   private $revIDs = array();
   private $commitHashes = array();
+  private $commitPHIDs = array();
   private $phids = array();
   private $responsibles = array();
   private $branches = array();
@@ -153,6 +154,20 @@ final class DifferentialRevisionQuery
     return $this;
   }
 
+  /**
+   * Filter results to revisions that have one of the provided PHIDs as
+   * commits. Calling this function will clear anything set by previous calls
+   * to @{method:withCommitPHIDs}.
+   *
+   * @param array List of PHIDs of commits
+   * @return this
+   * @task config
+   */
+  public function withCommitPHIDs(array $commit_phids) {
+    $this->commitPHIDs = $commit_phids;
+    return $this;
+  }
+
   /**
    * Filter results to revisions with a given status. Provide a class constant,
    * such as `DifferentialRevisionQuery::STATUS_OPEN`.
@@ -653,6 +668,13 @@ final class DifferentialRevisionQuery
         $this->draftAuthors);
     }
 
+    if ($this->commitPHIDs) {
+      $joins[] = qsprintf(
+        $conn_r,
+        'JOIN %T commits ON commits.revisionID = r.id',
+        DifferentialRevision::TABLE_COMMIT);
+    }
+
     $joins = implode(' ', $joins);
 
     return $joins;
@@ -714,6 +736,13 @@ final class DifferentialRevisionQuery
       $where[] = $hash_clauses;
     }
 
+    if ($this->commitPHIDs) {
+      $where[] = qsprintf(
+        $conn_r,
+        'commits.commitPHID IN (%Ls)',
+        $this->commitPHIDs);
+    }
+
     if ($this->phids) {
       $where[] = qsprintf(
         $conn_r,
diff --git a/src/applications/diffusion/conduit/DiffusionGetCommitsConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionGetCommitsConduitAPIMethod.php
index 52f662ba2f..e6cfd9730d 100644
--- a/src/applications/diffusion/conduit/DiffusionGetCommitsConduitAPIMethod.php
+++ b/src/applications/diffusion/conduit/DiffusionGetCommitsConduitAPIMethod.php
@@ -135,7 +135,7 @@ final class DiffusionGetCommitsConduitAPIMethod
     }
 
     $commits = $this->addRepositoryCommitDataInformation($commits);
-    $commits = $this->addDifferentialInformation($commits);
+    $commits = $this->addDifferentialInformation($commits, $request);
     $commits = $this->addManiphestInformation($commits);
 
     foreach ($commits as $name => $commit) {
@@ -238,31 +238,29 @@ final class DiffusionGetCommitsConduitAPIMethod
   /**
    * Enhance the commit list with Differential information.
    */
-  private function addDifferentialInformation(array $commits) {
+  private function addDifferentialInformation(
+    array $commits,
+    ConduitAPIRequest $request) {
+
     $commit_phids = ipull($commits, 'commitPHID');
 
-    // TODO: (T603) This should be policy checked, either by moving to
-    // DifferentialRevisionQuery or by doing a followup query to make sure
-    // the matched objects are visible.
-
-    $rev_conn_r = id(new DifferentialRevision())->establishConnection('r');
-    $revs = queryfx_all(
-      $rev_conn_r,
-      'SELECT r.id id, r.phid phid, c.commitPHID commitPHID FROM %T r JOIN %T c
-        ON r.id = c.revisionID
-        WHERE c.commitPHID in (%Ls)',
-      id(new DifferentialRevision())->getTableName(),
-      DifferentialRevision::TABLE_COMMIT,
-      $commit_phids);
-
-    $revs = ipull($revs, null, 'commitPHID');
-    foreach ($commits as $name => $commit) {
-      if (isset($revs[$commit['commitPHID']])) {
-        $rev = $revs[$commit['commitPHID']];
-        $commits[$name] += array(
-          'differentialRevisionID'    => 'D'.$rev['id'],
-          'differentialRevisionPHID'  => $rev['phid'],
-        );
+    $revisions = id(new DifferentialRevisionQuery())
+      ->setViewer($request->getUser())
+      ->withCommitPHIDs($commit_phids)
+      ->needCommitPHIDs(true)
+      ->execute();
+    $rev_phid_commit_phids_map = mpull($revisions, 'getCommitPHIDs', 'getPHID');
+    $revisions = mpull($revisions, null, 'getPHID');
+    foreach ($rev_phid_commit_phids_map as $rev_phid => $commit_phids) {
+      foreach ($commits as $name => $commit) {
+        $commit_phid = $commit['commitPHID'];
+        if (in_array($commit_phid, $commit_phids)) {
+          $revision = $revisions[$rev_phid];
+          $commits[$name] += array(
+            'differentialRevisionID'    => 'D'.$revision->getID(),
+            'differentialRevisionPHID'  => $revision->getPHID(),
+          );
+        }
       }
     }