Extend PhabricatorPolicyQuery from PhabricatorOffsetPagedQuery

Summary:
A few goals here:

  - Slightly simplify the Query classtree -- it's now linear: `Query` -> `OffsetPagedQuery` (adds offset/limit) -> `PolicyQuery` (adds policy filtering) -> `CursorPagedPolicyQuery` (adds cursors).
  - Allow us to move from non-policy queries to policy queries without any backward compatibility breaks, e.g. Conduit methods which accept 'offset'.
  - Separate the client limit ("limit") from the datafetch hint limit ("rawresultlimit") so we can make the heurstic smarter in the future if we want. Some discussion inline.

Test Plan: Expanded unit tests to cover offset behaviors.

Reviewers: vrana, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

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

src/applications/policy/__tests__/PhabricatorPolicyTestCase.php

@@ -102,6 +102,40 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase {
   }
 
 
+  /**
+   * Test offset-based filtering.
+   */
+  public function testOffsets() {
+    $results = array(
+      $this->buildObject(PhabricatorPolicies::POLICY_NOONE),
+      $this->buildObject(PhabricatorPolicies::POLICY_NOONE),
+      $this->buildObject(PhabricatorPolicies::POLICY_NOONE),
+      $this->buildObject(PhabricatorPolicies::POLICY_USER),
+      $this->buildObject(PhabricatorPolicies::POLICY_USER),
+      $this->buildObject(PhabricatorPolicies::POLICY_USER),
+    );
+
+    $query = new PhabricatorPolicyTestQuery();
+    $query->setResults($results);
+    $query->setViewer($this->buildUser('user'));
+
+    $this->assertEqual(
+      3,
+      count($query->setLimit(3)->setOffset(0)->execute()),
+      'Invisible objects are ignored.');
+
+    $this->assertEqual(
+      0,
+      count($query->setLimit(3)->setOffset(3)->execute()),
+      'Offset pages through visible objects only.');
+
+    $this->assertEqual(
+      2,
+      count($query->setLimit(3)->setOffset(1)->execute()),
+      'Offsets work correctly.');
+  }
+
+
   /**
    * Test an object for visibility across multiple user specifications.
    */

src/applications/policy/__tests__/PhabricatorPolicyTestQuery.php

@@ -23,18 +23,30 @@ final class PhabricatorPolicyTestQuery
   extends PhabricatorPolicyQuery {
 
   private $results;
+  private $offset = 0;
 
   public function setResults(array $results) {
     $this->results = $results;
     return $this;
   }
 
+  protected function willExecute() {
+    $this->offset = 0;
+  }
+
   public function loadPage() {
-    return $this->results;
+    if ($this->getRawResultLimit()) {
+      return array_slice(
+        $this->results,
+        $this->offset,
+        $this->getRawResultLimit());
+    } else {
+      return array_slice($this->results, $this->offset);
+    }
   }
 
   public function nextPage(array $page) {
-    return null;
+    $this->offset += count($page);
   }
 
 }

src/infrastructure/query/PhabricatorOffsetPagedQuery.php

@@ -35,7 +35,15 @@ abstract class PhabricatorOffsetPagedQuery extends PhabricatorQuery {
     return $this;
   }
 
