From a43473c4b65c8badab24283fe39e705bbffe8a47 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 11 Apr 2015 12:10:35 -0700
Subject: [PATCH] Begin formalizing query orders

Summary:
Ref T7803. Queries currently have a single `getPagingColumn()`, which is oversimplified and insufficient to describe many ordering operations. Frequently, orders must span multiple columns.

Move toward an "order vector", which is a list of orderable values like "name, id". These map directly to columns, and are sufficient to actually describe orders. The more modern Query classes (Maniphest, Repository) essentially do this manually anyway.

Test Plan:
  - Added and executed unit tests.
  - Browsed around, verified the correct ORDER BY clauses were generated.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12352
---
 src/__phutil_library_map__.php                |   9 ++
 .../query/order/PhabricatorQueryOrderItem.php |  62 ++++++++
 .../order/PhabricatorQueryOrderVector.php     | 115 +++++++++++++++
 .../PhabricatorQueryOrderTestCase.php         |  65 +++++++++
 ...PhabricatorCursorPagedPolicyAwareQuery.php | 133 +++++++++++++++---
 5 files changed, 368 insertions(+), 16 deletions(-)
 create mode 100644 src/infrastructure/query/order/PhabricatorQueryOrderItem.php
 create mode 100644 src/infrastructure/query/order/PhabricatorQueryOrderVector.php
 create mode 100644 src/infrastructure/query/order/__tests__/PhabricatorQueryOrderTestCase.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 0fedc28f14..9cca711845 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2320,6 +2320,9 @@ phutil_register_library_map(array(
     'PhabricatorProjectsPolicyRule' => 'applications/policy/rule/PhabricatorProjectsPolicyRule.php',
     'PhabricatorPygmentSetupCheck' => 'applications/config/check/PhabricatorPygmentSetupCheck.php',
     'PhabricatorQuery' => 'infrastructure/query/PhabricatorQuery.php',
+    'PhabricatorQueryOrderItem' => 'infrastructure/query/order/PhabricatorQueryOrderItem.php',
+    'PhabricatorQueryOrderTestCase' => 'infrastructure/query/order/__tests__/PhabricatorQueryOrderTestCase.php',
+    'PhabricatorQueryOrderVector' => 'infrastructure/query/order/PhabricatorQueryOrderVector.php',
     'PhabricatorRecaptchaConfigOptions' => 'applications/config/option/PhabricatorRecaptchaConfigOptions.php',
     'PhabricatorRedirectController' => 'applications/base/controller/PhabricatorRedirectController.php',
     'PhabricatorRefreshCSRFController' => 'applications/auth/controller/PhabricatorRefreshCSRFController.php',
@@ -5685,6 +5688,12 @@ phutil_register_library_map(array(
     'PhabricatorProjectWatchController' => 'PhabricatorProjectController',
     'PhabricatorProjectsPolicyRule' => 'PhabricatorPolicyRule',
     'PhabricatorPygmentSetupCheck' => 'PhabricatorSetupCheck',
+    'PhabricatorQueryOrderItem' => 'Phobject',
+    'PhabricatorQueryOrderTestCase' => 'PhabricatorTestCase',
+    'PhabricatorQueryOrderVector' => array(
+      'Phobject',
+      'Iterator',
+    ),
     'PhabricatorRecaptchaConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorRedirectController' => 'PhabricatorController',
     'PhabricatorRefreshCSRFController' => 'PhabricatorAuthController',
diff --git a/src/infrastructure/query/order/PhabricatorQueryOrderItem.php b/src/infrastructure/query/order/PhabricatorQueryOrderItem.php
new file mode 100644
index 0000000000..a37cb92e04
--- /dev/null
+++ b/src/infrastructure/query/order/PhabricatorQueryOrderItem.php
@@ -0,0 +1,62 @@
+<?php
+
+/**
+ * Structural class representing one item in an order vector.
+ *
+ * See @{class:PhabricatorQueryOrderVector} for discussion of order vectors.
+ * This represents one item in an order vector, like "id". When combined with
+ * the other items in the vector, a complete ordering (like "name, id") is
+ * described.
+ *
+ * Construct an item using @{method:newFromScalar}:
+ *
+ *   $item = PhabricatorQueryOrderItem::newFromScalar('id');
+ *
+ * This class is primarily internal to the query infrastructure, and most
+ * application code should not need to interact with it directly.
+ */
+final class PhabricatorQueryOrderItem
+  extends Phobject {
+
+  private $orderKey;
+  private $isReversed;
+
+  private function __construct() {
+    // <private>
+  }
+
+  public static function newFromScalar($scalar) {
+    // If the string is something like "-id", strip the "-" off and mark it
+    // as reversed.
+    $is_reversed = false;
+    if (!strncmp($scalar, '-', 1)) {
+      $is_reversed = true;
+      $scalar = substr($scalar, 1);
+    }
+
+    $item = new PhabricatorQueryOrderItem();
+    $item->orderKey = $scalar;
+    $item->isReversed = $is_reversed;
+
+    return $item;
+  }
+
+  public function getIsReversed() {
+    return $this->isReversed;
+  }
+
+  public function getOrderKey() {
+    return $this->orderKey;
+  }
+
+  public function getAsScalar() {
+    if ($this->getIsReversed()) {
+      $prefix = '-';
+    } else {
+      $prefix = '';
+    }
+
+    return $prefix.$this->getOrderKey();
+  }
+
+}
diff --git a/src/infrastructure/query/order/PhabricatorQueryOrderVector.php b/src/infrastructure/query/order/PhabricatorQueryOrderVector.php
new file mode 100644
index 0000000000..584c13ccac
--- /dev/null
+++ b/src/infrastructure/query/order/PhabricatorQueryOrderVector.php
@@ -0,0 +1,115 @@
+<?php
+
+/**
+ * Structural class representing a column ordering for a query.
+ *
+ * Queries often order results on multiple columns. For example, projects might
+ * be ordered by "name, id". This class wraps a list of column orderings and
+ * makes them easier to manage.
+ *
+ * To construct an order vector, use @{method:newFromVector}:
+ *
+ *   $vector = PhabricatorQueryOrderVector::newFromVector(array('name', 'id'));
+ *
+ * You can iterate over an order vector normally:
+ *
+ *   foreach ($vector as $item) {
+ *     // ...
+ *   }
+ *
+ * The items are objects of class @{class:PhabricatorQueryOrderItem}.
+ *
+ * This class is primarily internal to the query infrastructure, and most
+ * application code should not need to interact with it directly.
+ */
+final class PhabricatorQueryOrderVector
+  extends Phobject
+  implements Iterator {
+
+  private $items;
+  private $keys;
+  private $cursor;
+
+  private function __construct() {
+    // <private>
+  }
+
+  public static function newFromVector($vector) {
+    if ($vector instanceof PhabricatorQueryOrderVector) {
+      return (clone $vector);
+    }
+
+    if (!is_array($vector)) {
+      throw new Exception(
+        pht(
+          'An order vector can only be constructed from a list of strings or '.
+          'another order vector.'));
+    }
+
+    if (!$vector) {
+      throw new Exception(
+        pht(
+          'An order vector must not be empty.'));
+    }
+
+    $items = array();
+    foreach ($vector as $key => $scalar) {
+      if (!is_string($scalar)) {
+        throw new Exception(
+          pht(
+            'Value with key "%s" in order vector is not a string (it has '.
+            'type "%s"). An order vector must contain only strings.',
+            $key,
+            gettype($scalar)));
+      }
+
+      $item = PhabricatorQueryOrderItem::newFromScalar($scalar);
+
+      // Orderings like "id, id, id" or "id, -id" are meaningless and invalid.
+      if (isset($items[$item->getOrderKey()])) {
+        throw new Exception(
+          pht(
+            'Order vector "%s" specifies order "%s" twice. Each component '.
+            'of an ordering must be unique.',
+            implode(', ', $vector),
+            $item));
+      }
+
+      $items[$item->getOrderKey()] = $item;
+    }
+
+    $obj = new PhabricatorQueryOrderVector();
+    $obj->items = $items;
+    $obj->keys = array_keys($items);
+    return $obj;
+  }
+
+
+/* -(  Iterator Interface  )------------------------------------------------- */
+
+
+  public function rewind() {
+    $this->cursor = 0;
+  }
+
+
+  public function current() {
+    return $this->items[$this->key()];
+  }
+
+
+  public function key() {
+    return $this->keys[$this->cursor];
+  }
+
+
+  public function next() {
+    ++$this->cursor;
+  }
+
+
+  public function valid() {
+    return isset($this->keys[$this->cursor]);
+  }
+
+}
diff --git a/src/infrastructure/query/order/__tests__/PhabricatorQueryOrderTestCase.php b/src/infrastructure/query/order/__tests__/PhabricatorQueryOrderTestCase.php
new file mode 100644
index 0000000000..e0e6a2a16a
--- /dev/null
+++ b/src/infrastructure/query/order/__tests__/PhabricatorQueryOrderTestCase.php
@@ -0,0 +1,65 @@
+<?php
+
+final class PhabricatorQueryOrderTestCase extends PhabricatorTestCase {
+
+  public function testQueryOrderItem() {
+    $item = PhabricatorQueryOrderItem::newFromScalar('id');
+    $this->assertEqual('id', $item->getOrderKey());
+    $this->assertEqual(false, $item->getIsReversed());
+
+    $item = PhabricatorQueryOrderItem::newFromScalar('-id');
+    $this->assertEqual('id', $item->getOrderKey());
+    $this->assertEqual(true, $item->getIsReversed());
+  }
+
+  public function testQueryOrderBadVectors() {
+    $bad = array(
+      array(),
+      null,
+      1,
+      array(2),
+      array('id', 'id'),
+      array('id', '-id'),
+    );
+
+    foreach ($bad as $input) {
+      $caught = null;
+      try {
+        PhabricatorQueryOrderVector::newFromVector($input);
+      } catch (Exception $ex) {
+        $caught = $ex;
+      }
+
+      $this->assertTrue(($caught instanceof Exception));
+    }
+  }
+
+  public function testQueryOrderVector() {
+    $vector = PhabricatorQueryOrderVector::newFromVector(
+      array(
+        'a',
+        'b',
+        '-c',
+        'd',
+      ));
+
+    $this->assertEqual(
+      array(
+        'a' => 'a',
+        'b' => 'b',
+        'c' => 'c',
+        'd' => 'd',
+      ),
+      mpull(iterator_to_array($vector), 'getOrderKey'));
+
+    $this->assertEqual(
+      array(
+        'a' => false,
+        'b' => false,
+        'c' => true,
+        'd' => false,
+      ),
+      mpull(iterator_to_array($vector), 'getIsReversed'));
+  }
+
+}
diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
index 42ea200de1..a49eaf28b6 100644
--- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
+++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
@@ -5,6 +5,7 @@
  * performant than offset-based paging in the presence of policy filtering.
  *
  * @task appsearch Integration with ApplicationSearch
+ * @task order Result Ordering
  */
 abstract class PhabricatorCursorPagedPolicyAwareQuery
   extends PhabricatorPolicyAwareQuery {
@@ -14,6 +15,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
   private $applicationSearchConstraints = array();
   private $applicationSearchOrders = array();
   private $internalPaging;
+  private $orderVector;
 
   protected function getPagingColumn() {
     return 'id';
@@ -131,22 +133,6 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
     return null;
   }
 
-  final protected function buildOrderClause(AphrontDatabaseConnection $conn_r) {
-    if ($this->beforeID) {
-      return qsprintf(
-        $conn_r,
-        'ORDER BY %Q %Q',
-        $this->getPagingColumn(),
-        $this->getReversePaging() ? 'DESC' : 'ASC');
-    } else {
-      return qsprintf(
-        $conn_r,
-        'ORDER BY %Q %Q',
-        $this->getPagingColumn(),
-        $this->getReversePaging() ? 'ASC' : 'DESC');
-    }
-  }
-
   final protected function didLoadResults(array $results) {
     if ($this->beforeID) {
       $results = array_reverse($results, $preserve_keys = true);
@@ -284,6 +270,121 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
     return '('.implode(') OR (', $clauses).')';
   }
 
+
+/* -(  Result Ordering  )---------------------------------------------------- */
+
+
+  /**
+   * @task order
+   */
+  public function setOrderVector($vector) {
+    $vector = PhabricatorQueryOrderVector::newFromVector($vector);
+
+    $orderable = $this->getOrderableColumns();
+    foreach ($vector as $order) {
+      $key = $vector->getOrderKey();
+      if (empty($orderable[$key])) {
+        $valid = implode(', ', array_keys($orderable));
+        throw new Exception(
+          pht(
+            'This query ("%s") does not support sorting by order key "%s". '.
+            'Supported orders are: %s.',
+            get_class($this),
+            $valid));
+      }
+    }
+
+    $this->orderVector = $vector;
+    return $this;
+  }
+
+
+  /**
+   * @task order
+   */
+  private function getOrderVector() {
+    if (!$this->orderVector) {
+      $vector = $this->getDefaultOrderVector();
+      $vector = PhabricatorQueryOrderVector::newFromVector($vector);
+      $this->orderVector = $vector;
+    }
+
+    return $this->orderVector;
+  }
+
+
+  /**
+   * @task order
+   */
+  protected function getDefaultOrderVector() {
+    return array('id');
+  }
+
+
+  /**
+   * @task order
+   */
+  public function getOrderableColumns() {
+    // TODO: Remove this once all subclasses move off the old stuff.
+    if ($this->getPagingColumn() !== 'id') {
+      // This class has bad old custom logic around paging, so return nothing
+      // here. This deactivates the new order code.
+      return array();
+    }
+
+    return array(
+      'id' => array(
+        'table' => null,
+        'column' => 'id',
+        'reverse' => false,
+      ),
+    );
+  }
+
+
+  /**
+   * @task order
+   */
+  final protected function buildOrderClause(AphrontDatabaseConnection $conn) {
+    $orderable = $this->getOrderableColumns();
+
+    // TODO: Remove this once all subclasses move off the old stuff. We'll
+    // only enter this block for code using older ordering mechanisms. New
+    // code should expose an orderable column list.
+    if (!$orderable) {
+      if ($this->beforeID) {
+        return qsprintf(
+          $conn,
+          'ORDER BY %Q %Q',
+          $this->getPagingColumn(),
+          $this->getReversePaging() ? 'DESC' : 'ASC');
+      } else {
+        return qsprintf(
+          $conn,
+          'ORDER BY %Q %Q',
+          $this->getPagingColumn(),
+          $this->getReversePaging() ? 'ASC' : 'DESC');
+      }
+    }
+
+    $vector = $this->getOrderVector();
+
+    $parts = array();
+    foreach ($vector as $order) {
+      $part = $orderable[$order->getOrderKey()];
+      if ($order->getIsReversed()) {
+        $part['reverse'] = !idx($part, 'reverse', false);
+      }
+      $parts[] = $part;
+    }
+
+    return $this->formatOrderClause($conn, $parts);
+  }
+
+
+  /**
+   * @task order
+   */
   protected function formatOrderClause(
     AphrontDatabaseConnection $conn,
     array $parts) {