Skip to content

Commit 18938b5

Browse files
author
epriestley
committed
Migrate Differential comments to ApplicationTransactions
Summary: Ref T2222. This is the big one. This migrates each `DifferentialComment` to one or more ApplicationTransactions (action, cc, reviewers, update, comment, inlines), and makes `DifferentialComment` a double-reader for ApplicationTransactions. The migration is pretty straightforward: - If a comment took an action not otherwise covered, it gets an "action" transaction. This is something like "epriestley abandoned this revision.". - If a comment updated the diff, it gets an "updated diff" transaction. Very old transactions of this type may not have a diff ID (probably only at Facebook). - If a comment added or removed reviewers, it gets a "changed reviewers" transaction. - If a comment added CCs, it gets a "subscribers" transaction. - If a comment added comment text, it gets a "comment" transaction. - For each inline attached to a comment, we generate an "inline" transaction. Most comments generate a small number of transactions, but a few generate a significant number. At HEAD, the code is basically already doing this, so comments in the last day or two already obey these rules, roughly, and will all generate only one transaction (except inlines). Because we've already preallocated PHIDs in the comment text table, we only need to write to the transaction table. NOTE: This significantly degrades Differential, making inline comments pretty much useless (they each get their own transaction, and don't show line numbers or files). The data is all fine, but the UI is garbage now. This needs to be fixed before we can deploy this to users, but it's easily separable since it's all just display code. Specifically, they look like this: {F112270} Test Plan: I've migrated locally and put things through their paces, but it's hard to catch sketchy stuff locally because most of my test data is nonsense and bad migrations wouldn't necessarily look out of place. IMPORTANT: I'm planning to push this to a branch and then shift production over to the branch, and run it for a day or two before bringing it to master. I generally feel good about this change: it's not that big since we were able to separate a lot of pieces out of it, and it's pretty straightforward. That said, it's still one of the most scary/dangerous changes we've ever made. Reviewers: btrahan CC: chad, aran Maniphest Tasks: T2222 Differential Revision: https://secure.phabricator.com/D8210
1 parent 3092efd commit 18938b5

14 files changed

+576
-184
lines changed

‎resources/sql/autopatches/20140212.dx.1.armageddon.php

