From 167f06d3eb70ed187219031e3422fdb6ce8b465f Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 16 May 2019 12:56:58 -0700 Subject: [PATCH] Label transaction groups with a "group ID" so Herald can reconstruct them faithfully Summary: Ref T13283. See PHI1202. See D20519. When we apply a group of transactions, label all of them with the same "group ID". This allows other things, notably Herald, to figure out which transactions applied together in a faithful way rather than by guessing, even though the guess was probably pretty good most of the time. Also expose this to `transaction.search` in case callers want to do something similar. They get a list of transaction IDs from webhooks already anyway, but some callers use `transaction.search` outside of webhooks and this information may be useful. Test Plan: - Ran Herald Test Console, saw faithful selection of recent transactions. - Changed hard limit from 1000 to 1, saw exception. Users should be very hard-pressed to hit this normally (they'd have to add 990-ish custom fields, then edit every field at once, I think) so I'm just fataling rather than processing some subset of the transaction set. - Called `transaction.search`, saw group ID information available. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13283 Differential Revision: https://secure.phabricator.com/D20524 --- .../HeraldTestConsoleController.php | 44 ++++++++++++++----- .../TransactionSearchConduitAPIMethod.php | 8 ++++ ...habricatorApplicationTransactionEditor.php | 4 ++ .../PhabricatorApplicationTransaction.php | 8 ++++ 4 files changed, 54 insertions(+), 10 deletions(-) diff --git a/src/applications/herald/controller/HeraldTestConsoleController.php b/src/applications/herald/controller/HeraldTestConsoleController.php index c8dfded2eb..5962996bb2 100644 --- a/src/applications/herald/controller/HeraldTestConsoleController.php +++ b/src/applications/herald/controller/HeraldTestConsoleController.php @@ -260,24 +260,48 @@ final class HeraldTestConsoleController extends HeraldController { $query = PhabricatorApplicationTransactionQuery::newQueryForObject( $object); - $xactions = $query + $query ->withObjectPHIDs(array($object->getPHID())) - ->setViewer($viewer) - ->setLimit(100) - ->execute(); + ->setViewer($viewer); + + $xactions = new PhabricatorQueryIterator($query); $applied = array(); - // Pick the most recent group of transactions. This may not be exactly the - // same as what Herald acted on: for example, we may select a single group - // of transactions here which were really applied across two or more edits. - // Since this is relatively rare and we show you what we picked, it's okay - // that we just do roughly the right thing. + $recent_id = null; + $hard_limit = 1000; foreach ($xactions as $xaction) { - if (!$xaction->shouldDisplayGroupWith($applied)) { + $group_id = $xaction->getTransactionGroupID(); + + // If this is the first transaction, save the group ID: we want to + // select all transactions in the same group. + if (!$applied) { + $recent_id = $group_id; + if ($recent_id === null) { + // If the first transaction has no group ID, it is likely an older + // transaction from before the introduction of group IDs. In this + // case, select only the most recent transaction and bail out. + $applied[] = $xaction; + break; + } + } + + // If this transaction is from a different transaction group, we've + // found all the transactions applied in the most recent group. + if ($group_id !== $recent_id) { break; } + $applied[] = $xaction; + + if (count($applied) > $hard_limit) { + throw new Exception( + pht( + 'This object ("%s") has more than %s transactions in its most '. + 'recent transaction group; this is too many.', + $object->getPHID(), + new PhutilNumber($hard_limit))); + } } return $applied; diff --git a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php index 8892933903..6f7f713dab 100644 --- a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php +++ b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php @@ -260,6 +260,13 @@ EOREMARKUP } } + $group_id = $xaction->getTransactionGroupID(); + if (!strlen($group_id)) { + $group_id = null; + } else { + $group_id = (string)$group_id; + } + $data[] = array( 'id' => (int)$xaction->getID(), 'phid' => (string)$xaction->getPHID(), @@ -268,6 +275,7 @@ EOREMARKUP 'objectPHID' => (string)$xaction->getObjectPHID(), 'dateCreated' => (int)$xaction->getDateCreated(), 'dateModified' => (int)$xaction->getDateModified(), + 'groupID' => $group_id, 'comments' => $comment_data, 'fields' => $fields, ); diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index b383d0605c..c9d8a0d998 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1162,6 +1162,8 @@ abstract class PhabricatorApplicationTransactionEditor throw $ex; } + $group_id = Filesystem::readRandomCharacters(32); + foreach ($xactions as $xaction) { if ($was_locked) { $is_override = $this->isLockOverrideTransaction($xaction); @@ -1171,6 +1173,8 @@ abstract class PhabricatorApplicationTransactionEditor } $xaction->setObjectPHID($object->getPHID()); + $xaction->setTransactionGroupID($group_id); + if ($xaction->getComment()) { $xaction->setPHID($xaction->generatePHID()); $comment_editor->applyEdit($xaction, $xaction->getComment()); diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index a55d5ce73e..1250b7cb16 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -177,6 +177,14 @@ abstract class PhabricatorApplicationTransaction return (bool)$this->getMetadataValue('core.lock-override', false); } + public function setTransactionGroupID($group_id) { + return $this->setMetadataValue('core.groupID', $group_id); + } + + public function getTransactionGroupID() { + return $this->getMetadataValue('core.groupID', null); + } + public function attachComment( PhabricatorApplicationTransactionComment $comment) { $this->comment = $comment;