Remove indirect loads of Differential revisions from Releeph requests

Summary:
Ref T3551. Currently, there are many layers of indirection between pull requests and revisions. After D8822, revisions and other types of requested objects are recorded directly on the request. This allows us to simplify data access and querying.

A lot of stuff here is doing `instanceof` checks to keep APIs stable, but most of those can go away in the long run.

Test Plan:
  - Browsed requests.
  - Verified revision-dependent fields (like "Revision", "Size", "Churn") still render correctly.
  - Called `releeph.queryrequests`.
  - Called `releephwork.nextrequest`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3551

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

src/applications/releeph/conduit/ConduitAPI_releeph_queryrequests_Method.php

@@ -37,7 +37,7 @@ final class ConduitAPI_releeph_queryrequests_Method
     $query->setViewer($conduit_request->getUser());
 
     if ($revision_phids) {
-      $query->withRevisionPHIDs($revision_phids);
+      $query->withRequestedObjectPHIDs($revision_phids);
     } else if ($requested_commit_phids) {
       $query->withRequestedCommitPHIDs($requested_commit_phids);
     }
@@ -48,8 +48,14 @@ final class ConduitAPI_releeph_queryrequests_Method
       $branch = $releephRequest->getBranch();
 
       $request_commit_phid = $releephRequest->getRequestCommitPHID();
-      $revisionPHID =
+
-        $query->getRevisionPHID($request_commit_phid);
+      $object = $releephRequest->getRequestedObject();
+      if ($object instanceof DifferentialRevision) {
+        $object_phid = $object->getPHID();
+      }  else {
+        $object_phid = null;
+      }
+
       $status = $releephRequest->getStatus();
       $statusName = ReleephRequestStatus::getStatusDescriptionFor($status);
       $url = PhabricatorEnv::getProductionURI('/RQ'.$releephRequest->getID());
@@ -58,7 +64,7 @@ final class ConduitAPI_releeph_queryrequests_Method
         'branchBasename' => $branch->getBasename(),
         'branchSymbolic' => $branch->getSymbolicName(),
         'requestID'      => $releephRequest->getID(),
-        'revisionPHID'   => $revisionPHID,
+        'revisionPHID'   => $object_phid,
         'status'         => $status,
         'statusName'     => $statusName,
         'url'            => $url,

src/applications/releeph/conduit/work/ConduitAPI_releephwork_nextrequest_Method.php

@@ -116,7 +116,14 @@ final class ConduitAPI_releephwork_nextrequest_Method
 
     $diff_phid = null;
     $diff_rev_id = null;
