Introduce DifferentialChangesetQuery and remove loadHunks()

Summary: Ref T4045. Ref T5179. This removes all non-Query hunk loads.

Test Plan:
  - Viewed revisions.
  - Viewed standalone changesets.
  - Viewed raw old/new files.
  - Viewed vs diffs.
  - Enabled inline comments in mail and sent some transactions with inlines.
  - Called `differential.getrawdiff`.
  - Grepped for `loadHunks()`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4045, T5179

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

src/__phutil_library_map__.php

@@ -343,6 +343,7 @@ phutil_register_library_map(array(
     'DifferentialChangesetOneUpTestRenderer' => 'applications/differential/render/DifferentialChangesetOneUpTestRenderer.php',
     'DifferentialChangesetParser' => 'applications/differential/parser/DifferentialChangesetParser.php',
     'DifferentialChangesetParserTestCase' => 'applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php',
+    'DifferentialChangesetQuery' => 'applications/differential/query/DifferentialChangesetQuery.php',
     'DifferentialChangesetRenderer' => 'applications/differential/render/DifferentialChangesetRenderer.php',
     'DifferentialChangesetTestRenderer' => 'applications/differential/render/DifferentialChangesetTestRenderer.php',
     'DifferentialChangesetTwoUpRenderer' => 'applications/differential/render/DifferentialChangesetTwoUpRenderer.php',
@@ -3018,6 +3019,7 @@ phutil_register_library_map(array(
     'DifferentialChangesetOneUpRenderer' => 'DifferentialChangesetHTMLRenderer',
     'DifferentialChangesetOneUpTestRenderer' => 'DifferentialChangesetTestRenderer',
     'DifferentialChangesetParserTestCase' => 'PhabricatorTestCase',
+    'DifferentialChangesetQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'DifferentialChangesetTestRenderer' => 'DifferentialChangesetRenderer',
     'DifferentialChangesetTwoUpRenderer' => 'DifferentialChangesetHTMLRenderer',
     'DifferentialChangesetTwoUpTestRenderer' => 'DifferentialChangesetTestRenderer',

src/applications/differential/controller/DifferentialChangesetViewController.php

@@ -23,25 +23,33 @@ final class DifferentialChangesetViewController extends DifferentialController {
     $id = (int)$id;
     $vs = (int)$vs;
 
-    $changeset = id(new DifferentialChangeset())->load($id);
+    $load_ids = array($id);
+    if ($vs && ($vs != -1)) {
+      $load_ids[] = $vs;
+    }
+
+    $changesets = id(new DifferentialChangesetQuery())
+      ->setViewer($request->getUser())
+      ->withIDs($load_ids)
+      ->needHunks(true)
+      ->execute();
+    $changesets = mpull($changesets, null, 'getID');
+
+    $changeset = idx($changesets, $id);
     if (!$changeset) {
       return new Aphront404Response();
     }
 
-    // TODO: (T603) Make Changeset policy-aware. For now, just fake it
-    // by making sure we can see the diff.
-    $diff = id(new DifferentialDiffQuery())
-      ->setViewer($request->getUser())
-      ->withIDs(array($changeset->getDiffID()))
-      ->executeOne();
-    if (!$diff) {
-      return new Aphront404Response();
+    $vs_changeset = null;
+    if ($vs && ($vs != -1)) {
+      $vs_changeset = idx($changesets, $vs);
+      if (!$vs_changeset) {
+        return new Aphront404Response();
+      }
     }
 
-
     $view = $request->getStr('view');
     if ($view) {
-      $changeset->attachHunks($changeset->loadHunks());
       $phid = idx($changeset->getMetadata(), "$view:binary-phid");
       if ($phid) {
         return id(new AphrontRedirectResponse())->setURI("/file/info/$phid/");
@@ -50,12 +58,8 @@ final class DifferentialChangesetViewController extends DifferentialController {
         case 'new':
           return $this->buildRawFileResponse($changeset, $is_new = true);
         case 'old':
-          if ($vs && ($vs != -1)) {
-            $vs_changeset = id(new DifferentialChangeset())->load($vs);
-            if ($vs_changeset) {
-              $vs_changeset->attachHunks($vs_changeset->loadHunks());
-              return $this->buildRawFileResponse($vs_changeset, $is_new = true);
-            }
+          if ($vs_changeset) {
+            return $this->buildRawFileResponse($vs_changeset, $is_new = true);
           }
           return $this->buildRawFileResponse($changeset, $is_new = false);
         default:
@@ -63,13 +67,6 @@ final class DifferentialChangesetViewController extends DifferentialController {
       }
     }
 
-    if ($vs && ($vs != -1)) {
-      $vs_changeset = id(new DifferentialChangeset())->load($vs);
-      if (!$vs_changeset) {
-        return new Aphront404Response();
-      }
-    }
-
     if (!$vs) {
       $right = $changeset;
       $left  = null;
@@ -103,15 +100,6 @@ final class DifferentialChangesetViewController extends DifferentialController {
     }
 
     if ($left) {
-      $left->attachHunks($left->loadHunks());
-    }
-
-    if ($right) {
-      $right->attachHunks($right->loadHunks());
-    }
-
-    if ($left) {
-
       $left_data = $left->makeNewFile();
       if ($right) {
         $right_data = $right->makeNewFile();
@@ -264,9 +252,30 @@ final class DifferentialChangesetViewController extends DifferentialController {
       ),
       $detail->render()));
 
+    $crumbs = $this->buildApplicationCrumbs();
+
+    $revision_id = $changeset->getDiff()->getRevisionID();
+    if ($revision_id) {
+      $crumbs->addTextCrumb('D'.$revision_id, '/D'.$revision_id);
+    }
+
+    $diff_id = $changeset->getDiff()->getID();
+    if ($diff_id) {
+      $crumbs->addTextCrumb(
+        pht('Diff %d', $diff_id),
+        $this->getApplicationURI('diff/'.$diff_id));
+    }
+
+    $crumbs->addTextCrumb($changeset->getDisplayFilename());
+
+    $box = id(new PHUIObjectBoxView())
+      ->setHeaderText(pht('Standalone View'))
+      ->appendChild($panel);
+
     return $this->buildApplicationPage(
       array(
-        $panel
+        $crumbs,
+        $box,
       ),
       array(
         'title' => pht('Changeset View'),

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -844,9 +844,11 @@ final class DifferentialRevisionViewController extends DifferentialController {
 
     $viewer = $this->getRequest()->getUser();
 
-    foreach ($changesets as $changeset) {
-      $changeset->attachHunks($changeset->loadHunks());
-    }
+    id(new DifferentialHunkQuery())
+      ->setViewer($viewer)
+      ->withChangesets($changesets)
+      ->needAttachToChangesets(true)
+      ->execute();
 
     $diff = new DifferentialDiff();
     $diff->attachChangesets($changesets);

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -1284,15 +1284,13 @@ final class DifferentialTransactionEditor
       $changeset_ids[$id] = $id;
     }
 
-    // TODO: We should write a proper Query class for this eventually.
-    $changesets = id(new DifferentialChangeset())->loadAllWhere(
-      'id IN (%Ld)',
-      $changeset_ids);
     if ($show_context) {
       $hunk_parser = new DifferentialHunkParser();
-      foreach ($changesets as $changeset) {
-        $changeset->attachHunks($changeset->loadHunks());
-      }
+      $changesets = id(new DifferentialChangesetQuery())
+        ->setViewer($this->getActor())
+        ->withIDs($changeset_ids)
+        ->needHunks(true)
+        ->execute();
     }
 
     $inline_groups = DifferentialTransactionComment::sortAndGroupInlines(

src/applications/differential/query/DifferentialChangesetQuery.php

@@ -0,0 +1,158 @@
+<?php
+
+final class DifferentialChangesetQuery
+  extends PhabricatorCursorPagedPolicyAwareQuery {
+
+  private $ids;
+  private $diffs;
+
+  private $needAttachToDiffs;
+  private $needHunks;
+
+  public function withIDs(array $ids) {
+    $this->ids = $ids;
+    return $this;
+  }
+
+  public function withDiffs(array $diffs) {
+    assert_instances_of($diffs, 'DifferentialDiff');
+    $this->diffs = $diffs;
+    return $this;
+  }
+
+  public function needAttachToDiffs($attach) {
+    $this->needAttachToDiffs = $attach;
+    return $this;
+  }
+
+  public function needHunks($need) {
+    $this->needHunks = $need;
+    return $this;
+  }
+
+  public function willExecute() {
+    // If we fail to load any changesets (which is possible in the case of an
+    // empty commit) we'll never call didFilterPage(). Attach empty changeset
+    // lists now so that we end up with the right result.
+    if ($this->needAttachToDiffs) {
+      foreach ($this->diffs as $diff) {
+        $diff->attachChangesets(array());
+      }
+    }
+  }
+
+  public function loadPage() {
+    $table = new DifferentialChangeset();
+    $conn_r = $table->establishConnection('r');
+
+    $data = queryfx_all(
+      $conn_r,
+      'SELECT * FROM %T %Q %Q %Q',
+      $table->getTableName(),
+      $this->buildWhereClause($conn_r),
+      $this->buildOrderClause($conn_r),
+      $this->buildLimitClause($conn_r));
+
+    return $table->loadAllFromArray($data);
+  }
+
+  public function willFilterPage(array $changesets) {
+
+    // First, attach all the diffs we already have. We can just do this
+    // directly without worrying about querying for them. When we don't have
+    // a diff, record that we need to load it.
+    if ($this->diffs) {
+      $have_diffs = mpull($this->diffs, null, 'getID');
+    } else {
+      $have_diffs = array();
+    }
+
+    $must_load = array();
+    foreach ($changesets as $key => $changeset) {
+      $diff_id = $changeset->getDiffID();
+      if (isset($have_diffs[$diff_id])) {
+        $changeset->attachDiff($have_diffs[$diff_id]);
+      } else {
+        $must_load[$key] = $changeset;
+      }
+    }
+
+    // Load all the diffs we don't have.
+    $need_diff_ids = mpull($must_load, 'getDiffID');
+    $more_diffs = array();
+    if ($need_diff_ids) {
+      $more_diffs = id(new DifferentialDiffQuery())
+        ->setViewer($this->getViewer())
+        ->setParentQuery($this)
+        ->withIDs($need_diff_ids)
+        ->execute();
+      $more_diffs = mpull($more_diffs, null, 'getID');
+    }
+
+    // Attach the diffs we loaded.
+    foreach ($must_load as $key => $changeset) {
+      $diff_id = $changeset->getDiffID();
+      if (isset($more_diffs[$diff_id])) {
+        $changeset->attachDiff($more_diffs[$diff_id]);
+      } else {
+        // We didn't have the diff, and could not load it (it does not exist,
+        // or we can't see it), so filter this result out.
+        unset($changesets[$key]);
+      }
+    }
+
+    return $changesets;
+  }
+
+  public function didFilterPage(array $changesets) {
+    if ($this->needAttachToDiffs) {
+      $changeset_groups = mgroup($changesets, 'getDiffID');
+      foreach ($this->diffs as $diff) {
+        $diff_changesets = idx($changeset_groups, $diff->getID(), array());
+        $diff->attachChangesets($diff_changesets);
+      }
+    }
+
+    if ($this->needHunks) {
+      id(new DifferentialHunkQuery())
+        ->setViewer($this->getViewer())
+        ->setParentQuery($this)
+        ->withChangesets($changesets)
+        ->needAttachToChangesets(true)
+        ->execute();
+    }
+
+    return $changesets;
+  }
+
+  private function buildWhereClause(AphrontDatabaseConnection $conn_r) {
+    $where = array();
+
+    if ($this->diffs !== null) {
+      $where[] = qsprintf(
+        $conn_r,
+        'diffID IN (%Ld)',
+        mpull($this->diffs, 'getID'));
+    }
+
+    if ($this->ids !== null) {
+      $where[] = qsprintf(
+        $conn_r,
+        'id IN (%Ld)',
+        $this->ids);
+    }
+
+    $where[] = $this->buildPagingClause($conn_r);
+
+    return $this->formatWhereClause($where);
+  }
+
+  public function getQueryApplicationClass() {
+    return 'PhabricatorApplicationDifferential';
+  }
+
+  protected function getReversePaging() {
+    return true;
+  }
+
+}

src/applications/differential/query/DifferentialDiffQuery.php

@@ -88,26 +88,13 @@ final class DifferentialDiffQuery
   }
 
   private function loadChangesets(array $diffs) {
-    $diff_ids = mpull($diffs, 'getID');
-
-    $changesets = id(new DifferentialChangeset())->loadAllWhere(
-      'diffID IN (%Ld)',
-      $diff_ids);
-
-    if ($changesets) {
-      id(new DifferentialHunkQuery())
-        ->setViewer($this->getViewer())
-        ->setParentQuery($this)
-        ->withChangesets($changesets)
-        ->needAttachToChangesets(true)
-        ->execute();
-    }
-
-    $changeset_groups = mgroup($changesets, 'getDiffID');
-    foreach ($diffs as $diff) {
-      $diff_changesets = idx($changeset_groups, $diff->getID(), array());
-      $diff->attachChangesets($diff_changesets);
-    }
+    id(new DifferentialChangesetQuery())
+      ->setViewer($this->getViewer())
+      ->setParentQuery($this)
+      ->withDiffs($diffs)
+      ->needAttachToDiffs(true)
+      ->needHunks(true)
+      ->execute();
 
     return $diffs;
   }

src/applications/differential/storage/DifferentialChangeset.php

@@ -62,15 +62,6 @@ final class DifferentialChangeset extends DifferentialDAO
     return $this;
   }
 
-  public function loadHunks() {
-    if (!$this->getID()) {
-      return array();
-    }
-    return id(new DifferentialHunk())->loadAllWhere(
-      'changesetID = %d',
-      $this->getID());
-  }
-
   public function save() {
     $this->openTransaction();
       $ret = parent::save();
@@ -84,9 +75,14 @@ final class DifferentialChangeset extends DifferentialDAO
 
   public function delete() {
     $this->openTransaction();
-      foreach ($this->loadHunks() as $hunk) {
+
+      $hunks = id(new DifferentialHunk())->loadAllWhere(
+        'changesetID = %d',
+        $this->getID());
+      foreach ($hunks as $hunk) {
         $hunk->delete();
       }
+
       $this->unsavedHunks = array();
 
       queryfx(
@@ -174,6 +170,15 @@ final class DifferentialChangeset extends DifferentialDAO
     return false;
   }
 
+  public function attachDiff(DifferentialDiff $diff) {
+    $this->diff = $diff;
+    return $this;
+  }
+
+  public function getDiff() {
+    return $this->assertAttached($this->diff);
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
@@ -185,13 +190,11 @@ final class DifferentialChangeset extends DifferentialDAO
   }
 
   public function getPolicy($capability) {
-    // TODO: For now, these are never queried directly through the policy
-    // framework. Fix that up.
-    return PhabricatorPolicies::getMostOpenPolicy();
+    return $this->getDiff()->getPolicy($capability);
   }
 
   public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
-    return false;
+    return $this->getDiff()->hasAutomaticCapability($capability, $viewer);
   }
 
   public function describeAutomaticCapability($capability) {