diff --git a/src/infrastructure/query/order/PhabricatorQueryOrderVector.php b/src/infrastructure/query/order/PhabricatorQueryOrderVector.php index 584c13ccac..76e3a9bae3 100644 --- a/src/infrastructure/query/order/PhabricatorQueryOrderVector.php +++ b/src/infrastructure/query/order/PhabricatorQueryOrderVector.php @@ -72,7 +72,7 @@ final class PhabricatorQueryOrderVector 'Order vector "%s" specifies order "%s" twice. Each component '. 'of an ordering must be unique.', implode(', ', $vector), - $item)); + $item->getOrderKey())); } $items[$item->getOrderKey()] = $item; @@ -84,6 +84,14 @@ final class PhabricatorQueryOrderVector return $obj; } + public function getAsString() { + $scalars = array(); + foreach ($this->items as $item) { + $scalars[] = $item->getAsScalar(); + } + return implode(', ', $scalars); + } + /* -( Iterator Interface )------------------------------------------------- */ diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 2d23ec82e2..2f89890058 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 paging Paging * @task order Result Ordering */ abstract class PhabricatorCursorPagedPolicyAwareQuery @@ -111,28 +112,6 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } } - protected function buildPagingClause( - AphrontDatabaseConnection $conn_r) { - - if ($this->beforeID) { - return qsprintf( - $conn_r, - '%Q %Q %s', - $this->getPagingColumn(), - $this->getReversePaging() ? '<' : '>', - $this->beforeID); - } else if ($this->afterID) { - return qsprintf( - $conn_r, - '%Q %Q %s', - $this->getPagingColumn(), - $this->getReversePaging() ? '>' : '<', - $this->afterID); - } - - return null; - } - final protected function didLoadResults(array $results) { if ($this->beforeID) { $results = array_reverse($results, $preserve_keys = true); @@ -168,6 +147,89 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } +/* -( Paging )------------------------------------------------------------- */ + + + protected function buildPagingClause(AphrontDatabaseConnection $conn) { + $orderable = $this->getOrderableColumns(); + + // TODO: Remove this once subqueries modernize. + if (!$orderable) { + if ($this->beforeID) { + return qsprintf( + $conn, + '%Q %Q %s', + $this->getPagingColumn(), + $this->getReversePaging() ? '<' : '>', + $this->beforeID); + } else if ($this->afterID) { + return qsprintf( + $conn, + '%Q %Q %s', + $this->getPagingColumn(), + $this->getReversePaging() ? '>' : '<', + $this->afterID); + } else { + return null; + } + } + + $vector = $this->getOrderVector(); + + if ($this->beforeID !== null) { + $cursor = $this->beforeID; + $reversed = true; + } else if ($this->afterID !== null) { + $cursor = $this->afterID; + $reversed = false; + } else { + // No paging is being applied to this query so we do not need to + // construct a paging clause. + return ''; + } + + $keys = array(); + foreach ($vector as $order) { + $keys[] = $order->getOrderKey(); + } + + $value_map = $this->getPagingValueMap($cursor, $keys); + + $columns = array(); + foreach ($vector as $order) { + $key = $order->getOrderKey(); + + if (!array_key_exists($key, $value_map)) { + throw new Exception( + pht( + 'Query "%s" failed to return a value from getPagingValueMap() '. + 'for column "%s".', + get_class($this), + $key)); + } + + $column = $orderable[$key]; + $column['value'] = $value_map[$key]; + + $columns[] = $column; + } + + return $this->buildPagingClauseFromMultipleColumns( + $conn, + $columns, + array( + 'reversed' => $reversed, + )); + } + + protected function getPagingValueMap($cursor, array $keys) { + // TODO: This is a hack to make this work with existing classes for now. + return array( + 'id' => $cursor, + ); + } + + /** * Simplifies the task of constructing a paging clause across multiple * columns. In the general case, this looks like: @@ -214,11 +276,12 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery PhutilTypeSpec::checkMap( $column, array( - 'table' => 'optional string', + 'table' => 'optional string|null', 'column' => 'string', 'value' => 'wild', 'type' => 'string', 'reverse' => 'optional bool', + 'unique' => 'optional bool', )); } @@ -298,8 +361,11 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $vector = PhabricatorQueryOrderVector::newFromVector($vector); $orderable = $this->getOrderableColumns(); + + // Make sure that all the components identify valid columns. + $unique = array(); foreach ($vector as $order) { - $key = $vector->getOrderKey(); + $key = $order->getOrderKey(); if (empty($orderable[$key])) { $valid = implode(', ', array_keys($orderable)); throw new Exception( @@ -307,8 +373,38 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery 'This query ("%s") does not support sorting by order key "%s". '. 'Supported orders are: %s.', get_class($this), + $key, $valid)); } + + $unique[$key] = idx($orderable[$key], 'unique', false); + } + + // Make sure that the last column is unique so that this is a strong + // ordering which can be used for paging. + $last = last($unique); + if ($last !== true) { + throw new Exception( + pht( + 'Order vector "%s" is invalid: the last column in an order must '. + 'be a column with unique values, but "%s" is not unique.', + $vector->getAsString(), + last_key($unique))); + } + + // Make sure that other columns are not unique; an ordering like "id, name" + // does not make sense because only "id" can ever have an effect. + array_pop($unique); + foreach ($unique as $key => $is_unique) { + if ($is_unique) { + throw new Exception( + pht( + 'Order vector "%s" is invalid: only the last column in an order '. + 'may be unique, but "%s" is a unique column and not the last '. + 'column in the order.', + $vector->getAsString(), + $key)); + } } $this->orderVector = $vector; @@ -323,7 +419,10 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery if (!$this->orderVector) { $vector = $this->getDefaultOrderVector(); $vector = PhabricatorQueryOrderVector::newFromVector($vector); - $this->orderVector = $vector; + + // We call setOrderVector() here to apply checks to the default vector. + // This catches any errors in the implementation. + $this->setOrderVector($vector); } return $this->orderVector; @@ -354,6 +453,8 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery 'table' => null, 'column' => 'id', 'reverse' => false, + 'type' => 'int', + 'unique' => true, ), ); }