1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-23 22:10:55 +01:00

Convert complex query subclasses to use internal cursors

Summary:
Depends on D20292. Ref T13259. This converts the rest of the `getPagingValueMap()` callsites to operate on internal cursors instead.

These are pretty one-off for the most part, so I'll annotate them inline.

Test Plan:
  - Grouped tasks by project, sorted by title, paged through them, saw consistent outcomes.
  - Queried edges with "edge.search", paged through them using the "after" cursor.
  - Poked around the other stuff without catching any brokenness.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13259

Differential Revision: https://secure.phabricator.com/D20293
This commit is contained in:
epriestley 2019-03-18 13:55:57 -07:00
parent d4847c3eeb
commit 8449c1793a
9 changed files with 316 additions and 211 deletions

View file

@ -78,6 +78,16 @@ final class AlmanacInterfaceQuery
return $interfaces; return $interfaces;
} }
protected function buildSelectClauseParts(AphrontDatabaseConnection $conn) {
$select = parent::buildSelectClauseParts($conn);
if ($this->shouldJoinDeviceTable()) {
$select[] = qsprintf($conn, 'device.name');
}
return $select;
}
protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
$where = parent::buildWhereClauseParts($conn); $where = parent::buildWhereClauseParts($conn);
@ -186,15 +196,16 @@ final class AlmanacInterfaceQuery
); );
} }
protected function getPagingValueMap($cursor, array $keys) { protected function newPagingMapFromCursorObject(
$interface = $this->loadCursorObject($cursor); PhabricatorQueryCursor $cursor,
array $keys) {
$map = array( $interface = $cursor->getObject();
'id' => $interface->getID(),
'name' => $interface->getDevice()->getName(), return array(
'id' => (int)$interface->getID(),
'name' => $cursor->getRawRowProperty('device.name'),
); );
return $map;
} }
} }

View file