Lines changed: 222 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,222 @@
1+
<?php
2+
3+
$conn_w = id(new DifferentialRevision())->establishConnection('w');
4+
$rows = new LiskRawMigrationIterator($conn_w, 'differential_comment');
5+
6+
$content_source = PhabricatorContentSource::newForSource(
7+
PhabricatorContentSource::SOURCE_LEGACY,
8+
array())->serialize();
9+
10+
echo "Migrating Differential comments to modern storage...\n";
11+
foreach ($rows as $row) {
12+
$id = $row['id'];
13+
echo "Migrating comment {$id}...\n";
14+
15+
$revision = id(new DifferentialRevision())->load($row['revisionID']);
16+
if (!$revision) {
17+
echo "No revision, continuing.\n";
18+
continue;
19+
}
20+
21+
$revision_phid = $revision->getPHID();
22+
23+
$comments = queryfx_all(
24+
$conn_w,
25+
'SELECT * FROM %T WHERE legacyCommentID = %d',
26+
'differential_transaction_comment',
27+
$id);
28+
29+
$main_comments = array();
30+
$inline_comments = array();
31+
32+
foreach ($comments as $comment) {
33+
if ($comment['changesetID']) {
34+
$inline_comments[] = $comment;
35+
} else {
36+
$main_comments[] = $comment;
37+
}
38+
}
39+
40+
$metadata = json_decode($row['metadata'], true);
41+
if (!is_array($metadata)) {
42+
$metadata = array();
43+
}
44+
45+
$key_cc = DifferentialComment::METADATA_ADDED_CCS;
46+
$key_add_rev = DifferentialComment::METADATA_ADDED_REVIEWERS;
47+
$key_rem_rev = DifferentialComment::METADATA_REMOVED_REVIEWERS;
48+
$key_diff_id = DifferentialComment::METADATA_DIFF_ID;
49+
50+
$xactions = array();
51+
52+
// Build the main action transaction.
53+
switch ($row['action']) {
54+
case DifferentialAction::ACTION_COMMENT:
55+
case DifferentialAction::ACTION_ADDREVIEWERS:
56+
case DifferentialAction::ACTION_ADDCCS:
57+
case DifferentialAction::ACTION_UPDATE:
58+
case DifferentialTransaction::TYPE_INLINE:
59+
// These actions will have their transactions created by other rules.
60+
break;
61+
default:
62+
// Otherwise, this is a normal action (like an accept or reject).
63+
$xactions[] = array(
64+
'type' => DifferentialTransaction::TYPE_ACTION,
65+
'old' => null,
66+
'new' => $row['action'],
67+
);
68+
break;
69+
}
70+
71+
// Build the diff update transaction, if one exists.
72+
$diff_id = idx($metadata, $key_diff_id);
73+
if (!is_scalar($diff_id)) {
74+
$diff_id = null;
75+
}
76+
77+
if ($diff_id || $row['action'] == DifferentialAction::ACTION_UPDATE) {
78+
$xactions[] = array(
79+
'type' => DifferentialTransaction::TYPE_UPDATE,
80+
'old' => null,
81+
'new' => $diff_id,
82+
);
83+
}
84+
85+
// Build the add/remove reviewers transaction, if one exists.
86+
$add_rev = idx($metadata, $key_add_rev, array());
87+
if (!is_array($add_rev)) {
88+
$add_rev = array();
89+
}
90+
$rem_rev = idx($metadata, $key_rem_rev, array());
91+
if (!is_array($rem_rev)) {
92+
$rem_rev = array();
93+
}
94+
95+
if ($add_rev || $rem_rev) {
96+
$old = array();
97+
foreach ($rem_rev as $phid) {
98+
if (!is_scalar($phid)) {
99+
continue;
100+
}
101+
$old[$phid] = array(
102+
'src' => $revision_phid,
103+
'type' => PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER,
104+
'dst' => $phid,
105+
);
106+
}
107+
108+
$new = array();
109+
foreach ($add_rev as $phid) {
110+
if (!is_scalar($phid)) {
111+
continue;
112+
}
113+
$new[$phid] = array(
114+
'src' => $revision_phid,
115+
'type' => PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER,
116+
'dst' => $phid,
117+
);
118+
}
119+
120+
$xactions[] = array(
121+
'type' => PhabricatorTransactions::TYPE_EDGE,
122+
'old' => $old,
123+
'new' => $new,
124+
'meta' => array(
125+
'edge:type' => PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER,
126+
),
127+
);
128+
}
129+
130+
// Build the CC transaction, if one exists.
131+
$add_cc = idx($metadata, $key_cc, array());
132+
if (!is_array($add_cc)) {
133+
$add_cc = array();
134+
}
135+
136+
if ($add_cc) {
137+
$xactions[] = array(
138+
'type' => PhabricatorTransactions::TYPE_SUBSCRIBERS,
139+
'old' => array(),
140+
'new' => array_fuse($add_cc),
141+
);
142+
}
143+
144+
145+
// Build the main comment transaction.
146+
foreach ($main_comments as $main) {
147+
$xactions[] = array(
148+
'type' => PhabricatorTransactions::TYPE_COMMENT,
149+
'old' => null,
150+
'new' => null,
151+
'phid' => $main['transactionPHID'],
152+
'comment' => $main,
153+
);
154+
}
155+
156+
// Build inline comment transactions.
157+
foreach ($inline_comments as $inline) {
158+
$xactions[] = array(
159+
'type' => DifferentialTransaction::TYPE_INLINE,
160+
'old' => null,
161+
'new' => null,
162+
'phid' => $inline['transactionPHID'],
163+
'comment' => $inline,
164+
);
165+
}
166+
167+
foreach ($xactions as $xaction) {
168+
// Generate a new PHID, if we don't already have one from the comment
169+
// table. We pregenerated into the comment table to make this a little
170+
// easier, so we only need to write to one table.
171+
$xaction_phid = idx($xaction, 'phid');
172+
if (!$xaction_phid) {
173+
$xaction_phid = PhabricatorPHID::generateNewPHID(
174+
PhabricatorApplicationTransactionPHIDTypeTransaction::TYPECONST,
175+
DifferentialPHIDTypeRevision::TYPECONST);
176+
}
177+
unset($xaction['phid']);
178+
179+
$comment_phid = null;
180+
$comment_version = 0;
181+
if (idx($xaction, 'comment')) {
182+
$comment_phid = $xaction['comment']['phid'];
183+
$comment_version = 1;
184+
}
185+
186+
$old = idx($xaction, 'old');
187+
$new = idx($xaction, 'new');
188+
$meta = idx($xaction, 'meta', array());
189+
190+
queryfx(
191+
$conn_w,
192+
'INSERT INTO %T (phid, authorPHID, objectPHID, viewPolicy, editPolicy,
193+
commentPHID, commentVersion, transactionType, oldValue, newValue,
194+
contentSource, metadata, dateCreated, dateModified)
195+
VALUES (%s, %s, %s, %s, %s, %ns, %d, %s, %ns, %ns, %s, %s, %d, %d)',
196+
'differential_transaction',
197+
198+
// PHID, authorPHID, objectPHID
199+
$xaction_phid,
200+
(string)$row['authorPHID'],
201+
$revision_phid,
202+
203+
// viewPolicy, editPolicy, commentPHID, commentVersion
204+
'public',
205+
(string)$row['authorPHID'],
206+
$comment_phid,
207+
$comment_version,
208+
209+
// transactionType, oldValue, newValue, contentSource, metadata
210+
$xaction['type'],
211+
json_encode($old),
212+
json_encode($new),
213+
$content_source,
214+
json_encode($meta),
215+
216+
// dates
217+
$row['dateCreated'],
218+
$row['dateModified']);
219+
}
220+
221+
}
222+
echo "Done.\n";

