Improve handle semantics with HandlePool / HandleList

Summary:
Ref T7689, which discusses some of the motivation here. Briefly, these methods are awkward:

  - Controller->loadHandles()
  - Controller->loadViewerHandles()
  - Controller->renderHandlesForPHIDs()

This moves us toward better semantics, less awkwardness, and a more reasonable attack on T7688 which won't double-fetch a bunch of data.

Test Plan:
  - Added unit tests.
  - Converted one controller to the new stuff.
    - Viewed countdown lists, saw handles render.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7689

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

src/__phutil_library_map__.php

@@ -1873,7 +1873,10 @@ phutil_register_library_map(array(
     'PhabricatorGlobalLock' => 'infrastructure/util/PhabricatorGlobalLock.php',
     'PhabricatorGlobalUploadTargetView' => 'applications/files/view/PhabricatorGlobalUploadTargetView.php',
     'PhabricatorGoogleAuthProvider' => 'applications/auth/provider/PhabricatorGoogleAuthProvider.php',
+    'PhabricatorHandleList' => 'applications/phid/handle/pool/PhabricatorHandleList.php',
     'PhabricatorHandleObjectSelectorDataView' => 'applications/phid/handle/view/PhabricatorHandleObjectSelectorDataView.php',
+    'PhabricatorHandlePool' => 'applications/phid/handle/pool/PhabricatorHandlePool.php',
+    'PhabricatorHandlePoolTestCase' => 'applications/phid/handle/pool/__tests__/PhabricatorHandlePoolTestCase.php',
     'PhabricatorHandleQuery' => 'applications/phid/query/PhabricatorHandleQuery.php',
     'PhabricatorHarbormasterApplication' => 'applications/harbormaster/application/PhabricatorHarbormasterApplication.php',
     'PhabricatorHarbormasterConfigOptions' => 'applications/harbormaster/config/PhabricatorHarbormasterConfigOptions.php',
@@ -5195,6 +5198,14 @@ phutil_register_library_map(array(
     'PhabricatorGlobalLock' => 'PhutilLock',
     'PhabricatorGlobalUploadTargetView' => 'AphrontView',
     'PhabricatorGoogleAuthProvider' => 'PhabricatorOAuth2AuthProvider',
+    'PhabricatorHandleList' => array(
+      'Phobject',
+      'Iterator',
+      'ArrayAccess',
+      'Countable',
+    ),
+    'PhabricatorHandlePool' => 'Phobject',
+    'PhabricatorHandlePoolTestCase' => 'PhabricatorTestCase',
     'PhabricatorHandleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorHarbormasterApplication' => 'PhabricatorApplication',
     'PhabricatorHarbormasterConfigOptions' => 'PhabricatorApplicationConfigOptions',

src/applications/countdown/controller/PhabricatorCountdownViewController.php

@@ -100,10 +100,8 @@ final class PhabricatorCountdownViewController
     PhabricatorCountdown $countdown,
     PhabricatorActionListView $actions) {
 
-    $request = $this->getRequest();
+    $viewer = $this->getViewer();
-    $viewer = $request->getUser();
+    $handles = $viewer->loadHandles(array($countdown->getAuthorPHID()));
-
-    $this->loadHandles(array($countdown->getAuthorPHID()));
 
     $view = id(new PHUIPropertyListView())
       ->setUser($viewer)
@@ -111,7 +109,7 @@ final class PhabricatorCountdownViewController
 
     $view->addProperty(
       pht('Author'),
-      $this->getHandle($countdown->getAuthorPHID())->renderLink());
+      $handles[$countdown->getAuthorPHID()]->renderLink());
 
     return $view;
   }

src/applications/people/storage/PhabricatorUser.php

@@ -51,6 +51,7 @@ final class PhabricatorUser
   private $session = self::ATTACHABLE;
 
   private $authorities = array();
+  private $handlePool;
 
   protected function readField($field) {
     switch ($field) {
@@ -800,6 +801,26 @@ EOBODY;
   }
 
 
+/* -(  Handles  )------------------------------------------------------------ */
+
+
+  /**
+   * Get a @{class:PhabricatorHandleList} which benefits from this viewer's
+   * internal handle pool.
+   *
+   * @param list<phid> List of PHIDs to load.
+   * @return PhabricatorHandleList Handle list object.
+   */
+  public function loadHandles(array $phids) {
+    if ($this->handlePool === null) {
+      $this->handlePool = id(new PhabricatorHandlePool())
+        ->setViewer($this);
+    }
+
+    return $this->handlePool->newHandleList($phids);
+  }
+
+
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
 

src/applications/phid/handle/pool/PhabricatorHandleList.php

@@ -0,0 +1,124 @@
+<?php
+
+/**
+ * A list of object handles.
+ *
+ * This is a convenience class which behaves like an array but makes working
+ * with handles more convenient, improves their caching and batching semantics,
+ * and provides some utility behavior.
+ *
+ * Load a handle list by calling `loadHandles()` on a `$viewer`:
+ *
+ *   $handles = $viewer->loadHandles($phids);
+ *
+ * This creates a handle list object, which behaves like an array of handles.
+ * However, it benefits from the viewer's internal handle cache and performs
+ * just-in-time bulk loading.
+ */
+final class PhabricatorHandleList
+  extends Phobject
+  implements
+    Iterator,
+    ArrayAccess,
+    Countable {
+
+  private $handlePool;
+  private $phids;
+  private $handles;
+  private $cursor;
+
+  public function setHandlePool(PhabricatorHandlePool $pool) {
+    $this->handlePool = $pool;
+    return $this;
+  }
+
+  public function setPHIDs(array $phids) {
+    $this->phids = $phids;
+    return $this;
+  }
+
+  private function loadHandles() {
+    $this->handles = $this->handlePool->loadPHIDs($this->phids);
+  }
+
+  private function getHandle($phid) {
+    if ($this->handles === null) {
+      $this->loadHandles();
+    }
+
+    if (empty($this->handles[$phid])) {
+      throw new Exception(
+        pht(
+          'Requested handle "%s" was not loaded.',
+          $phid));
+    }
+
+    return $this->handles[$phid];
+  }
+
+
+/* -(  Iterator  )----------------------------------------------------------- */
+
+
+  public function rewind() {
+    $this->cursor = 0;
+  }
+
+  public function current() {
+    return $this->getHandle($this->phids[$this->cursor]);
+  }
+
+  public function key() {
+    return $this->phids[$this->cursor];
+  }
+
+  public function next() {
+    ++$this->cursor;
+  }
+
+  public function valid() {
+    return isset($this->phids[$this->cursor]);
+  }
+
+
+/* -(  ArrayAccess  )-------------------------------------------------------- */
+
+
+  public function offsetExists($offset) {
+    if ($this->handles === null) {
+      $this->loadHandles();
+    }
+    return isset($this->handles[$offset]);
+  }
+
+  public function offsetGet($offset) {
+    if ($this->handles === null) {
+      $this->loadHandles();
+    }
+    return $this->handles[$offset];
+  }
+
+  public function offsetSet($offset, $value) {
+    $this->raiseImmutableException();
+  }
+
+  public function offsetUnset($offset) {
+    $this->raiseImmutableException();
+  }
+
+  private function raiseImmutableException() {
+    throw new Exception(
+      pht(
+        'Trying to mutate a PhabricatorHandleList, but this is not permitted; '.
+        'handle lists are immutable.'));
+  }
+
+
+/* -(  Countable  )---------------------------------------------------------- */
+
+
+  public function count() {
+    return count($this->phids);
+  }
+
+}

src/applications/phid/handle/pool/PhabricatorHandlePool.php

@@ -0,0 +1,75 @@
+<?php
+
+/**
+ * Coordinates loading object handles.
+ *
+ * This is a low-level piece of plumbing which code will not normally interact
+ * with directly. For discussion of the handle pool mechanism, see
+ * @{class:PhabricatorHandleList}.
+ */
+final class PhabricatorHandlePool extends Phobject {
+
+  private $viewer;
+  private $handles = array();
+  private $unloadedPHIDs = array();
+
+  public function setViewer(PhabricatorUser $user) {
+    $this->viewer = $user;
+    return $this;
+  }
+
+  public function getViewer() {
+    return $this->viewer;
+  }
+
+  public function newHandleList(array $phids) {
+    // Mark any PHIDs we haven't loaded yet as unloaded. This will let us bulk
+    // load them later.
+    foreach ($phids as $phid) {
+      if (empty($this->handles[$phid])) {
+        $this->unloadedPHIDs[$phid] = true;
+      }
+    }
+
+    $unique = array();
+    foreach ($phids as $phid) {
+      $unique[$phid] = $phid;
+    }
+
+    return id(new PhabricatorHandleList())
+      ->setHandlePool($this)
+      ->setPHIDs(array_values($unique));
+  }
+
+  public function loadPHIDs(array $phids) {
+    $need = array();
+    foreach ($phids as $phid) {
+      if (empty($this->handles[$phid])) {
+        $need[$phid] = true;
+      }
+    }
+
+    foreach ($need as $phid => $ignored) {
+      if (empty($this->unloadedPHIDs[$phid])) {
+        throw new Exception(
+          pht(
+            'Attempting to load PHID "%s", but it was not requested by any '.
+            'handle list.',
+            $phid));
+      }
+    }
+
+    // If we need any handles, bulk load everything in the queue.
+    if ($need) {
+      $handles = id(new PhabricatorHandleQuery())
+        ->setViewer($this->getViewer())
+        ->withPHIDs(array_keys($this->unloadedPHIDs))
+        ->execute();
+      $this->handles += $handles;
+      $this->unloadedPHIDs = array();
+    }
+
+    return array_select_keys($this->handles, $phids);
+  }
+
+}

src/applications/phid/handle/pool/__tests__/PhabricatorHandlePoolTestCase.php

@@ -0,0 +1,58 @@
+<?php
+
+final class PhabricatorHandlePoolTestCase extends PhabricatorTestCase {
+
+  protected function getPhabricatorTestCaseConfiguration() {
+    return array(
+      self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES => true,
+    );
+  }
+
+  public function testHandlePools() {
+    // A lot of the batch/just-in-time/cache behavior of handle pools is not
+    // observable by design, so these tests don't directly cover it.
+
+    $viewer = $this->generateNewTestUser();
+    $viewer_phid = $viewer->getPHID();
+
+    $phids = array($viewer_phid);
+
+    $handles = $viewer->loadHandles($phids);
+
+    // The handle load hasn't happened yet, but we can't directly observe that.
+
+    // Test Countable behaviors.
+    $this->assertEqual(1, count($handles));
+
+    // Test ArrayAccess behaviors.
+    $this->assertEqual(
+      array($viewer_phid),
+      array_keys(iterator_to_array($handles)));
+    $this->assertEqual(true, $handles[$viewer_phid]->isComplete());
+    $this->assertEqual($viewer_phid, $handles[$viewer_phid]->getPHID());
+    $this->assertTrue(isset($handles[$viewer_phid]));
+    $this->assertFalse(isset($handles['quack']));
+
+    // Test Iterator behaviors.
+    foreach ($handles as $key => $handle) {
+      $this->assertEqual($viewer_phid, $key);
+      $this->assertEqual($viewer_phid, $handle->getPHID());
+    }
+
+    // Do this twice to make sure the handle list is rewindable.
+    foreach ($handles as $key => $handle) {
+      $this->assertEqual($viewer_phid, $key);
+      $this->assertEqual($viewer_phid, $handle->getPHID());
+    }
+
+    $more_handles = $viewer->loadHandles($phids);
+
+    // This is testing that we got back a reference to the exact same object,
+    // which implies the caching behavior is working correctly.
+    $this->assertEqual(
+      $handles[$viewer_phid],
+      $more_handles[$viewer_phid],
+      pht('Handles should use viewer handle pool cache.'));
+  }
+
+}