From f010730e49b0859052ee1ba3f77fd653afcaf497 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 19 Oct 2013 05:03:25 -0700 Subject: [PATCH] Migrate all Differential inline comments to ApplicationTransactions Summary: Ref T2222. This implements step (1) described there, which is moving over all the inline comments. The old and new tables are simliar. The only real trick here is that `transactionPHID` and `legacyCommentID` mean roughly the same thing (`null` if the inline is a draft, non-null if it has been submitted) but we don't have real `transactionPHID`s yet. We just make some up -- we'll backfill them later. Two risks here: - I need to take a second look at the keys on this table. I think we need to tweak them a bit, and it will be less disruptive to do that before this migration than after. - This will take a while for Facebook, and other large installs with tens of thousands of revisions. I'll communicate this. I'm otherwise pretty satisfied with this, seems to work well and is pretty low risk / non-disruptive. Test Plan: - Before migrating, then after migrating: - Made a bunch of inlines (drafts, submitted). - Edited and deleted inlines. - Verified inlines showed up in preview. - Verified that inlines aren't indexed when they're drafts (`bin/search index D935`). - Verified that inlines ARE indexed when they're not drafts. - Verified that drafts inlines make revisions appear as "with draft" in the revision list. - Made left, right, and draft inlines. - Migrated (`bin/storage upgrade`). - Verified that my inlines from before the migration still showed up. - (Repeated all the stuff above.) - Manually inspected the inline comment table. Reviewers: btrahan Reviewed By: btrahan CC: FacebookPOC, aran Maniphest Tasks: T2222 Differential Revision: https://secure.phabricator.com/D7139 --- resources/sql/patches/20130926.dinline.php | 90 ++++++++++++ ...ifferentialInlineCommentEditController.php | 2 +- .../query/DifferentialInlineCommentQuery.php | 59 ++++++-- .../query/DifferentialRevisionQuery.php | 7 + .../storage/DifferentialInlineComment.php | 128 ++++++++++++++---- .../DifferentialTransactionComment.php | 6 +- .../patch/PhabricatorBuiltinPatchList.php | 4 + 7 files changed, 252 insertions(+), 44 deletions(-) create mode 100644 resources/sql/patches/20130926.dinline.php diff --git a/resources/sql/patches/20130926.dinline.php b/resources/sql/patches/20130926.dinline.php new file mode 100644 index 0000000000..75d67efdbd --- /dev/null +++ b/resources/sql/patches/20130926.dinline.php @@ -0,0 +1,90 @@ +establishConnection('w'); +$conn_w->openTransaction(); + +$src_table = 'differential_inlinecomment'; +$dst_table = 'differential_transaction_comment'; + +echo "Migrating Differential inline comments to new format...\n"; + +$content_source = PhabricatorContentSource::newForSource( + PhabricatorContentSource::SOURCE_LEGACY, + array())->serialize(); + +$rows = new LiskRawMigrationIterator($conn_w, $src_table); +foreach ($rows as $row) { + $id = $row['id']; + + $revision_id = $row['revisionID']; + + echo "Migrating inline #{$id} (D{$revision_id})...\n"; + + $revision_row = queryfx_one( + $conn_w, + 'SELECT phid FROM %T WHERE id = %d', + $revision_table->getTableName(), + $revision_id); + if (!$revision_row) { + continue; + } + + $revision_phid = $revision_row['phid']; + + if ($row['commentID']) { + $xaction_phid = PhabricatorPHID::generateNewPHID( + PhabricatorApplicationTransactionPHIDTypeTransaction::TYPECONST, + DifferentialPHIDTypeRevision::TYPECONST); + } else { + $xaction_phid = null; + } + + $comment_phid = PhabricatorPHID::generateNewPHID( + PhabricatorPHIDConstants::PHID_TYPE_XCMT, + DifferentialPHIDTypeRevision::TYPECONST); + + queryfx( + $conn_w, + 'INSERT IGNORE INTO %T + (id, phid, transactionPHID, authorPHID, viewPolicy, editPolicy, + commentVersion, content, contentSource, isDeleted, + dateCreated, dateModified, revisionPHID, changesetID, + isNewFile, lineNumber, lineLength, hasReplies, legacyCommentID) + VALUES (%d, %s, %ns, %s, %s, %s, + %d, %s, %s, %d, + %d, %d, %s, %nd, + %d, %d, %d, %d, %nd)', + $dst_table, + + // id, phid, transactionPHID, authorPHID, viewPolicy, editPolicy + $row['id'], + $comment_phid, + $xaction_phid, + $row['authorPHID'], + 'public', + $row['authorPHID'], + + // commentVersion, content, contentSource, isDeleted + 1, + $row['content'], + $content_source, + 0, + + // dateCreated, dateModified, revisionPHID, changesetID + $row['dateCreated'], + $row['dateModified'], + $revision_phid, + $row['changesetID'], + + // isNewFile, lineNumber, lineLength, hasReplies, legacyCommentID + $row['isNewFile'], + $row['lineNumber'], + $row['lineLength'], + 0, + $row['commentID']); + +} + +$conn_w->saveTransaction(); +echo "Done.\n"; diff --git a/src/applications/differential/controller/DifferentialInlineCommentEditController.php b/src/applications/differential/controller/DifferentialInlineCommentEditController.php index 62c5d6c781..c09caa9374 100644 --- a/src/applications/differential/controller/DifferentialInlineCommentEditController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentEditController.php @@ -30,7 +30,7 @@ final class DifferentialInlineCommentEditController } return id(new DifferentialInlineComment()) - ->setRevisionID($revision_id) + ->setRevision($revision) ->setChangesetID($changeset_id); } diff --git a/src/applications/differential/query/DifferentialInlineCommentQuery.php b/src/applications/differential/query/DifferentialInlineCommentQuery.php index 215a4d61b6..8eb59760c1 100644 --- a/src/applications/differential/query/DifferentialInlineCommentQuery.php +++ b/src/applications/differential/query/DifferentialInlineCommentQuery.php @@ -51,7 +51,7 @@ final class DifferentialInlineCommentQuery } public function execute() { - $table = new DifferentialInlineComment(); + $table = new DifferentialTransactionComment(); $conn_r = $table->establishConnection('r'); $data = queryfx_all( @@ -61,7 +61,14 @@ final class DifferentialInlineCommentQuery $this->buildWhereClause($conn_r), $this->buildLimitClause($conn_r)); - return $table->loadAllFromArray($data); + $comments = $table->loadAllFromArray($data); + + foreach ($comments as $key => $value) { + $comments[$key] = DifferentialInlineComment::newFromModernComment( + $value); + } + + return $comments; } public function executeOne() { @@ -71,17 +78,35 @@ final class DifferentialInlineCommentQuery private function buildWhereClause(AphrontDatabaseConnection $conn_r) { $where = array(); + // Only find inline comments. + $where[] = qsprintf( + $conn_r, + 'changesetID IS NOT NULL'); + if ($this->revisionIDs) { - $where[] = qsprintf( + + // Look up revision PHIDs. + $revision_phids = queryfx_all( $conn_r, - 'revisionID IN (%Ld)', + 'SELECT phid FROM %T WHERE id IN (%Ld)', + id(new DifferentialRevision())->getTableName(), $this->revisionIDs); + + if (!$revision_phids) { + throw new PhabricatorEmptyQueryException(); + } + $revision_phids = ipull($revision_phids, 'phid'); + + $where[] = qsprintf( + $conn_r, + 'revisionPHID IN (%Ls)', + $revision_phids); } if ($this->notDraft) { $where[] = qsprintf( $conn_r, - 'commentID IS NOT NULL'); + 'transactionPHID IS NOT NULL'); } if ($this->ids) { @@ -95,31 +120,45 @@ final class DifferentialInlineCommentQuery list($phid, $ids) = $this->viewerAndChangesetIDs; $where[] = qsprintf( $conn_r, - 'changesetID IN (%Ld) AND (authorPHID = %s OR commentID IS NOT NULL)', + 'changesetID IN (%Ld) AND + (authorPHID = %s OR transactionPHID IS NOT NULL)', $ids, $phid); } if ($this->draftComments) { list($phid, $rev_id) = $this->draftComments; + + $rev_phid = queryfx_one( + $conn_r, + 'SELECT phid FROM %T WHERE id = %d', + id(new DifferentialRevision())->getTableName(), + $rev_id); + + if (!$rev_phid) { + throw new PhabricatorEmptyQueryException(); + } + + $rev_phid = $rev_phid['phid']; + $where[] = qsprintf( $conn_r, - 'authorPHID = %s AND revisionID = %d AND commentID IS NULL', + 'authorPHID = %s AND revisionPHID = %s AND transactionPHID IS NULL', $phid, - $rev_id); + $rev_phid); } if ($this->draftsByAuthors) { $where[] = qsprintf( $conn_r, - 'authorPHID IN (%Ls) AND commentID IS NULL', + 'authorPHID IN (%Ls) AND transactionPHID IS NULL', $this->draftsByAuthors); } if ($this->commentIDs) { $where[] = qsprintf( $conn_r, - 'commentID IN (%Ld)', + 'legacyCommentID IN (%Ld)', $this->commentIDs); } diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index 90284fefb9..556ec6a83c 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -491,6 +491,11 @@ final class DifferentialRevisionQuery $this->draftRevisions[] = substr($draft->getDraftKey(), $len); } + // TODO: Restore this after drafts are sorted out. It's now very + // expensive to get revision IDs. + + /* + $inlines = id(new DifferentialInlineCommentQuery()) ->withDraftsByAuthors($this->draftAuthors) ->execute(); @@ -498,6 +503,8 @@ final class DifferentialRevisionQuery $this->draftRevisions[] = $inline->getRevisionID(); } + */ + if (!$this->draftRevisions) { return array(); } diff --git a/src/applications/differential/storage/DifferentialInlineComment.php b/src/applications/differential/storage/DifferentialInlineComment.php index 469bd3ea5d..af30563776 100644 --- a/src/applications/differential/storage/DifferentialInlineComment.php +++ b/src/applications/differential/storage/DifferentialInlineComment.php @@ -1,21 +1,52 @@ proxy = new DifferentialTransactionComment(); + } - private $syntheticAuthor; + public function __clone() { + $this->proxy = clone $this->proxy; + } + + public function save() { + $content_source = PhabricatorContentSource::newForSource( + PhabricatorContentSource::SOURCE_LEGACY, + array()); + + $this->proxy + ->setViewPolicy('public') + ->setEditPolicy($this->getAuthorPHID()) + ->setContentSource($content_source) + ->setCommentVersion(1) + ->save(); + + return $this; + } + + public function delete() { + $this->proxy->delete(); + + return $this; + } + + public function getID() { + return $this->proxy->getID(); + } + + public static function newFromModernComment( + DifferentialTransactionComment $comment) { + + $obj = new DifferentialInlineComment(); + $obj->proxy = $comment; + + return $obj; + } public function setSyntheticAuthor($synthetic_author) { $this->syntheticAuthor = $synthetic_author; @@ -34,75 +65,112 @@ final class DifferentialInlineComment } public function setContent($content) { - $this->setCache(null); - $this->writeField('content', $content); + $this->proxy->setContent($content); return $this; } public function getContent() { - return $this->readField('content'); + return $this->proxy->getContent(); } public function isDraft() { return !$this->getCommentID(); } - // NOTE: We need to provide implementations so we conform to the shared - // interface; these are all trivial and just explicit versions of the Lisk - // defaults. - public function setChangesetID($id) { - $this->writeField('changesetID', $id); + $this->proxy->setChangesetID($id); return $this; } public function getChangesetID() { - return $this->readField('changesetID'); + return $this->proxy->getChangesetID(); } public function setIsNewFile($is_new) { - $this->writeField('isNewFile', $is_new); + $this->proxy->setIsNewFile($is_new); return $this; } public function getIsNewFile() { - return $this->readField('isNewFile'); + return $this->proxy->getIsNewFile(); } public function setLineNumber($number) { - $this->writeField('lineNumber', $number); + $this->proxy->setLineNumber($number); return $this; } public function getLineNumber() { - return $this->readField('lineNumber'); + return $this->proxy->getLineNumber(); } public function setLineLength($length) { - $this->writeField('lineLength', $length); + $this->proxy->setLineLength($length); return $this; } public function getLineLength() { - return $this->readField('lineLength'); + return $this->proxy->getLineLength(); } public function setCache($cache) { - $this->writeField('cache', $cache); return $this; } public function getCache() { - return $this->readField('cache'); + return null; } public function setAuthorPHID($phid) { - $this->writeField('authorPHID', $phid); + $this->proxy->setAuthorPHID($phid); return $this; } public function getAuthorPHID() { - return $this->readField('authorPHID'); + return $this->proxy->getAuthorPHID(); + } + + public function setRevision(DifferentialRevision $revision) { + $this->proxy->setRevisionPHID($revision->getPHID()); + return $this; + } + + // Although these are purely transitional, they're also *extra* dumb. + + public function setRevisionID($revision_id) { + $revision = id(new DifferentialRevision())->load($revision_id); + return $this->setRevision($revision); + } + + public function getRevisionID() { + $phid = $this->proxy->getRevisionPHID(); + if (!$phid) { + return null; + } + + $revision = id(new DifferentialRevision())->loadOneWhere( + 'phid = %s', + $phid); + if (!$revision) { + return null; + } + return $revision->getID(); + } + + // When setting a comment ID, we also generate a phantom transaction PHID for + // the future transaction. + + public function setCommentID($id) { + $this->proxy->setLegacyCommentID($id); + $this->proxy->setTransactionPHID( + PhabricatorPHID::generateNewPHID( + PhabricatorApplicationTransactionPHIDTypeTransaction::TYPECONST, + DifferentialPHIDTypeRevision::TYPECONST)); + return $this; + } + + public function getCommentID() { + return $this->proxy->getLegacyCommentID(); } diff --git a/src/applications/differential/storage/DifferentialTransactionComment.php b/src/applications/differential/storage/DifferentialTransactionComment.php index c46d0c4b58..d527c44100 100644 --- a/src/applications/differential/storage/DifferentialTransactionComment.php +++ b/src/applications/differential/storage/DifferentialTransactionComment.php @@ -5,9 +5,9 @@ final class DifferentialTransactionComment protected $revisionPHID; protected $changesetID; - protected $isNewFile; - protected $lineNumber; - protected $lineLength; + protected $isNewFile = 0; + protected $lineNumber = 0; + protected $lineLength = 0; protected $fixedState; protected $hasReplies = 0; protected $replyToCommentPHID; diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index 638d1cf283..fab4e35706 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1680,6 +1680,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('20130915.maniphestqdrop.sql'), ), + '20130926.dinline.php' => array( + 'type' => 'php', + 'name' => $this->getPatchPath('20130926.dinline.php'), + ), ); } } -- 2.51.2