mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-27 01:02:42 +01:00
When paging by Ferret "rank", page using "HAVING rank > ...", not "WHERE rank > ..."
Summary: Ref T13091. The Ferret "rank" column is a function of the query text and looks something like `SELECT ..., 2 + 2 AS rank, ...`. You can't apply conditions to this kind of dynamic column with a WHERE clause: you get a slightly unhelpful error like "column rank unknown in where clause". You must use HAVING: ``` mysql> SELECT 2 + 2 AS x WHERE x = 4; ERROR 1054 (42S22): Unknown column 'x' in 'where clause' mysql> SELECT 2 + 2 AS x HAVING x = 4; +---+ | x | +---+ | 4 | +---+ 1 row in set (0.00 sec) ``` Add a flag to paging column definitions to let them specify that they must be applied with HAVING, then apply the whole paging clause with HAVING if any column requires HAVING. Test Plan: - In Maniphest, ran a fulltext search matching more than 100 results, ordered by "Relevance", then clicked "Next Page". - Before patch: query with `... WHERE rank > 123 OR ...` caused MySQL error because `rank` is not a WHERE-able column. - After patch: query builds as `... HAVING rank > 123 OR ...`, pages properly, no MySQL error. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13091 Differential Revision: https://secure.phabricator.com/D20298
This commit is contained in:
parent
f047b90d93
commit
7f90570636
1 changed files with 66 additions and 3 deletions
|
@ -83,6 +83,13 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
|
||||||
|
|
||||||
$this->applyExternalCursorConstraintsToQuery($query, $cursor);
|
$this->applyExternalCursorConstraintsToQuery($query, $cursor);
|
||||||
|
|
||||||
|
// If we have a Ferret fulltext query, copy it to the subquery so that we
|
||||||
|
// generate ranking columns appropriately, and compute the correct object
|
||||||
|
// ranking score for the current query.
|
||||||
|
if ($this->ferretEngine) {
|
||||||
|
$query->withFerretConstraint($this->ferretEngine, $this->ferretTokens);
|
||||||
|
}
|
||||||
|
|
||||||
// We're executing the subquery normally to make sure the viewer can
|
// We're executing the subquery normally to make sure the viewer can
|
||||||
// actually see the object, and that it's a completely valid object which
|
// actually see the object, and that it's a completely valid object which
|
||||||
// passes all filtering and policy checks. You aren't allowed to use an
|
// passes all filtering and policy checks. You aren't allowed to use an
|
||||||
|
@ -204,6 +211,19 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
|
||||||
get_class($this)));
|
get_class($this)));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if ($this->supportsFerretEngine()) {
|
||||||
|
if ($this->getFerretTokens()) {
|
||||||
|
$map += array(
|
||||||
|
'rank' =>
|
||||||
|
$cursor->getRawRowProperty(self::FULLTEXT_RANK),
|
||||||
|
'fulltext-modified' =>
|
||||||
|
$cursor->getRawRowProperty(self::FULLTEXT_MODIFIED),
|
||||||
|
'fulltext-created' =>
|
||||||
|
$cursor->getRawRowProperty(self::FULLTEXT_CREATED),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
foreach ($keys as $key) {
|
foreach ($keys as $key) {
|
||||||
if (!array_key_exists($key, $map)) {
|
if (!array_key_exists($key, $map)) {
|
||||||
throw new Exception(
|
throw new Exception(
|
||||||
|
@ -295,6 +315,8 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
|
||||||
}
|
}
|
||||||
|
|
||||||
protected function didLoadRawRows(array $rows) {
|
protected function didLoadRawRows(array $rows) {
|
||||||
|
$this->rawCursorRow = last($rows);
|
||||||
|
|
||||||
if ($this->ferretEngine) {
|
if ($this->ferretEngine) {
|
||||||
foreach ($rows as $row) {
|
foreach ($rows as $row) {
|
||||||
$phid = $row['phid'];
|
$phid = $row['phid'];
|
||||||
|
@ -312,8 +334,6 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
$this->rawCursorRow = last($rows);
|
|
||||||
|
|
||||||
return $rows;
|
return $rows;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -467,7 +487,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
|
||||||
*/
|
*/
|
||||||
protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
|
protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
|
||||||
$where = array();
|
$where = array();
|
||||||
$where[] = $this->buildPagingClause($conn);
|
$where[] = $this->buildPagingWhereClause($conn);
|
||||||
$where[] = $this->buildEdgeLogicWhereClause($conn);
|
$where[] = $this->buildEdgeLogicWhereClause($conn);
|
||||||
$where[] = $this->buildSpacesWhereClause($conn);
|
$where[] = $this->buildSpacesWhereClause($conn);
|
||||||
$where[] = $this->buildNgramsWhereClause($conn);
|
$where[] = $this->buildNgramsWhereClause($conn);
|
||||||
|
@ -482,6 +502,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
|
||||||
*/
|
*/
|
||||||
protected function buildHavingClause(AphrontDatabaseConnection $conn) {
|
protected function buildHavingClause(AphrontDatabaseConnection $conn) {
|
||||||
$having = $this->buildHavingClauseParts($conn);
|
$having = $this->buildHavingClauseParts($conn);
|
||||||
|
$having[] = $this->buildPagingHavingClause($conn);
|
||||||
return $this->formatHavingClause($conn, $having);
|
return $this->formatHavingClause($conn, $having);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -539,6 +560,45 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
|
||||||
/* -( Paging )------------------------------------------------------------- */
|
/* -( Paging )------------------------------------------------------------- */
|
||||||
|
|
||||||
|
|
||||||
|
private function buildPagingWhereClause(AphrontDatabaseConnection $conn) {
|
||||||
|
if ($this->shouldPageWithHavingClause()) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
return $this->buildPagingClause($conn);
|
||||||
|
}
|
||||||
|
|
||||||
|
private function buildPagingHavingClause(AphrontDatabaseConnection $conn) {
|
||||||
|
if (!$this->shouldPageWithHavingClause()) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
return $this->buildPagingClause($conn);
|
||||||
|
}
|
||||||
|
|
||||||
|
private function shouldPageWithHavingClause() {
|
||||||
|
// If any of the paging conditions reference dynamic columns, we need to
|
||||||
|
// put the paging conditions in a "HAVING" clause instead of a "WHERE"
|
||||||
|
// clause.
|
||||||
|
|
||||||
|
// For example, this happens when paging on the Ferret "rank" column,
|
||||||
|
// since the "rank" value is computed dynamically in the SELECT statement.
|
||||||
|
|
||||||
|
$orderable = $this->getOrderableColumns();
|
||||||
|
$vector = $this->getOrderVector();
|
||||||
|
|
||||||
|
foreach ($vector as $order) {
|
||||||
|
$key = $order->getOrderKey();
|
||||||
|
$column = $orderable[$key];
|
||||||
|
|
||||||
|
if (!empty($column['having'])) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @task paging
|
* @task paging
|
||||||
*/
|
*/
|
||||||
|
@ -655,6 +715,8 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
|
||||||
'reverse' => 'optional bool',
|
'reverse' => 'optional bool',
|
||||||
'unique' => 'optional bool',
|
'unique' => 'optional bool',
|
||||||
'null' => 'optional string|null',
|
'null' => 'optional string|null',
|
||||||
|
'requires-ferret' => 'optional bool',
|
||||||
|
'having' => 'optional bool',
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -1106,6 +1168,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
|
||||||
'column' => self::FULLTEXT_RANK,
|
'column' => self::FULLTEXT_RANK,
|
||||||
'type' => 'int',
|
'type' => 'int',
|
||||||
'requires-ferret' => true,
|
'requires-ferret' => true,
|
||||||
|
'having' => true,
|
||||||
);
|
);
|
||||||
$columns['fulltext-created'] = array(
|
$columns['fulltext-created'] = array(
|
||||||
'table' => null,
|
'table' => null,
|
||||||
|
|
Loading…
Reference in a new issue