Add a differential.getdiffs method

Summary: I kind of made a mess of the API doing T2784. I figure just adding this is fine but LMK if you'd prefer something like diffquery got cleaned up more to handle this.  Also adds an idx() call as I was getting errors looking at old diffs. Fixes T3823.

Test Plan: used the new api via test console - great success.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Maniphest Tasks: T3823

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

src/__phutil_library_map__.php

@@ -138,6 +138,7 @@ phutil_register_library_map(array(
     'ConduitAPI_differential_markcommitted_Method' => 'applications/differential/conduit/ConduitAPI_differential_markcommitted_Method.php',
     'ConduitAPI_differential_parsecommitmessage_Method' => 'applications/differential/conduit/ConduitAPI_differential_parsecommitmessage_Method.php',
     'ConduitAPI_differential_query_Method' => 'applications/differential/conduit/ConduitAPI_differential_query_Method.php',
+    'ConduitAPI_differential_querydiffs_Method' => 'applications/differential/conduit/ConduitAPI_differential_querydiffs_Method.php',
     'ConduitAPI_differential_setdiffproperty_Method' => 'applications/differential/conduit/ConduitAPI_differential_setdiffproperty_Method.php',
     'ConduitAPI_differential_updaterevision_Method' => 'applications/differential/conduit/ConduitAPI_differential_updaterevision_Method.php',
     'ConduitAPI_differential_updateunitresults_Method' => 'applications/differential/conduit/ConduitAPI_differential_updateunitresults_Method.php',
@@ -2200,6 +2201,7 @@ phutil_register_library_map(array(
     'ConduitAPI_differential_markcommitted_Method' => 'ConduitAPIMethod',
     'ConduitAPI_differential_parsecommitmessage_Method' => 'ConduitAPIMethod',
     'ConduitAPI_differential_query_Method' => 'ConduitAPIMethod',
+    'ConduitAPI_differential_querydiffs_Method' => 'ConduitAPIMethod',
     'ConduitAPI_differential_setdiffproperty_Method' => 'ConduitAPIMethod',
     'ConduitAPI_differential_updaterevision_Method' => 'ConduitAPIMethod',
     'ConduitAPI_differential_updateunitresults_Method' => 'ConduitAPIMethod',

src/applications/differential/conduit/ConduitAPI_differential_getalldiffs_Method.php

@@ -6,6 +6,15 @@
 final class ConduitAPI_differential_getalldiffs_Method
   extends ConduitAPIMethod {
 
+  public function getMethodStatus() {
+    return self::METHOD_STATUS_DEPRECATED;
+  }
+
+  public function getMethodStatusDescription() {
+    return pht(
+      'This method has been deprecated in favor of differential.querydiffs.');
+  }
+
   public function getMethodDescription() {
     return "Load all diffs for given revisions from Differential.";
   }

src/applications/differential/conduit/ConduitAPI_differential_getdiff_Method.php

@@ -3,10 +3,22 @@
 /**
  * @group conduit
  */
-final class ConduitAPI_differential_getdiff_Method extends ConduitAPIMethod {
+final class ConduitAPI_differential_getdiff_Method
+  extends ConduitAPIMethod {
+
+  public function getMethodStatus() {
+    return self::METHOD_STATUS_DEPRECATED;
+  }
+
+  public function getMethodStatusDescription() {
+    return pht(
+      'This method has been deprecated in favor of differential.querydiffs.');
+  }
+
 
   public function getMethodDescription() {
-    return "Load the content of a diff from Differential.";
+    return pht('Load the content of a diff from Differential by revision id '.
+               'or diff id.');
   }
 
   public function defineParamTypes() {
@@ -22,7 +34,6 @@ final class ConduitAPI_differential_getdiff_Method extends ConduitAPIMethod {
 
   public function defineErrorTypes() {
     return array(
-      'ERR_BAD_REVISION'    => 'No such revision exists.',
       'ERR_BAD_DIFF'        => 'No such diff exists.',
     );
   }
@@ -33,50 +44,32 @@ final class ConduitAPI_differential_getdiff_Method extends ConduitAPIMethod {
 
   protected function execute(ConduitAPIRequest $request) {
     $diff = null;
+    $revision_ids = array();
+    $diff_ids = array();
 
     $revision_id = $request->getValue('revision_id');
     if ($revision_id) {
-      $revision = id(new DifferentialRevision())->load($revision_id);
-      if (!$revision) {
-        throw new ConduitException('ERR_BAD_REVISION');
-      }
-      $diff = id(new DifferentialDiff())->loadOneWhere(
-        'revisionID = %d ORDER BY id DESC LIMIT 1',
-        $revision->getID());
-    } else {
-      $diff_id = $request->getValue('diff_id');
-      if ($diff_id) {
-        $diff = id(new DifferentialDiffQuery())
-          ->setViewer($request->getUser())
-          ->withIDs(array($diff_id))
-          ->executeOne();
-      }
+      $revision_ids = array($revision_id);
+    }
+    $diff_id = $request->getValue('diff_id');
+    if ($diff_id) {
+      $diff_ids = array($diff_id);
+    }
+    if ($diff_ids || $revision_ids) {
+      $diff = id(new DifferentialDiffQuery())
+        ->setViewer($request->getUser())
+        ->withIDs($diff_ids)
+        ->withRevisionIDs($revision_ids)
+        ->needChangesets(true)
+        ->needArcanistProjects(true)
+        ->executeOne();
     }
 
     if (!$diff) {
       throw new ConduitException('ERR_BAD_DIFF');
     }
 
-    $diff->attachChangesets(
-      $diff->loadRelatives(new DifferentialChangeset(), 'diffID'));
-    foreach ($diff->getChangesets() as $changeset) {
-      $changeset->attachHunks(
-        $changeset->loadRelatives(new DifferentialHunk(), 'changesetID'));
-    }
-
-    $basic_dict = $diff->getDiffDict();
-
-    // for conduit calls, the basic dict is not enough
-    // we also need to include the arcanist project and author information
-    $project = $diff->loadArcanistProject();
-    if ($project) {
-      $project_name = $project->getName();
-    } else {
-      $project_name = null;
-    }
-    $basic_dict['projectName'] = $project_name;
-
-    return $basic_dict;
+    return $diff->getDiffDict();
   }
 
 }

src/applications/differential/conduit/ConduitAPI_differential_getrevision_Method.php

@@ -51,17 +51,12 @@ final class ConduitAPI_differential_getrevision_Method
 
     $reviewer_phids = array_values($revision->getReviewers());
 
-    $diffs = $revision->loadDiffs();
-
-    $diff_dicts = array();
-    foreach ($diffs as $diff) {
-      $diff->attachChangesets($diff->loadChangesets());
-      // TODO: We could batch this to improve performance.
-      foreach ($diff->getChangesets() as $changeset) {
-        $changeset->attachHunks($changeset->loadHunks());
-      }
-      $diff_dicts[] = $diff->getDiffDict();
-    }
+    $diffs = id(new DifferentialDiffQuery())
+      ->withRevisionIDs(array($revision_id))
+      ->needChangesets(true)
+      ->needArcanistProjects(true)
+      ->execute();
+    $diff_dicts = mpull($diffs, 'getDiffDict');
 
     $commit_dicts = array();
     $commit_phids = $revision->loadCommitPHIDs();

src/applications/differential/conduit/ConduitAPI_differential_querydiffs_Method.php

@@ -0,0 +1,45 @@
+<?php
+
+/**
+ * @group conduit
+ */
+final class ConduitAPI_differential_querydiffs_Method
+  extends ConduitAPIMethod {
+
+  public function getMethodDescription() {
+    return pht('Query differential diffs which match certain criteria.');
+  }
+
+  public function defineParamTypes() {
+    return array(
+      'ids' => 'optional list<uint>',
+      'revison_ids' => 'optional list<uint>',
+    );
+  }
+
+  public function defineErrorTypes() {
+    return array();
+  }
+
+  public function defineReturnType() {
+    return 'list<dict>';
+  }
+
+  protected function execute(ConduitAPIRequest $request) {
+    $ids = $request->getValue('ids', array());
+    $revision_ids = $request->getValue('revision_ids', array());
+    $diffs = array();
+    $diff_dicts = array();
+    if ($ids || $revision_ids) {
+      $diffs = id(new DifferentialDiffQuery())
+        ->setViewer($request->getUser())
+        ->withIDs($ids)
+        ->withRevisionIDs($revision_ids)
+        ->needChangesets(true)
+        ->needArcanistProjects(true)
+        ->execute();
+    }
+
+    return mpull($diffs, 'getDiffDict', 'getID');
+  }
+}

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -29,7 +29,10 @@ final class DifferentialRevisionViewController extends DifferentialController {
       return new Aphront404Response();
     }
 
-    $diffs = $revision->loadDiffs();
+    $diffs = id(new DifferentialDiffQuery())
+      ->setViewer($request->getUser())
+      ->withRevisionIDs(array($this->revisionID))
+      ->execute();
 
     if (!$diffs) {
       throw new Exception(
@@ -925,10 +928,9 @@ final class DifferentialRevisionViewController extends DifferentialController {
 
     $diff = new DifferentialDiff();
     $diff->attachChangesets($generated_changesets);
-    $diff_dict = $diff->getDiffDict();
-
+    $raw_changes = $diff->buildChangesList();
     $changes = array();
-    foreach ($diff_dict['changes'] as $changedict) {
+    foreach ($raw_changes as $changedict) {
       $changes[] = ArcanistDiffChange::newFromDictionary($changedict);
     }
     $bundle = ArcanistBundle::newFromChanges($changes);

src/applications/differential/mail/DifferentialReviewRequestMail.php

@@ -87,7 +87,10 @@ abstract class DifferentialReviewRequestMail extends DifferentialMail {
       $revision = $this->getRevision();
       $revision_id = $revision->getID();
 
-      $diffs = $revision->loadDiffs();
+      $diffs = id(new DifferentialDiffQuery())
+        ->setViewer($this->getActor())
+        ->withRevisionIDs(array($revision_id))
+        ->execute();
       $diff_number = count($diffs);
 
       $attachments[] = new PhabricatorMetaMTAAttachment(
@@ -112,16 +115,15 @@ abstract class DifferentialReviewRequestMail extends DifferentialMail {
 
   private function buildPatch() {
     $diff = new DifferentialDiff();
-
     $diff->attachChangesets($this->getChangesets());
-    // TODO: We could batch this to improve performance.
     foreach ($diff->getChangesets() as $changeset) {
-      $changeset->attachHunks($changeset->loadHunks());
+      $changeset->attachHunks(
+        $changeset->loadRelatives(new DifferentialHunk(), 'changesetID'));
     }
-    $diff_dict = $diff->getDiffDict();
 
+    $raw_changes = $diff->buildChangesList();
     $changes = array();
-    foreach ($diff_dict['changes'] as $changedict) {
+    foreach ($raw_changes as $changedict) {
       $changes[] = ArcanistDiffChange::newFromDictionary($changedict);
     }
     $bundle = ArcanistBundle::newFromChanges($changes);

src/applications/differential/query/DifferentialDiffQuery.php

@@ -5,6 +5,8 @@ final class DifferentialDiffQuery
 
   private $ids;
   private $revisionIDs;
+  private $needChangesets = false;
+  private $needArcanistProjects = false;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -16,6 +18,16 @@ final class DifferentialDiffQuery
     return $this;
   }
 
+  public function needChangesets($bool) {
+    $this->needChangesets = $bool;
+    return $this;
+  }
+
+  public function needArcanistProjects($bool) {
+    $this->needArcanistProjects = $bool;
+    return $this;
+  }
+
   public function loadPage() {
     $table = new DifferentialDiff();
     $conn_r = $table->establishConnection('r');
@@ -31,6 +43,50 @@ final class DifferentialDiffQuery
     return $table->loadAllFromArray($data);
   }
 
+  public function willFilterPage(array $diffs) {
+    if ($this->needChangesets) {
+      $this->loadChangesets($diffs);
+    }
+
+    if ($this->needArcanistProjects) {
+      $this->loadArcanistProjects($diffs);
+    }
+
+    return $diffs;
+  }
+
+  private function loadChangesets(array $diffs) {
+    foreach ($diffs as $diff) {
+      $diff->attachChangesets(
+        $diff->loadRelatives(new DifferentialChangeset(), 'diffID'));
+      foreach ($diff->getChangesets() as $changeset) {
+        $changeset->attachHunks(
+          $changeset->loadRelatives(new DifferentialHunk(), 'changesetID'));
+      }
+    }
+    return $diffs;
+  }
+
+  private function loadArcanistProjects(array $diffs) {
+    $phids = array_filter(mpull($diffs, 'getArcanistProjectPHID'));
+    $projects = array();
+    $project_map = array();
+    if ($phids) {
+      $projects = id(new PhabricatorRepositoryArcanistProject())
+        ->loadAllWhere(
+          'phid IN (%Ls)',
+          $phids);
+      $project_map = mpull($projects, null, 'getPHID');
+      foreach ($diffs as $diff) {
+        if ($diff->getArcanistProjectPHID()) {
+          $project = $project_map[$diff->getArcanistProjectPHID()];
+          $diff->attachArcanistProject($project);
+        }
+      }
+    }
+    return $diffs;
+  }
+
   private function buildWhereClause(AphrontDatabaseConnection $conn_r) {
     $where = array();
 

src/applications/differential/storage/DifferentialDiff.php

@@ -31,6 +31,7 @@ final class DifferentialDiff
 
   private $unsavedChangesets = array();
   private $changesets = self::ATTACHABLE;
+  private $arcanistProject = self::ATTACHABLE;
 
   public function addUnsavedChangeset(DifferentialChangeset $changeset) {
     if ($this->changesets === null) {
@@ -60,6 +61,25 @@ final class DifferentialDiff
       $this->getID());
   }
 
+  public function attachArcanistProject(
+    PhabricatorRepositoryArcanistProject $project) {
+    $this->arcanistProject = $project;
+    return $this;
+  }
+
+  public function getArcanistProject() {
+    return $this->assertAttached($this->arcanistProject);
+  }
+
+  public function getArcanistProjectName() {
+    $name = '';
+    if ($this->arcanistProject) {
+      $project = $this->getArcanistProject();
+      $name = $project->getName();
+    }
+    return $name;
+  }
+
   public function loadArcanistProject() {
     if (!$this->getArcanistProjectPHID()) {
       return null;
@@ -204,8 +224,31 @@ final class DifferentialDiff
       'lintStatus' => $this->getLintStatus(),
       'changes' => array(),
       'properties' => array(),
+      'projectName' => $this->getArcanistProjectName()
     );
 
+    $dict['changes'] = $this->buildChangesList();
+
+    $properties = id(new DifferentialDiffProperty())->loadAllWhere(
+      'diffID = %d',
+      $this->getID());
+    foreach ($properties as $property) {
+      $dict['properties'][$property->getName()] = $property->getData();
+
+      if ($property->getName() == 'local:commits') {
+        foreach ($property->getData() as $commit) {
+          $dict['authorName'] = $commit['author'];
+          $dict['authorEmail'] = idx($commit, 'authorEmail');
+          break;
+        }
+      }
+    }
+
+    return $dict;
+  }
+
+  public function buildChangesList() {
+    $changes = array();
     foreach ($this->getChangesets() as $changeset) {
       $hunks = array();
       foreach ($changeset->getHunks() as $hunk) {
@@ -236,25 +279,9 @@ final class DifferentialDiff
         'delLines'      => $changeset->getDelLines(),
         'hunks'         => $hunks,
       );
-      $dict['changes'][] = $change;
+      $changes[] = $change;
     }
-
-    $properties = id(new DifferentialDiffProperty())->loadAllWhere(
-      'diffID = %d',
-      $this->getID());
-    foreach ($properties as $property) {
-      $dict['properties'][$property->getName()] = $property->getData();
-
-      if ($property->getName() == 'local:commits') {
-        foreach ($property->getData() as $commit) {
-          $dict['authorName'] = $commit['author'];
-          $dict['authorEmail'] = $commit['authorEmail'];
-          break;
-        }
-      }
-    }
-
-    return $dict;
+    return $changes;
   }
 
 

src/applications/differential/storage/DifferentialRevision.php

@@ -132,15 +132,6 @@ final class DifferentialRevision extends DifferentialDAO
       DifferentialPHIDTypeRevision::TYPECONST);
   }
 
-  public function loadDiffs() {
-    if (!$this->getID()) {
-      return array();
-    }
-    return id(new DifferentialDiff())->loadAllWhere(
-      'revisionID = %d',
-      $this->getID());
-  }
-
   public function loadComments() {
     if (!$this->getID()) {
       return array();
@@ -165,7 +156,10 @@ final class DifferentialRevision extends DifferentialDAO
 
   public function delete() {
     $this->openTransaction();
-      $diffs = $this->loadDiffs();
+    $diffs = id(new DifferentialDiffQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withRevisionIDs(array($this->getID()))
+      ->execute();
       foreach ($diffs as $diff) {
         $diff->delete();
       }