diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b2a7ae4559..60ca2aa07f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -621,8 +621,11 @@ phutil_register_library_map(array( 'PhabricatorDraftDAO' => 'applications/draft/storage/PhabricatorDraftDAO.php', 'PhabricatorEdgeConfig' => 'infrastructure/edges/constants/PhabricatorEdgeConfig.php', 'PhabricatorEdgeConstants' => 'infrastructure/edges/constants/PhabricatorEdgeConstants.php', + 'PhabricatorEdgeCycleException' => 'infrastructure/edges/exception/PhabricatorEdgeCycleException.php', 'PhabricatorEdgeEditor' => 'infrastructure/edges/editor/PhabricatorEdgeEditor.php', + 'PhabricatorEdgeGraph' => 'infrastructure/edges/util/PhabricatorEdgeGraph.php', 'PhabricatorEdgeQuery' => 'infrastructure/edges/query/PhabricatorEdgeQuery.php', + 'PhabricatorEdgeTestCase' => 'infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php', 'PhabricatorEmailLoginController' => 'applications/auth/controller/PhabricatorEmailLoginController.php', 'PhabricatorEmailTokenController' => 'applications/auth/controller/PhabricatorEmailTokenController.php', 'PhabricatorEmailVerificationController' => 'applications/people/controller/PhabricatorEmailVerificationController.php', @@ -1643,7 +1646,10 @@ phutil_register_library_map(array( 'PhabricatorDraft' => 'PhabricatorDraftDAO', 'PhabricatorDraftDAO' => 'PhabricatorLiskDAO', 'PhabricatorEdgeConfig' => 'PhabricatorEdgeConstants', + 'PhabricatorEdgeCycleException' => 'Exception', + 'PhabricatorEdgeGraph' => 'AbstractDirectedGraph', 'PhabricatorEdgeQuery' => 'PhabricatorQuery', + 'PhabricatorEdgeTestCase' => 'PhabricatorTestCase', 'PhabricatorEmailLoginController' => 'PhabricatorAuthController', 'PhabricatorEmailTokenController' => 'PhabricatorAuthController', 'PhabricatorEmailVerificationController' => 'PhabricatorPeopleController', diff --git a/src/infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php b/src/infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php new file mode 100644 index 0000000000..ac07e3aaeb --- /dev/null +++ b/src/infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php @@ -0,0 +1,81 @@ + true, + ); + } + + public function testCycleDetection() { + + // The editor should detect that this introduces a cycle and prevent the + // edit. + + $user = new PhabricatorUser(); + + $obj1 = id(new HarbormasterObject())->save(); + $obj2 = id(new HarbormasterObject())->save(); + $phid1 = $obj1->getPHID(); + $phid2 = $obj2->getPHID(); + + $editor = id(new PhabricatorEdgeEditor()) + ->setUser($user) + ->addEdge($phid1, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid2) + ->addEdge($phid2, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid1); + + $caught = null; + try { + $editor->save(); + } catch (Exception $ex) { + $caught = $ex; + } + + $this->assertEqual( + true, + $caught instanceof Exception); + + + // The first edit should go through (no cycle), bu the second one should + // fail (it introduces a cycle). + + $editor = id(new PhabricatorEdgeEditor()) + ->setUser($user) + ->addEdge($phid1, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid2) + ->save(); + + $editor = id(new PhabricatorEdgeEditor()) + ->setUser($user) + ->addEdge($phid2, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid1); + + $caught = null; + try { + $editor->save(); + } catch (Exception $ex) { + $caught = $ex; + } + + $this->assertEqual( + true, + $caught instanceof Exception); + } + + +} diff --git a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php index 0dfa2e34f9..627aa2277a 100644 --- a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php +++ b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php @@ -24,6 +24,8 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { const TYPE_TASK_HAS_COMMIT = 1; const TYPE_COMMIT_HAS_TASK = 2; + 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, @@ -33,6 +35,13 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { return idx($map, $edge_type); } + public static function shouldPreventCycles($edge_type) { + static $map = array( + self::TYPE_TEST_NO_CYCLE => true, + ); + return isset($map[$edge_type]); + } + public static function establishConnection($phid_type, $conn_type) { static $class_map = array( PhabricatorPHIDConstants::PHID_TYPE_TASK => 'ManiphestTask', @@ -43,6 +52,7 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { PhabricatorPHIDConstants::PHID_TYPE_PROJ => 'PhabricatorProject', PhabricatorPHIDConstants::PHID_TYPE_MLST => 'PhabricatorMetaMTAMailingList', + PhabricatorPHIDConstants::PHID_TYPE_TOBJ => 'HarbormasterObject', ); $class = idx($class_map, $phid_type); diff --git a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php index 77653e89be..f4f7c811c3 100644 --- a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php +++ b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php @@ -32,6 +32,7 @@ * ->save(); * * @task edit Editing Edges + * @task cycles Cycle Prevention * @task internal Internals */ final class PhabricatorEdgeEditor { @@ -113,26 +114,61 @@ final class PhabricatorEdgeEditor { */ public function save() { - // NOTE: We write edge data first, before doing any transactions, since - // it's OK if we just leave it hanging out in space unattached to anything. + $cycle_types = $this->getPreventCyclesEdgeTypes(); - $this->writeEdgeData(); + $locks = array(); + $caught = null; + try { - static $id = 0; - $id++; + // NOTE: We write edge data first, before doing any transactions, since + // it's OK if we just leave it hanging out in space unattached to + // anything. + $this->writeEdgeData(); - $this->sendEvent($id, PhabricatorEventType::TYPE_EDGE_WILLEDITEDGES); + // If we're going to perform cycle detection, lock the edge type before + // doing edits. + if ($cycle_types) { + $src_phids = ipull($this->addEdges, 'src'); + foreach ($cycle_types as $cycle_type) { + $key = 'edge.cycle:'.$cycle_type; + $locks[] = PhabricatorGlobalLock::newLock($key)->lock(15); + } + } - // NOTE: Removes first, then adds, so that "remove + add" is a useful - // operation meaning "overwrite". + static $id = 0; + $id++; - $this->executeRemoves(); - $this->executeAdds(); + $this->sendEvent($id, PhabricatorEventType::TYPE_EDGE_WILLEDITEDGES); - $this->sendEvent($id, PhabricatorEventType::TYPE_EDGE_DIDEDITEDGES); + // NOTE: Removes first, then adds, so that "remove + add" is a useful + // operation meaning "overwrite". + + $this->executeRemoves(); + $this->executeAdds(); + + foreach ($cycle_types as $cycle_type) { + $this->detectCycles($src_phids, $cycle_type); + } + + $this->sendEvent($id, PhabricatorEventType::TYPE_EDGE_DIDEDITEDGES); + + $this->saveTransactions(); + } catch (Exception $ex) { + $caught = $ex; + } - $this->saveTransactions(); + if ($caught) { + $this->killTransactions(); + } + + foreach ($locks as $lock) { + $lock->unlock(); + } + + if ($caught) { + throw $caught; + } } @@ -327,6 +363,13 @@ final class PhabricatorEdgeEditor { } } + private function killTransactions() { + foreach ($this->openTransactions as $key => $conn_w) { + $conn_w->killTransaction(); + unset($this->openTransactions[$key]); + } + } + private function sendEvent($edit_id, $event_type) { $event = new PhabricatorEvent( $event_type, @@ -339,4 +382,58 @@ final class PhabricatorEdgeEditor { PhutilEventEngine::dispatchEvent($event); } + +/* -( Cycle Prevention )--------------------------------------------------- */ + + + /** + * Get a list of all edge types which are being added, and which we should + * prevent cycles on. + * + * @return list List of edge types which should have cycles prevented. + * @task cycle + */ + private function getPreventCyclesEdgeTypes() { + $edge_types = array(); + foreach ($this->addEdges as $edge) { + $edge_types[$edge['type']] = true; + } + foreach ($edge_types as $type => $ignored) { + if (!PhabricatorEdgeConfig::shouldPreventCycles($type)) { + unset($edge_types[$type]); + } + } + return array_keys($edge_types); + } + + + /** + * Detect graph cycles of a given edge type. If the edit introduces a cycle, + * a @{class:PhabricatorEdgeCycleException} is thrown with details. + * + * @return void + * @task cycle + */ + private function detectCycles(array $phids, $edge_type) { + // For simplicity, we just seed the graph with the affected nodes rather + // than seeding it with their edges. To do this, we just add synthetic + // edges from an imaginary '' node to the known edges. + + + $graph = id(new PhabricatorEdgeGraph()) + ->setEdgeType($edge_type) + ->addNodes( + array( + '' => $phids, + )) + ->loadGraph(); + + foreach ($phids as $phid) { + $cycle = $graph->detectCycles($phid); + if ($cycle) { + throw new PhabricatorEdgeCycleException($edge_type, $cycle); + } + } + } + } diff --git a/src/infrastructure/edges/exception/PhabricatorEdgeCycleException.php b/src/infrastructure/edges/exception/PhabricatorEdgeCycleException.php new file mode 100644 index 0000000000..98c7561748 --- /dev/null +++ b/src/infrastructure/edges/exception/PhabricatorEdgeCycleException.php @@ -0,0 +1,42 @@ +cycleEdgeType = $cycle_edge_type; + $this->cycle = $cycle; + + $cycle_list = implode(', ', $cycle); + + parent::__construct( + "Graph cycle detected (type={$cycle_edge_type}, cycle={$cycle_list})."); + } + + public function getCycle() { + return $this->cycle; + } + + public function getCycleEdgeType() { + return $this->cycleEdgeType; + } + +} diff --git a/src/infrastructure/edges/util/PhabricatorEdgeGraph.php b/src/infrastructure/edges/util/PhabricatorEdgeGraph.php new file mode 100644 index 0000000000..5cd3bfc5bb --- /dev/null +++ b/src/infrastructure/edges/util/PhabricatorEdgeGraph.php @@ -0,0 +1,50 @@ +edgeType = $edge_type; + return $this; + } + + protected function loadEdges(array $nodes) { + if (!$this->edgeType) { + throw new Exception("Set edge type before loading graph!"); + } + + $edges = id(new PhabricatorEdgeQuery()) + ->withSourcePHIDs($nodes) + ->withEdgeTypes(array($this->edgeType)) + ->execute(); + + $results = array_fill_keys($nodes, array()); + foreach ($edges as $src => $types) { + foreach ($types as $type => $dsts) { + foreach ($dsts as $dst => $edge) { + $results[$src][] = $dst; + } + } + } + + return $results; + } + +}