1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-28 16:30:59 +01:00

Use Edges to store dependencies between tasks in Maniphest

Summary:
  - Use edges to store "X depends on Y" information in Maniphest.
  - Show both "Depends On" and "Dependent Tasks".
  - Migrate all the old edges.

Test Plan:
  - Added some relationships, migrated, verified they were preserved.
  - Added some new valid relationships, verified tasks got updated with sensible transactions and sent reasonable emails.
  - Tried to add a cycle, got an ugly but effective error.

Reviewers: vrana, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1162

Differential Revision: https://secure.phabricator.com/D3006
This commit is contained in:
epriestley 2012-07-18 20:41:42 -07:00
parent 409974fbd6
commit 9196a6bd9f
11 changed files with 285 additions and 170 deletions

View file

@ -0,0 +1,42 @@
<?php
/*
* Copyright 2012 Facebook, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
echo "Migrating task dependencies to edges...\n";
foreach (new LiskMigrationIterator(new ManiphestTask()) as $task) {
$id = $task->getID();
echo "Task {$id}: ";
$deps = $task->getAttachedPHIDs(PhabricatorPHIDConstants::PHID_TYPE_TASK);
if (!$deps) {
echo "-\n";
continue;
}
$editor = new PhabricatorEdgeEditor();
$editor->setSuppressEvents(true);
foreach ($deps as $dep) {
$editor->addEdge(
$task->getPHID(),
PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK,
$dep);
}
$editor->save();
echo "OKAY\n";
}
echo "Done.\n";

View file

@ -815,7 +815,6 @@ phutil_register_library_map(array(
'PhabricatorOAuthServerTokenController' => 'applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php',
'PhabricatorOAuthUnlinkController' => 'applications/auth/controller/PhabricatorOAuthUnlinkController.php',
'PhabricatorObjectAttachmentEditor' => 'applications/search/editor/PhabricatorObjectAttachmentEditor.php',
'PhabricatorObjectGraph' => 'applications/phid/PhabricatorObjectGraph.php',
'PhabricatorObjectHandle' => 'applications/phid/PhabricatorObjectHandle.php',
'PhabricatorObjectHandleConstants' => 'applications/phid/handle/const/PhabricatorObjectHandleConstants.php',
'PhabricatorObjectHandleData' => 'applications/phid/handle/PhabricatorObjectHandleData.php',
@ -1824,7 +1823,6 @@ phutil_register_library_map(array(
'PhabricatorOAuthServerTestController' => 'PhabricatorOAuthServerController',
'PhabricatorOAuthServerTokenController' => 'PhabricatorAuthController',
'PhabricatorOAuthUnlinkController' => 'PhabricatorAuthController',
'PhabricatorObjectGraph' => 'AbstractDirectedGraph',
'PhabricatorObjectHandleStatus' => 'PhabricatorObjectHandleConstants',
'PhabricatorOffsetPagedQuery' => 'PhabricatorQuery',
'PhabricatorOwnersController' => 'PhabricatorController',

View file

@ -51,11 +51,28 @@ final class ManiphestTaskDetailController extends ManiphestController {
'taskID = %d ORDER BY id ASC',
$task->getID());
$commit_phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
$task->getPHID(),
PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT);
$e_commit = PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT;
$e_dep_on = PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK;
$e_dep_by = PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK;
$phid = $task->getPHID();
$query = id(new PhabricatorEdgeQuery())
->withSourcePHIDs(array($phid))
->withEdgeTypes(
array(
$e_commit,
$e_dep_on,
$e_dep_by,
));
$edges = $query->execute();
$commit_phids = array_keys($edges[$phid][$e_commit]);
$dep_on_tasks = array_keys($edges[$phid][$e_dep_on]);
$dep_by_tasks = array_keys($edges[$phid][$e_dep_by]);
$phids = array_fill_keys($query->getDestinationPHIDs(), true);
$phids = array_fill_keys($commit_phids, true);
foreach ($transactions as $transaction) {
foreach ($transaction->extractPHIDs() as $phid) {
$phids[$phid] = true;
@ -150,33 +167,25 @@ final class ManiphestTaskDetailController extends ManiphestController {
}
}
$dtasks = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_TASK);
if ($dtasks) {
$dtask_links = array();
foreach ($dtasks as $dtask => $info) {
$dtask_links[] = $handles[$dtask]->renderLink();
}
$dtask_links = implode('<br />', $dtask_links);
$dict['Depends On'] = $dtask_links;
if ($dep_by_tasks) {
$dict['Dependent Tasks'] = $this->renderHandleList(
array_select_keys($handles, $dep_by_tasks));
}
if ($dep_on_tasks) {
$dict['Depends On'] = $this->renderHandleList(
array_select_keys($handles, $dep_on_tasks));
}
$revs = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_DREV);
if ($revs) {
$rev_links = array();
foreach ($revs as $rev => $info) {
$rev_links[] = $handles[$rev]->renderLink();
}
$rev_links = implode('<br />', $rev_links);
$dict['Revisions'] = $rev_links;
$dict['Revisions'] = $this->renderHandleList(
array_select_keys($handles, $revs));
}
if ($commit_phids) {
$commit_links = array();
foreach ($commit_phids as $phid) {
$commit_links[] = $handles[$phid]->renderLink();
}
$commit_links = implode('<br />', $commit_links);
$dict['Commits'] = $commit_links;
$dict['Commits'] = $this->renderHandleList(
array_select_keys($handles, $commit_phids));
}
$file_infos = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_FILE);
@ -534,4 +543,12 @@ final class ManiphestTaskDetailController extends ManiphestController {
));
}
private function renderHandleList(array $handles) {
$links = array();
foreach ($handles as $handle) {
$links[] = $handle->renderLink();
}
return implode('<br />', $links);
}
}

View file

@ -60,6 +60,18 @@ final class ManiphestTask extends ManiphestDAO
) + parent::getConfiguration();
}
public function loadDependsOnTaskPHIDs() {
return PhabricatorEdgeQuery::loadDestinationPHIDs(
$this->getPHID(),
PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK);
}
public function loadDependedOnByTaskPHIDs() {
return PhabricatorEdgeQuery::loadDestinationPHIDs(
$this->getPHID(),
PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK);
}
public function getAttachedPHIDs($type) {
return array_keys(idx($this->attached, $type, array()));
}

View file

@ -657,8 +657,16 @@ final class ManiphestTransactionDetailView extends ManiphestView {
}
}
/**
* @task strings
*/
private function getEdgeEmailTitle($type, $count) {
switch ($type) {
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
return pht('DEPENDS ON %d TASK(S)', $count);
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK:
return pht('DEPENDENT %d TASK(s)', $count);
case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT:
return pht('ATTACHED %d COMMIT(S)', $count);
default:
@ -672,6 +680,10 @@ final class ManiphestTransactionDetailView extends ManiphestView {
*/
private function getEdgeAddVerb($type) {
switch ($type) {
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
return pht('Added Dependency');
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK:
return pht('Added Dependent Task');
case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT:
return pht('Added Commit');
default:
@ -685,6 +697,10 @@ final class ManiphestTransactionDetailView extends ManiphestView {
*/
private function getEdgeRemVerb($type) {
switch ($type) {
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
return pht('Removed Dependency');
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK:
return pht('Removed Dependent Task');
case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT:
return pht('Removed Commit');
default:
@ -698,6 +714,10 @@ final class ManiphestTransactionDetailView extends ManiphestView {
*/
private function getEdgeEditVerb($type) {
switch ($type) {
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
return pht('Changed Dependencies');
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK:
return pht('Changed Dependent Tasks');
case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT:
return pht('Changed Commits');
default:
@ -713,6 +733,10 @@ final class ManiphestTransactionDetailView extends ManiphestView {
$list = $this->renderHandles(array_keys($add));
switch ($type) {
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
return pht('added %d dependencie(s): %s', $add, $list);
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK:
return pht('added %d dependent task(s): %s', $add, $list);
case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT:
return pht('added %d commit(s): %s', $add, $list);
default:
@ -728,6 +752,10 @@ final class ManiphestTransactionDetailView extends ManiphestView {
$list = $this->renderHandles(array_keys($rem));
switch ($type) {
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
return pht('removed %d dependencie(s): %s', $rem, $list);
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK:
return pht('removed %d dependent task(s): %s', $rem, $list);
case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT:
return pht('removed %d commit(s): %s', $rem, $list);
default:
@ -744,6 +772,22 @@ final class ManiphestTransactionDetailView extends ManiphestView {
$rem_list = $this->renderHandles(array_keys($rem));
switch ($type) {
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
return pht(
'changed %d dependencie(s), added %d: %s; removed %d: %s',
count($add) + count($rem),
$add,
$add_list,
$rem,
$rem_list);
case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK:
return pht(
'changed %d dependent task(s), added %d: %s; removed %d: %s',
count($add) + count($rem),
$add,
$add_list,
$rem,
$rem_list);
case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT:
return pht(
'changed %d commit(s), added %d: %s; removed %d: %s',

View file

@ -1,49 +0,0 @@
<?php
/*
* Copyright 2011 Facebook, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
final class PhabricatorObjectGraph extends AbstractDirectedGraph {
private $edgeType;
public function setEdgeType($edge_type) {
$this->edgeType = $edge_type;
return $this;
}
protected function loadEdges(array $nodes) {
if (!$this->edgeType) {
throw new Exception("Set edge type before loading graph!");
}
$handle_data = new PhabricatorObjectHandleData($nodes);
$objects = $handle_data->loadObjects();
$result = array();
foreach ($nodes as $phid) {
$object = idx($objects, $phid);
if ($object) {
$result[$phid] = $object->getAttachedPHIDs($this->edgeType);
} else {
$result[$phid] = array();
}
}
return $result;
}
}

View file

@ -56,74 +56,76 @@ final class PhabricatorSearchAttachController
return new Aphront404Response();
}
$edge_type = null;
switch ($this->action) {
case self::ACTION_EDGE:
case self::ACTION_DEPENDENCIES:
$edge_type = $this->getEdgeType($object_type, $attach_type);
break;
}
if ($request->isFormPost()) {
$phids = explode(';', $request->getStr('phids'));
$phids = array_filter($phids);
$phids = array_values($phids);
switch ($this->action) {
case self::ACTION_MERGE:
return $this->performMerge($object, $handle, $phids);
if ($edge_type) {
$old_phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
$this->phid,
$edge_type);
$add_phids = $phids;
$rem_phids = array_diff($old_phids, $add_phids);
case self::ACTION_EDGE:
$edge_type = $this->getEdgeType($object_type, $attach_type);
$editor = id(new PhabricatorEdgeEditor());
$editor->setUser($user);
foreach ($add_phids as $phid) {
$editor->addEdge($this->phid, $edge_type, $phid);
}
foreach ($rem_phids as $phid) {
$editor->removeEdge($this->phid, $edge_type, $phid);
}
$old_phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
$this->phid,
$edge_type);
$add_phids = $phids;
$rem_phids = array_diff($old_phids, $add_phids);
$editor = id(new PhabricatorEdgeEditor());
$editor->setUser($user);
foreach ($add_phids as $phid) {
$editor->addEdge($this->phid, $edge_type, $phid);
}
foreach ($rem_phids as $phid) {
$editor->removeEdge($this->phid, $edge_type, $phid);
}
try {
$editor->save();
} catch (PhabricatorEdgeCycleException $ex) {
$this->raiseGraphCycleException($ex);
}
return id(new AphrontReloadResponse())->setURI($handle->getURI());
case self::ACTION_DEPENDENCIES:
case self::ACTION_ATTACH:
$two_way = true;
if ($this->action == self::ACTION_DEPENDENCIES) {
$two_way = false;
$this->detectGraphCycles(
$object,
return id(new AphrontReloadResponse())->setURI($handle->getURI());
} else {
switch ($this->action) {
case self::ACTION_MERGE:
return $this->performMerge($object, $handle, $phids);
case self::ACTION_ATTACH:
$editor = new PhabricatorObjectAttachmentEditor(
$object_type,
$object);
$editor->setUser($this->getRequest()->getUser());
$editor->attachObjects(
$attach_type,
$phids);
}
$phids,
$two_way = false);
$editor = new PhabricatorObjectAttachmentEditor(
$object_type,
$object);
$editor->setUser($this->getRequest()->getUser());
$editor->attachObjects(
$attach_type,
$phids,
$two_way);
return id(new AphrontReloadResponse())->setURI($handle->getURI());
default:
throw new Exception("Unsupported attach action.");
return id(new AphrontReloadResponse())->setURI($handle->getURI());
default:
throw new Exception("Unsupported attach action.");
}
}
} else {
switch ($this->action) {
case self::ACTION_ATTACH:
case self::ACTION_DEPENDENCIES:
$phids = $object->getAttachedPHIDs($attach_type);
break;
case self::ACTION_EDGE:
$edge_type = $this->getEdgeType($object_type, $attach_type);
$phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
$this->phid,
$edge_type);
break;
default:
$phids = array();
break;
if ($edge_type) {
$phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
$this->phid,
$edge_type);
} else {
switch ($this->action) {
case self::ACTION_ATTACH:
case self::ACTION_DEPENDENCIES:
$phids = $object->getAttachedPHIDs($attach_type);
break;
default:
$phids = array();
break;
}
}
}
@ -269,41 +271,6 @@ final class PhabricatorSearchAttachController
);
}
private function detectGraphCycles(
$object,
$attach_type,
array $phids) {
// Detect graph cycles.
$graph = new PhabricatorObjectGraph();
$graph->setEdgeType($attach_type);
$graph->addNodes(array(
$object->getPHID() => $phids,
));
$graph->loadGraph();
foreach ($phids as $phid) {
$cycle = $graph->detectCycles($phid);
if (!$cycle) {
continue;
}
// TODO: Improve this behavior so it's not all-or-nothing?
$handles = id(new PhabricatorObjectHandleData($cycle))
->loadHandles();
$names = array();
foreach ($cycle as $cycle_phid) {
$names[] = $handles[$cycle_phid]->getFullName();
}
$names = implode(" \xE2\x86\x92 ", $names);
$which = $handles[$phid]->getFullName();
throw new Exception(
"You can not create a dependency on '{$which}' because it ".
"would create a circular dependency: {$names}.");
}
}
private function getEdgeType($src_type, $dst_type) {
$t_cmit = PhabricatorPHIDConstants::PHID_TYPE_CMIT;
$t_task = PhabricatorPHIDConstants::PHID_TYPE_TASK;
@ -314,14 +281,30 @@ final class PhabricatorSearchAttachController
),
$t_task => array(
$t_cmit => PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT,
$t_task => PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK,
),
);
if (empty($map[$src_type][$dst_type])) {
throw new Exception("Unknown edge type!");
return null;
}
return $map[$src_type][$dst_type];
}
private function raiseGraphCycleException(PhabricatorEdgeCycleException $ex) {
$cycle = $ex->getCycle();
$handles = id(new PhabricatorObjectHandleData($cycle))
->loadHandles();
$names = array();
foreach ($cycle as $cycle_phid) {
$names[] = $handles[$cycle_phid]->getFullName();
}
$names = implode(" \xE2\x86\x92 ", $names);
throw new Exception(
"You can not create that dependency, because it would create a ".
"circular dependency: {$names}.");
}
}

View file

@ -21,15 +21,20 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants {
const TABLE_NAME_EDGE = 'edge';
const TABLE_NAME_EDGEDATA = 'edgedata';
const TYPE_TASK_HAS_COMMIT = 1;
const TYPE_COMMIT_HAS_TASK = 2;
const TYPE_TASK_HAS_COMMIT = 1;
const TYPE_COMMIT_HAS_TASK = 2;
const TYPE_TASK_DEPENDS_ON_TASK = 3;
const TYPE_TASK_DEPENDED_ON_BY_TASK = 4;
const TYPE_TEST_NO_CYCLE = 9000;
const TYPE_TEST_NO_CYCLE = 9000;
public static function getInverse($edge_type) {
static $map = array(
self::TYPE_TASK_HAS_COMMIT => self::TYPE_COMMIT_HAS_TASK,
self::TYPE_COMMIT_HAS_TASK => self::TYPE_TASK_HAS_COMMIT,
self::TYPE_TASK_DEPENDS_ON_TASK => self::TYPE_TASK_DEPENDED_ON_BY_TASK,
self::TYPE_TASK_DEPENDED_ON_BY_TASK => self::TYPE_TASK_DEPENDS_ON_TASK,
);
return idx($map, $edge_type);
@ -37,7 +42,8 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants {
public static function shouldPreventCycles($edge_type) {
static $map = array(
self::TYPE_TEST_NO_CYCLE => true,
self::TYPE_TEST_NO_CYCLE => true,
self::TYPE_TASK_DEPENDS_ON_TASK => true,
);
return isset($map[$edge_type]);
}

View file

@ -41,6 +41,7 @@ final class PhabricatorEdgeEditor {
private $remEdges = array();
private $openTransactions = array();
private $user;
private $suppressEvents;
public function setUser(PhabricatorUser $user) {
$this->user = $user;
@ -370,7 +371,26 @@ final class PhabricatorEdgeEditor {
}
}
/**
* Suppress edge edit events. This prevents listeners from making updates in
* response to edits, and is primarily useful when performing migrations. You
* should not normally need to use it.
*
* @param bool True to supress events related to edits.
* @return this
* @task internal
*/
public function setSuppressEvents($suppress) {
$this->suppressEvents = $suppress;
return $this;
}
private function sendEvent($edit_id, $event_type) {
if ($this->suppressEvents) {
return;
}
$event = new PhabricatorEvent(
$event_type,
array(
@ -436,4 +456,5 @@ final class PhabricatorEdgeEditor {
}
}
}

View file

@ -66,6 +66,43 @@ abstract class PhabricatorBaseEnglishTranslation
'ATTACHED COMMITS',
'ATTACHED COMMIT',
),
'added %d dependencie(s): %s' => array(
'added dependencies: %2$s',
'added dependency: %2$s',
),
'added %d dependent task(s): %s' => array(
'added dependent tasks: %2$s',
'added dependent task: %2$s',
),
'removed %d dependencie(s): %s' => array(
'removed dependencies: %2$s',
'removed dependency: %2$s',
),
'removed %d dependent task(s): %s' => array(
'removed dependent tasks: %2$s',
'removed dependent task: %2$s',
),
'changed %d dependencie(s), added %d: %s; removed %d: %s' =>
'changed dependencies, added: %3$s; removed: %5$s',
'changed %d dependent task(s), added %d: %s; removed %d: %s',
'changed dependent tasks, added: %3$s; removed: %5$s',
'DEPENDENT %d TASK(s)' => array(
'DEPENDENT TASKS',
'DEPENDENT TASK',
),
'DEPENDS ON %d TASK(S)' => array(
'DEPENDS ON TASKS',
'DEPENDS ON TASK',
),
);
}

View file

@ -915,6 +915,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList {
'type' => 'sql',
'name' => $this->getPatchPath('maniphestxcache.sql'),
),
'migrate-maniphest-dependencies.php' => array(
'type' => 'php',
'name' => $this->getPatchPath('migrate-maniphest-dependencies.php'),
),
);
}