-    $diff_rev = $releeph_request->loadDifferentialRevision();
+
+    $requested_object = $releeph_request->getRequestedObject();
+    if ($requested_object instanceof DifferentialRevision) {
+      $diff_rev = $requested_object;
+    } else {
+      $diff_rev = null;
+    }
+
     if ($diff_rev) {
       $diff_phid = $diff_rev->getPHID();
       $phids[] = $diff_phid;

src/applications/releeph/field/specification/ReleephDependsOnFieldSpecification.php

@@ -19,13 +19,13 @@ final class ReleephDependsOnFieldSpecification
   }
 
   private function getDependentRevisionPHIDs() {
-    $revision = $this
+    $requested_object = $this->getObject()->getRequestedObjectPHID();
-      ->getReleephRequest()
+    if (!($requested_object instanceof DifferentialRevision)) {
-      ->loadDifferentialRevision();
-    if (!$revision) {
       return array();
     }
 
+    $revision = $requested_object;
+
     return PhabricatorEdgeQuery::loadDestinationPHIDs(
       $revision->getPHID(),
       PhabricatorEdgeConfig::TYPE_DREV_DEPENDS_ON_DREV);

src/applications/releeph/field/specification/ReleephDiffChurnFieldSpecification.php

@@ -17,15 +17,14 @@ final class ReleephDiffChurnFieldSpecification
   }
 
   public function renderPropertyViewValue(array $handles) {
-    $diff_rev = $this->getReleephRequest()->loadDifferentialRevision();
+    $requested_object = $this->getObject()->getRequestedObject();
-    if (!$diff_rev) {
+    if (!($requested_object instanceof DifferentialRevision)) {
       return null;
     }
-
+    $diff_rev = $requested_object;
-    $diff_rev = $this->getReleephRequest()->loadDifferentialRevision();
 
     $xactions = id(new DifferentialTransactionQuery())
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->setViewer($this->getViewer())
       ->withObjectPHIDs(array($diff_rev->getPHID()))
       ->execute();
 

src/applications/releeph/field/specification/ReleephDiffSizeFieldSpecification.php

@@ -16,10 +16,11 @@ final class ReleephDiffSizeFieldSpecification
   }
 
   public function renderPropertyViewValue(array $handles) {
-    $diff_rev = $this->getReleephRequest()->loadDifferentialRevision();
+    $requested_object = $this->getObject()->getRequestedObject();
-    if (!$diff_rev) {
+    if (!($requested_object instanceof DifferentialRevision)) {
       return null;
     }
+    $diff_rev = $requested_object;
 
     $diffs = $diff_rev->loadRelatives(
       new DifferentialDiff(),

src/applications/releeph/field/specification/ReleephRevisionFieldSpecification.php

@@ -12,14 +12,14 @@ final class ReleephRevisionFieldSpecification
   }
 
   public function getRequiredHandlePHIDsForPropertyView() {
-    $phids = array();
+    $requested_object = $this->getObject()->getRequestedObjectPHID();
-
+    if (!($requested_object instanceof DifferentialRevision)) {
-    $phid = $this->getReleephRequest()->loadRequestCommitDiffPHID();
+      return array();
-    if ($phid) {
-      $phids[] = $phid;
     }
 
-    return $phids;
+    return array(
+      $requested_object->getPHID(),
+    );
   }
 
   public function renderPropertyViewValue(array $handles) {

src/applications/releeph/query/ReleephRequestQuery.php

@@ -4,13 +4,12 @@ final class ReleephRequestQuery
   extends PhabricatorCursorPagedPolicyAwareQuery {
 
   private $requestedCommitPHIDs;
-  private $commitToRevMap;
   private $ids;
   private $phids;
   private $severities;
   private $requestorPHIDs;
   private $branchIDs;
-  private $revisionPHIDs;
+  private $requestedObjectPHIDs;
 
   const STATUS_ALL          = 'status-all';
   const STATUS_OPEN         = 'status-open';
@@ -39,14 +38,6 @@ final class ReleephRequestQuery
     return $this;
   }
 
-  public function getRevisionPHID($commit_phid) {
-    if ($this->commitToRevMap) {
-      return idx($this->commitToRevMap, $commit_phid, null);
-    }
-
-    return null;
-  }
-
   public function withStatus($status) {
     $this->status = $status;
     return $this;
@@ -67,8 +58,8 @@ final class ReleephRequestQuery
     return $this;
   }
 
-  public function withRevisionPHIDs(array $revision_phids) {
+  public function withRequestedObjectPHIDs(array $phids) {
-    $this->revisionPHIDs = $revision_phids;
+    $this->requestedObjectPHIDs = $phids;
     return $this;
   }
 
@@ -89,6 +80,25 @@ final class ReleephRequestQuery
 
   public function willFilterPage(array $requests) {
 
+    // Load requested objects: you must be able to see an object to see
+    // requests for it.
+    $object_phids = mpull($requests, 'getRequestedObjectPHID');
+    $objects = id(new PhabricatorObjectQuery())
+      ->setViewer($this->getViewer())
+      ->setParentQuery($this)
+      ->withPHIDs($object_phids)
+      ->execute();
+
+    foreach ($requests as $key => $request) {
+      $object_phid = $request->getRequestedObjectPHID();
+      $object = idx($objects, $object_phid);
+      if (!$object) {
+        unset($requests[$key]);
+        continue;
+      }
+      $request->attachRequestedObject($object);
+    }
+
     if ($this->severities) {
       $severities = array_fuse($this->severities);
       foreach ($requests as $key => $request) {
@@ -141,64 +151,46 @@ final class ReleephRequestQuery
   private function buildWhereClause(AphrontDatabaseConnection $conn_r) {
     $where = array();
 
-    if ($this->ids) {
+    if ($this->ids !== null) {
       $where[] = qsprintf(
         $conn_r,
         'id IN (%Ld)',
         $this->ids);
     }
 
-    if ($this->phids) {
+    if ($this->phids !== null) {
       $where[] = qsprintf(
         $conn_r,
         'phid IN (%Ls)',
         $this->phids);
     }
 
-    if ($this->branchIDs) {
+    if ($this->branchIDs !== null) {
       $where[] = qsprintf(
         $conn_r,
         'branchID IN (%Ld)',
         $this->branchIDs);
     }
 
-    if ($this->requestedCommitPHIDs) {
+    if ($this->requestedCommitPHIDs !== null) {
       $where[] = qsprintf(
         $conn_r,
         'requestCommitPHID IN (%Ls)',
         $this->requestedCommitPHIDs);
     }
 
-    if ($this->requestorPHIDs) {
+    if ($this->requestorPHIDs !== null) {
       $where[] = qsprintf(
         $conn_r,
         'requestUserPHID IN (%Ls)',
         $this->requestorPHIDs);
     }
 
-    if ($this->revisionPHIDs) {
+    if ($this->requestedObjectPHIDs !== null) {
-      $type = PhabricatorEdgeConfig::TYPE_DREV_HAS_COMMIT;
-
-      $edges = id(new PhabricatorEdgeQuery())
-        ->withSourcePHIDs($this->revisionPHIDs)
-        ->withEdgeTypes(array($type))
-        ->execute();
-
-      $this->commitToRevMap = array();
-      foreach ($edges as $revision_phid => $edge) {
-        foreach ($edge[$type] as $commitPHID => $item) {
-          $this->commitToRevMap[$commitPHID] = $revision_phid;
-        }
-      }
-
-      if (!$this->commitToRevMap) {
-        throw new PhabricatorEmptyQueryException("Malformed Revision Phids");
-      }
-
       $where[] = qsprintf(
         $conn_r,
-        'requestCommitPHID IN (%Ls)',
+        'requestedObjectPHID IN (%Ls)',
-        array_keys($this->commitToRevMap));
+        $this->requestedObjectPHIDs);
     }
 
     $where[] = $this->buildPagingClause($conn_r);

src/applications/releeph/storage/ReleephRequest.php

@@ -30,6 +30,7 @@ final class ReleephRequest extends ReleephDAO
 
   private $customFields = self::ATTACHABLE;
   private $branch = self::ATTACHABLE;
+  private $requestedObject = self::ATTACHABLE;
 
 
 /* -(  Constants and helper methods  )--------------------------------------- */
@@ -105,6 +106,15 @@ final class ReleephRequest extends ReleephDAO
     return $this;
   }
 
+  public function getRequestedObject() {
+    return $this->assertAttached($this->requestedObject);
+  }
+
+  public function attachRequestedObject($object) {
+    $this->requestedObject = $object;
+    return $this;
+  }
+
   private function calculateStatus() {
     if ($this->shouldBeInBranch()) {
       if ($this->getInBranch()) {
@@ -214,19 +224,6 @@ final class ReleephRequest extends ReleephDAO
     return $summary;
   }
 
-  public function loadRequestCommitDiffPHID() {
-    $phids = array();
-    $commit = $this->loadPhabricatorRepositoryCommit();
-    if ($commit) {
-      $phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
-        $commit->getPHID(),
-        PhabricatorEdgeConfig::TYPE_COMMIT_HAS_DREV);
-    }
-
-    return head($phids);
-  }
-
-
 /* -(  Loading external objects  )------------------------------------------- */
 
   public function loadPhabricatorRepositoryCommit() {
@@ -245,18 +242,6 @@ final class ReleephRequest extends ReleephDAO
     }
   }
 
-  // TODO: (T603) Get rid of all this one-off ad-hoc loading.
-  public function loadDifferentialRevision() {
-    $diff_phid = $this->loadRequestCommitDiffPHID();
-    if (!$diff_phid) {
-      return null;
-    }
-    return $this->loadOneRelative(
-        new DifferentialRevision(),
-        'phid',
-        'loadRequestCommitDiffPHID');
-  }
-
 
 /* -(  State change helpers  )----------------------------------------------- */