@ -147,17 +147,21 @@ final class PhabricatorFeedQuery
); );
} }
protected function getPagingValueMap($cursor, array $keys) { protected function applyExternalCursorConstraintsToQuery(
return array( PhabricatorCursorPagedPolicyAwareQuery $subquery,
'key' => $cursor, $cursor) {
); $subquery->withChronologicalKeys(array($cursor));
} }
protected function getResultCursor($item) { protected function newExternalCursorStringForResult($object) {
if ($item instanceof PhabricatorFeedStory) { return $object->getChronologicalKey();
return $item->getChronologicalKey(); }
}
return $item['chronologicalKey']; protected function newPagingMapFromPartialObject($object) {
// This query is unusual, and the "object" is a raw result row.
return array(
'key' => $object['chronologicalKey'],
);
} }
protected function getPrimaryTableAlias() { protected function getPrimaryTableAlias() {

View file

@ -27,6 +27,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
private $closedEpochMax; private $closedEpochMax;
private $closerPHIDs; private $closerPHIDs;
private $columnPHIDs; private $columnPHIDs;
private $specificGroupByProjectPHID;
private $status = 'status-any'; private $status = 'status-any';
const STATUS_ANY = 'status-any'; const STATUS_ANY = 'status-any';
@ -227,6 +228,11 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
return $this; return $this;
} }
public function withSpecificGroupByProjectPHID($project_phid) {
$this->specificGroupByProjectPHID = $project_phid;
return $this;
}
public function newResultObject() { public function newResultObject() {
return new ManiphestTask(); return new ManiphestTask();
} }
@ -534,6 +540,13 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
$select_phids); $select_phids);
} }
if ($this->specificGroupByProjectPHID !== null) {
$where[] = qsprintf(
$conn,
'projectGroupName.indexedObjectPHID = %s',
$this->specificGroupByProjectPHID);
}
return $where; return $where;
} }
@ -824,16 +837,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
return array_mergev($phids); return array_mergev($phids);
} }
protected function getResultCursor($result) {
$id = $result->getID();
if ($this->groupBy == self::GROUP_PROJECT) {
return rtrim($id.'.'.$result->getGroupByProjectPHID(), '.');
}
return $id;
}
public function getBuiltinOrders() { public function getBuiltinOrders() {
$orders = array( $orders = array(
'priority' => array( 'priority' => array(
@ -926,39 +929,37 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
); );
} }
protected function getPagingValueMap($cursor, array $keys) { protected function newPagingMapFromCursorObject(
$cursor_parts = explode('.', $cursor, 2); PhabricatorQueryCursor $cursor,
$task_id = $cursor_parts[0]; array $keys) {
$group_id = idx($cursor_parts, 1);
$task = $this->loadCursorObject($task_id); $task = $cursor->getObject();
$map = array( $map = array(
'id' => $task->getID(), 'id' => (int)$task->getID(),
'priority' => $task->getPriority(), 'priority' => (int)$task->getPriority(),
'owner' => $task->getOwnerOrdering(), 'owner' => $task->getOwnerOrdering(),
'status' => $task->getStatus(), 'status' => $task->getStatus(),
'title' => $task->getTitle(), 'title' => $task->getTitle(),
'updated' => $task->getDateModified(), 'updated' => (int)$task->getDateModified(),
'closed' => $task->getClosedEpoch(), 'closed' => $task->getClosedEpoch(),
); );
foreach ($keys as $key) { if (isset($keys['project'])) {
switch ($key) { $value = null;
case 'project':
$value = null; $group_phid = $task->getGroupByProjectPHID();
if ($group_id) { if ($group_phid) {
$paging_projects = id(new PhabricatorProjectQuery()) $paging_projects = id(new PhabricatorProjectQuery())
->setViewer($this->getViewer()) ->setViewer($this->getViewer())
->withPHIDs(array($group_id)) ->withPHIDs(array($group_phid))
->execute(); ->execute();
if ($paging_projects) { if ($paging_projects) {
$value = head($paging_projects)->getName(); $value = head($paging_projects)->getName();
} }
}
$map[$key] = $value;
break;
} }
$map['project'] = $value;
} }
foreach ($keys as $key) { foreach ($keys as $key) {
@ -971,6 +972,77 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
return $map; return $map;
} }
protected function newExternalCursorStringForResult($object) {
$id = $object->getID();
if ($this->groupBy == self::GROUP_PROJECT) {
return rtrim($id.'.'.$object->getGroupByProjectPHID(), '.');
}
return $id;
}
protected function newInternalCursorFromExternalCursor($cursor) {
list($task_id, $group_phid) = $this->parseCursor($cursor);
$cursor_object = parent::newInternalCursorFromExternalCursor($cursor);
if ($group_phid !== null) {
$project = id(new PhabricatorProjectQuery())
->setViewer($this->getViewer())
->withPHIDs(array($group_phid))
->execute();
if (!$project) {
$this->throwCursorException(
pht(
'Group PHID ("%s") component of cursor ("%s") is not valid.',
$group_phid,
$cursor));
}
$cursor_object->getObject()->attachGroupByProjectPHID($group_phid);
}
return $cursor_object;
}
protected function applyExternalCursorConstraintsToQuery(
PhabricatorCursorPagedPolicyAwareQuery $subquery,
$cursor) {
list($task_id, $group_phid) = $this->parseCursor($cursor);
$subquery->withIDs(array($task_id));
if ($group_phid) {
$subquery->setGroupBy(self::GROUP_PROJECT);
// The subquery needs to return exactly one result. If a task is in
// several projects, the query may naturally return several results.
// Specify that we want only the particular instance of the task in
// the specified project.
$subquery->withSpecificGroupByProjectPHID($group_phid);
}
}
private function parseCursor($cursor) {
// Split a "123.PHID-PROJ-abcd" cursor into a "Task ID" part and a
// "Project PHID" part.
$parts = explode('.', $cursor, 2);
if (count($parts) < 2) {
$parts[] = null;
}
if (!strlen($parts[1])) {
$parts[1] = null;
}
return $parts;
}
protected function getPrimaryTableAlias() { protected function getPrimaryTableAlias() {
return 'task'; return 'task';
} }

View file

@ -168,10 +168,20 @@ final class PhrictionDocumentQuery
return $documents; return $documents;
} }
protected function buildSelectClauseParts(AphrontDatabaseConnection $conn) {
$select = parent::buildSelectClauseParts($conn);
if ($this->shouldJoinContentTable()) {
$select[] = qsprintf($conn, 'c.title');
}
return $select;
}
protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
$joins = parent::buildJoinClauseParts($conn); $joins = parent::buildJoinClauseParts($conn);
if ($this->getOrderVector()->containsKey('updated')) { if ($this->shouldJoinContentTable()) {
$content_dao = new PhrictionContent(); $content_dao = new PhrictionContent();
$joins[] = qsprintf( $joins[] = qsprintf(
$conn, $conn,
@ -182,6 +192,10 @@ final class PhrictionDocumentQuery
return $joins; return $joins;
} }
private function shouldJoinContentTable() {
return $this->getOrderVector()->containsKey('updated');
}
protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
$where = parent::buildWhereClauseParts($conn); $where = parent::buildWhereClauseParts($conn);
@ -354,35 +368,25 @@ final class PhrictionDocumentQuery
); );
} }
protected function getPagingValueMap($cursor, array $keys) { protected function newPagingMapFromCursorObject(
$document = $this->loadCursorObject($cursor); PhabricatorQueryCursor $cursor,
array $keys) {
$document = $cursor->getObject();
$map = array( $map = array(
'id' => $document->getID(), 'id' => (int)$document->getID(),
'depth' => $document->getDepth(), 'depth' => $document->getDepth(),
'updated' => $document->getEditedEpoch(), 'updated' => (int)$document->getEditedEpoch(),
); );
foreach ($keys as $key) { if (isset($keys['title'])) {
switch ($key) { $map['title'] = $cursor->getRawRowProperty('c.title');
case 'title':
$map[$key] = $document->getContent()->getTitle();
break;
}
} }
return $map; return $map;
} }
protected function willExecuteCursorQuery(
PhabricatorCursorPagedPolicyAwareQuery $query) {
$vector = $this->getOrderVector();
if ($vector->containsKey('title')) {
$query->needContent(true);
}
}
protected function getPrimaryTableAlias() { protected function getPrimaryTableAlias() {
return 'd'; return 'd';
} }

