From 52e65f3d47616950581a396d2d9a74759c79f2b0 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Tue, 17 Sep 2013 13:55:41 -0700 Subject: [PATCH] Add a differential.getdiffs method Summary: I kind of made a mess of the API doing T2784. I figure just adding this is fine but LMK if you'd prefer something like diffquery got cleaned up more to handle this. Also adds an idx() call as I was getting errors looking at old diffs. Fixes T3823. Test Plan: used the new api via test console - great success. Reviewers: epriestley Reviewed By: epriestley CC: Korvin, aran Maniphest Tasks: T3823 Differential Revision: https://secure.phabricator.com/D6966 --- src/__phutil_library_map__.php | 2 + ...uitAPI_differential_getalldiffs_Method.php | 9 +++ ...ConduitAPI_differential_getdiff_Method.php | 69 +++++++++---------- ...uitAPI_differential_getrevision_Method.php | 17 ++--- ...duitAPI_differential_querydiffs_Method.php | 45 ++++++++++++ .../DifferentialRevisionViewController.php | 10 +-- .../mail/DifferentialReviewRequestMail.php | 14 ++-- .../query/DifferentialDiffQuery.php | 56 +++++++++++++++ .../differential/storage/DifferentialDiff.php | 63 ++++++++++++----- .../storage/DifferentialRevision.php | 14 ++-- 10 files changed, 212 insertions(+), 87 deletions(-) create mode 100644 src/applications/differential/conduit/ConduitAPI_differential_querydiffs_Method.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2e8ba176e5..f481357713 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -138,6 +138,7 @@ phutil_register_library_map(array( 'ConduitAPI_differential_markcommitted_Method' => 'applications/differential/conduit/ConduitAPI_differential_markcommitted_Method.php', 'ConduitAPI_differential_parsecommitmessage_Method' => 'applications/differential/conduit/ConduitAPI_differential_parsecommitmessage_Method.php', 'ConduitAPI_differential_query_Method' => 'applications/differential/conduit/ConduitAPI_differential_query_Method.php', + 'ConduitAPI_differential_querydiffs_Method' => 'applications/differential/conduit/ConduitAPI_differential_querydiffs_Method.php', 'ConduitAPI_differential_setdiffproperty_Method' => 'applications/differential/conduit/ConduitAPI_differential_setdiffproperty_Method.php', 'ConduitAPI_differential_updaterevision_Method' => 'applications/differential/conduit/ConduitAPI_differential_updaterevision_Method.php', 'ConduitAPI_differential_updateunitresults_Method' => 'applications/differential/conduit/ConduitAPI_differential_updateunitresults_Method.php', @@ -2200,6 +2201,7 @@ phutil_register_library_map(array( 'ConduitAPI_differential_markcommitted_Method' => 'ConduitAPIMethod', 'ConduitAPI_differential_parsecommitmessage_Method' => 'ConduitAPIMethod', 'ConduitAPI_differential_query_Method' => 'ConduitAPIMethod', + 'ConduitAPI_differential_querydiffs_Method' => 'ConduitAPIMethod', 'ConduitAPI_differential_setdiffproperty_Method' => 'ConduitAPIMethod', 'ConduitAPI_differential_updaterevision_Method' => 'ConduitAPIMethod', 'ConduitAPI_differential_updateunitresults_Method' => 'ConduitAPIMethod', diff --git a/src/applications/differential/conduit/ConduitAPI_differential_getalldiffs_Method.php b/src/applications/differential/conduit/ConduitAPI_differential_getalldiffs_Method.php index 4b06a80e87..b67ac232f8 100644 --- a/src/applications/differential/conduit/ConduitAPI_differential_getalldiffs_Method.php +++ b/src/applications/differential/conduit/ConduitAPI_differential_getalldiffs_Method.php @@ -6,6 +6,15 @@ final class ConduitAPI_differential_getalldiffs_Method extends ConduitAPIMethod { + public function getMethodStatus() { + return self::METHOD_STATUS_DEPRECATED; + } + + public function getMethodStatusDescription() { + return pht( + 'This method has been deprecated in favor of differential.querydiffs.'); + } + public function getMethodDescription() { return "Load all diffs for given revisions from Differential."; } diff --git a/src/applications/differential/conduit/ConduitAPI_differential_getdiff_Method.php b/src/applications/differential/conduit/ConduitAPI_differential_getdiff_Method.php index 6067e5ae70..e2efa11310 100644 --- a/src/applications/differential/conduit/ConduitAPI_differential_getdiff_Method.php +++ b/src/applications/differential/conduit/ConduitAPI_differential_getdiff_Method.php @@ -3,10 +3,22 @@ /** * @group conduit */ -final class ConduitAPI_differential_getdiff_Method extends ConduitAPIMethod { +final class ConduitAPI_differential_getdiff_Method + extends ConduitAPIMethod { + + public function getMethodStatus() { + return self::METHOD_STATUS_DEPRECATED; + } + + public function getMethodStatusDescription() { + return pht( + 'This method has been deprecated in favor of differential.querydiffs.'); + } + public function getMethodDescription() { - return "Load the content of a diff from Differential."; + return pht('Load the content of a diff from Differential by revision id '. + 'or diff id.'); } public function defineParamTypes() { @@ -22,7 +34,6 @@ final class ConduitAPI_differential_getdiff_Method extends ConduitAPIMethod { public function defineErrorTypes() { return array( - 'ERR_BAD_REVISION' => 'No such revision exists.', 'ERR_BAD_DIFF' => 'No such diff exists.', ); } @@ -33,50 +44,32 @@ final class ConduitAPI_differential_getdiff_Method extends ConduitAPIMethod { protected function execute(ConduitAPIRequest $request) { $diff = null; + $revision_ids = array(); + $diff_ids = array(); $revision_id = $request->getValue('revision_id'); if ($revision_id) { - $revision = id(new DifferentialRevision())->load($revision_id); - if (!$revision) { - throw new ConduitException('ERR_BAD_REVISION'); - } - $diff = id(new DifferentialDiff())->loadOneWhere( - 'revisionID = %d ORDER BY id DESC LIMIT 1', - $revision->getID()); - } else { - $diff_id = $request->getValue('diff_id'); - if ($diff_id) { - $diff = id(new DifferentialDiffQuery()) - ->setViewer($request->getUser()) - ->withIDs(array($diff_id)) - ->executeOne(); - } + $revision_ids = array($revision_id); + } + $diff_id = $request->getValue('diff_id'); + if ($diff_id) { + $diff_ids = array($diff_id); + } + if ($diff_ids || $revision_ids) { + $diff = id(new DifferentialDiffQuery()) + ->setViewer($request->getUser()) + ->withIDs($diff_ids) + ->withRevisionIDs($revision_ids) + ->needChangesets(true) + ->needArcanistProjects(true) + ->executeOne(); } if (!$diff) { throw new ConduitException('ERR_BAD_DIFF'); } - $diff->attachChangesets( - $diff->loadRelatives(new DifferentialChangeset(), 'diffID')); - foreach ($diff->getChangesets() as $changeset) { - $changeset->attachHunks( - $changeset->loadRelatives(new DifferentialHunk(), 'changesetID')); - } - - $basic_dict = $diff->getDiffDict(); - - // for conduit calls, the basic dict is not enough - // we also need to include the arcanist project and author information - $project = $diff->loadArcanistProject(); - if ($project) { - $project_name = $project->getName(); - } else { - $project_name = null; - } - $basic_dict['projectName'] = $project_name; - - return $basic_dict; + return $diff->getDiffDict(); } } diff --git a/src/applications/differential/conduit/ConduitAPI_differential_getrevision_Method.php b/src/applications/differential/conduit/ConduitAPI_differential_getrevision_Method.php index 272e143448..5371810cdb 100644 --- a/src/applications/differential/conduit/ConduitAPI_differential_getrevision_Method.php +++ b/src/applications/differential/conduit/ConduitAPI_differential_getrevision_Method.php @@ -51,17 +51,12 @@ final class ConduitAPI_differential_getrevision_Method $reviewer_phids = array_values($revision->getReviewers()); - $diffs = $revision->loadDiffs(); - - $diff_dicts = array(); - foreach ($diffs as $diff) { - $diff->attachChangesets($diff->loadChangesets()); - // TODO: We could batch this to improve performance. - foreach ($diff->getChangesets() as $changeset) { - $changeset->attachHunks($changeset->loadHunks()); - } - $diff_dicts[] = $diff->getDiffDict(); - } + $diffs = id(new DifferentialDiffQuery()) + ->withRevisionIDs(array($revision_id)) + ->needChangesets(true) + ->needArcanistProjects(true) + ->execute(); + $diff_dicts = mpull($diffs, 'getDiffDict'); $commit_dicts = array(); $commit_phids = $revision->loadCommitPHIDs(); diff --git a/src/applications/differential/conduit/ConduitAPI_differential_querydiffs_Method.php b/src/applications/differential/conduit/ConduitAPI_differential_querydiffs_Method.php new file mode 100644 index 0000000000..991d7c09a6 --- /dev/null +++ b/src/applications/differential/conduit/ConduitAPI_differential_querydiffs_Method.php @@ -0,0 +1,45 @@ + 'optional list', + 'revison_ids' => 'optional list', + ); + } + + public function defineErrorTypes() { + return array(); + } + + public function defineReturnType() { + return 'list'; + } + + protected function execute(ConduitAPIRequest $request) { + $ids = $request->getValue('ids', array()); + $revision_ids = $request->getValue('revision_ids', array()); + $diffs = array(); + $diff_dicts = array(); + if ($ids || $revision_ids) { + $diffs = id(new DifferentialDiffQuery()) + ->setViewer($request->getUser()) + ->withIDs($ids) + ->withRevisionIDs($revision_ids) + ->needChangesets(true) + ->needArcanistProjects(true) + ->execute(); + } + + return mpull($diffs, 'getDiffDict', 'getID'); + } +} diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 2610ce97dc..7b9bae6b4b 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -29,7 +29,10 @@ final class DifferentialRevisionViewController extends DifferentialController { return new Aphront404Response(); } - $diffs = $revision->loadDiffs(); + $diffs = id(new DifferentialDiffQuery()) + ->setViewer($request->getUser()) + ->withRevisionIDs(array($this->revisionID)) + ->execute(); if (!$diffs) { throw new Exception( @@ -925,10 +928,9 @@ final class DifferentialRevisionViewController extends DifferentialController { $diff = new DifferentialDiff(); $diff->attachChangesets($generated_changesets); - $diff_dict = $diff->getDiffDict(); - + $raw_changes = $diff->buildChangesList(); $changes = array(); - foreach ($diff_dict['changes'] as $changedict) { + foreach ($raw_changes as $changedict) { $changes[] = ArcanistDiffChange::newFromDictionary($changedict); } $bundle = ArcanistBundle::newFromChanges($changes); diff --git a/src/applications/differential/mail/DifferentialReviewRequestMail.php b/src/applications/differential/mail/DifferentialReviewRequestMail.php index 0cf85f6c07..a1c3e01c17 100644 --- a/src/applications/differential/mail/DifferentialReviewRequestMail.php +++ b/src/applications/differential/mail/DifferentialReviewRequestMail.php @@ -87,7 +87,10 @@ abstract class DifferentialReviewRequestMail extends DifferentialMail { $revision = $this->getRevision(); $revision_id = $revision->getID(); - $diffs = $revision->loadDiffs(); + $diffs = id(new DifferentialDiffQuery()) + ->setViewer($this->getActor()) + ->withRevisionIDs(array($revision_id)) + ->execute(); $diff_number = count($diffs); $attachments[] = new PhabricatorMetaMTAAttachment( @@ -112,16 +115,15 @@ abstract class DifferentialReviewRequestMail extends DifferentialMail { private function buildPatch() { $diff = new DifferentialDiff(); - $diff->attachChangesets($this->getChangesets()); - // TODO: We could batch this to improve performance. foreach ($diff->getChangesets() as $changeset) { - $changeset->attachHunks($changeset->loadHunks()); + $changeset->attachHunks( + $changeset->loadRelatives(new DifferentialHunk(), 'changesetID')); } - $diff_dict = $diff->getDiffDict(); + $raw_changes = $diff->buildChangesList(); $changes = array(); - foreach ($diff_dict['changes'] as $changedict) { + foreach ($raw_changes as $changedict) { $changes[] = ArcanistDiffChange::newFromDictionary($changedict); } $bundle = ArcanistBundle::newFromChanges($changes); diff --git a/src/applications/differential/query/DifferentialDiffQuery.php b/src/applications/differential/query/DifferentialDiffQuery.php index e7ee463a2e..71375e0305 100644 --- a/src/applications/differential/query/DifferentialDiffQuery.php +++ b/src/applications/differential/query/DifferentialDiffQuery.php @@ -5,6 +5,8 @@ final class DifferentialDiffQuery private $ids; private $revisionIDs; + private $needChangesets = false; + private $needArcanistProjects = false; public function withIDs(array $ids) { $this->ids = $ids; @@ -16,6 +18,16 @@ final class DifferentialDiffQuery return $this; } + public function needChangesets($bool) { + $this->needChangesets = $bool; + return $this; + } + + public function needArcanistProjects($bool) { + $this->needArcanistProjects = $bool; + return $this; + } + public function loadPage() { $table = new DifferentialDiff(); $conn_r = $table->establishConnection('r'); @@ -31,6 +43,50 @@ final class DifferentialDiffQuery return $table->loadAllFromArray($data); } + public function willFilterPage(array $diffs) { + if ($this->needChangesets) { + $this->loadChangesets($diffs); + } + + if ($this->needArcanistProjects) { + $this->loadArcanistProjects($diffs); + } + + return $diffs; + } + + private function loadChangesets(array $diffs) { + foreach ($diffs as $diff) { + $diff->attachChangesets( + $diff->loadRelatives(new DifferentialChangeset(), 'diffID')); + foreach ($diff->getChangesets() as $changeset) { + $changeset->attachHunks( + $changeset->loadRelatives(new DifferentialHunk(), 'changesetID')); + } + } + return $diffs; + } + + private function loadArcanistProjects(array $diffs) { + $phids = array_filter(mpull($diffs, 'getArcanistProjectPHID')); + $projects = array(); + $project_map = array(); + if ($phids) { + $projects = id(new PhabricatorRepositoryArcanistProject()) + ->loadAllWhere( + 'phid IN (%Ls)', + $phids); + $project_map = mpull($projects, null, 'getPHID'); + foreach ($diffs as $diff) { + if ($diff->getArcanistProjectPHID()) { + $project = $project_map[$diff->getArcanistProjectPHID()]; + $diff->attachArcanistProject($project); + } + } + } + return $diffs; + } + private function buildWhereClause(AphrontDatabaseConnection $conn_r) { $where = array(); diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index 9b6c35937a..9305fdd4d5 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -31,6 +31,7 @@ final class DifferentialDiff private $unsavedChangesets = array(); private $changesets = self::ATTACHABLE; + private $arcanistProject = self::ATTACHABLE; public function addUnsavedChangeset(DifferentialChangeset $changeset) { if ($this->changesets === null) { @@ -60,6 +61,25 @@ final class DifferentialDiff $this->getID()); } + public function attachArcanistProject( + PhabricatorRepositoryArcanistProject $project) { + $this->arcanistProject = $project; + return $this; + } + + public function getArcanistProject() { + return $this->assertAttached($this->arcanistProject); + } + + public function getArcanistProjectName() { + $name = ''; + if ($this->arcanistProject) { + $project = $this->getArcanistProject(); + $name = $project->getName(); + } + return $name; + } + public function loadArcanistProject() { if (!$this->getArcanistProjectPHID()) { return null; @@ -204,8 +224,31 @@ final class DifferentialDiff 'lintStatus' => $this->getLintStatus(), 'changes' => array(), 'properties' => array(), + 'projectName' => $this->getArcanistProjectName() ); + $dict['changes'] = $this->buildChangesList(); + + $properties = id(new DifferentialDiffProperty())->loadAllWhere( + 'diffID = %d', + $this->getID()); + foreach ($properties as $property) { + $dict['properties'][$property->getName()] = $property->getData(); + + if ($property->getName() == 'local:commits') { + foreach ($property->getData() as $commit) { + $dict['authorName'] = $commit['author']; + $dict['authorEmail'] = idx($commit, 'authorEmail'); + break; + } + } + } + + return $dict; + } + + public function buildChangesList() { + $changes = array(); foreach ($this->getChangesets() as $changeset) { $hunks = array(); foreach ($changeset->getHunks() as $hunk) { @@ -236,25 +279,9 @@ final class DifferentialDiff 'delLines' => $changeset->getDelLines(), 'hunks' => $hunks, ); - $dict['changes'][] = $change; + $changes[] = $change; } - - $properties = id(new DifferentialDiffProperty())->loadAllWhere( - 'diffID = %d', - $this->getID()); - foreach ($properties as $property) { - $dict['properties'][$property->getName()] = $property->getData(); - - if ($property->getName() == 'local:commits') { - foreach ($property->getData() as $commit) { - $dict['authorName'] = $commit['author']; - $dict['authorEmail'] = $commit['authorEmail']; - break; - } - } - } - - return $dict; + return $changes; } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 75902087d9..9f438e6b1f 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -132,15 +132,6 @@ final class DifferentialRevision extends DifferentialDAO DifferentialPHIDTypeRevision::TYPECONST); } - public function loadDiffs() { - if (!$this->getID()) { - return array(); - } - return id(new DifferentialDiff())->loadAllWhere( - 'revisionID = %d', - $this->getID()); - } - public function loadComments() { if (!$this->getID()) { return array(); @@ -165,7 +156,10 @@ final class DifferentialRevision extends DifferentialDAO public function delete() { $this->openTransaction(); - $diffs = $this->loadDiffs(); + $diffs = id(new DifferentialDiffQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withRevisionIDs(array($this->getID())) + ->execute(); foreach ($diffs as $diff) { $diff->delete(); }