‎src/__phutil_library_map__.php

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,7 @@
464464
'DifferentialTitleFieldSpecification' => 'applications/differential/field/specification/DifferentialTitleFieldSpecification.php',
465465
'DifferentialTransaction' => 'applications/differential/storage/DifferentialTransaction.php',
466466
'DifferentialTransactionComment' => 'applications/differential/storage/DifferentialTransactionComment.php',
467+
'DifferentialTransactionQuery' => 'applications/differential/query/DifferentialTransactionQuery.php',
467468
'DifferentialUnitFieldSpecification' => 'applications/differential/field/specification/DifferentialUnitFieldSpecification.php',
468469
'DifferentialUnitStatus' => 'applications/differential/constants/DifferentialUnitStatus.php',
469470
'DifferentialUnitTestResult' => 'applications/differential/constants/DifferentialUnitTestResult.php',
@@ -2856,11 +2857,7 @@
28562857
'DifferentialChangesetTwoUpRenderer' => 'DifferentialChangesetHTMLRenderer',
28572858
'DifferentialChangesetTwoUpTestRenderer' => 'DifferentialChangesetTestRenderer',
28582859
'DifferentialChangesetViewController' => 'DifferentialController',
2859-
'DifferentialComment' =>
2860-
array(
2861-
0 => 'DifferentialDAO',
2862-
1 => 'PhabricatorMarkupInterface',
2863-
),
2860+
'DifferentialComment' => 'PhabricatorMarkupInterface',
28642861
'DifferentialCommentEditor' => 'PhabricatorEditor',
28652862
'DifferentialCommentMail' => 'DifferentialMail',
28662863
'DifferentialCommentPreviewController' => 'DifferentialController',
@@ -2981,6 +2978,7 @@
29812978
'DifferentialTitleFieldSpecification' => 'DifferentialFreeformFieldSpecification',
29822979
'DifferentialTransaction' => 'PhabricatorApplicationTransaction',
29832980
'DifferentialTransactionComment' => 'PhabricatorApplicationTransactionComment',
2981+
'DifferentialTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
29842982
'DifferentialUnitFieldSpecification' => 'DifferentialFieldSpecification',
29852983
'DifferentialViewPolicyFieldSpecification' => 'DifferentialFieldSpecification',
29862984
'DiffusionBranchTableController' => 'DiffusionController',

