revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 00:42:41 +01:00
Code Issues Releases Wiki Activity

Modernize task/revision edges and write inverse transactions

Browse source 
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
This commit is contained in:
epriestley 2014-07-17 15:41:08 -07:00
parent ace1feb702
commit 533e799c5f
13 changed files with 323 additions and 37 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
resources/sql/patches/migrate-maniphest-revisions.php
View file

@ -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();

4
src/__phutil_library_map__.php
View file

@ -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',

4
src/applications/differential/customfield/DifferentialManiphestTasksField.php
View file

@ -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,
);
}

105
src/applications/differential/edge/DifferentialRevisionHasTaskEdgeType.php Normal file
View file

@ -0,0 +1,105 @@
<?php
final class DifferentialRevisionHasTaskEdgeType extends PhabricatorEdgeType {
const EDGECONST = 12;
public function getInverseEdgeConstant() {
return ManiphestTaskHasRevisionEdgeType::EDGECONST;
}
public function shouldWriteInverseTransactions() {
return true;
}
public function getTransactionAddString(
$actor,
$add_count,
$add_edges) {
return pht(
'%s added %s task(s): %s.',
$actor,
$add_count,
$add_edges);
}
public function getTransactionRemoveString(
$actor,
$rem_count,
$rem_edges) {
return pht(
'%s removed %s task(s): %s.',
$actor,
$rem_count,
$rem_edges);
}
public function getTransactionEditString(
$actor,
$total_count,
$add_count,
$add_edges,
$rem_count,
$rem_edges) {
return pht(
'%s edited %s task(s), added %s: %s; removed %s: %s.',
$actor,
$total_count,
$add_count,
$add_edges,
$rem_count,
$rem_edges);
}
public function getFeedAddString(
$actor,
$object,
$add_count,
$add_edges) {
return pht(
'%s added %s task(s) to %s: %s.',
$actor,
$add_count,
$object,
$add_edges);
}
public function getFeedRemoveString(
$actor,
$object,
$rem_count,
$rem_edges) {
return pht(
'%s removed %s task(s) from %s: %s.',
$actor,
$rem_count,
$object,
$rem_edges);
}
public function getFeedEditString(
$actor,
$object,
$total_count,
$add_count,
$add_edges,
$rem_count,
$rem_edges) {
return pht(
'%s edited %s task(s) for %s, added %s: %s; removed %s: %s.',
$actor,
$total_count,
$object,
$add_count,
$add_edges,
$rem_count,
$rem_edges);
}
}

4
src/applications/differential/editor/DifferentialTransactionEditor.php
View file

@ -254,7 +254,7 @@ final class DifferentialTransactionEditor
$status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED;
$edge_reviewer = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER;
$edge_ref_task = PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK;
$edge_ref_task = DifferentialRevisionHasTaskEdgeType::EDGECONST;
$is_sticky_accept = PhabricatorEnv::getEnvConfig(
'differential.sticky-accept');
@ -1250,7 +1250,7 @@ final class DifferentialTransactionEditor
->execute();
if ($tasks) {
$edge_related = PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK;
$edge_related = DifferentialRevisionHasTaskEdgeType::EDGECONST;
$edges[$edge_related] = mpull($tasks, 'getPHID', 'getPHID');
}
}

2
src/applications/differential/event/DifferentialHovercardEventListener.php
View file

@ -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(

14
src/applications/maniphest/controller/ManiphestTaskDetailController.php
View file

@ -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',

105
src/applications/maniphest/edge/ManiphestTaskHasRevisionEdgeType.php Normal file
View file

@ -0,0 +1,105 @@
<?php
final class ManiphestTaskHasRevisionEdgeType extends PhabricatorEdgeType {
const EDGECONST = 11;
public function getInverseEdgeConstant() {
return DifferentialRevisionHasTaskEdgeType::EDGECONST;
}
public function shouldWriteInverseTransactions() {
return true;
}
public function getTransactionAddString(
$actor,
$add_count,
$add_edges) {
return pht(
'%s added %s revision(s): %s.',
$actor,
$add_count,
$add_edges);
}
public function getTransactionRemoveString(
$actor,
$rem_count,
$rem_edges) {
return pht(
'%s removed %s revision(s): %s.',
$actor,
$rem_count,
$rem_edges);
}
public function getTransactionEditString(
$actor,
$total_count,
$add_count,
$add_edges,
$rem_count,
$rem_edges) {
return pht(
'%s edited %s revision(s), added %s: %s; removed %s: %s.',
$actor,
$total_count,
$add_count,
$add_edges,
$rem_count,
$rem_edges);
}
public function getFeedAddString(
$actor,
$object,
$add_count,
$add_edges) {
return pht(
'%s added %s revision(s) to %s: %s.',
$actor,
$add_count,
$object,
$add_edges);
}
public function getFeedRemoveString(
$actor,
$object,
$rem_count,
$rem_edges) {
return pht(
'%s removed %s revision(s) from %s: %s.',
$actor,
$rem_count,
$object,
$rem_edges);
}
public function getFeedEditString(
$actor,
$object,
$total_count,
$add_count,
$add_edges,
$rem_count,
$rem_edges) {
return pht(
'%s edited %s revision(s) for %s, added %s: %s; removed %s: %s.',
$actor,
$total_count,
$object,
$add_count,
$add_edges,
$rem_count,
$rem_edges);
}
}

4
src/applications/search/controller/PhabricatorSearchAttachController.php
View file

@ -295,12 +295,12 @@ final class PhabricatorSearchAttachController
$t_task => 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,

86
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
View file

@ -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));
}
}
}

18
src/infrastructure/edges/constants/PhabricatorEdgeConfig.php
View file

@ -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.';

8
src/infrastructure/edges/editor/PhabricatorEdgeEditor.php
View file

@ -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]);
}
}

4
src/infrastructure/edges/type/PhabricatorEdgeType.php
View file

@ -42,6 +42,10 @@ abstract class PhabricatorEdgeType extends Phobject {
return false;
}
public function shouldWriteInverseTransactions() {
return false;
}
public function getTransactionAddString(
$actor,
$add_count,