From 533e799c5f7e18892af39a73bac33890b09fc4f7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 17 Jul 2014 15:41:08 -0700 Subject: [PATCH] Modernize task/revision edges and write inverse transactions Summary: Ref T5245. See some discussion in D9838. When we attach object A to object B, we'd like to write transactions on both sides but only write the actual edges once. To do this, allow edge types to `shouldWriteInverseTransactions()`. When an edge type opts into this, have editors apply the inverse transactions before writing the edge. These inverse transactions don't actually apply effects, they just show up in the transaction log. Test Plan: Attached and detached revisions from tasks, saw transactions appear on both sides of the operation. Reviewers: chad, btrahan, joshuaspence Reviewed By: btrahan, joshuaspence Subscribers: epriestley Maniphest Tasks: T5245 Differential Revision: https://secure.phabricator.com/D9839 --- .../patches/migrate-maniphest-revisions.php | 2 +- src/__phutil_library_map__.php | 4 + .../DifferentialManiphestTasksField.php | 4 +- .../DifferentialRevisionHasTaskEdgeType.php | 105 ++++++++++++++++++ .../editor/DifferentialTransactionEditor.php | 4 +- .../DifferentialHovercardEventListener.php | 2 +- .../ManiphestTaskDetailController.php | 14 +-- .../edge/ManiphestTaskHasRevisionEdgeType.php | 105 ++++++++++++++++++ .../PhabricatorSearchAttachController.php | 4 +- ...habricatorApplicationTransactionEditor.php | 86 +++++++++++++- .../edges/constants/PhabricatorEdgeConfig.php | 18 +-- .../edges/editor/PhabricatorEdgeEditor.php | 8 +- .../edges/type/PhabricatorEdgeType.php | 4 + 13 files changed, 323 insertions(+), 37 deletions(-) create mode 100644 src/applications/differential/edge/DifferentialRevisionHasTaskEdgeType.php create mode 100644 src/applications/maniphest/edge/ManiphestTaskHasRevisionEdgeType.php diff --git a/resources/sql/patches/migrate-maniphest-revisions.php b/resources/sql/patches/migrate-maniphest-revisions.php index 788da47e92..3bcb05c855 100644 --- a/resources/sql/patches/migrate-maniphest-revisions.php +++ b/resources/sql/patches/migrate-maniphest-revisions.php @@ -19,7 +19,7 @@ foreach (new LiskMigrationIterator($table) as $task) { foreach ($revs as $rev) { $editor->addEdge( $task->getPHID(), - PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV, + ManiphestTaskHasRevisionEdgeType::EDGECONST, $rev); } $editor->save(); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2fbc1f25fd..9b7b80ebeb 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -438,6 +438,7 @@ phutil_register_library_map(array( 'DifferentialRevisionControlSystem' => 'applications/differential/constants/DifferentialRevisionControlSystem.php', 'DifferentialRevisionDetailView' => 'applications/differential/view/DifferentialRevisionDetailView.php', 'DifferentialRevisionEditController' => 'applications/differential/controller/DifferentialRevisionEditController.php', + 'DifferentialRevisionHasTaskEdgeType' => 'applications/differential/edge/DifferentialRevisionHasTaskEdgeType.php', 'DifferentialRevisionIDField' => 'applications/differential/customfield/DifferentialRevisionIDField.php', 'DifferentialRevisionLandController' => 'applications/differential/controller/DifferentialRevisionLandController.php', 'DifferentialRevisionListController' => 'applications/differential/controller/DifferentialRevisionListController.php', @@ -944,6 +945,7 @@ phutil_register_library_map(array( 'ManiphestTaskDescriptionPreviewController' => 'applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php', 'ManiphestTaskDetailController' => 'applications/maniphest/controller/ManiphestTaskDetailController.php', 'ManiphestTaskEditController' => 'applications/maniphest/controller/ManiphestTaskEditController.php', + 'ManiphestTaskHasRevisionEdgeType' => 'applications/maniphest/edge/ManiphestTaskHasRevisionEdgeType.php', 'ManiphestTaskListController' => 'applications/maniphest/controller/ManiphestTaskListController.php', 'ManiphestTaskListView' => 'applications/maniphest/view/ManiphestTaskListView.php', 'ManiphestTaskMailReceiver' => 'applications/maniphest/mail/ManiphestTaskMailReceiver.php', @@ -3153,6 +3155,7 @@ phutil_register_library_map(array( ), 'DifferentialRevisionDetailView' => 'AphrontView', 'DifferentialRevisionEditController' => 'DifferentialController', + 'DifferentialRevisionHasTaskEdgeType' => 'PhabricatorEdgeType', 'DifferentialRevisionIDField' => 'DifferentialCustomField', 'DifferentialRevisionLandController' => 'DifferentialController', 'DifferentialRevisionListController' => 'DifferentialController', @@ -3700,6 +3703,7 @@ phutil_register_library_map(array( 'ManiphestTaskDescriptionPreviewController' => 'ManiphestController', 'ManiphestTaskDetailController' => 'ManiphestController', 'ManiphestTaskEditController' => 'ManiphestController', + 'ManiphestTaskHasRevisionEdgeType' => 'PhabricatorEdgeType', 'ManiphestTaskListController' => 'ManiphestController', 'ManiphestTaskListView' => 'ManiphestView', 'ManiphestTaskMailReceiver' => 'PhabricatorObjectMailReceiver', diff --git a/src/applications/differential/customfield/DifferentialManiphestTasksField.php b/src/applications/differential/customfield/DifferentialManiphestTasksField.php index 6e12b46a01..fbf05c708f 100644 --- a/src/applications/differential/customfield/DifferentialManiphestTasksField.php +++ b/src/applications/differential/customfield/DifferentialManiphestTasksField.php @@ -42,7 +42,7 @@ final class DifferentialManiphestTasksField return PhabricatorEdgeQuery::loadDestinationPHIDs( $revision->getPHID(), - PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK); + DifferentialRevisionHasTaskEdgeType::EDGECONST); } public function getApplicationTransactionType() { @@ -51,7 +51,7 @@ final class DifferentialManiphestTasksField public function getApplicationTransactionMetadata() { return array( - 'edge:type' => PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK, + 'edge:type' => DifferentialRevisionHasTaskEdgeType::EDGECONST, ); } diff --git a/src/applications/differential/edge/DifferentialRevisionHasTaskEdgeType.php b/src/applications/differential/edge/DifferentialRevisionHasTaskEdgeType.php new file mode 100644 index 0000000000..de9a9cad57 --- /dev/null +++ b/src/applications/differential/edge/DifferentialRevisionHasTaskEdgeType.php @@ -0,0 +1,105 @@ +execute(); if ($tasks) { - $edge_related = PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK; + $edge_related = DifferentialRevisionHasTaskEdgeType::EDGECONST; $edges[$edge_related] = mpull($tasks, 'getPHID', 'getPHID'); } } diff --git a/src/applications/differential/event/DifferentialHovercardEventListener.php b/src/applications/differential/event/DifferentialHovercardEventListener.php index ae9fd0011b..06b5cfde96 100644 --- a/src/applications/differential/event/DifferentialHovercardEventListener.php +++ b/src/applications/differential/event/DifferentialHovercardEventListener.php @@ -28,7 +28,7 @@ final class DifferentialHovercardEventListener $rev->loadRelationships(); $reviewer_phids = $rev->getReviewers(); - $e_task = PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK; + $e_task = DifferentialRevisionHasTaskEdgeType::EDGECONST; $edge_query = id(new PhabricatorEdgeQuery()) ->withSourcePHIDs(array($phid)) ->withEdgeTypes( diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 8d24d59a49..b75c7c1ce1 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -54,7 +54,7 @@ final class ManiphestTaskDetailController extends ManiphestController { $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; - $e_rev = PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV; + $e_rev = ManiphestTaskHasRevisionEdgeType::EDGECONST; $e_mock = PhabricatorEdgeConfig::TYPE_TASK_HAS_MOCK; $phid = $task->getPHID(); @@ -590,13 +590,13 @@ final class ManiphestTaskDetailController extends ManiphestController { $edge_types = array( PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK - => pht('Blocks'), + => pht('Blocks'), PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK - => pht('Blocked By'), - PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV - => pht('Differential Revisions'), + => pht('Blocked By'), + ManiphestTaskHasRevisionEdgeType::EDGECONST + => pht('Differential Revisions'), PhabricatorEdgeConfig::TYPE_TASK_HAS_MOCK - => pht('Pholio Mocks'), + => pht('Pholio Mocks'), ); $revisions_commits = array(); @@ -616,7 +616,7 @@ final class ManiphestTaskDetailController extends ManiphestController { $revision_phid = key($drev_edges[$phid][$commit_drev]); $revision_handle = idx($handles, $revision_phid); if ($revision_handle) { - $task_drev = PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV; + $task_drev = ManiphestTaskHasRevisionEdgeType::EDGECONST; unset($edges[$task_drev][$revision_phid]); $revisions_commits[$phid] = hsprintf( '%s / %s', diff --git a/src/applications/maniphest/edge/ManiphestTaskHasRevisionEdgeType.php b/src/applications/maniphest/edge/ManiphestTaskHasRevisionEdgeType.php new file mode 100644 index 0000000000..6bd1e6c94e --- /dev/null +++ b/src/applications/maniphest/edge/ManiphestTaskHasRevisionEdgeType.php @@ -0,0 +1,105 @@ + array( $t_cmit => PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT, $t_task => PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK, - $t_drev => PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV, + $t_drev => ManiphestTaskHasRevisionEdgeType::EDGECONST, $t_mock => PhabricatorEdgeConfig::TYPE_TASK_HAS_MOCK, ), $t_drev => array( $t_drev => PhabricatorEdgeConfig::TYPE_DREV_DEPENDS_ON_DREV, - $t_task => PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK, + $t_task => DifferentialRevisionHasTaskEdgeType::EDGECONST, ), $t_mock => array( $t_task => PhabricatorEdgeConfig::TYPE_MOCK_HAS_TASK, diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 28b803c659..9f99017c23 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -24,6 +24,7 @@ abstract class PhabricatorApplicationTransactionEditor private $isPreview; private $isHeraldEditor; + private $isInverseEdgeEditor; private $actingAsPHID; private $disableEmail; @@ -120,6 +121,15 @@ abstract class PhabricatorApplicationTransactionEditor return $this->isPreview; } + public function setIsInverseEdgeEditor($is_inverse_edge_editor) { + $this->isInverseEdgeEditor = $is_inverse_edge_editor; + return $this; + } + + public function getIsInverseEdgeEditor() { + return $this->isInverseEdgeEditor; + } + public function setIsHeraldEditor($is_herald_editor) { $this->isHeraldEditor = $is_herald_editor; return $this; @@ -377,10 +387,25 @@ abstract class PhabricatorApplicationTransactionEditor break; case PhabricatorTransactions::TYPE_EDGE: + if ($this->getIsInverseEdgeEditor()) { + // If we're writing an inverse edge transaction, don't actually + // do anything. The initiating editor on the other side of the + // transaction will take care of the edge writes. + break; + } + $old = $xaction->getOldValue(); $new = $xaction->getNewValue(); $src = $object->getPHID(); - $type = $xaction->getMetadataValue('edge:type'); + $const = $xaction->getMetadataValue('edge:type'); + + $type = PhabricatorEdgeType::getByConstant($const); + if ($type->shouldWriteInverseTransactions()) { + $this->applyInverseEdgeTransactions( + $object, + $xaction, + $type->getInverseEdgeConstant()); + } foreach ($new as $dst_phid => $edge) { $new[$dst_phid]['src'] = $src; @@ -395,7 +420,7 @@ abstract class PhabricatorApplicationTransactionEditor continue; } } - $editor->removeEdge($src, $type, $dst_phid); + $editor->removeEdge($src, $const, $dst_phid); } foreach ($new as $dst_phid => $edge) { @@ -409,7 +434,7 @@ abstract class PhabricatorApplicationTransactionEditor 'data' => $edge['data'], ); - $editor->addEdge($src, $type, $dst_phid, $data); + $editor->addEdge($src, $const, $dst_phid, $data); } $editor->save(); @@ -2335,4 +2360,59 @@ abstract class PhabricatorApplicationTransactionEditor $editor->save(); } + private function applyInverseEdgeTransactions( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction, + $inverse_type) { + + $old = $xaction->getOldValue(); + $new = $xaction->getNewValue(); + + $add = array_keys(array_diff_key($new, $old)); + $rem = array_keys(array_diff_key($old, $new)); + + $add = array_fuse($add); + $rem = array_fuse($rem); + $all = $add + $rem; + + $nodes = id(new PhabricatorObjectQuery()) + ->setViewer($this->requireActor()) + ->withPHIDs($all) + ->execute(); + + foreach ($nodes as $node) { + if (!($node instanceof PhabricatorApplicationTransactionInterface)) { + continue; + } + + $editor = $node->getApplicationTransactionEditor(); + $template = $node->getApplicationTransactionTemplate(); + $target = $node->getApplicationTransactionObject(); + + if (isset($add[$node->getPHID()])) { + $edge_edit_type = '+'; + } else { + $edge_edit_type = '-'; + } + + $template + ->setTransactionType($xaction->getTransactionType()) + ->setMetadataValue('edge:type', $inverse_type) + ->setNewValue( + array( + $edge_edit_type => array($object->getPHID() => $object->getPHID()), + )); + + $editor + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) + ->setParentMessageID($this->getParentMessageID()) + ->setIsInverseEdgeEditor(true) + ->setActor($this->requireActor()) + ->setContentSource($this->getContentSource()); + + $editor->applyTransactions($target, array($template)); + } + } + } diff --git a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php index eb951356c4..bfff37a813 100644 --- a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php +++ b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php @@ -19,9 +19,6 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { const TYPE_BLOG_HAS_BLOGGER = 9; const TYPE_BLOGGER_HAS_BLOG = 10; - const TYPE_TASK_HAS_RELATED_DREV = 11; - const TYPE_DREV_HAS_RELATED_TASK = 12; - const TYPE_PROJ_MEMBER = 13; const TYPE_MEMBER_OF_PROJ = 14; @@ -137,7 +134,7 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { return $map; } - public static function getInverse($edge_type) { + private 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, @@ -153,9 +150,6 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { self::TYPE_BLOG_HAS_BLOGGER => self::TYPE_BLOGGER_HAS_BLOG, self::TYPE_BLOGGER_HAS_BLOG => self::TYPE_BLOG_HAS_BLOGGER, - self::TYPE_TASK_HAS_RELATED_DREV => self::TYPE_DREV_HAS_RELATED_TASK, - self::TYPE_DREV_HAS_RELATED_TASK => self::TYPE_TASK_HAS_RELATED_DREV, - self::TYPE_PROJ_MEMBER => self::TYPE_MEMBER_OF_PROJ, self::TYPE_MEMBER_OF_PROJ => self::TYPE_PROJ_MEMBER, @@ -226,7 +220,7 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { return idx($map, $edge_type); } - public static function shouldPreventCycles($edge_type) { + private static function shouldPreventCycles($edge_type) { static $map = array( self::TYPE_TEST_NO_CYCLE => true, self::TYPE_TASK_DEPENDS_ON_TASK => true, @@ -272,12 +266,10 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { case self::TYPE_COMMIT_HAS_TASK: case self::TYPE_TASK_DEPENDS_ON_TASK: case self::TYPE_TASK_DEPENDED_ON_BY_TASK: - case self::TYPE_DREV_HAS_RELATED_TASK: case self::TYPE_MOCK_HAS_TASK: return '%s edited task(s), added %d: %s; removed %d: %s.'; case self::TYPE_DREV_DEPENDS_ON_DREV: case self::TYPE_DREV_DEPENDED_ON_BY_DREV: - case self::TYPE_TASK_HAS_RELATED_DREV: case self::TYPE_COMMIT_HAS_DREV: case self::TYPE_REVIEWER_FOR_DREV: return '%s edited revision(s), added %d: %s; removed %d: %s.'; @@ -354,11 +346,9 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { case self::TYPE_TASK_DEPENDED_ON_BY_TASK: return '%s added %d blocked task(s): %s.'; case self::TYPE_COMMIT_HAS_TASK: - case self::TYPE_DREV_HAS_RELATED_TASK: case self::TYPE_MOCK_HAS_TASK: return '%s added %d task(s): %s.'; case self::TYPE_DREV_DEPENDED_ON_BY_DREV: - case self::TYPE_TASK_HAS_RELATED_DREV: case self::TYPE_COMMIT_HAS_DREV: case self::TYPE_REVIEWER_FOR_DREV: return '%s added %d revision(s): %s.'; @@ -432,12 +422,10 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { case self::TYPE_TASK_DEPENDED_ON_BY_TASK: return '%s removed %d blocked task(s): %s.'; case self::TYPE_COMMIT_HAS_TASK: - case self::TYPE_DREV_HAS_RELATED_TASK: case self::TYPE_MOCK_HAS_TASK: return '%s removed %d task(s): %s.'; case self::TYPE_DREV_DEPENDS_ON_DREV: case self::TYPE_DREV_DEPENDED_ON_BY_DREV: - case self::TYPE_TASK_HAS_RELATED_DREV: case self::TYPE_COMMIT_HAS_DREV: case self::TYPE_REVIEWER_FOR_DREV: return '%s removed %d revision(s): %s.'; @@ -507,12 +495,10 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { case self::TYPE_COMMIT_HAS_TASK: case self::TYPE_TASK_DEPENDS_ON_TASK: case self::TYPE_TASK_DEPENDED_ON_BY_TASK: - case self::TYPE_DREV_HAS_RELATED_TASK: case self::TYPE_MOCK_HAS_TASK: return '%s updated tasks of %s.'; case self::TYPE_DREV_DEPENDS_ON_DREV: case self::TYPE_DREV_DEPENDED_ON_BY_DREV: - case self::TYPE_TASK_HAS_RELATED_DREV: case self::TYPE_COMMIT_HAS_DREV: case self::TYPE_REVIEWER_FOR_DREV: return '%s updated revisions of %s.'; diff --git a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php index 7de1b43f3d..c5df01728a 100644 --- a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php +++ b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php @@ -180,8 +180,9 @@ final class PhabricatorEdgeEditor extends PhabricatorEditor { 'data' => $data, ); - $inverse = PhabricatorEdgeConfig::getInverse($type); - if ($inverse) { + $type_obj = PhabricatorEdgeType::getByConstant($type); + $inverse = $type_obj->getInverseEdgeConstant(); + if ($inverse !== null) { // If `inverse_data` is set, overwrite the edge data. Normally, just // write the same data to the inverse edge. @@ -398,7 +399,8 @@ final class PhabricatorEdgeEditor extends PhabricatorEditor { $edge_types[$edge['type']] = true; } foreach ($edge_types as $type => $ignored) { - if (!PhabricatorEdgeConfig::shouldPreventCycles($type)) { + $type_obj = PhabricatorEdgeType::getByConstant($type); + if (!$type_obj->shouldPreventCycles()) { unset($edge_types[$type]); } } diff --git a/src/infrastructure/edges/type/PhabricatorEdgeType.php b/src/infrastructure/edges/type/PhabricatorEdgeType.php index f26bac0be2..6c2f28e584 100644 --- a/src/infrastructure/edges/type/PhabricatorEdgeType.php +++ b/src/infrastructure/edges/type/PhabricatorEdgeType.php @@ -42,6 +42,10 @@ abstract class PhabricatorEdgeType extends Phobject { return false; } + public function shouldWriteInverseTransactions() { + return false; + } + public function getTransactionAddString( $actor, $add_count,