diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2320,6 +2320,9 @@ '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 @@ '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 --- /dev/null +++ b/src/infrastructure/query/order/PhabricatorQueryOrderItem.php @@ -0,0 +1,62 @@ + + } + + 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 --- /dev/null +++ b/src/infrastructure/query/order/PhabricatorQueryOrderVector.php @@ -0,0 +1,115 @@ + + } + + 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 --- /dev/null +++ b/src/infrastructure/query/order/__tests__/PhabricatorQueryOrderTestCase.php @@ -0,0 +1,65 @@ +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 --- 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 @@ private $applicationSearchConstraints = array(); private $applicationSearchOrders = array(); private $internalPaging; + private $orderVector; protected function getPagingColumn() { return 'id'; @@ -131,22 +133,6 @@ 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 @@ 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) {