‎src/applications/differential/conduit/ConduitAPI_differential_getrevisioncomments_Method.php

Lines changed: 27 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,24 @@
11
<?php
22

3-
/**
4-
* @group conduit
5-
*/
63
final class ConduitAPI_differential_getrevisioncomments_Method
74
extends ConduitAPI_differential_Method {
85

6+
public function getMethodStatus() {
7+
return self::METHOD_STATUS_DEPRECATED;
8+
}
9+
10+
public function getMethodStatusDescription() {
11+
return pht('Obsolete and doomed, see T2222.');
12+
}
13+
914
public function getMethodDescription() {
1015
return "Retrieve Differential Revision Comments.";
1116
}
1217

1318
public function defineParamTypes() {
1419
return array(
1520
'ids' => 'required list<int>',
16-
'inlines' => 'optional bool',
21+
'inlines' => 'optional bool (deprecated)',
1722
);
1823
}
1924

@@ -34,47 +39,36 @@ protected function execute(ConduitAPIRequest $request) {
3439
return $results;
3540
}
3641

37-
$comments = id(new DifferentialCommentQuery())
38-
->withRevisionIDs($revision_ids)
42+
$revisions = id(new DifferentialRevisionQuery())
43+
->setViewer($request->getUser())
44+
->withIDs($revision_ids)
3945
->execute();
4046

41-
$with_inlines = $request->getValue('inlines');
42-
if ($with_inlines) {
43-
$inlines = id(new DifferentialInlineCommentQuery())
44-
->withRevisionIDs($revision_ids)
45-
->execute();
46-
$changesets = array();
47-
if ($inlines) {
48-
$changesets = id(new DifferentialChangeset())->loadAllWhere(
49-
'id IN (%Ld)',
50-
array_unique(mpull($inlines, 'getChangesetID')));
51-
$inlines = mgroup($inlines, 'getCommentID');
52-
}
47+
if (!$revisions) {
48+
return $results;
5349
}
5450

51+
$comments = id(new DifferentialCommentQuery())
52+
->withRevisionPHIDs(mpull($revisions, 'getPHID'))
53+
->execute();
54+
55+
$revisions = mpull($revisions, null, 'getPHID');
56+
5557
foreach ($comments as $comment) {
56-
// TODO: Sort this out in the ID -> PHID change.
57-
$revision_id = $comment->getRevisionID();
58+
$revision = idx($revisions, $comment->getRevisionPHID());
59+
if (!$revision) {
60+
continue;
61+
}
62+
5863
$result = array(
59-
'revisionID' => $revision_id,
64+
'revisionID' => $revision->getID(),
6065
'action' => $comment->getAction(),
6166
'authorPHID' => $comment->getAuthorPHID(),
6267
'dateCreated' => $comment->getDateCreated(),
6368
'content' => $comment->getContent(),
6469
);
6570

66-
if ($with_inlines) {
67-
$result['inlines'] = array();
68-
foreach (idx($inlines, $comment->getID(), array()) as $inline) {
69-
$changeset = idx($changesets, $inline->getChangesetID());
70-
$result['inlines'][] = $this->buildInlineInfoDictionary(
71-
$inline,
72-
$changeset);
73-
}
74-
// TODO: Put synthetic inlines without an attached comment somewhere.
75-
}
76-
77-
$results[$revision_id][] = $result;
71+
$results[$revision->getID()][] = $result;
7872
}
7973

8074
return $results;

‎src/applications/differential/constants/DifferentialAction.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,11 @@ public static function getBasicStoryText($action, $author_name) {
9090
$title = pht('%s reopened this revision.',
9191
$author_name);
9292
break;
93+
case DifferentialTransaction::TYPE_INLINE:
94+
$title = pht(
95+
'%s added an inline comment.',
96+
$author_name);
97+
break;
9398
default:
9499
$title = pht('Ghosts happened to this revision.');
95100
break;

0 commit comments

Comments
 (0)