Add DifferentialHunkQuery to start hiding hunk storage details

Summary:
Ref T4045. We have a lot of direct queries against the hunk table right now. These are messy, not really policy-aware, and limit our options on T4045.

This query is unusual (it requires changesets, and does not accept IDs). This keeps us from having to load changeset -> diff -> revision in order to do policy checks. We could also fix this with smarter policy checks and caching, but I'd rather not open that can of worms for now. This object is very low level and relatively unusual, and this small deviation from convention seems like the cleanest cut to make to keep this from snowballing.

Test Plan: Used Herald dry runs to verify that the affected rules still output the same data.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4045

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

src/__phutil_library_map__.php

@@ -380,6 +380,7 @@ phutil_register_library_map(array(
     'DifferentialHunk' => 'applications/differential/storage/DifferentialHunk.php',
     'DifferentialHunkParser' => 'applications/differential/parser/DifferentialHunkParser.php',
     'DifferentialHunkParserTestCase' => 'applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php',
+    'DifferentialHunkQuery' => 'applications/differential/query/DifferentialHunkQuery.php',
     'DifferentialHunkTestCase' => 'applications/differential/storage/__tests__/DifferentialHunkTestCase.php',
     'DifferentialInlineComment' => 'applications/differential/storage/DifferentialInlineComment.php',
     'DifferentialInlineCommentEditController' => 'applications/differential/controller/DifferentialInlineCommentEditController.php',
@@ -2958,8 +2959,13 @@ phutil_register_library_map(array(
     'DifferentialGitSVNIDField' => 'DifferentialCustomField',
     'DifferentialHostField' => 'DifferentialCustomField',
     'DifferentialHovercardEventListener' => 'PhabricatorEventListener',
-    'DifferentialHunk' => 'DifferentialDAO',
+    'DifferentialHunk' =>
+    array(
+      0 => 'DifferentialDAO',
+      1 => 'PhabricatorPolicyInterface',
+    ),
     'DifferentialHunkParserTestCase' => 'PhabricatorTestCase',
+    'DifferentialHunkQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'DifferentialHunkTestCase' => 'ArcanistPhutilTestCase',
     'DifferentialInlineComment' => 'PhabricatorInlineCommentInterface',
     'DifferentialInlineCommentEditController' => 'PhabricatorInlineCommentController',

src/applications/differential/query/DifferentialHunkQuery.php

@@ -0,0 +1,87 @@
+<?php
+
+final class DifferentialHunkQuery
+  extends PhabricatorCursorPagedPolicyAwareQuery {
+
+  private $changesets;
+  private $shouldAttachToChangesets;
+
+  public function withChangesets(array $changesets) {
+    assert_instances_of($changesets, 'DifferentialChangeset');
+    $this->changesets = $changesets;
+    return $this;
+  }
+
+  public function needAttachToChangesets($attach) {
+    $this->shouldAttachToChangesets = $attach;
+    return $this;
+  }
+
+  public function loadPage() {
+    $table = new DifferentialHunk();
+    $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 $hunks) {
+
+    $changesets = mpull($this->changesets, null, 'getID');
+    foreach ($hunks as $key => $hunk) {
+      $changeset = idx($changesets, $hunk->getChangesetID());
+      if (!$changeset) {
+        unset($hunks[$key]);
+      }
+      $hunk->attachChangeset($changeset);
+    }
+
+    return $hunks;
+  }
+
+  public function didFilterPage(array $hunks) {
+    if ($this->shouldAttachToChangesets) {
+      $hunk_groups = mgroup($hunks, 'getChangesetID');
+      foreach ($this->changesets as $changeset) {
+        $hunks = idx($hunk_groups, $changeset->getID(), array());
+        $changeset->attachHunks($hunks);
+      }
+    }
+
+    return $hunks;
+  }
+
+  private function buildWhereClause(AphrontDatabaseConnection $conn_r) {
+    $where = array();
+
+    if (!$this->changesets) {
+      throw new Exception(
+        pht('You must load hunks via changesets, with withChangesets()!'));
+    }
+
+    $where[] = qsprintf(
+      $conn_r,
+      'changesetID IN (%Ld)',
+      mpull($this->changesets, 'getID'));
+
+    $where[] = $this->buildPagingClause($conn_r);
+
+    return $this->formatWhereClause($where);
+  }
+
+  public function getQueryApplicationClass() {
+    return 'PhabricatorApplicationDifferential';
+  }
+
+  protected function getReversePaging() {
+    return true;
+  }
+
+}

src/applications/differential/storage/DifferentialHunk.php

@@ -1,6 +1,7 @@
 <?php
 
-final class DifferentialHunk extends DifferentialDAO {
+final class DifferentialHunk extends DifferentialDAO
+  implements PhabricatorPolicyInterface {
 
   protected $changesetID;
   protected $changes;
@@ -9,6 +10,8 @@ final class DifferentialHunk extends DifferentialDAO {
   protected $newOffset;
   protected $newLen;
 
+  private $changeset;
+
   const FLAG_LINES_ADDED     = 1;
   const FLAG_LINES_REMOVED   = 2;
   const FLAG_LINES_STABLE    = 4;
@@ -101,4 +104,35 @@ final class DifferentialHunk extends DifferentialDAO {
     return $results;
   }
 
+  public function getChangeset() {
+    return $this->assertAttached($this->changeset);
+  }
+
+  public function attachChangeset(DifferentialChangeset $changeset) {
+    $this->changeset = $changeset;
+    return $this;
+  }
+
+
+/* -(  PhabricatorPolicyInterface  )----------------------------------------- */
+
+
+  public function getCapabilities() {
+    return array(
+      PhabricatorPolicyCapability::CAN_VIEW,
+    );
+  }
+
+  public function getPolicy($capability) {
+    return $this->getChangeset()->getPolicy($capability);
+  }
+
+  public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
+    return $this->getChangeset()->hasAutomaticCapability($capability, $viewer);
+  }
+
+  public function describeAutomaticCapability($capability) {
+    return null;
+  }
+
 }

src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php

@@ -19,6 +19,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
   protected $repository;
   protected $affectedPackages;
   protected $changesets;
+  private $haveHunks;
 
   public function getAdapterApplicationClass() {
     return 'PhabricatorApplicationDifferential';
@@ -180,6 +181,22 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
     return $this->changesets;
   }
 
+  private function loadChangesetsWithHunks() {
+    $changesets = $this->loadChangesets();
+
+    if ($changesets && !$this->haveHunks) {
+      $this->haveHunks = true;
+
+      id(new DifferentialHunkQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withChangesets($changesets)
+        ->needAttachToChangesets(true)
+        ->execute();
+    }
+
+    return $changesets;
+  }
+
   protected function loadAffectedPaths() {
     $changesets = $this->loadChangesets();
 
@@ -219,24 +236,16 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
   }
 
   private function loadContentWithMask($mask) {
-    $changesets = $this->loadChangesets();
+    $changesets = $this->loadChangesetsWithHunks();
-
-    $hunks = array();
-    if ($changesets) {
-      $hunks = id(new DifferentialHunk())->loadAllWhere(
-        'changesetID in (%Ld)',
-        mpull($changesets, 'getID'));
-    }
 
     $dict = array();
-    $hunks = mgroup($hunks, 'getChangesetID');
+    foreach ($changesets as $changeset) {
-    $changesets = mpull($changesets, null, 'getID');
-    foreach ($changesets as $id => $changeset) {
-      $path = $this->getAbsoluteRepositoryPathForChangeset($changeset);
       $content = array();
-      foreach (idx($hunks, $id, array()) as $hunk) {
+      foreach ($changeset->getHunks() as $hunk) {
         $content[] = $hunk->getContentWithMask($mask);
       }
+
+      $path = $this->getAbsoluteRepositoryPathForChangeset($changeset);
       $dict[$path] = implode("\n", $content);
     }