From 46b4fa85d097d90cc917fe7338675844e96af8c6 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Sat, 2 Aug 2014 18:22:16 +1000 Subject: [PATCH] Support custom fields in "Order By" for Maniphest Summary: Resolves T4659. This implements support for sorting tasks by custom fields. Some of this feels hacky in the way it's hooked up to the Maniphest search engine and task query. Test Plan: Queryed on a custom date field, with a small page size, and moved back and forth through the result set. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: epriestley, Korvin Maniphest Tasks: T4659 Differential Revision: https://secure.phabricator.com/D10106 --- .../maniphest/query/ManiphestTaskQuery.php | 158 ++++++++++-------- .../query/ManiphestTaskSearchEngine.php | 17 +- .../PhabricatorApplicationSearchEngine.php | 59 +++++++ .../field/PhabricatorCustomField.php | 22 +++ .../PhabricatorStandardCustomField.php | 4 + .../PhabricatorStandardCustomFieldBool.php | 4 + .../PhabricatorStandardCustomFieldDate.php | 4 + .../PhabricatorStandardCustomFieldInt.php | 4 + ...PhabricatorCursorPagedPolicyAwareQuery.php | 92 ++++++++++ 9 files changed, 290 insertions(+), 74 deletions(-) diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php index 7afbd4e0fb..c3528f219c 100644 --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -572,6 +572,8 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { } private function buildCustomOrderClause(AphrontDatabaseConnection $conn) { + $reverse = ($this->getBeforeID() xor $this->getReversePaging()); + $order = array(); switch ($this->groupBy) { @@ -593,33 +595,35 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { throw new Exception("Unknown group query '{$this->groupBy}'!"); } - switch ($this->orderBy) { - case self::ORDER_PRIORITY: - $order[] = 'priority'; - $order[] = 'subpriority'; - $order[] = 'dateModified'; - break; - case self::ORDER_CREATED: - $order[] = 'id'; - break; - case self::ORDER_MODIFIED: - $order[] = 'dateModified'; - break; - case self::ORDER_TITLE: - $order[] = 'title'; - break; - default: - throw new Exception("Unknown order query '{$this->orderBy}'!"); + $app_order = $this->buildApplicationSearchOrders($conn, $reverse); + + if (!$app_order) { + switch ($this->orderBy) { + case self::ORDER_PRIORITY: + $order[] = 'priority'; + $order[] = 'subpriority'; + $order[] = 'dateModified'; + break; + case self::ORDER_CREATED: + $order[] = 'id'; + break; + case self::ORDER_MODIFIED: + $order[] = 'dateModified'; + break; + case self::ORDER_TITLE: + $order[] = 'title'; + break; + default: + throw new Exception("Unknown order query '{$this->orderBy}'!"); + } } $order = array_unique($order); - if (empty($order)) { + if (empty($order) && empty($app_order)) { return null; } - $reverse = ($this->getBeforeID() xor $this->getReversePaging()); - foreach ($order as $k => $column) { switch ($column) { case 'subpriority': @@ -653,6 +657,18 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { } } + if ($app_order) { + foreach ($app_order as $order_by) { + $order[] = $order_by; + } + + if ($reverse) { + $order[] = 'task.id ASC'; + } else { + $order[] = 'task.id DESC'; + } + } + return 'ORDER BY '.implode(', ', $order); } @@ -903,55 +919,65 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { throw new Exception("Unknown group query '{$this->groupBy}'!"); } - switch ($this->orderBy) { - case self::ORDER_PRIORITY: - if ($this->groupBy != self::GROUP_PRIORITY) { + $app_columns = $this->buildApplicationSearchPagination($conn_r, $cursor); + if ($app_columns) { + $columns = array_merge($columns, $app_columns); + $columns[] = array( + 'name' => 'task.id', + 'value' => (int)$cursor->getID(), + 'type' => 'int', + ); + } else { + switch ($this->orderBy) { + case self::ORDER_PRIORITY: + if ($this->groupBy != self::GROUP_PRIORITY) { + $columns[] = array( + 'name' => 'task.priority', + 'value' => (int)$cursor->getPriority(), + 'type' => 'int', + ); + } $columns[] = array( - 'name' => 'task.priority', - 'value' => (int)$cursor->getPriority(), + 'name' => 'task.subpriority', + 'value' => (int)$cursor->getSubpriority(), + 'type' => 'int', + 'reverse' => true, + ); + $columns[] = array( + 'name' => 'task.dateModified', + 'value' => (int)$cursor->getDateModified(), 'type' => 'int', ); - } - $columns[] = array( - 'name' => 'task.subpriority', - 'value' => (int)$cursor->getSubpriority(), - 'type' => 'int', - 'reverse' => true, - ); - $columns[] = array( - 'name' => 'task.dateModified', - 'value' => (int)$cursor->getDateModified(), - 'type' => 'int', - ); - break; - case self::ORDER_CREATED: - $columns[] = array( - 'name' => 'task.id', - 'value' => (int)$cursor->getID(), - 'type' => 'int', - ); - break; - case self::ORDER_MODIFIED: - $columns[] = array( - 'name' => 'task.dateModified', - 'value' => (int)$cursor->getDateModified(), - 'type' => 'int', - ); - break; - case self::ORDER_TITLE: - $columns[] = array( - 'name' => 'task.title', - 'value' => $cursor->getTitle(), - 'type' => 'string', - ); - $columns[] = array( - 'name' => 'task.id', - 'value' => $cursor->getID(), - 'type' => 'int', - ); - break; - default: - throw new Exception("Unknown order query '{$this->orderBy}'!"); + break; + case self::ORDER_CREATED: + $columns[] = array( + 'name' => 'task.id', + 'value' => (int)$cursor->getID(), + 'type' => 'int', + ); + break; + case self::ORDER_MODIFIED: + $columns[] = array( + 'name' => 'task.dateModified', + 'value' => (int)$cursor->getDateModified(), + 'type' => 'int', + ); + break; + case self::ORDER_TITLE: + $columns[] = array( + 'name' => 'task.title', + 'value' => $cursor->getTitle(), + 'type' => 'string', + ); + $columns[] = array( + 'name' => 'task.id', + 'value' => $cursor->getID(), + 'type' => 'int', + ); + break; + default: + throw new Exception("Unknown order query '{$this->orderBy}'!"); + } } return $this->buildPagingClauseFromMultipleColumns( diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index ff72c1203e..2ac9cfcf81 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -151,13 +151,10 @@ final class ManiphestTaskSearchEngine $query->withPriorities($priorities); } - $order = $saved->getParameter('order'); - $order = idx($this->getOrderValues(), $order); - if ($order) { - $query->setOrderBy($order); - } else { - $query->setOrderBy(head($this->getOrderValues())); - } + $this->applyOrderByToQuery( + $query, + $this->getOrderValues(), + $saved->getParameter('order')); $group = $saved->getParameter('group'); $group = idx($this->getGroupValues(), $group); @@ -306,6 +303,10 @@ final class ManiphestTaskSearchEngine $ids = $saved->getParameter('ids', array()); + $builtin_orders = $this->getOrderOptions(); + $custom_orders = $this->getCustomFieldOrderOptions(); + $all_orders = $builtin_orders + $custom_orders; + $form ->appendChild( id(new AphrontFormTokenizerControl()) @@ -385,7 +386,7 @@ final class ManiphestTaskSearchEngine ->setName('order') ->setLabel(pht('Order By')) ->setValue($saved->getParameter('order')) - ->setOptions($this->getOrderOptions())); + ->setOptions($all_orders)); } $form diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php index 531f88e70d..d7327c67d7 100644 --- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php +++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php @@ -764,6 +764,65 @@ abstract class PhabricatorApplicationSearchEngine { } } + protected function applyOrderByToQuery( + PhabricatorCursorPagedPolicyAwareQuery $query, + array $standard_values, + $order) { + + if (substr($order, 0, 7) === 'custom:') { + $list = $this->getCustomFieldList(); + if (!$list) { + $query->setOrderBy(head($standard_values)); + return; + } + + foreach ($list->getFields() as $field) { + $key = $this->getKeyForCustomField($field); + + if ($key === $order) { + $index = $field->buildOrderIndex(); + + if ($index === null) { + $query->setOrderBy(head($standard_values)); + return; + } + + $query->withApplicationSearchOrder( + $field, + $index, + false); + break; + } + } + } else { + $order = idx($standard_values, $order); + if ($order) { + $query->setOrderBy($order); + } else { + $query->setOrderBy(head($standard_values)); + } + } + } + + + protected function getCustomFieldOrderOptions() { + $list = $this->getCustomFieldList(); + if (!$list) { + return; + } + + $custom_order = array(); + foreach ($list->getFields() as $field) { + if ($field->shouldAppearInApplicationSearch()) { + if ($field->buildOrderIndex() !== null) { + $key = $this->getKeyForCustomField($field); + $custom_order[$key] = $field->getFieldName(); + } + } + } + + return $custom_order; + } /** * Get a unique key identifying a field. diff --git a/src/infrastructure/customfield/field/PhabricatorCustomField.php b/src/infrastructure/customfield/field/PhabricatorCustomField.php index 2741cf6b25..f0da81932f 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomField.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php @@ -620,6 +620,28 @@ abstract class PhabricatorCustomField { } + /** + * Return an index against which this field can be meaningfully ordered + * against to implement ApplicationSearch. + * + * This should be a single index, normally built using + * @{method:newStringIndex} and @{method:newNumericIndex}. + * + * The value of the index is not used. + * + * Return null from this method if the field can not be ordered. + * + * @return PhabricatorCustomFieldIndexStorage A single index to order by. + * @task appsearch + */ + public function buildOrderIndex() { + if ($this->proxy) { + return $this->proxy->buildOrderIndex(); + } + return null; + } + + /** * Build a new empty storage object for storing string indexes. Normally, * this should be a concrete subclass of diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php index 4eafe7c264..f7acb02a94 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php @@ -260,6 +260,10 @@ abstract class PhabricatorStandardCustomField return array(); } + public function buildOrderIndex() { + return null; + } + public function readApplicationSearchValueFromRequest( PhabricatorApplicationSearchEngine $engine, AphrontRequest $request) { diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBool.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBool.php index cde3beb5d9..a066dcfe34 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBool.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBool.php @@ -18,6 +18,10 @@ final class PhabricatorStandardCustomFieldBool return $indexes; } + public function buildOrderIndex() { + return $this->newNumericIndex(0); + } + public function getValueForStorage() { $value = $this->getFieldValue(); if (strlen($value)) { diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php index 8c2e1e4ad1..f8222d9559 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php @@ -18,6 +18,10 @@ final class PhabricatorStandardCustomFieldDate return $indexes; } + public function buildOrderIndex() { + return $this->newNumericIndex(0); + } + public function getValueForStorage() { $value = $this->getFieldValue(); if (strlen($value)) { diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php index aafa30ea99..94a24d01b0 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php @@ -18,6 +18,10 @@ final class PhabricatorStandardCustomFieldInt return $indexes; } + public function buildOrderIndex() { + return $this->newNumericIndex(0); + } + public function getValueForStorage() { $value = $this->getFieldValue(); if (strlen($value)) { diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index ef5ab3839e..e4be99cbd4 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -12,6 +12,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery private $afterID; private $beforeID; private $applicationSearchConstraints = array(); + private $applicationSearchOrders = array(); private $internalPaging; protected function getPagingColumn() { @@ -359,6 +360,32 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } + /** + * Order the results by an ApplicationSearch index. + * + * @param PhabricatorCustomField Field to which the index belongs. + * @param PhabricatorCustomFieldIndexStorage Table where the index is stored. + * @param bool True to sort ascending. + * @return this + * @task appsearch + */ + public function withApplicationSearchOrder( + PhabricatorCustomField $field, + PhabricatorCustomFieldIndexStorage $index, + $ascending) { + + $this->applicationSearchOrders[] = array( + 'key' => $field->getFieldKey(), + 'type' => $index->getIndexValueType(), + 'table' => $index->getTableName(), + 'index' => $index->getIndexKey(), + 'ascending' => $ascending, + ); + + return $this; + } + + /** * Get the name of the query's primary object PHID column, for constructing * JOIN clauses. Normally (and by default) this is just `"phid"`, but if the @@ -535,7 +562,72 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } } + foreach ($this->applicationSearchOrders as $key => $order) { + $table = $order['table']; + $alias = 'appsearch_order_'.$key; + $index = $order['index']; + $phid_column = $this->getApplicationSearchObjectPHIDColumn(); + + $joins[] = qsprintf( + $conn_r, + 'JOIN %T %T ON %T.objectPHID = %Q + AND %T.indexKey = %s', + $table, + $alias, + $alias, + $phid_column, + $alias, + $index); + } + return implode(' ', $joins); } + protected function buildApplicationSearchOrders( + AphrontDatabaseConnection $conn_r, + $reverse) { + + $orders = array(); + foreach ($this->applicationSearchOrders as $key => $order) { + $alias = 'appsearch_order_'.$key; + + if ($order['ascending'] xor $reverse) { + $orders[] = qsprintf($conn_r, '%T.indexValue ASC', $alias); + } else { + $orders[] = qsprintf($conn_r, '%T.indexValue DESC', $alias); + } + } + + return $orders; + } + + protected function buildApplicationSearchPagination( + AphrontDatabaseConnection $conn_r, + $cursor) { + + // We have to get the current field values on the cursor object. + $fields = PhabricatorCustomField::getObjectFields( + $cursor, + PhabricatorCustomField::ROLE_APPLICATIONSEARCH); + $fields->setViewer($this->getViewer()); + $fields->readFieldsFromStorage($cursor); + + $fields = mpull($fields->getFields(), null, 'getFieldKey'); + + $columns = array(); + foreach ($this->applicationSearchOrders as $key => $order) { + $alias = 'appsearch_order_'.$key; + + $field = idx($fields, $order['key']); + + $columns[] = array( + 'name' => $alias.'.indexValue', + 'value' => $field->getValueForStorage(), + 'type' => $order['type'], + ); + } + + return $columns; + } + }