Skip to content

Commit 889440e

Browse files
author
epriestley
committed
Allow structured destruction of Differential Revisions
Summary: Ref T4749. Ref T3265. Ref T4909. - Remove old "destroy revision" script. - Move to structured `bin/remove` destruction. - Fix some edge issues. - Add transaction destruction support. Test Plan: - Destroyed a bunch of revisions. - Saw diffs, changesets, hunks, transactions, edges, and inlines also get wiped out. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4749, T4909, T3265 Differential Revision: https://secure.phabricator.com/D8943
1 parent 827fbb3 commit 889440e

File tree

8 files changed

+161
-118
lines changed

8 files changed

+161
-118
lines changed

‎scripts/differential/destroy_revision.php‎

Lines changed: 0 additions & 51 deletions
This file was deleted.

‎src/__phutil_library_map__.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2982,6 +2982,7 @@
29822982
1 => 'PhabricatorPolicyInterface',
29832983
2 => 'HarbormasterBuildableInterface',
29842984
3 => 'PhabricatorApplicationTransactionInterface',
2985+
4 => 'PhabricatorDestructableInterface',
29852986
),
29862987
'DifferentialDiffCreateController' => 'DifferentialController',
29872988
'DifferentialDiffProperty' => 'DifferentialDAO',
@@ -3047,6 +3048,7 @@
30473048
6 => 'PhabricatorSubscribableInterface',
30483049
7 => 'PhabricatorCustomFieldInterface',
30493050
8 => 'PhabricatorApplicationTransactionInterface',
3051+
9 => 'PhabricatorDestructableInterface',
30503052
),
30513053
'DifferentialRevisionDetailView' => 'AphrontView',
30523054
'DifferentialRevisionEditController' => 'DifferentialController',
@@ -3915,12 +3917,14 @@
39153917
array(
39163918
0 => 'PhabricatorLiskDAO',
39173919
1 => 'PhabricatorPolicyInterface',
3920+
2 => 'PhabricatorDestructableInterface',
39183921
),
39193922
'PhabricatorApplicationTransactionComment' =>
39203923
array(
39213924
0 => 'PhabricatorLiskDAO',
39223925
1 => 'PhabricatorMarkupInterface',
39233926
2 => 'PhabricatorPolicyInterface',
3927+
3 => 'PhabricatorDestructableInterface',
39243928
),
39253929
'PhabricatorApplicationTransactionCommentEditController' => 'PhabricatorApplicationTransactionController',
39263930
'PhabricatorApplicationTransactionCommentEditor' => 'PhabricatorEditor',

‎src/applications/differential/storage/DifferentialDiff.php‎