View file

@ -442,47 +442,24 @@ final class PhabricatorRepositoryQuery
); );
} }
protected function willExecuteCursorQuery( protected function newPagingMapFromCursorObject(
PhabricatorCursorPagedPolicyAwareQuery $query) { PhabricatorQueryCursor $cursor,
$vector = $this->getOrderVector(); array $keys) {
if ($vector->containsKey('committed')) { $repository = $cursor->getObject();
$query->needMostRecentCommits(true);
}
if ($vector->containsKey('size')) {
$query->needCommitCounts(true);
}
}
protected function getPagingValueMap($cursor, array $keys) {
$repository = $this->loadCursorObject($cursor);
$map = array( $map = array(
'id' => $repository->getID(), 'id' => (int)$repository->getID(),
'callsign' => $repository->getCallsign(), 'callsign' => $repository->getCallsign(),
'name' => $repository->getName(), 'name' => $repository->getName(),
); );
foreach ($keys as $key) { if (isset($keys['committed'])) {
switch ($key) { $map['committed'] = $cursor->getRawRowProperty('epoch');
case 'committed': }
$commit = $repository->getMostRecentCommit();
if ($commit) { if (isset($keys['size'])) {
$map[$key] = $commit->getEpoch(); $map['size'] = $cursor->getRawRowProperty('size');
} else {
$map[$key] = null;
}
break;
case 'size':
$count = $repository->getCommitCount();
if ($count) {
$map[$key] = $count;
} else {
$map[$key] = null;
}
break;
}
} }
return $map; return $map;
@ -491,8 +468,6 @@ final class PhabricatorRepositoryQuery
protected function buildSelectClauseParts(AphrontDatabaseConnection $conn) { protected function buildSelectClauseParts(AphrontDatabaseConnection $conn) {
$parts = parent::buildSelectClauseParts($conn); $parts = parent::buildSelectClauseParts($conn);
$parts[] = qsprintf($conn, 'r.*');
if ($this->shouldJoinSummaryTable()) { if ($this->shouldJoinSummaryTable()) {
$parts[] = qsprintf($conn, 's.*'); $parts[] = qsprintf($conn, 's.*');
} }

View file

@ -8,14 +8,18 @@ final class PhabricatorEdgeObject
private $src; private $src;
private $dst; private $dst;
private $type; private $type;
private $dateCreated;
private $sequence;
public static function newFromRow(array $row) { public static function newFromRow(array $row) {
$edge = new self(); $edge = new self();
$edge->id = $row['id']; $edge->id = idx($row, 'id');
$edge->src = $row['src']; $edge->src = idx($row, 'src');
$edge->dst = $row['dst']; $edge->dst = idx($row, 'dst');
$edge->type = $row['type']; $edge->type = idx($row, 'type');
$edge->dateCreated = idx($row, 'dateCreated');
$edge->sequence = idx($row, 'seq');
return $edge; return $edge;
} }
@ -40,6 +44,15 @@ final class PhabricatorEdgeObject
return null; return null;
} }
public function getDateCreated() {
return $this->dateCreated;
}
public function getSequence() {
return $this->sequence;
}
/* -( PhabricatorPolicyInterface )----------------------------------------- */ /* -( PhabricatorPolicyInterface )----------------------------------------- */

View file

@ -12,7 +12,6 @@ final class PhabricatorEdgeObjectQuery
private $edgeTypes; private $edgeTypes;
private $destinationPHIDs; private $destinationPHIDs;
public function withSourcePHIDs(array $source_phids) { public function withSourcePHIDs(array $source_phids) {
$this->sourcePHIDs = $source_phids; $this->sourcePHIDs = $source_phids;
return $this; return $this;
@ -85,18 +84,6 @@ final class PhabricatorEdgeObjectQuery
return $result; return $result;
} }
protected function buildSelectClauseParts(AphrontDatabaseConnection $conn) {
$parts = parent::buildSelectClauseParts($conn);
// TODO: This is hacky, because we don't have real IDs on this table.
$parts[] = qsprintf(
$conn,
'CONCAT(dateCreated, %s, seq) AS id',
'_');
return $parts;
}
protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
$parts = parent::buildWhereClauseParts($conn); $parts = parent::buildWhereClauseParts($conn);
@ -151,13 +138,45 @@ final class PhabricatorEdgeObjectQuery
return array('dateCreated', 'sequence'); return array('dateCreated', 'sequence');
} }
protected function getPagingValueMap($cursor, array $keys) { protected function newInternalCursorFromExternalCursor($cursor) {
$parts = explode('_', $cursor); list($epoch, $sequence) = $this->parseCursor($cursor);
// Instead of actually loading an edge, we're just making a fake edge
// with the properties the cursor describes.
$edge_object = PhabricatorEdgeObject::newFromRow(
array(
'dateCreated' => $epoch,
'seq' => $sequence,
));
return id(new PhabricatorQueryCursor())
->setObject($edge_object);
}
protected function newPagingMapFromPartialObject($object) {
return array( return array(
'dateCreated' => $parts[0], 'dateCreated' => $object->getDateCreated(),
'sequence' => $parts[1], 'sequence' => $object->getSequence(),
); );
} }
protected function newExternalCursorStringForResult($object) {
return sprintf(
'%d_%d',
$object->getDateCreated(),
$object->getSequence());
}
private function parseCursor($cursor) {
if (!preg_match('/^\d+_\d+\z/', $cursor)) {
$this->throwCursorException(
pht(
'Expected edge cursor in the form "0123_6789", got "%s".',
$cursor));
}
return explode('_', $cursor);
}
} }

