Make diffusion.filecontentquery return file PHIDs instead of raw content

Summary:
Fixes T9319. Proxied requests (e.g., in the cluster) for binary files (like images) currently fail because we can not return binary data over Conduit in JSON.

Although Conduit will eventually support binary-safe encodings, a cleaner approach to this is just to return a `filePHID` instead of the raw content. This is generally faster and more flexible, and gives us more opportunities to add caching later.

After making the call, the client pulls the file data separately.

We also no longer need to return a complex data structure because we don't do blame over this call any longer.

Test Plan:
  - Viewed images in Diffusion.
  - Viewed READMEs in Diffusion.
  - Used `bin/differential attach-commit rX Dy` to hit attach pathway.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9319

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

src/__phutil_library_map__.php

@@ -598,7 +598,6 @@ phutil_register_library_map(array(
     'DiffusionExternalController' => 'applications/diffusion/controller/DiffusionExternalController.php',
     'DiffusionExternalSymbolQuery' => 'applications/diffusion/symbol/DiffusionExternalSymbolQuery.php',
     'DiffusionExternalSymbolsSource' => 'applications/diffusion/symbol/DiffusionExternalSymbolsSource.php',
-    'DiffusionFileContent' => 'applications/diffusion/data/DiffusionFileContent.php',
     'DiffusionFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionFileContentQuery.php',
     'DiffusionFileContentQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php',
     'DiffusionFindSymbolsConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionFindSymbolsConduitAPIMethod.php',
@@ -4571,7 +4570,6 @@ phutil_register_library_map(array(
     'DiffusionExternalController' => 'DiffusionController',
     'DiffusionExternalSymbolQuery' => 'Phobject',
     'DiffusionExternalSymbolsSource' => 'Phobject',
-    'DiffusionFileContent' => 'Phobject',
     'DiffusionFileContentQuery' => 'DiffusionQuery',
     'DiffusionFileContentQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod',
     'DiffusionFindSymbolsConduitAPIMethod' => 'DiffusionConduitAPIMethod',

src/applications/differential/engine/DifferentialDiffExtractionEngine.php

@@ -156,9 +156,21 @@ final class DifferentialDiffExtractionEngine extends Phobject {
             'commit' => $identifier,
             'path' => $path,
           ));
-        $corpus = $response['corpus'];
 
-        if ($files[$file_phid]->loadFileData() != $corpus) {
+        $new_file_phid = $response['filePHID'];
+        if (!$new_file_phid) {
+          return true;
+        }
+
+        $new_file = id(new PhabricatorFileQuery())
+          ->setViewer($viewer)
+          ->withPHIDs(array($new_file_phid))
+          ->executeOne();
+        if (!$new_file) {
+          return true;
+        }
+
+        if ($files[$file_phid]->loadFileData() != $new_file->loadFileData()) {
           return true;
         }
       } else {

src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php

@@ -27,8 +27,7 @@ final class DiffusionFileContentQueryConduitAPIMethod
   protected function getResult(ConduitAPIRequest $request) {
     $drequest = $this->getDiffusionRequest();
 
-    $file_query = DiffusionFileContentQuery::newFromDiffusionRequest($drequest)
+    $file_query = DiffusionFileContentQuery::newFromDiffusionRequest($drequest);
-      ->setViewer($request->getUser());
 
     $timeout = $request->getValue('timeout');
     if ($timeout) {
@@ -40,16 +39,44 @@ final class DiffusionFileContentQueryConduitAPIMethod
       $file_query->setByteLimit($byte_limit);
     }
 
-    $file_content = $file_query->loadFileContent();
+    $content = $file_query->execute();
 
-    $text_list = $rev_list = $blame_dict = array();
+    $too_slow = (bool)$file_query->getExceededTimeLimit();
+    $too_huge = (bool)$file_query->getExceededByteLimit();
 
-    $file_content
+    $file_phid = null;
-      ->setBlameDict($blame_dict)
+    if (!$too_slow && !$too_huge) {
-      ->setRevList($rev_list)
+      $file = $this->newFile($drequest, $content);
-      ->setTextList($text_list);
+      $file_phid = $file->getPHID();
+    }
 
-    return $file_content->toDictionary();
+    return array(
+      'tooSlow' => $too_slow,
+      'tooHuge' => $too_huge,
+      'filePHID' => $file_phid,
+    );
+  }
+
+  private function newFile(DiffusionRequest $drequest, $content) {
+    $path = $drequest->getPath();
+    $name = basename($path);
+
+    $repository = $drequest->getRepository();
+    $repository_phid = $repository->getPHID();
+
+    $file = PhabricatorFile::buildFromFileDataOrHash(
+      $content,
+      array(
+        'name' => $name,
+        'ttl' => time() + phutil_units('48 hours in seconds'),
+        'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
+      ));
+
+    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+      $file->attachToObject($repository_phid);
+    unset($unguarded);
+
+    return $file;
   }
 
 }

src/applications/diffusion/controller/DiffusionBrowseController.php

@@ -153,27 +153,41 @@ final class DiffusionBrowseController extends DiffusionController {
       );
     }
 
-    $file_content = DiffusionFileContent::newFromConduit(
+    $response = $this->callConduitWithDiffusionRequest(
-      $this->callConduitWithDiffusionRequest(
       'diffusion.filecontentquery',
-        $params));
+      $params);
-    $data = $file_content->getCorpus();
 
-    if ($view === 'raw') {
+    $hit_byte_limit = $response['tooHuge'];
-      return $this->buildRawResponse($path, $data);
+    $hit_time_limit = $response['tooSlow'];
-    }
 
-    $this->loadLintMessages();
+    $file_phid = $response['filePHID'];
-    $this->coverage = $drequest->loadCoverage();
+    if ($hit_byte_limit) {
-
-    if ($byte_limit && (strlen($data) == $byte_limit)) {
       $corpus = $this->buildErrorCorpus(
         pht(
           'This file is larger than %s byte(s), and too large to display '.
           'in the web UI.',
-          $byte_limit));
+          phutil_format_bytes($byte_limit)));
-    } else if (ArcanistDiffUtils::isHeuristicBinaryFile($data)) {
+    } else if ($hit_time_limit) {
-      $file = $this->loadFileForData($path, $data);
+      $corpus = $this->buildErrorCorpus(
+        pht(
+          'This file took too long to load from the repository (more than '.
+          '%s second(s)).',
+          new PhutilNumber($time_limit)));
+    } else {
+      $file = id(new PhabricatorFileQuery())
+        ->setViewer($viewer)
+        ->withPHIDs(array($file_phid))
+        ->executeOne();
+      if (!$file) {
+        throw new Exception(pht('Failed to load content file!'));
+      }
+
+      if ($view === 'raw') {
+        return $file->getRedirectResponse();
+      }
+
+      $data = $file->loadFileData();
+      if (ArcanistDiffUtils::isHeuristicBinaryFile($data)) {
         $file_uri = $file->getBestURI();
 
         if ($file->isViewableImage()) {
@@ -182,16 +196,20 @@ final class DiffusionBrowseController extends DiffusionController {
           $corpus = $this->buildBinaryCorpus($file_uri, $data);
         }
       } else {
+        $this->loadLintMessages();
+        $this->coverage = $drequest->loadCoverage();
+
         // Build the content of the file.
         $corpus = $this->buildCorpus(
           $show_blame,
           $show_color,
-        $file_content,
+          $data,
           $needs_blame,
           $drequest,
           $path,
           $data);
       }
+    }
 
     if ($request->isAjax()) {
       return id(new AphrontAjaxResponse())->setContent($corpus);
@@ -327,23 +345,7 @@ final class DiffusionBrowseController extends DiffusionController {
     }
 
     $content[] = $this->buildOpenRevisions();
-
+    $content[] = $this->renderDirectoryReadme($results);
-
-    $readme_path = $results->getReadmePath();
-    if ($readme_path) {
-      $readme_content = $this->callConduitWithDiffusionRequest(
-        'diffusion.filecontentquery',
-        array(
-          'path' => $readme_path,
-          'commit' => $drequest->getStableCommit(),
-        ));
-      if ($readme_content) {
-        $content[] = id(new DiffusionReadmeView())
-          ->setUser($this->getViewer())
-          ->setPath($readme_path)
-          ->setContent($readme_content['corpus']);
-      }
-    }
 
     $crumbs = $this->buildCrumbs(
       array(
@@ -584,7 +586,7 @@ final class DiffusionBrowseController extends DiffusionController {
   private function buildCorpus(
     $show_blame,
     $show_color,
-    DiffusionFileContent $file_content,
+    $file_corpus,
     $needs_blame,
     DiffusionRequest $drequest,
     $path,
@@ -594,7 +596,6 @@ final class DiffusionBrowseController extends DiffusionController {
     $blame_timeout = 15;
     $blame_failed = false;
 
-    $file_corpus = $file_content->getCorpus();
     $highlight_limit = DifferentialChangesetParser::HIGHLIGHT_BYTE_LIMIT;
     $blame_limit = DifferentialChangesetParser::HIGHLIGHT_BYTE_LIMIT;
     $can_highlight = (strlen($file_corpus) <= $highlight_limit);
@@ -1256,28 +1257,6 @@ final class DiffusionBrowseController extends DiffusionController {
     return $rows;
   }
 
-  private function loadFileForData($path, $data) {
-    $file = PhabricatorFile::buildFromFileDataOrHash(
-      $data,
-      array(
-        'name' => basename($path),
-        'ttl' => time() + 60 * 60 * 24,
-        'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
-      ));
-
-    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-      $file->attachToObject(
-        $this->getDiffusionRequest()->getRepository()->getPHID());
-    unset($unguarded);
-
-    return $file;
-  }
-
-  private function buildRawResponse($path, $data) {
-    $file = $this->loadFileForData($path, $data);
-    return $file->getRedirectResponse();
-  }
-
   private function buildImageCorpus($file_uri) {
     $properties = new PHUIPropertyListView();
 
@@ -1299,7 +1278,6 @@ final class DiffusionBrowseController extends DiffusionController {
   }
 
   private function buildBinaryCorpus($file_uri, $data) {
-
     $size = new PhutilNumber(strlen($data));
     $text = pht('This is a binary file. It is %s byte(s) in length.', $size);
     $text = id(new PHUIBoxView())

src/applications/diffusion/controller/DiffusionController.php

@@ -287,4 +287,49 @@ abstract class DiffusionController extends PhabricatorController {
       ->appendChild($pager);
   }
 
+  protected function renderDirectoryReadme(DiffusionBrowseResultSet $browse) {
+    $readme_path = $browse->getReadmePath();
+    if ($readme_path === null) {
+      return null;
+    }
+
+    $drequest = $this->getDiffusionRequest();
+    $viewer = $this->getViewer();
+
+    try {
+      $result = $this->callConduitWithDiffusionRequest(
+        'diffusion.filecontentquery',
+        array(
+          'path' => $readme_path,
+          'commit' => $drequest->getStableCommit(),
+        ));
+    } catch (Exception $ex) {
+      return null;
+    }
+
+    $file_phid = $result['filePHID'];
+    if (!$file_phid) {
+      return null;
+    }
+
+    $file = id(new PhabricatorFileQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($file_phid))
+      ->executeOne();
+    if (!$file) {
+      return null;
+    }
+
+    $corpus = $file->loadFileData();
+
+    if (!strlen($corpus)) {
+      return null;
+    }
+
+    return id(new DiffusionReadmeView())
+      ->setUser($this->getViewer())
+      ->setPath($readme_path)
+      ->setContent($corpus);
+  }
+
 }

src/applications/diffusion/controller/DiffusionRepositoryController.php

@@ -161,23 +161,10 @@ final class DiffusionRepositoryController extends DiffusionController {
     $phids = array_keys($phids);
     $handles = $this->loadViewerHandles($phids);
 
-    $readme = null;
     if ($browse_results) {
-      $readme_path = $browse_results->getReadmePath();
+      $readme = $this->renderDirectoryReadme($browse_results);
-      if ($readme_path) {
+    } else {
-        $readme_content = $this->callConduitWithDiffusionRequest(
+      $readme = null;
-          'diffusion.filecontentquery',
-          array(
-            'path' => $readme_path,
-            'commit' => $drequest->getStableCommit(),
-          ));
-        if ($readme_content) {
-          $readme = id(new DiffusionReadmeView())
-            ->setUser($this->getViewer())
-            ->setPath($readme_path)
-            ->setContent($readme_content['corpus']);
-        }
-      }
     }
 
     $content[] = $this->buildBrowseTable(

src/applications/diffusion/data/DiffusionFileContent.php

@@ -1,63 +0,0 @@
-<?php
-
-final class DiffusionFileContent extends Phobject {
-
-  private $corpus;
-  private $blameDict;
-  private $revList;
-  private $textList;
-
-  public function setTextList(array $text_list) {
-    $this->textList = $text_list;
-    return $this;
-  }
-  public function getTextList() {
-    if (!$this->textList) {
-      return phutil_split_lines($this->getCorpus(), $retain_ends = false);
-    }
-    return $this->textList;
-  }
-
-  public function setRevList(array $rev_list) {
-    $this->revList = $rev_list;
-    return $this;
-  }
-  public function getRevList() {
-    return $this->revList;
-  }
-
-  public function setBlameDict(array $blame_dict) {
-    $this->blameDict = $blame_dict;
-    return $this;
-  }
-  public function getBlameDict() {
-    return $this->blameDict;
-  }
-
-  public function setCorpus($corpus) {
-    $this->corpus = $corpus;
-    return $this;
-  }
-
-  public function getCorpus() {
-    return $this->corpus;
-  }
-
-  public function toDictionary() {
-    return array(
-      'corpus' => $this->getCorpus(),
-      'blameDict' => $this->getBlameDict(),
-      'revList' => $this->getRevList(),
-      'textList' => $this->getTextList(),
-    );
-  }
-
-  public static function newFromConduit(array $dict) {
-    return id(new DiffusionFileContent())
-      ->setCorpus($dict['corpus'])
-      ->setBlameDict($dict['blameDict'])
-      ->setRevList($dict['revList'])
-      ->setTextList($dict['textList']);
-  }
-
-}

src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php

@@ -1,18 +1,13 @@
 <?php
 
-/**
- * NOTE: this class should only be used where local access to the repository
- * is guaranteed and NOT from within the Diffusion application. Diffusion
- * should use Conduit method 'diffusion.filecontentquery' to get this sort
- * of data.
- */
 abstract class DiffusionFileContentQuery extends DiffusionQuery {
 
-  private $fileContent;
-  private $viewer;
   private $timeout;
   private $byteLimit;
 
+  private $didHitByteLimit = false;
+  private $didHitTimeLimit = false;
+
   public function setTimeout($timeout) {
     $this->timeout = $timeout;
     return $this;
@@ -31,71 +26,51 @@ abstract class DiffusionFileContentQuery extends DiffusionQuery {
     return $this->byteLimit;
   }
 
-  public function setViewer(PhabricatorUser $user) {
-    $this->viewer = $user;
-    return $this;
-  }
-
-  public function getViewer() {
-    return $this->viewer;
-  }
-
   final public static function newFromDiffusionRequest(
     DiffusionRequest $request) {
     return parent::newQueryObject(__CLASS__, $request);
   }
 
-  abstract public function getFileContentFuture();
+  final public function getExceededByteLimit() {
-  abstract protected function executeQueryFromFuture(Future $future);
+    return $this->didHitByteLimit;
-
-  final public function loadFileContentFromFuture(Future $future) {
-
-    if ($this->timeout) {
-      $future->setTimeout($this->timeout);
   }
 
-    if ($this->getByteLimit()) {
+  final public function getExceededTimeLimit() {
-      $future->setStdoutSizeLimit($this->getByteLimit());
+    return $this->didHitTimeLimit;
+  }
+
+  abstract protected function getFileContentFuture();
+  abstract protected function resolveFileContentFuture(Future $future);
+
+  final protected function executeQuery() {
+    $future = $this->getFileContentFuture();
+
+    if ($this->getTimeout()) {
+      $future->setTimeout($this->getTimeout());
+    }
+
+    $byte_limit = $this->getByteLimit();
+    if ($byte_limit) {
+      $future->setStdoutSizeLimit($byte_limit + 1);
     }
 
     try {
-      $file_content = $this->executeQueryFromFuture($future);
+      $file_content = $this->resolveFileContentFuture($future);
     } catch (CommandException $ex) {
       if (!$future->getWasKilledByTimeout()) {
         throw $ex;
       }
 
-      $message = pht(
+      $this->didHitTimeLimit = true;
-        '<Attempt to load this file was terminated after %s second(s).>',
+      $file_content = null;
-        $this->timeout);
-
-      $file_content = new DiffusionFileContent();
-      $file_content->setCorpus($message);
     }
 
-    $this->fileContent = $file_content;
+    if ($byte_limit && (strlen($file_content) > $byte_limit)) {
-
+      $this->didHitByteLimit = true;
-    $repository = $this->getRequest()->getRepository();
+      $file_content = null;
-    $try_encoding = $repository->getDetail('encoding');
-    if ($try_encoding) {
-        $this->fileContent->setCorpus(
-          phutil_utf8_convert(
-            $this->fileContent->getCorpus(), 'UTF-8', $try_encoding));
     }
 
-    return $this->fileContent;
+    return $file_content;
-  }
-
-  final protected function executeQuery() {
-    return $this->loadFileContentFromFuture($this->getFileContentFuture());
-  }
-
-  final public function loadFileContent() {
-    return $this->executeQuery();
-  }
-
-  final public function getRawData() {
-    return $this->fileContent->getCorpus();
   }
 
 }

src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php

@@ -2,7 +2,7 @@
 
 final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery {
 
-  public function getFileContentFuture() {
+  protected function getFileContentFuture() {
     $drequest = $this->getRequest();
 
     $repository = $drequest->getRepository();
@@ -15,13 +15,9 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery {
       $path);
   }
 
-  protected function executeQueryFromFuture(Future $future) {
+  protected function resolveFileContentFuture(Future $future) {
     list($corpus) = $future->resolvex();
-
+    return $corpus;
-    $file_content = new DiffusionFileContent();
-    $file_content->setCorpus($corpus);
-
-    return $file_content;
   }
 
 }

src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php

@@ -3,7 +3,7 @@
 final class DiffusionMercurialFileContentQuery
   extends DiffusionFileContentQuery {
 
-  public function getFileContentFuture() {
+  protected function getFileContentFuture() {
     $drequest = $this->getRequest();
 
     $repository = $drequest->getRepository();
@@ -16,13 +16,9 @@ final class DiffusionMercurialFileContentQuery
       $path);
   }
 
-  protected function executeQueryFromFuture(Future $future) {
+  protected function resolveFileContentFuture(Future $future) {
     list($corpus) = $future->resolvex();
-
+    return $corpus;
-    $file_content = new DiffusionFileContent();
-    $file_content->setCorpus($corpus);
-
-    return $file_content;
   }
 
 }

src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php

@@ -2,7 +2,7 @@
 
 final class DiffusionSvnFileContentQuery extends DiffusionFileContentQuery {
 
-  public function getFileContentFuture() {
+  protected function getFileContentFuture() {
     $drequest = $this->getRequest();
 
     $repository = $drequest->getRepository();
@@ -14,30 +14,9 @@ final class DiffusionSvnFileContentQuery extends DiffusionFileContentQuery {
       $repository->getSubversionPathURI($path, $commit));
   }
 
-  protected function executeQueryFromFuture(Future $future) {
+  protected function resolveFileContentFuture(Future $future) {
-    try {
     list($corpus) = $future->resolvex();
-    } catch (CommandException $ex) {
+    return $corpus;
-      $stderr = $ex->getStdErr();
-      if (preg_match('/path not found$/', trim($stderr))) {
-        // TODO: Improve user experience for this. One way to end up here
-        // is to have the parser behind and look at a file which was recently
-        // nuked; Diffusion will think it still exists and try to grab content
-        // at HEAD.
-        throw new Exception(
-          pht(
-            'Failed to retrieve file content from Subversion. The file may '.
-            'have been recently deleted, or the Diffusion cache may be out of '.
-            'date.'));
-      } else {
-        throw $ex;
-      }
-    }
-
-    $file_content = new DiffusionFileContent();
-    $file_content->setCorpus($corpus);
-
-    return $file_content;
   }
 
 }