Lines changed: 26 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@ final class DifferentialDiff
55
implements
66
PhabricatorPolicyInterface,
77
HarbormasterBuildableInterface,
8-
PhabricatorApplicationTransactionInterface {
8+
PhabricatorApplicationTransactionInterface,
9+
PhabricatorDestructableInterface {
910

1011
protected $revisionID;
1112
protected $authorPHID;
@@ -107,24 +108,6 @@ public function save() {
107108
return $ret;
108109
}
109110

110-
public function delete() {
111-
$this->openTransaction();
112-
foreach ($this->loadChangesets() as $changeset) {
113-
$changeset->delete();
114-
}
115-
116-
$properties = id(new DifferentialDiffProperty())->loadAllWhere(
117-
'diffID = %d',
118-
$this->getID());
119-
foreach ($properties as $prop) {
120-
$prop->delete();
121-
}
122-
123-
$ret = parent::delete();
124-
$this->saveTransaction();
125-
return $ret;
126-
}
127-
128111
public static function newFromRawChanges(array $changes) {
129112
assert_instances_of($changes, 'ArcanistDiffChange');
130113
$diff = new DifferentialDiff();
@@ -376,4 +359,28 @@ public function getApplicationTransactionTemplate() {
376359
return $this->getRevision()->getApplicationTransactionTemplate();
377360
}
378361

362+
363+
/* -( PhabricatorDestructableInterface )----------------------------------- */
364+
365+
366+
public function destroyObjectPermanently(
367+
PhabricatorDestructionEngine $engine) {
368+
369+
$this->openTransaction();
370+
$this->delete();
371+
372+
foreach ($this->loadChangesets() as $changeset) {
373+
$changeset->delete();
374+
}
375+
376+
$properties = id(new DifferentialDiffProperty())->loadAllWhere(
377+
'diffID = %d',
378+
$this->getID());
379+
foreach ($properties as $prop) {
380+
$prop->delete();
381+
}
382+
383+
$this->saveTransaction();
384+
}
385+
379386
}

‎src/applications/differential/storage/DifferentialRevision.php‎

Lines changed: 51 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@ final class DifferentialRevision extends DifferentialDAO
99
HarbormasterBuildableInterface,
1010
PhabricatorSubscribableInterface,
1111
PhabricatorCustomFieldInterface,
12-
PhabricatorApplicationTransactionInterface {
12+
PhabricatorApplicationTransactionInterface,
13+
PhabricatorDestructableInterface {
1314

1415
protected $title = '';
1516
protected $originalTitle;
@@ -174,45 +175,6 @@ public function save() {
174175
return parent::save();
175176
}
176177

177-
public function delete() {
178-
$this->openTransaction();
179-
$diffs = id(new DifferentialDiffQuery())
180-
->setViewer(PhabricatorUser::getOmnipotentUser())
181-
->withRevisionIDs(array($this->getID()))
182-
->execute();
183-
foreach ($diffs as $diff) {
184-
$diff->delete();
185-
}
186-
187-
$conn_w = $this->establishConnection('w');
188-
189-
queryfx(
190-
$conn_w,
191-
'DELETE FROM %T WHERE revisionID = %d',
192-
self::TABLE_COMMIT,
193-
$this->getID());
194-
195-
$inlines = id(new DifferentialInlineCommentQuery())
196-
->withRevisionIDs(array($this->getID()))
197-
->execute();
198-
foreach ($inlines as $inline) {
199-
$inline->delete();
200-
}
201-
202-
// we have to do paths a little differentally as they do not have
203-
// an id or phid column for delete() to act on
204-
$dummy_path = new DifferentialAffectedPath();
205-
queryfx(
206-
$conn_w,
207-
'DELETE FROM %T WHERE revisionID = %d',
208-
$dummy_path->getTableName(),
209-
$this->getID());
210-
211-
$result = parent::delete();
212-
$this->saveTransaction();
213-
return $result;
214-
}
215-
216178
public function loadRelationships() {
217179
if (!$this->getID()) {
218180
$this->relationships = array();
@@ -477,4 +439,53 @@ public function getApplicationTransactionTemplate() {
477439
return new DifferentialTransaction();
478440
}
479441

442+
443+
/* -( PhabricatorDestructableInterface )----------------------------------- */
444+
445+
446+
public function destroyObjectPermanently(
447+
PhabricatorDestructionEngine $engine) {
448+
449+
$this->openTransaction();
450+
$diffs = id(new DifferentialDiffQuery())
451+
->setViewer(PhabricatorUser::getOmnipotentUser())
452+
->withRevisionIDs(array($this->getID()))
453+
->execute();
454+
foreach ($diffs as $diff) {
455+
$engine->destroyObject($diff);
456+
}
457+
458+
$conn_w = $this->establishConnection('w');
459+
460+
queryfx(
461+
$conn_w,
462+
'DELETE FROM %T WHERE revisionID = %d',
463+
self::TABLE_COMMIT,
464+
$this->getID());
465+
466+
try {
467+
$inlines = id(new DifferentialInlineCommentQuery())
468+
->withRevisionIDs(array($this->getID()))
469+
->execute();
470+
foreach ($inlines as $inline) {
471+
$inline->delete();
472+
}
473+
} catch (PhabricatorEmptyQueryException $ex) {
474+
// TODO: There's still some funky legacy wrapping going on here, and
475+
// we might catch a raw query exception.
476+
}
477+
478+
// we have to do paths a little differentally as they do not have
479+
// an id or phid column for delete() to act on
480+
$dummy_path = new DifferentialAffectedPath();
481+
queryfx(
482+
$conn_w,
483+
'DELETE FROM %T WHERE revisionID = %d',
484+
$dummy_path->getTableName(),
485+
$this->getID());
486+
487+
$this->delete();
488+
$this->saveTransaction();
489+
}
490+
480491
}

‎src/applications/system/engine/PhabricatorDestructionEngine.php‎

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,20 +41,48 @@ public function destroyObject(PhabricatorDestructableInterface $object) {
4141

4242
if ($object_phid) {
4343
$this->destroyEdges($object_phid);
44+
45+
if ($object instanceof PhabricatorApplicationTransactionInterface) {
46+
$template = $object->getApplicationTransactionTemplate();
47+
$this->destroyTransactions($template, $object_phid);
48+
}
4449
}
50+
51+
// TODO: PhabricatorFlaggableInterface
52+
// TODO: PhabricatorTokenReceiverInterface
4553
}
4654

4755
private function destroyEdges($src_phid) {
48-
$edges = id(new PhabricatorEdgeQuery())
49-
->withSourcePHIDs(array($src_phid))
50-
->execute();
56+
try {
57+
$edges = id(new PhabricatorEdgeQuery())
58+
->withSourcePHIDs(array($src_phid))
59+
->execute();
60+
} catch (Exception $ex) {
61+
// This is (presumably) a "no edges for this PHID type" exception.
62+
return;
63+
}
5164

5265
$editor = id(new PhabricatorEdgeEditor())
5366
->setSuppressEvents(true);
54-
foreach ($edges as $edge) {
55-
$editor->removeEdge($edge['src'], $edge['type'], $edge['dst']);
67+
foreach ($edges as $type => $type_edges) {
68+
foreach ($type_edges as $src => $src_edges) {
69+
foreach ($src_edges as $dst => $edge) {
70+
$editor->removeEdge($edge['src'], $edge['type'], $edge['dst']);
71+
}
72+
}
5673
}
5774
$editor->save();
5875
}
5976

77+
private function destroyTransactions(
78+
PhabricatorApplicationTransaction $template,
79+
$object_phid) {
80+
81+
$xactions = $template->loadAllWhere('objectPHID = %s', $object_phid);
82+
foreach ($xactions as $xaction) {
83+
$this->destroyObject($xaction);
84+
}
85+
86+
}
87+
6088
}

‎src/applications/system/management/PhabricatorSystemRemoveLogWorkflow.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,9 @@ public function execute(PhutilArgumentParser $args) {
1717
$table = new PhabricatorSystemDestructionLog();
1818
foreach (new LiskMigrationIterator($table) as $row) {
1919
$console->writeOut(
20-
"[%s]\t%s\t%s\t%s\n",
20+
"[%s]\t%s %s\t%s\t%s\n",
2121
phabricator_datetime($row->getEpoch(), $this->getViewer()),
22+
($row->getRootLogID() ? ' ' : '*'),
2223
$row->getObjectClass(),
2324
$row->getObjectPHID(),
2425
$row->getObjectMonogram());

‎src/applications/transactions/storage/PhabricatorApplicationTransaction.php‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@
22

33
abstract class PhabricatorApplicationTransaction
44
extends PhabricatorLiskDAO
5-
implements PhabricatorPolicyInterface {
5+
implements
6+
PhabricatorPolicyInterface,
7+
PhabricatorDestructableInterface {
68

79
const TARGET_TEXT = 'text';
810
const TARGET_HTML = 'html';
@@ -942,4 +944,32 @@ public function describeAutomaticCapability($capability) {
942944
}
943945

944946

947+
/* -( PhabricatorDestructableInterface )----------------------------------- */
948+
949+
950+
public function destroyObjectPermanently(
951+
PhabricatorDestructionEngine $engine) {
952+
953+
$this->openTransaction();
954+
$comment_template = null;
955+
try {
956+
$comment_template = $this->getApplicationTransactionCommentObject();
957+
} catch (Exception $ex) {
958+
// Continue; no comments for these transactions.
959+
}
960+
961+
if ($comment_template) {
962+
$comments = $comment_template->loadAllWhere(
963+
'transactionPHID = %s',
964+
$this->getPHID());
965+
foreach ($comments as $comment) {
966+
$engine->destroyObject($comment);
967+
}
968+
}
969+
970+
$this->delete();
971+
$this->saveTransaction();
972+
}
973+
974+
945975
}

‎src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@
22

33
abstract class PhabricatorApplicationTransactionComment
44
extends PhabricatorLiskDAO
5-
implements PhabricatorMarkupInterface, PhabricatorPolicyInterface {
5+
implements
6+
PhabricatorMarkupInterface,
7+
PhabricatorPolicyInterface,
8+
PhabricatorDestructableInterface {
69

710
const MARKUP_FIELD_COMMENT = 'markup:comment';
811

@@ -118,4 +121,14 @@ public function describeAutomaticCapability($capability) {
118121
return null;
119122
}
120123

124+
125+
/* -( PhabricatorDestructableInterface )----------------------------------- */
126+
127+
public function destroyObjectPermanently(
128+
PhabricatorDestructionEngine $engine) {
129+
$this->openTransaction();
130+
$this->delete();
131+
$this->saveTransaction();
132+
}
133+
121134
}

0 commit comments

Comments
 (0)