View file

@ -19,6 +19,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
private $externalCursorString; private $externalCursorString;
private $internalCursorObject; private $internalCursorObject;
private $isQueryOrderReversed = false; private $isQueryOrderReversed = false;
private $rawCursorRow;
private $applicationSearchConstraints = array(); private $applicationSearchConstraints = array();
private $internalPaging; private $internalPaging;
@ -53,7 +54,60 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
} }
protected function newInternalCursorFromExternalCursor($cursor) { protected function newInternalCursorFromExternalCursor($cursor) {
return $this->newInternalCursorObjectFromID($cursor); $viewer = $this->getViewer();
$query = newv(get_class($this), array());
$query
->setParentQuery($this)
->setViewer($viewer);
// We're copying our order vector to the subquery so that the subquery
// knows it should generate any supplemental information required by the
// ordering.
// For example, Phriction documents may be ordered by title, but the title
// isn't a column in the "document" table: the query must JOIN the
// "content" table to perform the ordering. Passing the ordering to the
// subquery tells it that we need it to do that JOIN and attach relevant
// paging information to the internal cursor object.
// We only expect to load a single result, so the actual result order does
// not matter. We only want the internal cursor for that result to look
// like a cursor this parent query would generate.
$query->setOrderVector($this->getOrderVector());
$this->applyExternalCursorConstraintsToQuery($query, $cursor);
// 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
// passes all filtering and policy checks. You aren't allowed to use an
// object you can't see as a cursor, since this can leak information.
$result = $query->executeOne();
if (!$result) {
$this->throwCursorException(
pht(
'Cursor "%s" does not identify a valid object in query "%s".',
$cursor,
get_class($this)));
}
// Now that we made sure the viewer can actually see the object the
// external cursor identifies, return the internal cursor the query
// generated as a side effect while loading the object.
return $query->getInternalCursorObject();
}
final protected function throwCursorException($message) {
// TODO: Raise a more tailored exception here and make the UI a little
// prettier?
throw new Exception($message);
}
protected function applyExternalCursorConstraintsToQuery(
PhabricatorCursorPagedPolicyAwareQuery $subquery,
$cursor) {
$subquery->withIDs(array($cursor));
} }
protected function newPagingMapFromCursorObject( protected function newPagingMapFromCursorObject(
@ -71,51 +125,6 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
); );
} }
final protected function newInternalCursorObjectFromID($id) {
$viewer = $this->getViewer();
$query = newv(get_class($this), array());
$query
->setParentQuery($this)
->setViewer($viewer)
->withIDs(array((int)$id));
// We're copying our order vector to the subquery so that the subquery
// knows it should generate any supplemental information required by the
// ordering.
// For example, Phriction documents may be ordered by title, but the title
// isn't a column in the "document" table: the query must JOIN the
// "content" table to perform the ordering. Passing the ordering to the
// subquery tells it that we need it to do that JOIN and attach relevant
// paging information to the internal cursor object.
// We only expect to load a single result, so the actual result order does
// not matter. We only want the internal cursor for that result to look
// like a cursor this parent query would generate.
$query->setOrderVector($this->getOrderVector());
// 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
// passes all filtering and policy checks. You aren't allowed to use an
// object you can't see as a cursor, since this can leak information.
$result = $query->executeOne();
if (!$result) {
// TODO: Raise a more tailored exception here and make the UI a little
// prettier?
throw new Exception(
pht(
'Cursor "%s" does not identify a valid object in query "%s".',
$id,
get_class($this)));
}
// Now that we made sure the viewer can actually see the object the
// external cursor identifies, return the internal cursor the query
// generated as a side effect while loading the object.
return $query->getInternalCursorObject();
}
final private function getExternalCursorStringForResult($object) { final private function getExternalCursorStringForResult($object) {
$cursor = $this->newExternalCursorStringForResult($object); $cursor = $this->newExternalCursorStringForResult($object);
@ -215,6 +224,10 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
$cursor = id(new PhabricatorQueryCursor()) $cursor = id(new PhabricatorQueryCursor())
->setObject(last($page)); ->setObject(last($page));
if ($this->rawCursorRow) {
$cursor->setRawRow($this->rawCursorRow);
}
$this->setInternalCursorObject($cursor); $this->setInternalCursorObject($cursor);
} }
@ -295,46 +308,9 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
} }
} }
return $rows; $this->rawCursorRow = last($rows);
}
/** return $rows;
* Get the viewer for making cursor paging queries.
*
* NOTE: You should ONLY use this viewer to load cursor objects while
* building paging queries.
*
* Cursor paging can happen in two ways. First, the user can request a page
* like `/stuff/?after=33`, which explicitly causes paging. Otherwise, we
* can fall back to implicit paging if we filter some results out of a
* result list because the user can't see them and need to go fetch some more
* results to generate a large enough result list.
*
* In the first case, want to use the viewer's policies to load the object.
* This prevents an attacker from figuring out information about an object
* they can't see by executing queries like `/stuff/?after=33&order=name`,
* which would otherwise give them a hint about the name of the object.
* Generally, if a user can't see an object, they can't use it to page.
*
* In the second case, we need to load the object whether the user can see
* it or not, because we need to examine new results. For example, if a user
* loads `/stuff/` and we run a query for the first 100 items that they can
* see, but the first 100 rows in the database aren't visible, we need to
* be able to issue a query for the next 100 results. If we can't load the
* cursor object, we'll fail or issue the same query over and over again.
* So, generally, internal paging must bypass policy controls.
*
* This method returns the appropriate viewer, based on the context in which
* the paging is occurring.
*
* @return PhabricatorUser Viewer for executing paging queries.
*/
final protected function getPagingViewer() {
if ($this->internalPaging) {
return PhabricatorUser::getOmnipotentUser();
} else {
return $this->getViewer();
}
} }
final protected function buildLimitClause(AphrontDatabaseConnection $conn) { final protected function buildLimitClause(AphrontDatabaseConnection $conn) {
@ -590,6 +566,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
foreach ($vector as $order) { foreach ($vector as $order) {
$keys[] = $order->getOrderKey(); $keys[] = $order->getOrderKey();
} }
$keys = array_fuse($keys);
$value_map = $this->getPagingMapFromCursorObject( $value_map = $this->getPagingMapFromCursorObject(
$cursor_object, $cursor_object,

View file

@ -4,6 +4,7 @@ final class PhabricatorQueryCursor
extends Phobject { extends Phobject {
private $object; private $object;
private $rawRow;
public function setObject($object) { public function setObject($object) {
$this->object = $object; $this->object = $object;
@ -14,4 +15,33 @@ final class PhabricatorQueryCursor
return $this->object; return $this->object;
} }
public function setRawRow(array $raw_row) {
$this->rawRow = $raw_row;
return $this;
}
public function getRawRow() {
return $this->rawRow;
}
public function getRawRowProperty($key) {
if (!is_array($this->rawRow)) {
throw new Exception(
pht(
'Caller is trying to "getRawRowProperty()" with key "%s", but this '.
'cursor has no raw row.',
$key));
}
if (!array_key_exists($key, $this->rawRow)) {
throw new Exception(
pht(
'Caller is trying to access raw row property "%s", but the row '.
'does not have this property.',
$key));
}
return $this->rawRow[$key];
}
} }