From 1a3ac099751b08f350b63b60406b4e7a08357785 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 20 Apr 2014 11:55:18 -0700 Subject: [PATCH] Add "requestedObjectPHID" to ReleephRequest Summary: Ref T3551. Currently, ReleephRequests don't have a direct concept of the //object// being requested. You can request `D123`, but that is just a convenient way to write `rXyyyy`. When the UI wants to display information about a revision, it deduces it by examining the commit. This is primarily an attack on T3551, so we don't need to load edge -> revision> (in an ad-hoc way) to get revisions. Instead, when you request a revision we keep track of it and can load it directly later. Later, this will let us do more things: for example, if you request a branch, we can automatically update the commits (as GitHub does), etc. (Repository branches will need PHIDs first, of course.) This adds and populates the column but doesn't use it yet. The second part of the migration could safely be run while Phabricator is up, although even for Facebook this table is probably quite small. Test Plan: - Ran migration. - Verified existing requests associated sensibly. - Created a new commit request. - Created a new revision request. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T3551 Differential Revision: https://secure.phabricator.com/D8822 --- .../autopatches/20140420.rel.1.objectphid.sql | 5 +++ .../autopatches/20140420.rel.2.objectmig.php | 45 +++++++++++++++++++ .../commitfinder/ReleephCommitFinder.php | 22 ++++++++- .../ConduitAPI_releeph_request_Method.php | 9 +++- .../request/ReleephRequestEditController.php | 12 ++++- .../releeph/storage/ReleephRequest.php | 22 +++++++++ 6 files changed, 112 insertions(+), 3 deletions(-) create mode 100644 resources/sql/autopatches/20140420.rel.1.objectphid.sql create mode 100644 resources/sql/autopatches/20140420.rel.2.objectmig.php diff --git a/resources/sql/autopatches/20140420.rel.1.objectphid.sql b/resources/sql/autopatches/20140420.rel.1.objectphid.sql new file mode 100644 index 0000000000..397b988d92 --- /dev/null +++ b/resources/sql/autopatches/20140420.rel.1.objectphid.sql @@ -0,0 +1,5 @@ +ALTER TABLE {$NAMESPACE}_releeph.releeph_request + ADD COLUMN requestedObjectPHID VARCHAR(64) COLLATE utf8_bin NOT NULL; + +ALTER TABLE {$NAMESPACE}_releeph.releeph_request + ADD KEY `key_requestedObject` (requestedObjectPHID); diff --git a/resources/sql/autopatches/20140420.rel.2.objectmig.php b/resources/sql/autopatches/20140420.rel.2.objectmig.php new file mode 100644 index 0000000000..04263a38ab --- /dev/null +++ b/resources/sql/autopatches/20140420.rel.2.objectmig.php @@ -0,0 +1,45 @@ +getTableName(); +$conn_w = $pull_table->establishConnection('w'); + +echo "Setting object PHIDs for requests...\n"; +foreach (new LiskMigrationIterator($pull_table) as $pull) { + $id = $pull->getID(); + + echo "Migrating pull request {$id}...\n"; + if ($pull->getRequestedObjectPHID()) { + // We already have a valid PHID, so skip this request. + continue; + } + + $commit_phids = $pull->getCommitPHIDs(); + if (count($commit_phids) != 1) { + // At the time this migration was written, all requests had exactly one + // commit. If a request has more than one, we don't have the information + // we need to process it. + continue; + } + + $commit_phid = head($commit_phids); + + $revision_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( + $commit_phid, + PhabricatorEdgeConfig::TYPE_COMMIT_HAS_DREV); + + if ($revision_phids) { + $object_phid = head($revision_phids); + } else { + $object_phid = $commit_phid; + } + + queryfx( + $conn_w, + 'UPDATE %T SET requestedObjectPHID = %s WHERE id = %d', + $table_name, + $object_phid, + $id); +} + +echo "Done.\n"; diff --git a/src/applications/releeph/commitfinder/ReleephCommitFinder.php b/src/applications/releeph/commitfinder/ReleephCommitFinder.php index 1c7bfce23a..e1d569d92f 100644 --- a/src/applications/releeph/commitfinder/ReleephCommitFinder.php +++ b/src/applications/releeph/commitfinder/ReleephCommitFinder.php @@ -4,6 +4,7 @@ final class ReleephCommitFinder { private $releephProject; private $user; + private $objectPHID; public function setUser(PhabricatorUser $user) { $this->user = $user; @@ -18,7 +19,13 @@ final class ReleephCommitFinder { return $this; } + public function getRequestedObjectPHID() { + return $this->objectPHID; + } + public function fromPartial($partial_string) { + $this->objectPHID = null; + // Look for diffs $matches = array(); if (preg_match('/^D([1-9]\d*)$/', $partial_string, $matches)) { @@ -36,6 +43,8 @@ final class ReleephCommitFinder { "{$partial_string} has no commits associated with it yet."); } + $this->objectPHID = $diff_rev->getPHID(); + $commits = id(new PhabricatorRepositoryCommit())->loadAllWhere( 'phid IN (%Ls) ORDER BY epoch ASC', $commit_phids); @@ -43,7 +52,7 @@ final class ReleephCommitFinder { } // Look for a raw commit number, or r. - $repository = $this->releephProject->loadPhabricatorRepository(); + $repository = $this->releephProject->getRepository(); $dr_data = null; $matches = array(); if (preg_match('/^r(?P[A-Z]+)(?P\w+)$/', @@ -79,6 +88,17 @@ final class ReleephCommitFinder { "The commit {$partial_string} doesn't exist in this repository."); } + // When requesting a single commit, if it has an associated review we + // imply the review was requested instead. This is always correct for now + // and consistent with the older behavior, although it might not be the + // right rule in the future. + $phids = PhabricatorEdgeQuery::loadDestinationPHIDs( + $phabricator_repository_commit->getPHID(), + PhabricatorEdgeConfig::TYPE_COMMIT_HAS_DREV); + if ($phids) { + $this->objectPHID = head($phids); + } + return $phabricator_repository_commit; } diff --git a/src/applications/releeph/conduit/ConduitAPI_releeph_request_Method.php b/src/applications/releeph/conduit/ConduitAPI_releeph_request_Method.php index 762b4a4c9f..88ae176c79 100644 --- a/src/applications/releeph/conduit/ConduitAPI_releeph_request_Method.php +++ b/src/applications/releeph/conduit/ConduitAPI_releeph_request_Method.php @@ -49,6 +49,7 @@ final class ConduitAPI_releeph_request_Method // Find the requested commit identifiers $requested_commits = array(); + $requested_object_phids = array(); $things = $request->getValue('things'); $finder = id(new ReleephCommitFinder()) ->setUser($user) @@ -56,6 +57,11 @@ final class ConduitAPI_releeph_request_Method foreach ($things as $thing) { try { $requested_commits[$thing] = $finder->fromPartial($thing); + $object_phid = $finder->getRequestedObjectPHID(); + if (!$object_phid) { + $object_phid = $requested_commits[$thing]->getPHID(); + } + $requested_object_phids[$thing] = $object_phid; } catch (ReleephCommitFinderException $ex) { throw id(new ConduitException('ERR_NO_MATCHES')) ->setErrorDescription($ex->getMessage()); @@ -99,7 +105,8 @@ final class ConduitAPI_releeph_request_Method $releeph_request = id(new ReleephRequest()) ->setRequestUserPHID($user->getPHID()) ->setBranchID($releeph_branch->getID()) - ->setInBranch(0); + ->setInBranch(0) + ->setRequestedObjectPHID($requested_object_phids[$thing]); $xactions = array(); diff --git a/src/applications/releeph/controller/request/ReleephRequestEditController.php b/src/applications/releeph/controller/request/ReleephRequestEditController.php index 3784885a92..b87e48794f 100644 --- a/src/applications/releeph/controller/request/ReleephRequestEditController.php +++ b/src/applications/releeph/controller/request/ReleephRequestEditController.php @@ -43,7 +43,8 @@ final class ReleephRequestEditController extends ReleephBranchController { $pull = id(new ReleephRequest()) ->setRequestUserPHID($viewer->getPHID()) ->setBranchID($branch->getID()) - ->setInBranch(0); + ->setInBranch(0) + ->attachBranch($branch); $is_edit = false; } @@ -116,6 +117,15 @@ final class ReleephRequestEditController extends ReleephBranchController { $errors[] = "The requested commit hasn't been parsed yet."; } } + + if (!$errors) { + $object_phid = $finder->getRequestedObjectPHID(); + if (!$object_phid) { + $object_phid = $pr_commit->getPHID(); + } + + $pull->setRequestedObjectPHID($object_phid); + } } if (!$errors) { diff --git a/src/applications/releeph/storage/ReleephRequest.php b/src/applications/releeph/storage/ReleephRequest.php index 3c5af69850..4ff2af1ae7 100644 --- a/src/applications/releeph/storage/ReleephRequest.php +++ b/src/applications/releeph/storage/ReleephRequest.php @@ -13,6 +13,13 @@ final class ReleephRequest extends ReleephDAO protected $pickStatus; protected $mailKey; + /** + * The object which is being requested. Normally this is a commit, but it + * might also be a revision. In the future, it could be a repository branch + * or an external object (like a GitHub pull request). + */ + protected $requestedObjectPHID; + // Information about the thing being requested protected $requestCommitPHID; @@ -20,6 +27,7 @@ final class ReleephRequest extends ReleephDAO protected $commitIdentifier; protected $commitPHID; + private $customFields = self::ATTACHABLE; private $branch = self::ATTACHABLE; @@ -163,6 +171,20 @@ final class ReleephRequest extends ReleephDAO return $this; } + + /** + * Get the commit PHIDs this request is requesting. + * + * NOTE: For now, this always returns one PHID. + * + * @return list Commit PHIDs requested by this request. + */ + public function getCommitPHIDs() { + return array( + $this->requestCommitPHID, + ); + } + public function getReason() { // Backward compatibility: reason used to be called comments $reason = $this->getDetail('reason');