-  final protected function buildLimitClause(AphrontDatabaseConnection $conn_r) {
+  final public function getOffset() {
+    return $this->offset;
+  }
+
+  final public function getLimit() {
+    return $this->limit;
+  }
+
+  protected function buildLimitClause(AphrontDatabaseConnection $conn_r) {
     if ($this->limit && $this->offset) {
       return qsprintf($conn_r, 'LIMIT %d, %d', $this->offset, $this->limit);
     } else if ($this->limit) {

src/infrastructure/query/policy/PhabricatorCursorPagedPolicyQuery.php

@@ -53,8 +53,8 @@ abstract class PhabricatorCursorPagedPolicyQuery
   }
 
   final protected function buildLimitClause(AphrontDatabaseConnection $conn_r) {
-    if ($this->getLimit()) {
-      return qsprintf($conn_r, 'LIMIT %d', $this->getLimit());
+    if ($this->getRawResultLimit()) {
+      return qsprintf($conn_r, 'LIMIT %d', $this->getRawResultLimit());
     } else {
       return '';
     }

src/infrastructure/query/policy/PhabricatorPolicyQuery.php

@@ -16,30 +16,98 @@
  * limitations under the License.
  */
 
-abstract class PhabricatorPolicyQuery extends PhabricatorQuery {
+/**
+ * A @{class:PhabricatorQuery} which filters results according to visibility
+ * policies for the querying user. Broadly, this class allows you to implement
+ * a query that returns only objects the user is allowed to see.
+ *
+ *   $results = id(new ExampleQuery())
+ *     ->setViewer($user)
+ *     ->withConstraint($example)
+ *     ->execute();
+ *
+ * Normally, you should extend @{class:PhabricatorCursorPagedPolicyQuery}, not
+ * this class. @{class:PhabricatorCursorPagedPolicyQuery} provides a more
+ * practical interface for building usable queries against most object types.
+ *
+ * NOTE: Although this class extends @{class:PhabricatorOffsetPagedQuery},
+ * offset paging with policy filtering is not efficient. All results must be
+ * loaded into the application and filtered here: skipping `N` rows via offset
+ * is an `O(N)` operation with a large constant. Prefer cursor-based paging
+ * with @{class:PhabricatorCursorPagedPolicyQuery}, which can filter far more
+ * efficiently in MySQL.
+ *
+ * @task config     Query Configuration
+ * @task exec       Executing Queries
+ * @task policyimpl Policy Query Implementation
+ */
+abstract class PhabricatorPolicyQuery extends PhabricatorOffsetPagedQuery {
 
-  private $limit;
   private $viewer;
   private $raisePolicyExceptions;
+  private $rawResultLimit;
 
-  final public function setViewer($viewer) {
+
+/* -(  Query Configuration  )------------------------------------------------ */
+
+
+  /**
+   * Set the viewer who is executing the query. Results will be filtered
+   * according to the viewer's capabilities. You must set a viewer to execute
+   * a policy query.
+   *
+   * @param PhabricatorUser The viewing user.
+   * @return this
+   * @task config
+   */
+  final public function setViewer(PhabricatorUser $viewer) {
     $this->viewer = $viewer;
     return $this;
   }
 
+
+  /**
+   * Get the query's viewer.
+   *
+   * @return PhabricatorUser The viewing user.
+   * @task config
+   */
   final public function getViewer() {
     return $this->viewer;
   }
 
-  final public function setLimit($limit) {
-    $this->limit = $limit;
-    return $this;
-  }
 
-  final public function getLimit() {
-    return $this->limit;
-  }
+/* -(  Query Execution  )---------------------------------------------------- */
 
+
+  /**
+   * Execute the query, expecting a single result. This method simplifies
+   * loading objects for detail pages or edit views.
+   *
+   *   // Load one result by ID.
+   *   $obj = id(new ExampleQuery())
+   *     ->setViewer($user)
+   *     ->withIDs(array($id))
+   *     ->executeOne();
+   *   if (!$obj) {
+   *     return new Aphront404Response();
+   *   }
+   *
+   * If zero results match the query, this method returns `null`.
+   *
+   * If one result matches the query, this method returns that result.
+   *
+   * If two or more results match the query, this method throws an exception.
+   * You should use this method only when the query constraints guarantee at
+   * most one match (e.g., selecting a specific ID or PHID).
+   *
+   * If one result matches the query but it is caught by the policy filter (for
+   * example, the user is trying to view or edit an object which exists but
+   * which they do not have permission to see) a policy exception is thrown.
+   *
+   * @return mixed Single result, or null.
+   * @task exec
+   */
   final public function executeOne() {
 
     $this->raisePolicyExceptions = true;
@@ -53,9 +121,21 @@ abstract class PhabricatorPolicyQuery extends PhabricatorQuery {
     if (count($results) > 1) {
       throw new Exception("Expected a single result!");
     }
+
+    if (!$results) {
+      return null;
+    }
+
     return head($results);
   }
 
+
+  /**
+   * Execute the query, loading all visible results.
+   *
+   * @return list<PhabricatorPolicyInterface> Result objects.
+   * @task exec
+   */
   final public function execute() {
     if (!$this->viewer) {
       throw new Exception("Call setViewer() before execute()!");
@@ -69,18 +149,69 @@ abstract class PhabricatorPolicyQuery extends PhabricatorQuery {
 
     $filter->raisePolicyExceptions($this->raisePolicyExceptions);
 
+    $offset = (int)$this->getOffset();
+    $limit  = (int)$this->getLimit();
+    $count  = 0;
+
+    $need = null;
+    if ($offset) {
+      $need = $offset + $limit;
+    }
+
+    $this->willExecute();
+
     do {
+
+      // Figure out how many results to load. "0" means "all results".
+      $load = 0;
+      if ($need && ($count < $offset)) {
+        // This cap is just an arbitrary limit to keep memory usage from going
+        // crazy for large offsets; we can't execute them efficiently, but
+        // it should be possible to execute them without crashing.
+        $load = min($need, 1024);
+      } else if ($limit) {
+        // Otherwise, just load the number of rows we're after. Note that it
+        // might be more efficient to load more rows than this (if we expect
+        // about 5% of objects to be filtered, loading 105% of the limit might
+        // be better) or fewer rows than this (if we already have 95 rows and
+        // only need 100, loading only 5 rows might be better), but we currently
+        // just use the simplest heuristic since we don't have enough data
+        // about policy queries in the real world to tweak it.
+        $load = $limit;
+      }
+      $this->rawResultLimit = $load;
+
+
       $page = $this->loadPage();
 
       $visible = $filter->apply($page);
       foreach ($visible as $key => $result) {
-        $results[$key] = $result;
-        if ($this->getLimit() && count($results) >= $this->getLimit()) {
+        ++$count;
+
+        // If we have an offset, we just ignore that many results and start
+        // storing them only once we've hit the offset. This reduces memory
+        // requirements for large offsets, compared to storing them all and
+        // slicing them away later.
+        if ($count > $offset) {
+          $results[$key] = $result;
+        }
+
+        if ($need && ($count >= $need)) {
+          // If we have all the rows we need, break out of the paging query.
           break 2;
         }
       }
 
-      if (!$this->getLimit() || (count($page) < $this->getLimit())) {
+      if (!$load) {
+        // If we don't have a load count, we loaded all the results. We do
+        // not need to load another page.
+        break;
+      }
+
+      if (count($page) < $load) {
+        // If we have a load count but the unfiltered results contained fewer
+        // objects, we know this was the last page of objects; we do not need
+        // to load another page because we can deduce it would be empty.
         break;
       }
 
@@ -90,7 +221,54 @@ abstract class PhabricatorPolicyQuery extends PhabricatorQuery {
     return $results;
   }
 
+
+/* -(  Policy Query Implementation  )---------------------------------------- */
+
+
+  /**
+   * Get the number of results @{method:loadPage} should load. If the value is
+   * 0, @{method:loadPage} should load all available results.
+   *
+   * @return int The number of results to load, or 0 for all results.
+   * @task policyimpl
+   */
+  final protected function getRawResultLimit() {
+    return $this->rawResultLimit;
+  }
+
+
+  /**
+   * Hook invoked before query execution. Generally, implementations should
+   * reset any internal cursors.
+   *
+   * @return void
+   * @task policyimpl
+   */
+  protected function willExecute() {
+    return;
+  }
+
+
+  /**
+   * Load a raw page of results. Generally, implementations should load objects
+   * from the database. They should attempt to return the number of results
+   * hinted by @{method:getRawResultLimit}.
+   *
+   * @return list<PhabricatorPolicyInterface> List of filterable policy objects.
+   * @task policyimpl
+   */
   abstract protected function loadPage();
+
+
+  /**
+   * Update internal state so that the next call to @{method:loadPage} will
+   * return new results. Generally, you should adjust a cursor position based
+   * on the provided result page.
+   *
+   * @param list<PhabricatorPolicyInterface> The current page of results.
+   * @return void
+   * @task policyimpl
+   */
   abstract protected function nextPage(array $page);
 
 }