From 889440ead0666b6e4fd1608dfcfe7b700dbea832 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 1 May 2014 18:25:30 -0700 Subject: [PATCH] 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 --- scripts/differential/destroy_revision.php | 51 ----------- src/__phutil_library_map__.php | 4 + .../differential/storage/DifferentialDiff.php | 45 +++++---- .../storage/DifferentialRevision.php | 91 +++++++++++-------- .../engine/PhabricatorDestructionEngine.php | 38 +++++++- .../PhabricatorSystemRemoveLogWorkflow.php | 3 +- .../PhabricatorApplicationTransaction.php | 32 ++++++- ...abricatorApplicationTransactionComment.php | 15 ++- 8 files changed, 161 insertions(+), 118 deletions(-) delete mode 100755 scripts/differential/destroy_revision.php diff --git a/scripts/differential/destroy_revision.php b/scripts/differential/destroy_revision.php deleted file mode 100755 index 8f24e6d17d..0000000000 --- a/scripts/differential/destroy_revision.php +++ /dev/null @@ -1,51 +0,0 @@ -#!/usr/bin/env php -setTagline('permanently destroy a Differential Revision'); -$args->setSynopsis(<<parseStandardArguments(); -$args->parse( - array( - array( - 'name' => 'revision', - 'wildcard' => true, - ), - )); - -$revisions = $args->getArg('revision'); -if (count($revisions) != 1) { - $args->printHelpAndExit(); -} - -$id = trim(strtolower(head($revisions)), 'd '); -$revision = id(new DifferentialRevision())->load($id); - -if (!$revision) { - throw new Exception("No revision '{$id}' exists!"); -} - -$title = $revision->getTitle(); -$ok = phutil_console_confirm("Really destroy 'D{$id}: {$title}' forever?"); -if (!$ok) { - throw new Exception("User aborted workflow."); -} - -$revision->delete(); -echo "OK, destroyed revision.\n"; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 1768e557ba..ea567da921 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2982,6 +2982,7 @@ phutil_register_library_map(array( 1 => 'PhabricatorPolicyInterface', 2 => 'HarbormasterBuildableInterface', 3 => 'PhabricatorApplicationTransactionInterface', + 4 => 'PhabricatorDestructableInterface', ), 'DifferentialDiffCreateController' => 'DifferentialController', 'DifferentialDiffProperty' => 'DifferentialDAO', @@ -3047,6 +3048,7 @@ phutil_register_library_map(array( 6 => 'PhabricatorSubscribableInterface', 7 => 'PhabricatorCustomFieldInterface', 8 => 'PhabricatorApplicationTransactionInterface', + 9 => 'PhabricatorDestructableInterface', ), 'DifferentialRevisionDetailView' => 'AphrontView', 'DifferentialRevisionEditController' => 'DifferentialController', @@ -3915,12 +3917,14 @@ phutil_register_library_map(array( array( 0 => 'PhabricatorLiskDAO', 1 => 'PhabricatorPolicyInterface', + 2 => 'PhabricatorDestructableInterface', ), 'PhabricatorApplicationTransactionComment' => array( 0 => 'PhabricatorLiskDAO', 1 => 'PhabricatorMarkupInterface', 2 => 'PhabricatorPolicyInterface', + 3 => 'PhabricatorDestructableInterface', ), 'PhabricatorApplicationTransactionCommentEditController' => 'PhabricatorApplicationTransactionController', 'PhabricatorApplicationTransactionCommentEditor' => 'PhabricatorEditor', diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index 88398e856a..237fab7e59 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -5,7 +5,8 @@ final class DifferentialDiff implements PhabricatorPolicyInterface, HarbormasterBuildableInterface, - PhabricatorApplicationTransactionInterface { + PhabricatorApplicationTransactionInterface, + PhabricatorDestructableInterface { protected $revisionID; protected $authorPHID; @@ -107,24 +108,6 @@ final class DifferentialDiff return $ret; } - public function delete() { - $this->openTransaction(); - foreach ($this->loadChangesets() as $changeset) { - $changeset->delete(); - } - - $properties = id(new DifferentialDiffProperty())->loadAllWhere( - 'diffID = %d', - $this->getID()); - foreach ($properties as $prop) { - $prop->delete(); - } - - $ret = parent::delete(); - $this->saveTransaction(); - return $ret; - } - public static function newFromRawChanges(array $changes) { assert_instances_of($changes, 'ArcanistDiffChange'); $diff = new DifferentialDiff(); @@ -376,4 +359,28 @@ final class DifferentialDiff return $this->getRevision()->getApplicationTransactionTemplate(); } + +/* -( PhabricatorDestructableInterface )----------------------------------- */ + + + public function destroyObjectPermanently( + PhabricatorDestructionEngine $engine) { + + $this->openTransaction(); + $this->delete(); + + foreach ($this->loadChangesets() as $changeset) { + $changeset->delete(); + } + + $properties = id(new DifferentialDiffProperty())->loadAllWhere( + 'diffID = %d', + $this->getID()); + foreach ($properties as $prop) { + $prop->delete(); + } + + $this->saveTransaction(); + } + } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 398ffa985b..59ef677541 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -9,7 +9,8 @@ final class DifferentialRevision extends DifferentialDAO HarbormasterBuildableInterface, PhabricatorSubscribableInterface, PhabricatorCustomFieldInterface, - PhabricatorApplicationTransactionInterface { + PhabricatorApplicationTransactionInterface, + PhabricatorDestructableInterface { protected $title = ''; protected $originalTitle; @@ -174,45 +175,6 @@ final class DifferentialRevision extends DifferentialDAO return parent::save(); } - public function delete() { - $this->openTransaction(); - $diffs = id(new DifferentialDiffQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withRevisionIDs(array($this->getID())) - ->execute(); - foreach ($diffs as $diff) { - $diff->delete(); - } - - $conn_w = $this->establishConnection('w'); - - queryfx( - $conn_w, - 'DELETE FROM %T WHERE revisionID = %d', - self::TABLE_COMMIT, - $this->getID()); - - $inlines = id(new DifferentialInlineCommentQuery()) - ->withRevisionIDs(array($this->getID())) - ->execute(); - foreach ($inlines as $inline) { - $inline->delete(); - } - - // we have to do paths a little differentally as they do not have - // an id or phid column for delete() to act on - $dummy_path = new DifferentialAffectedPath(); - queryfx( - $conn_w, - 'DELETE FROM %T WHERE revisionID = %d', - $dummy_path->getTableName(), - $this->getID()); - - $result = parent::delete(); - $this->saveTransaction(); - return $result; - } - public function loadRelationships() { if (!$this->getID()) { $this->relationships = array(); @@ -477,4 +439,53 @@ final class DifferentialRevision extends DifferentialDAO return new DifferentialTransaction(); } + +/* -( PhabricatorDestructableInterface )----------------------------------- */ + + + public function destroyObjectPermanently( + PhabricatorDestructionEngine $engine) { + + $this->openTransaction(); + $diffs = id(new DifferentialDiffQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withRevisionIDs(array($this->getID())) + ->execute(); + foreach ($diffs as $diff) { + $engine->destroyObject($diff); + } + + $conn_w = $this->establishConnection('w'); + + queryfx( + $conn_w, + 'DELETE FROM %T WHERE revisionID = %d', + self::TABLE_COMMIT, + $this->getID()); + + try { + $inlines = id(new DifferentialInlineCommentQuery()) + ->withRevisionIDs(array($this->getID())) + ->execute(); + foreach ($inlines as $inline) { + $inline->delete(); + } + } catch (PhabricatorEmptyQueryException $ex) { + // TODO: There's still some funky legacy wrapping going on here, and + // we might catch a raw query exception. + } + + // we have to do paths a little differentally as they do not have + // an id or phid column for delete() to act on + $dummy_path = new DifferentialAffectedPath(); + queryfx( + $conn_w, + 'DELETE FROM %T WHERE revisionID = %d', + $dummy_path->getTableName(), + $this->getID()); + + $this->delete(); + $this->saveTransaction(); + } + } diff --git a/src/applications/system/engine/PhabricatorDestructionEngine.php b/src/applications/system/engine/PhabricatorDestructionEngine.php index 99ea47b2be..7b40c26726 100644 --- a/src/applications/system/engine/PhabricatorDestructionEngine.php +++ b/src/applications/system/engine/PhabricatorDestructionEngine.php @@ -41,20 +41,48 @@ final class PhabricatorDestructionEngine extends Phobject { if ($object_phid) { $this->destroyEdges($object_phid); + + if ($object instanceof PhabricatorApplicationTransactionInterface) { + $template = $object->getApplicationTransactionTemplate(); + $this->destroyTransactions($template, $object_phid); + } } + + // TODO: PhabricatorFlaggableInterface + // TODO: PhabricatorTokenReceiverInterface } private function destroyEdges($src_phid) { - $edges = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs(array($src_phid)) - ->execute(); + try { + $edges = id(new PhabricatorEdgeQuery()) + ->withSourcePHIDs(array($src_phid)) + ->execute(); + } catch (Exception $ex) { + // This is (presumably) a "no edges for this PHID type" exception. + return; + } $editor = id(new PhabricatorEdgeEditor()) ->setSuppressEvents(true); - foreach ($edges as $edge) { - $editor->removeEdge($edge['src'], $edge['type'], $edge['dst']); + foreach ($edges as $type => $type_edges) { + foreach ($type_edges as $src => $src_edges) { + foreach ($src_edges as $dst => $edge) { + $editor->removeEdge($edge['src'], $edge['type'], $edge['dst']); + } + } } $editor->save(); } + private function destroyTransactions( + PhabricatorApplicationTransaction $template, + $object_phid) { + + $xactions = $template->loadAllWhere('objectPHID = %s', $object_phid); + foreach ($xactions as $xaction) { + $this->destroyObject($xaction); + } + + } + } diff --git a/src/applications/system/management/PhabricatorSystemRemoveLogWorkflow.php b/src/applications/system/management/PhabricatorSystemRemoveLogWorkflow.php index 768ff30735..bf3a6cb49d 100644 --- a/src/applications/system/management/PhabricatorSystemRemoveLogWorkflow.php +++ b/src/applications/system/management/PhabricatorSystemRemoveLogWorkflow.php @@ -17,8 +17,9 @@ final class PhabricatorSystemRemoveLogWorkflow $table = new PhabricatorSystemDestructionLog(); foreach (new LiskMigrationIterator($table) as $row) { $console->writeOut( - "[%s]\t%s\t%s\t%s\n", + "[%s]\t%s %s\t%s\t%s\n", phabricator_datetime($row->getEpoch(), $this->getViewer()), + ($row->getRootLogID() ? ' ' : '*'), $row->getObjectClass(), $row->getObjectPHID(), $row->getObjectMonogram()); diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index c42f367892..d074f3f799 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -2,7 +2,9 @@ abstract class PhabricatorApplicationTransaction extends PhabricatorLiskDAO - implements PhabricatorPolicyInterface { + implements + PhabricatorPolicyInterface, + PhabricatorDestructableInterface { const TARGET_TEXT = 'text'; const TARGET_HTML = 'html'; @@ -942,4 +944,32 @@ abstract class PhabricatorApplicationTransaction } +/* -( PhabricatorDestructableInterface )----------------------------------- */ + + + public function destroyObjectPermanently( + PhabricatorDestructionEngine $engine) { + + $this->openTransaction(); + $comment_template = null; + try { + $comment_template = $this->getApplicationTransactionCommentObject(); + } catch (Exception $ex) { + // Continue; no comments for these transactions. + } + + if ($comment_template) { + $comments = $comment_template->loadAllWhere( + 'transactionPHID = %s', + $this->getPHID()); + foreach ($comments as $comment) { + $engine->destroyObject($comment); + } + } + + $this->delete(); + $this->saveTransaction(); + } + + } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php b/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php index 7f0f01af60..d68a524c02 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php @@ -2,7 +2,10 @@ abstract class PhabricatorApplicationTransactionComment extends PhabricatorLiskDAO - implements PhabricatorMarkupInterface, PhabricatorPolicyInterface { + implements + PhabricatorMarkupInterface, + PhabricatorPolicyInterface, + PhabricatorDestructableInterface { const MARKUP_FIELD_COMMENT = 'markup:comment'; @@ -118,4 +121,14 @@ abstract class PhabricatorApplicationTransactionComment return null; } + +/* -( PhabricatorDestructableInterface )----------------------------------- */ + + public function destroyObjectPermanently( + PhabricatorDestructionEngine $engine) { + $this->openTransaction(); + $this->delete(); + $this->saveTransaction(); + } + } -- 2.51.2